1407143 - Consider not collecting the nursery for each marking GCSlice

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P1)

Description

[Paul Bone [:pbone]]

We can probably avoid collecting, or at least avoid evicting, the Nursery for each slice of an incremental collection.

Comment 1

[Paul Bone [:pbone]]

Jon and I were discussing this over e-mail. but I'd like to share that information here so it's recorded.

I'm not sure if I was totally clear.  We do want to collect the nursery for
the first slice of each major collection, it's the remaining slices that we
won't want to collect for.  We just need to make sure that a remembered set
that tracks nursery->tenured pointers is updated in between the slices.
Nursery objects are considered black, but we don't need to do anything to
track that.  Therefore the nursery doesn't need mark bits for this purpose.

if we want to compact evicting the nursery and optionally shrinking it is a reasoanble idea.  Since we tend to compact in order to release memory.  This is much easier than having to update pointers from the nursery into the tenured heap.

What I'm concerned about istracking down all the various instances of barriers used.
Since each object type can have its own barrier implementation.  And add a
new post-write barrier that is active during incremental collection.  It can
be de-activated after the collection (I'm not sure I like how our barrier
code checks if it is needed right now).  This also has implications for
JITed code.  It may help to work together on this next week.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Paul Bone [:pbone] from comment #1)
> And add a
> new post-write barrier that is active during incremental collection.

I'm a bit worried about this. Barriers are complicated, affect performance, bloat JIT code. That cost is worth it for pre-barriers (we need incremental GC) and the current post-barriers (we need generational GC), but I don't know if it's worth it in this case.

Comment 3

[Paul Bone [:pbone]]

Jonco and I had an e-mail discussion some time ago.  Since then we've both
forgotten what was decided / not-decided then, but we know it's important.
I've re-read that conversation and summarised it below.

First some obvious stuff
------------------------

 + Barriers are tricky/add cost etc, we all agree.
 + We collect the nursery in the first slice of each collection.
 + We attempt to avoid nursery collection on the other slices. this could
   save around 20% of each slice budget, letting us make more use of that
   5ms (default) budget.  The sooner we complete collection, the less time
   our incremental barriers need to be active.

A fairly simple good idea
-------------------------

 + We implement this bit-by-bit.  First try to avoid the nursery collection
   when we're in a sweeping phase, but still do it in the marking phase.
   Then try to avoid it also during marking phases.

Paul's new barrier
------------------

If we're currently doing a collection, and that collection is incremental,
and we don't want to collect the nursery in the next mark slice then we need
a barrier that:
 + Captures whenever a tenured object's reference is written into a nursery
   objects field.
 + Places the pointed-to tenured object on the mark stack.

We need this barrier (in jonco's words) to ensure we mark live tenured
objects that are only referenced via a chain of pointers that goes through
the nursery, which we don't trace through because we already assume
everything in the nursery is black.

Tricky stuff
------------

 Q: Do we really need the new barrier?

 A: Jon is suggesting that we determine this experimentally.  I prefer to
    reason things through.  Is it possible that on a slice that's not the
    first slice a tenured object that is referenced only from the nursery is
    not marked?  I think Jon may be right and snapshot at the beginning
    makes this okay, here's how I'm reasoning through that.

    If a tenured object is only referenced from the nursery in a slice
    that's not the first slice.  Either it was allocated after GC started (and
    is therefore marked) or was pointed-to by something else, and a
    pre-barrier ensured that it got marked.  I can only think of these two
    situations.

    This is enough for me to move on to experimenting with this without the
    barrier.

 Q: What if the collector is compacting, in a compacting collection we need
    to update pointers as things move.  If there are objects in the nursery
    pointing into the tenured heap we'd need to update them, and there's no
    infrastructure for doing that.

 A: Collect the nursery for each compacting slice.  Then there'll be no
    nursery->tenured pointers to worry about.  Since we're compacting to
    free up memory, reducing the nursery size at this time is also a good
    idea, but should be tracked as a separate bug to isolate it's affect on
    performance.

 Q: The tenured->nursery remembered set may include pointers from dead
    tenured objects.  If the nursery is collected after sweeping and we
    attempt to update a pointer in a dead object bad things will happen.

 A: I think we need to collect the nursery once before sweeping
    begins.  It only needs to be once, rather than each sweep slice because
    after that point it is not possible to update a dead object.  No objects
    can "die" after then (they have been marked and will stay marked until
    the GC ends).
    
    Even better, due to snapshot-at-the-beginning and collecting the nursery
    at the beginning it is impossible for a tenured->nursery pointer to
    point from something that wasn't marked.  In other words, we could not
    have written a nursery pointer, something that was allocated after GC
    started, into something that died before GC started.  So we can also
    avoid this collection.

