1277036 - Re-enable the UsefulToMergeZones implementation in XPCJSRuntime

Status: RESOLVED WONTFIX

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See discussion starting at bug 1276112 comment 16.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Implement XPCJSContext::UsefulToMergeZones again.

MozReview-Commit-ID: Dq73qiJ0f87

Comment 2

[Andrew McCreight [:mccr8]]

I started looking at this again. With the attached patch, we hit an assertion very quickly. nsCycleCollector::MarkRoots() now has an AutoAssertOnGC at the start of it (added in bug 1004276 which landed in 51), and the traverse function for zones makes a call to js::IterateGrayObjects(), which can trigger a minor GC, setting off the assertion in MarkRoots() (the minor GC was added in bug 945250).

The comment on ZoneCellIter says "The JSObject case needs to GC, or more precisely to empty the nursery and clear out the store buffer, so that it can see all objects to iterate over (the nursery is not iterable) and remove the possibility of having pointers from the store buffer to data hanging off stuff we're iterating over that we are going to delete.". For the former part, we only want to see gray objects, and there should be no gray objects in the nursery, so I think that's not an issue. I don't understand the latter part, but this particular callback the CC uses does not destroy anything, so it doesn't seem like an issue.

Maybe we could get away without the minor GC, but it doesn't look like there's any way to call into this method.

One possible work around would be to always run a minor GC before we start the CC. I worry a little what this might do to the CC pause times, but maybe that's not a big issue. Also, the minor GC would have been be placed carefully to avoid reentrance if it ends up triggering a major GC (assuming that is a possibility). Passing along the "empty" token into the iterator that runs much later would also be a pain.

Jon, do you have any ideas here? (I'd as sfink, but it looks like he's on PTO.)

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #2)
I'll write a patch to do this without evicting the nursery first.

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Jon Coppeard (:jonco) from comment #3)
> I'll write a patch to do this without evicting the nursery first.

Thanks! You should probably spin that off into a new bug blocking this one, as the patch here had some other issues with OOMs last time bz tried to land it, so it may not be ready to go even with your fix.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Implement XPCJSContext::UsefulToMergeZones again.

MozReview-Commit-ID: Dq73qiJ0f87

Comment 6

[Andrew McCreight [:mccr8]]

The XP gl mochitest issue seems okay. I think there has been some recent work to make that better:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=db023984a245e6be7dc3fcf532665875b8001223
But it does look like it is somehow causing bug 1262224 about half of the time.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Implement XPCJSContext::UsefulToMergeZones again.

Updated to call IterateGrayObjectsUnderCC.

Comment 8

[Andrew McCreight [:mccr8]]

I think heuristics like this that we can't run all of the time are too difficult to get right, so I'm going to close this for now. Kind of a shame, as it was a big speedup when it worked.