Paul Bone [:pbone] Assignee
	Description • 6 years ago

This bug is split off from Bug 1433007 since it may have an affect on performance regardless of that other bug.  Since not all nursery collections collect when the nursery is full, poisoning the whole nursery in those cases is wasteful.

This will likely have the greatest performance affect with either the nursery is collected early because one of the remembered sets is full, or the nursery is collected because we want it empty to collect the main heap.  The savings probably won't be noticable but may be measureable.

	Paul Bone [:pbone] Assignee
	Comment 2 • 6 years ago

Comment on attachment 9016152 [details] [diff] [review]
Bug 1498095 - Only poison the portion of the nursery we used

Oops, I forgot something.

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 3 • 6 years ago

It's fine to do this, but we shouldn't be doing performance measurements with poisoning enabled.  It's not enabled in release builds so this will mainly affect nightly.

	Paul Bone [:pbone] Assignee
	Comment 4 • 6 years ago

We have to do this both for sweeping and when setting the current chunk.  We
set the current chunk after sweeping so to have an effect when there's only
one chunk (the main reason for this code) we need to do this correctly then
also.

	Paul Bone [:pbone] Assignee
	Comment 5 • 6 years ago

I'm currently waiting on benchmark results for this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1120b07574644b829cc27182def722365

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 6 • 6 years ago

Comment on attachment 9016237 [details] [diff] [review]
Bug 1498095 - Only poison the portion of the nursery we used

Review of attachment 9016237 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Nursery.cpp
@@ +1164,5 @@
> +        //  1. The first time it was used it was fully poisoned.
> +        //  2. When it is swept, only the used part is poisoned.
> +        //  3. We repoison that part only here.
> +        chunk(chunkno).poisonAndInit(runtime(), position_ - chunk(chunkno).start());
> +    }

This is kind of fiddly, what with setting position_ to a special value before calling this and then testing it here.

How about we fully poison the first chunk in enable() and then poison only the used part here.  Would that work?

	Paul Bone [:pbone] Assignee
	Comment 7 • 6 years ago

(In reply to Jon Coppeard (:jonco) from comment #6)
> Comment on attachment 9016237 [details] [diff] [review]
> Bug 1498095 - Only poison the portion of the nursery we used
> 
> Review of attachment 9016237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/Nursery.cpp
> @@ +1164,5 @@
> > +        //  1. The first time it was used it was fully poisoned.
> > +        //  2. When it is swept, only the used part is poisoned.
> > +        //  3. We repoison that part only here.
> > +        chunk(chunkno).poisonAndInit(runtime(), position_ - chunk(chunkno).start());
> > +    }
> 
> This is kind of fiddly, what with setting position_ to a special value
> before calling this and then testing it here.
> 
> How about we fully poison the first chunk in enable() and then poison only
> the used part here.  Would that work?

It'll work for this purpose, but it won[t be enough for the resident memory benifits of bug 1433007.  I can tidy up the fiddlyness in this patch, either poisoning all of it or finding a better solution.

	Paul Bone [:pbone] Assignee
	Comment 8 • 6 years ago