Comment 4

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 1) - Fix nonsensical comment

Comment 5

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 2) - Avoid a minor GC for a slice if it's sweeping

I'm sorry I forgot to share my work-in-progress with you earlier.  Here it is.

This first part of this change attempts to avoid excessive minor GCs (one
per slice) during the sweeping phase.  I'll follow up with a change that
does the same for the marking phase.

This will still collect the nursery at the beginning of a slice if:
 * the slice is a compacting slice (also marking slices),
 * the nursery wants to be collected or has less than 1KB of memory left,
 * the GC is or is becoming non-incremental,

I've removed and relaxed some assertions including adding a more relaxed
version of CheckHeapAfterGC that is used when the nursery is empty.  An
assertion has been added before calling this to make sure the nursery is
empty when it should be.

This now passes the jit-tests, but I still want to run it with a more
aggressive zeal mode, run some Firefox tests on try, and run some
performance comparisons (locally & talos).

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8947717 [details] [diff] [review]
Bug 1407143 (Part 1) - Fix nonsensical comment

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

Great, thanks for fixing that.

::: js/src/gc/Barrier.h
@@ +120,5 @@
>   * Post barriers are used to track this remembered set.
>   *
> + * Whenever a slot which could contain such a pointer is written, we check
> + * whether the pointed-to thing is in the nursery (if storeBuffer() returns a
> + * buffer).  If so we  and if so we add the cell into the store buffer, which is

Nit: one space between sentences, here and below.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8947719 [details] [diff] [review]
Bug 1407143 (Part 2) - Avoid a minor GC for a slice if it's sweeping

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

Thanks for posting this.  The patch looks good.  

Let me know what the test results are like.

::: js/src/gc/Verifier.cpp
@@ +629,4 @@
>  void
>  CheckHeapTracer::checkCell(Cell* cell)
>  {
> +    if (!IsValidGCThingPointer(cell))

IsGCThingValidAfterMovingGC() looks like this:

IsGCThingValidAfterMovingGC(T* t)
{
    return !IsInsideNursery(t) && !RelocationOverlay::isCellForwarded(t);
}

For this checkCell() method we still want to do the second half of that test, it's just that we allow pointers to still be in the nursery.

::: js/src/jsgc.cpp
@@ +7273,5 @@
>      gcstats::AutoGCSlice agc(stats(), scanZonesBeforeGC(), invocationKind, budget, reason);
>  
> +    switch (incrementalState) {
> +        case State::NotActive:
> +        case State::MarkRoots:

I don't think we ever enter a slice in the MarkRoots state - we transition from NotActive to MarkRoots to Mark before we have the chance to yield.

@@ +7284,5 @@
> +                budget.isUnlimited() ||
> +                nursery().minorGCRequested() ||
> +                nursery().freeSpace() < 1024)
> +            {
> +                minorGC(reason, gcstats::PhaseKind::EVICT_NURSERY_FOR_MAJOR_GC);

One pattern that we use quite a bit in the GC is to factor out code like this into  predicate function (e.g. ShouldCollectNursery()) and then say |if (predicate()) doThing()|.  This would result in only one call to minorGC() here.

@@ +7542,5 @@
> +             */
> +            incrementalState == State::MarkRoots ||
> +            incrementalState == State::Mark,
> +          // (end if)
> +          nursery().isEmpty());

I like the comments but this assertion is kinda huge.  Maybe factor out the condition?  And also let's make this assertion happen all the time, not just if the zeal mode is on.

Comment 8

[Paul Bone [:pbone]]

> ::: js/src/gc/Verifier.cpp
> @@ +629,4 @@
> >  void
> >  CheckHeapTracer::checkCell(Cell* cell)
> >  {
> > +    if (!IsValidGCThingPointer(cell))
> 
> IsGCThingValidAfterMovingGC() looks like this:
> 
> IsGCThingValidAfterMovingGC(T* t)
> {
>     return !IsInsideNursery(t) && !RelocationOverlay::isCellForwarded(t);
> }
> 
> For this checkCell() method we still want to do the second half of that
> test, it's just that we allow pointers to still be in the nursery.
> 

thanks for catching that, I saw that while I was working but most have forgotten to ensure it was replicated.

> ::: js/src/jsgc.cpp
> @@ +7273,5 @@
> >      gcstats::AutoGCSlice agc(stats(), scanZonesBeforeGC(), invocationKind, budget, reason);
> >  
> > +    switch (incrementalState) {
> > +        case State::NotActive:
> > +        case State::MarkRoots:
> 
> I don't think we ever enter a slice in the MarkRoots state - we transition
> from NotActive to MarkRoots to Mark before we have the chance to yield.

I'll add a case for this in debug builds that asserts.

> @@ +7542,5 @@
> > +             */
> > +            incrementalState == State::MarkRoots ||
> > +            incrementalState == State::Mark,
> > +          // (end if)
> > +          nursery().isEmpty());
> 
> I like the comments but this assertion is kinda huge.  Maybe factor out the
> condition?  And also let's make this assertion happen all the time, not just
> if the zeal mode is on.

Good idea, the isEmpty() check is so cheap it's not like the Verifier code.

Comment 9

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 2) - Avoid a minor GC for a slice if it's sweeping

This now passes the SpiderMonkey test suites, but octane crashes.

Comment 10

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 3) - Refactor a test for particular zeal modes

