Closed Bug 1407143 Opened 2 years ago Closed 2 years ago

Consider not collecting the nursery for each marking GCSlice

Categories

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

59 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox63 --- fixed

People

(Reporter: pbone, Assigned: pbone)

References

(Blocks 1 open bug)

Details

(Whiteboard: [js:tech-debt][qf:p3])

Attachments

(7 files, 13 obsolete files)

1.45 KB, patch
jonco
: review+
Details | Diff | Splinter Review
1.03 KB, patch
sfink
: review+
Details | Diff | Splinter Review
3.28 KB, patch
jonco
: review+
Details | Diff | Splinter Review
1.02 KB, patch
jonco
: review+
Details | Diff | Splinter Review
9.94 KB, patch
jonco
: review+
Details | Diff | Splinter Review
979 bytes, patch
jonco
: review+
Details | Diff | Splinter Review
1.42 KB, patch
jonco
: review+
Details | Diff | Splinter Review
We can probably avoid collecting, or at least avoid evicting, the Nursery for each slice of an incremental collection.
Priority: -- → P3
Whiteboard: [js:tech-debt]
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Priority: P3 → P2
Whiteboard: [js:tech-debt] → [js:tech-debt], [qf]
Target Milestone: --- → mozilla59
Version: 55 Branch → 59 Branch
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.
(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.
Whiteboard: [js:tech-debt], [qf] → [js:tech-debt][qf:p3]
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.
Attachment #8947717 - Flags: review?(jcoppeard)
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).
Attachment #8947719 - Flags: feedback?(jcoppeard)
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.
Attachment #8947717 - Flags: review?(jcoppeard) → review+
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.
Attachment #8947719 - Flags: feedback?(jcoppeard) → feedback+
> ::: 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.
This now passes the SpiderMonkey test suites, but octane crashes.
Attachment #8947719 - Attachment is obsolete: true
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.
Attachment #8955007 - Flags: review?(sphink)
Depends on: 1443147
No longer depends on: 1443147
Paul, this will add some un-safety in an ugly edge case. Please check with :jandem or me before trying to land this.
Flags: needinfo?(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.
Flags: needinfo?(pbone)
Attachment #8952053 - Attachment is obsolete: true
Attachment #8952054 - Attachment is obsolete: true
Attachment #8952055 - Attachment is obsolete: true
Attachment #8952056 - Attachment is obsolete: true
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 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 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 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().
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.
Priority: P2 → P1
Summary: Consider not collecting the nursery for each GCSlice → Consider not collecting the nursery for each marking GCSlice
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.
Attachment #8955007 - Attachment is obsolete: true
Attachment #8970413 - Flags: review?(sphink)
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.
Attachment #8970413 - Flags: review?(sphink) → review+
Depends on: 1458154
Attachment #8968832 - Attachment is obsolete: true
Depends on: 1460098
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`
No longer depends on: 1460098
NP bbouvier, I'll fix those.
Depends on: 1466633
Depends on: 1468786
Attachment #8968827 - Attachment is obsolete: true
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].
Attachment #8968829 - Attachment is obsolete: true
Attachment #8985546 - Flags: feedback?(jcoppeard)
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 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.
Attachment #8985546 - Flags: feedback?(jcoppeard) → feedback+
Attachment #8968828 - Attachment is obsolete: true
(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.
This change isn't really required but may tighten some other conditions /
assertions.
Attachment #8986678 - Flags: review?(jcoppeard)
Attachment #8985546 - Attachment is obsolete: true
Attachment #8986680 - Flags: review?(jcoppeard)
Attachment #8968833 - Attachment is obsolete: true
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.
Attachment #8986659 - Flags: review?(jcoppeard) → review+
Attachment #8986678 - Flags: review?(jcoppeard) → review+
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.
Attachment #8986680 - Flags: review?(jcoppeard) → review+
Attachment #8986681 - Flags: review?(jcoppeard) → review+
Comment on attachment 8986682 [details] [diff] [review]
Bug 1407143 (Part 7) - Add some nursery().isEmpty() assertions

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

Nice.
Attachment #8986682 - Flags: review?(jcoppeard) → review+
Thanks for the reviews Jon.

I'm doing the final testing on try and will hopefully land this shortly.
Blocks: 1470369
(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 :)
Yeah,  I just checked and found out that the 25th is merge day.  so yeah, I'll wait until afterwards.  Oh well..
Pushed by pbone@mozilla.com:
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
Blocks: 1510145
No longer blocks: 1510145
Depends on: 1510145
You need to log in before you can comment on or make changes to this bug.