js::DumpHeap() shouldn't call UnmarkGray on the cells it is iterating over

RESOLVED FIXED in Firefox 54



JavaScript: GC
a year ago
a year ago


(Reporter: mccr8, Assigned: jonco)


Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 affected, firefox54 fixed)



(1 attachment)



a year ago
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.

Comment 1

a year 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)

Comment 2

a year 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.

Comment 3

a year ago
Created attachment 8839184 [details] [diff] [review]

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 on attachment 8839184 [details] [diff] [review]

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.


@@ +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+

Comment 5

a year ago
(In reply to Steve Fink [:sfink] [:s:] from comment #4)
Good idea, I'm going with IterateHeapUnbarriered and IterateHeapUnbarrieredForZone.

Comment 6

a year ago
Pushed by jcoppeard@mozilla.com:
Avoid triggering read barrier in DumpHeap and when collecting stats r=sfink

Comment 7

a year ago
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 8

a year 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.