(In reply to Jon Coppeard (:jonco) from comment #3)
> It's fine to do this, but we shouldn't be doing performance measurements
> with poisoning enabled.  It's not enabled in release builds so this will
> mainly affect nightly.

Yeah, I realised that part way through Bug 1433007.  WHich means it'll only really have a performance impact when poisoning is used, since nursery collection will only touch the live memory.  However that will still help with memory usage.

I know we can disable poisoning as a build thing, but should we have it as a perf so we can turn it off for benchmarking and such easilly, particularly for our automated tests?

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 9 • 6 years ago

(In reply to Paul Bone [:pbone] from comment #7)
> It'll work for this purpose, but it won[t be enough for the resident memory
> benifits of bug 1433007.

Why would this will end up with a different outcome to what you've got here?

> I know we can disable poisoning as a build thing, but should we have it as a
> perf so we can turn it off for benchmarking and such easilly, particularly
> for our automated tests?

There's the JSGC_DISABLE_POISONING environment variable but yes, we should maybe make a pref for this as well.

	Paul Bone [:pbone] Assignee
	Comment 10 • 6 years ago

(In reply to Jon Coppeard (:jonco) from comment #9)
> (In reply to Paul Bone [:pbone] from comment #7)
> > It'll work for this purpose, but it won[t be enough for the resident memory
> > benifits of bug 1433007.
> 
> Why would this will end up with a different outcome to what you've got here?

I'm struggling to parse your question clearly.  But I also stepped through my explaination and realised my mistake was somewhat of a mistake.  So I'll just set it all out again:

The cost of collecting the nursery is O(traced objects) + O(nursery size for poisoning).  (Sweeping and marking are both linear in the number of traced objects.

So this change is supposed to help when the number of traced objects is small, it'll help us avoid touching memory across the whole nursery.  But only when poisioning is being used.

It's also supposed to work with Bug 1433007 since if that reduces the nursery to 256KB, we wouldn't want to poision a 1MB chunk each collection, that'd potentially make things _slower_ (since with 256KB nursery we'd collect more often).

Here's a mistake I made: I said that Bug 1433007 might not have an affect on performance when poisoning is not used.  Since without poisioning, the 1MB of memory won't be touched anyway, and wouldn't have an affect on cache performance.  But this is incorrect, it is touched, /by the mutator/.  So that bug, either without poisioning, or with this bug also applied, should improve the cache behaviour of the mutator.

> > I know we can disable poisoning as a build thing, but should we have it as a
> > perf so we can turn it off for benchmarking and such easilly, particularly
> > for our automated tests?
> 
> There's the JSGC_DISABLE_POISONING environment variable but yes, we should
> maybe make a pref for this as well.

Ah, I forgot it was already an environment variable.

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 12 • 6 years ago

(In reply to Paul Bone [:pbone] from comment #10)
> The cost of collecting the nursery is O(traced objects) + O(nursery size for
> poisoning).  (Sweeping and marking are both linear in the number of traced
> objects.
> 
> So this change is supposed to help when the number of traced objects is
> small, it'll help us avoid touching memory across the whole nursery.  But
> only when poisioning is being used.

Poisoning doesn't happen in release builds so in general we shouldn't worry too much about optimising it.  It would be nice to see the memory reductions possible with using less then a whole chunk on nightly though, so I think this makes sense.

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 13 • 6 years ago

Comment on attachment 9017121 [details] [diff] [review]
Bug 1498095 - Only poison the portion of the nursery we used

Review of attachment 9017121 [details] [diff] [review]:
-----------------------------------------------------------------

This is nicer what with passing a flag to request full poisoning of the chunk.  Thanks for the updated patch.

::: js/src/gc/Nursery.cpp
@@ +1158,5 @@
> +        position_ < chunk(chunkno).end() &&
> +        position_ >= chunk(chunkno).start())
> +    {
> +        // When we setup a new chunk the whole chunk must be poisoned with the
> +        // correct value, it will be because:

nit: Run-on sentence.  Needs full stop after "correct value".

::: js/src/gc/Nursery.h
@@ +509,5 @@
>          return *chunks_[index];
>      }
>  
> +    /*
> +     * Set the current chunk, this updates the currentChunk_, position_

nit: Run-on sentence.  Needs a full stop after "Set the current chunk".

@@ +510,5 @@
>      }
>  
> +    /*
> +     * Set the current chunk, this updates the currentChunk_, position_
> +     * currentEnd_ and currentStringEnd_ values as approprite.  It'll also

nit: Single space between sentences please.

	Paul Bone [:pbone] Assignee
	Comment 14 • 6 years ago

(In reply to Jon Coppeard (:jonco) from comment #12)
> (In reply to Paul Bone [:pbone] from comment #10)
> > The cost of collecting the nursery is O(traced objects) + O(nursery size for
> > poisoning).  (Sweeping and marking are both linear in the number of traced
> > objects.
> > 
> > So this change is supposed to help when the number of traced objects is
> > small, it'll help us avoid touching memory across the whole nursery.  But
> > only when poisioning is being used.
> 
> Poisoning doesn't happen in release builds so in general we shouldn't worry
> too much about optimising it.  It would be nice to see the memory reductions
> possible with using less then a whole chunk on nightly though, so I think
> this makes sense.

Yes, I thought that's what I was saying / trying to say :-\

	Pulsebot
	Comment 15 • 6 years ago

Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a49f77c86816
Only poison the portion of the nursery we used r=jonco

	Andrei Ciure[:aciure]
	Comment 16 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a49f77c86816