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

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Andrew McCreight [:mccr8]]

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

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Jon Coppeard (:jonco)]

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

[Jon Coppeard (:jonco)]

Attachment: 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.)

Comment 4

[Steve Fink [:sfink] [:s:]]

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.

Comment 5

[Jon Coppeard (:jonco)]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4e84832765
Avoid triggering read barrier in DumpHeap and when collecting stats r=sfink

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b4e84832765

Comment 8

[Andrew McCreight [:mccr8]]

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.)