Comment 11

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 4) - Avoid minor GC during mark slices also

Comment 12

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 5) - Add a tunable for the new nursery threshold

Comment 13

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 6) - Add a new nurseryEmpty assertion after collection

Comment 14

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 7) - Add a comment clarifying how some code works

Comment 15

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8955007 [details] [diff] [review]
Bug 1407143 (Part 7) - Add a comment clarifying how some code works

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +1528,5 @@
> +             * If the current element is garbage then remove it from the
> +             * vector by moving the last one into its place.  Post-decrement i
> +             * so that on the next iteration we check the formerly-last
> +             * element.
> +             */

Hm. I agree the code is cryptic. But the comment kind of is too.

How about first we change the body to be 

    if (IsAboutToBeFinalized...) {
       views[i] = views.back();
       i--;
       views.popBack();
    }

then I'm not sure if the first sentence of your comment is needed, and the second sentence could probably be placed on the decrement line:

       i--; // Revisit the current index.

or perhaps the whole thing would make more sense as

  size_t i = 0;
  while (i < views.length()) {
      if (IsAbout...) {
          views[i] = views.back();
          views.popBack();
      } else {
          i++;
      }
  }

If any comment is needed, it could be in the garbage block

          // Reuse the index that is known to contain garbage.

Comment 16

[Ted Campbell [:tcampbell]]

Paul, this will add some un-safety in an ugly edge case. Please check with :jandem or me before trying to land this.

Comment 17

[Paul Bone [:pbone]]

Hi tcampbell,

Thanks for the heads up, I've still got some work to do before this lands, and I'm currently distracted by other bugs. But I would like to talk about it.

Can we chat tomorrow morning (this evening for you) about this?   I sent you an invite.

Comment 18

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 2) - Avoid a minor GC for a slice if it's sweeping

Comment 19

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 3) - Refactor a test for particular zeal modes

Comment 20

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 4) - Avoid minor GC during mark slices also

Comment 21

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 5) - Add a tunable for the new nursery threshold

Comment 22

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 6) - Add a new nurseryEmpty assertion after collection

Comment 23

[Paul Bone [:pbone]]

I've updated the patches here to make sure they match my workspace, I'm not sure if anything actually changed, but I wanted to make sure they matched.

Comment 24

[Jon Coppeard (:jonco)]

Comment on attachment 8968827 [details] [diff] [review]
Bug 1407143 (Part 2) - Avoid a minor GC for a slice if it's sweeping

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

