I figured out why I was able to reproduce the leak in bug 1336811 so easily: when you take a GC log using DumpHeap, it calls unmark gray on everything! That is going to make it hard to investigate any leaks. This is a regression from a patch landed for bug 1322420, but I don't see that commit linked in the bug, so I'm not sure which branches it is on. https://hg.mozilla.org/mozilla-central/rev/4e919ac282a4b42906d8695a0e1671e8ec2a044c
I can back that out locally, but somebody should figure out what the right thing to do about it is, and also what the right bug number is for the patch.
It's the fix for bug 1322971, which I mistakenly landed with the wrong bug number. I guess for DumpHeap we really don't want unmark everything. I'll take a look.
Created attachment 8839184 [details] [diff] [review] bug1340597-dump-heap-barrier The IterateZone(s)CompartmentsArenasCells functions are only used for DumpHeap and collecting memory statistics. I made these use an iterator that doesn't trigger a read barrier and appended 'Unbarriered' to their names. (I considered renaming them to IterateZoneContentsUnbarriered or something because it's getting a bit ridiculous -- let me know if you think that's a good idea.)
Assignee: nobody → jcoppeard
Attachment #8839184 - Flags: review?(sphink)
Comment on attachment 8839184 [details] [diff] [review] bug1340597-dump-heap-barrier Review of attachment 8839184 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsgc.h @@ +1019,5 @@ > * every compartment, |arenaCallback| on every in-use arena, and |cellCallback| > * on every in-use cell in the GC heap. > + * > + * Note that no read barrier is triggered on the cells passed to cellCallback, > + * so no these pointers must not escape the callback. s/no// @@ +1024,3 @@ > */ > extern void > +IterateZonesCompartmentsArenasCellsUnbarriered(JSContext* cx, void* data, Or should this be IterateZoneGroupsZonesCompartmentsArenasCellsUnbarriered? :-) Actually no, because it'd probably be within one zone group. Do we really need the intervening layers? I'm thinking IterateCellsUnbarriered or IterateAllCellsUnbarriered or perhaps (now or later) IterateZoneGroupCellsUnbarriered. Although I can see your point that you're not just iterating over the cells, you're also doing callbacks for zones and compartments and arenas. So yes, I think I'll go for either your IterateZoneContentsUnbarriered or IterateHeapUnbarriered.
Attachment #8839184 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #4) Good idea, I'm going with IterateHeapUnbarriered and IterateHeapUnbarrieredForZone.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4e84832765 Avoid triggering read barrier in DumpHeap and when collecting stats r=sfink
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
If you could uplift this to Aurora, that would be good, in the unlikely chance we have to investigate a leak there. (Beta is almost out the door, so it isn't needed there.)
status-firefox51: --- → wontfix
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox-esr45: --- → wontfix
You need to log in before you can comment on or make changes to this bug.