1306250 - Allow iterating gray objects without evicting the nursery

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Evicting the nursery is unnecessary because nursery objects cannot be gray.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1306250-iterate-gray

This just uses our internal iterator classes to get around evicting the nursery first.

Comment 2

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

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.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1306250-iterate-gray

You're right, making initForTenuredIteration is a much better approach.

I added a specific iterator subclass too.

Comment 4

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

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. :-)

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1306250-iterate-gray v2

Andrew, how does this patch work for you?  This also adds js::IterateGrayObjectsUnderCC.

Comment 7

[Andrew McCreight [:mccr8]]

(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 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

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

\o/

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 8798840 [details] [diff] [review]
bug1306250-iterate-gray v2

Great news :)

Comment 11

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

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62817c443347
Iterate gray objects without evicting the nursery r=sfink

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25ec37510c7f
Fix opt build bustage r=me

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62817c443347
https://hg.mozilla.org/mozilla-central/rev/25ec37510c7f