::: js/src/gc/Verifier.cpp
@@ +635,4 @@
>  void
>  CheckHeapTracer::checkCell(Cell* cell)
>  {
> +    if (!IsValidGCThingPointer(cell) && !RelocationOverlay::isCellForwarded(cell))

The condition needs to be | !IsValidGCThingPointer(cell) || RelocationOverlay::isCellForwarded(cell) | here - we want to ensure there are no reachable forwarded cells.

Comment 25

[Jon Coppeard (:jonco)]

Comment on attachment 8968828 [details] [diff] [review]
Bug 1407143 (Part 3) - Refactor a test for particular zeal modes

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

I think I have separately made this same refactoring :)

Comment 26

[Jon Coppeard (:jonco)]

Comment on attachment 8968832 [details] [diff] [review]
Bug 1407143 (Part 5) - Add a tunable for the new nursery threshold

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

We should make this threshold the same as is used for Nursery::needIdleTimeCollection().

Comment 27

[Paul Bone [:pbone]]

We will attempt to test and land this for marking slices, and separately work on sweeping slices which have some more edge cases.  I previously wanted to do sweeping first, but changed my mind after finding more edge cases there.

Comment 28

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 2) - Refactor sweepEntry() for readability

Hi Steve,

I think your last suggestion for this loop is best.  I've implemented that.

I'm moving this to Part 2, I'll renumber the other patches later.

Comment 29

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8970413 [details] [diff] [review]
Bug 1407143 (Part 2) - Refactor sweepEntry() for readability

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +1558,5 @@
>          if (IsAboutToBeFinalizedUnbarriered(&views[i])) {
> +            /*
> +             * If the current element is garbage then remove it from the
> +             * vector by moving the last one into its place.
> +             */

Despite the various exceptions in this file, these should be // comments.

Comment 30

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8947717 [details] [diff] [review]
Bug 1407143 (Part 1) - Fix nonsensical comment

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

(sorry for drive-by comment)

::: js/src/gc/Barrier.h
@@ +120,5 @@
>   * Post barriers are used to track this remembered set.
>   *
> + * Whenever a slot which could contain such a pointer is written, we check
> + * whether the pointed-to thing is in the nursery (if storeBuffer() returns a
> + * buffer).  If so we  and if so we add the cell into the store buffer, which is

nit: the same sentence contains `if so we  and if so we`, so one of them is redundant

@@ +121,5 @@
>   *
> + * Whenever a slot which could contain such a pointer is written, we check
> + * whether the pointed-to thing is in the nursery (if storeBuffer() returns a
> + * buffer).  If so we  and if so we add the cell into the store buffer, which is
> + * the collector's representation of the remembered set.  This means than when

pre-existing nit: `than` => `that`

Comment 31

[Paul Bone [:pbone]]

NP bbouvier, I'll fix those.

Comment 32

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 4) - Don't collect nursery for every mark slice

This seems to work, in other words it seems to crash in the same ways as the change in Bug 1468786 Attachment #8985531 [details] [diff].

Comment 33

[Paul Bone [:pbone]]

This is now passing tests. The performance metrics don't seem to show any sagnificant difference (informally so far).  I didn't expect anything dramatic until the second part of this is done that makes the same change but for sweeping.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=78c8de86caa39f5fd18f214d9573ead3a12c520d&framework=1&showOnlyComparable=1&selectedTimeRange=86400

Comment 34

[Jon Coppeard (:jonco)]

Comment on attachment 8985546 [details] [diff] [review]
Bug 1407143 (Part 4) - Don't collect nursery for every mark slice

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

::: js/src/gc/GC.cpp
@@ +7143,3 @@
>          initialReason = reason;
>          cleanUpEverything = ShouldCleanUpEverything(reason, invocationKind);
>          isCompacting = shouldCompact();

I think this patch means that lastMarkSlice will always be false here.  In that case it's probably best to assert it.

@@ +7189,4 @@
>           * YieldBeforeMarking mode and we always yield in YieldBeforeSweeping
>           * mode.
>           *
> +         * Also yeild if the nursery is not-empty to collect it before sweeping.

Spelling: 'yield'

