Closed Bug 1277036 Opened 5 years ago Closed 4 years ago

Re-enable the UsefulToMergeZones implementation in XPCJSRuntime

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox49 --- affected

People

(Reporter: bzbarsky, Assigned: mccr8)

References

(Depends on 1 open bug)

Details

(Whiteboard: btpp-backlog)

Attachments

(1 file, 2 obsolete files)

See discussion starting at bug 1276112 comment 16.
Depends on: 1276112
Whiteboard: bttp-backlog
Whiteboard: bttp-backlog → btpp-backlog
MozReview-Commit-ID: Dq73qiJ0f87
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.)
Flags: needinfo?(jcoppeard)
(In reply to Andrew McCreight [:mccr8] from comment #2)
I'll write a patch to do this without evicting the nursery first.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
(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.
Depends on: 1306250
MozReview-Commit-ID: Dq73qiJ0f87
Attachment #8794904 - Attachment is obsolete: true
Depends on: 1309664
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.
Assignee: jcoppeard → continuation
Depends on: 1262224
Updated to call IterateGrayObjectsUnderCC.
Attachment #8799994 - Attachment is obsolete: true
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.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.