Closed Bug 1306250 Opened 3 years ago Closed 3 years ago

Allow iterating gray objects without evicting the nursery

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file, 2 obsolete files)

Evicting the nursery is unnecessary because nursery objects cannot be gray.
Attached patch bug1306250-iterate-gray (obsolete) — Splinter Review
This just uses our internal iterator classes to get around evicting the nursery first.
Attachment #8796496 - Flags: review?(sphink)
Comment on attachment 8796496 [details] [diff] [review]
bug1306250-iterate-gray

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

::: js/src/gc/Iteration.cpp
@@ +122,5 @@
> +        for (ArenaIter arena(zone, kind); !arena.done(); arena.next()) {
> +            for (ArenaCellIterImpl cell(arena.get()); !cell.done(); cell.next()) {
> +                if (cell.getCell()->isMarked(GRAY))
> +                    cellCallback(data, JS::GCCellPtr(cell.get<JSObject>()));
> +            }

Hm. Rather than duplicating this code, how about renaming ZoneCellIter<TenuredCell>::init to initUnchecked, friending js::IterateGrayObjects, and make an init that just does 

  JSRuntime* rt = zone->runtimeFromAnyThread()
  MOZ_ASSERT_IF(IsNurseryAllocable(kind), rt->gc.nursery.isEmpty());
  initUnchecked(zone, kind);

Is there anything else in there that you don't want?

Then you'd do

  ZoneCellIter<TenuredCell> iter;
  for (iter.initUnchecked(zone, kind); !iter.done(); iter.next()) {
    if (iter.get()->isMarked(GRAY)) ...;
  }

Or maybe it should be initForTenuredIteration or initIgnoreNursery or something.
Attachment #8796496 - Flags: review?(sphink)
Attached patch bug1306250-iterate-gray (obsolete) — Splinter Review
You're right, making initForTenuredIteration is a much better approach.

I added a specific iterator subclass too.
Attachment #8796496 - Attachment is obsolete: true
Attachment #8797563 - Flags: review?(sphink)
Comment on attachment 8797563 [details] [diff] [review]
bug1306250-iterate-gray

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

lgtm, though it introduces a fight between en-US/en-GB spellings of gray/grey. :-)
Attachment #8797563 - Flags: review?(sphink) → review+
Unfortunately this does not quite work for my purposes because isHeapBusy is true when the heap state is JS::HeapState::CycleCollecting, which it is, inside the CC. I tried changing that assertion to
  MOZ_ASSERT(!rt->isHeapBusy() || rt->isCycleCollecting());
but then I hit the assertion "rt->heapState_ == JS::HeapState::Idle" in AutoTraceSession::AutoTraceSession(). Sorry for the trouble.
Andrew, how does this patch work for you?  This also adds js::IterateGrayObjectsUnderCC.
Attachment #8797563 - Attachment is obsolete: true
Attachment #8798840 - Flags: feedback?(continuation)
(In reply to Jon Coppeard (:jonco) from comment #6)
> Andrew, how does this patch work for you?  This also adds js::IterateGrayObjectsUnderCC.

Thanks, I updated the patch in bug 1277036 to use the function. I'm not sure how well it really works, because I think before I can call that code, I'm hitting an assertion in js::ZoneGlobalsAreAllGray(), because it is calling UnmarkGray during CC. (Of course, it is pretty bogus to call unmark gray because we're trying to figure out if the global is gray!) I'll try fixing that up and see how it goes.
Comment on attachment 8798840 [details] [diff] [review]
bug1306250-iterate-gray v2

With my patch in bug 1309664, I am able to successfully run zone merging CCs.
Attachment #8798840 - Flags: feedback?(continuation) → feedback+
Comment on attachment 8798840 [details] [diff] [review]
bug1306250-iterate-gray v2

Great news :)
Attachment #8798840 - Flags: review?(sphink)
Comment on attachment 8798840 [details] [diff] [review]
bug1306250-iterate-gray v2

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

::: js/src/jsfriendapi.h
@@ +528,5 @@
>  extern JS_FRIEND_API(void)
>  IterateGrayObjects(JS::Zone* zone, GCThingCallback cellCallback, void* data);
>  
> +/**
> + * Invoke cellCallback on every gray JS_OBJECT in the given zone while cycle

Strange capitalization of JSObject.
Attachment #8798840 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62817c443347
Iterate gray objects without evicting the nursery r=sfink
https://hg.mozilla.org/mozilla-central/rev/62817c443347
https://hg.mozilla.org/mozilla-central/rev/25ec37510c7f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.