@@ +7218,5 @@
> +            if (!nursery().isEmpty()) {
> +                lastMarkSlice = true;
> +                stats().writeLogMessage(
> +                    "returning to collect the nursery before sweeping");
> +                return IncrementalResult::YeildForNursery;

Is it possible to common up these two blocks that return early to trigger nursery eviction?  Something like | if (!lastMarkSlice && !nursery().isEmpty()) |?  I haven't tried this though.

@@ +7233,5 @@
> +
> +        if (performSweepActions(budget) == NotFinished) {
> +            /*
> +             * We need to reset this here rather than after the mark slice,
> +             * since it is checked at the beginning fo sweeping.

I don't understand this comment.  Where do we check lastMarkSlice at the beginning of sweeping?

::: js/src/gc/GCRuntime.h
@@ +500,5 @@
>    private:
>      enum IncrementalResult
>      {
> +        ResetIncremental = 0,
> +        YeildForNursery,

nit: spelling of 'yield'

Also, we don't actually yield to the mutator here, so maybe we should pick another name for this.

@@ +562,5 @@
> +     * Returns true if we "reset" an existing incremental GC, which would force
> +     * us to run another cycle.
> +     * unlimitedAfterLock returns if we made a decision to use an unlimited
> +     * budget _after_ taking the exclusive lock and therefore after collecting
> +     * the nursery which is done outside that lock.

I think you copied this from the .cpp file, but this comment is out of date wrt returning a bool and could use updating.

The unlimitedAfterLock parameter is not present so I guess that's out of date too.

@@ +571,5 @@
>      bool shouldRepeatForDeadZone(JS::gcreason::Reason reason);
> +    IncrementalResult incrementalCollectSlice(SliceBudget& budget,
> +                                              JS::gcreason::Reason reason,
> +                                              AutoTraceSession& session);
> +    MOZ_MUST_USE bool shouldCollectNurseryForSlice(bool NonIncrementalTrigger,

nit: argument name should start with lowercase.

Comment 35

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 3) - Relax some nursery empty assertions

Comment 36

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) from comment #34)
> Comment on attachment 8985546 [details] [diff] [review]
> Bug 1407143 (Part 4) - Don't collect nursery for every mark slice
> 
> Review of attachment 8985546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/GC.cpp
> @@ +7143,3 @@
> >          initialReason = reason;
> >          cleanUpEverything = ShouldCleanUpEverything(reason, invocationKind);
> >          isCompacting = shouldCompact();
> 
> I think this patch means that lastMarkSlice will always be false here.  In
> that case it's probably best to assert it.

Good idea.

> @@ +7218,5 @@
> > +            if (!nursery().isEmpty()) {
> > +                lastMarkSlice = true;
> > +                stats().writeLogMessage(
> > +                    "returning to collect the nursery before sweeping");
> > +                return IncrementalResult::YeildForNursery;
> 
> Is it possible to common up these two blocks that return early to trigger
> nursery eviction?  Something like | if (!lastMarkSlice &&
> !nursery().isEmpty()) |?  I haven't tried this though.

I think it is, although the condition would have to be "or".  However I'm not sure it's useful, we're yeilding for two different reasons.  I'm happier keeping those as two seperate conditions/blocks because I think that helps with readability.

I've attempted to break it apart further to make it even clearer, it is a bit more repedative but it's also fairly clear.  If the blocks get more complex then I'm happy to merge them once more.

Let me know what you think.

> @@ +7233,5 @@
> > +
> > +        if (performSweepActions(budget) == NotFinished) {
> > +            /*
> > +             * We need to reset this here rather than after the mark slice,
> > +             * since it is checked at the beginning fo sweeping.
> 
> I don't understand this comment.  Where do we check lastMarkSlice at the
> beginning of sweeping?

Ah, That referred to an earlier version of this patch that had to check this when doing unmarking for premarking.  Now that's gone the comment dosn't make sense anymore.

> ::: js/src/gc/GCRuntime.h
> @@ +500,5 @@
> >    private:
> >      enum IncrementalResult
> >      {
> > +        ResetIncremental = 0,
> > +        YeildForNursery,
> 
> nit: spelling of 'yield'

Thanks.

> Also, we don't actually yield to the mutator here, so maybe we should pick
> another name for this.

Okay.  I'll call it "ReturnToEvictNursery"

Thank you, I've also made the other changes that you requested here.

Comment 37

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 4) - Reset lastMarkSlice earlier

This change isn't really required but may tighten some other conditions /
assertions.

Comment 38

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 5) - Don't collect nursery for every mark slice

Comment 39

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 6) - Log the lastMarkSlice value when entering GC

Comment 40

[Paul Bone [:pbone]]

Attachment: Bug 1407143 (Part 7) - Add some nursery().isEmpty() assertions

Comment 41

[Jon Coppeard (:jonco)]

Comment on attachment 8986659 [details] [diff] [review]
Bug 1407143 (Part 3) - Relax some nursery empty assertions

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

::: js/src/gc/Verifier.cpp
@@ +660,4 @@
>      MOZ_RELEASE_ASSERT(failures == 0);
>  }
>  
> +class CheckHeapTracerMovingGC final : public CheckHeapTracer

It would be simpler to pass a flag rather than having separate classes.

Comment 42

[Jon Coppeard (:jonco)]

Comment on attachment 8986680 [details] [diff] [review]
Bug 1407143 (Part 5) - Don't collect nursery for every mark slice

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

Looks good.

::: js/src/gc/GC.cpp
@@ +7197,5 @@
> +            } else if (useZeal && hasZealMode(ZealMode::YieldBeforeSweeping)) {
> +                lastMarkSlice = true;
> +                stats().writeLogMessage("Yielding before starting sweeping");
> +                break;
> +            }

The bodies of the two if/else blocks are the same, so please common these up.

::: js/src/gc/GCRuntime.h
@@ +558,5 @@
>      void collect(bool nonincrementalByAPI, SliceBudget budget, JS::gcreason::Reason reason) JS_HAZ_GC_CALL;
> +
> +    /*
> +     * Run one GC "cycle" (either a slice of incremental GC or an entire
> +     * non-incremental GC.

Missing ).

@@ +565,5 @@
> +     *  * ResetIncremental if we "reset" an existing incremental GC, which would
> +     *    force us to run another cycle or
> +     *  * ReturnToEvictNursery if the collector needs the nursery to be
> +     *    evicted before it can continue or
> +     *  * Ok otherwise.

Thanks for updating the comments.

Comment 43

[Jon Coppeard (:jonco)]

Comment on attachment 8986682 [details] [diff] [review]
Bug 1407143 (Part 7) - Add some nursery().isEmpty() assertions

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

Nice.

Comment 44

[Paul Bone [:pbone]]

Thanks for the reviews Jon.

I'm doing the final testing on try and will hopefully land this shortly.

Comment 45

[Jan de Mooij [:jandem]]

(In reply to Paul Bone [:pbone] from comment #44)
> I'm doing the final testing on try and will hopefully land this shortly.

Next week right? We probably shouldn't land a change like this two days before the merge :)

Comment 46

[Paul Bone [:pbone]]

Yeah,  I just checked and found out that the 25th is merge day.  so yeah, I'll wait until afterwards.  Oh well..

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acfeb7bae5bb
(Part 1) - Fix nonsensical comment r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/195348dbc5ec
(Part 2) - Refactor sweepEntry() for readability r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/868ac1e5e070
(Part 3) - Relax some nursery empty assertions r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ffd9d8370a
(Part 4) - Reset lastMarkSlice earlier r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/49caa22a6bbf
(Part 5) - Don't collect nursery for every mark slice r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc5765ae8368
(Part 6) - Log the lastMarkSlice value when entering GC r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/91f49041e744
(Part 7) - Add some nursery().isEmpty() assertions r=jonco

Comment 48

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/acfeb7bae5bb
https://hg.mozilla.org/mozilla-central/rev/195348dbc5ec
https://hg.mozilla.org/mozilla-central/rev/868ac1e5e070
https://hg.mozilla.org/mozilla-central/rev/c8ffd9d8370a
https://hg.mozilla.org/mozilla-central/rev/49caa22a6bbf
https://hg.mozilla.org/mozilla-central/rev/dc5765ae8368
https://hg.mozilla.org/mozilla-central/rev/91f49041e744