Closed
Bug 1340597
Opened 7 years ago
Closed 7 years ago
js::DumpHeap() shouldn't call UnmarkGray on the cells it is iterating over
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: mccr8, Assigned: jonco)
References
Details
Attachments
(1 file)
11.85 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•7 years ago
|
||
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.
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
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
Flags: needinfo?(jcoppeard)
Attachment #8839184 -
Flags: review?(sphink)
Comment 4•7 years ago
|
||
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+
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #4) Good idea, I'm going with IterateHeapUnbarriered and IterateHeapUnbarrieredForZone.
Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4e84832765 Avoid triggering read barrier in DumpHeap and when collecting stats r=sfink
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8b4e84832765
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Reporter | ||
Comment 8•7 years ago
|
||
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.
Description
•