Closed Bug 1359342 Opened 3 years ago Closed 2 years ago

Investigate simplifying finalisation check to remove allocatedDuringIncremental check

Categories

(Core :: JavaScript: GC, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

(Keywords: triage-deferred)

Attachments

(4 files, 5 obsolete files)

There's a few places in the engine where we check if a GC thing is dead by doing:

  !thing->isMarked() && !thing->arena()->allocatedDuringIncremental

We could simplify this if we set all the mark flags for any arenas allocated during an incremental GC when the arena is allocated.  We'd have make sure this doesn't affect what happens when we mark the contents of the arena later though.
Whoa... so the stuff in the arena would be "marked but not allocated"?

Would we need a list of arenas allocated during iGC so we could scan through them to clear the unallocated mark bits when the iGC ends? Or are they already consecutive in an existing list? (Or are there few enough that we can scan them all?)

It sounds like it'd work, though.
> Or are they already consecutive in an existing list?

That's what Arena::auxNextLink [1] is for, right? Among other things, anyway.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/gc/Heap.h#524
(In reply to Steve Fink [:sfink] [:s:] from comment #1)
> Whoa... so the stuff in the arena would be "marked but not allocated"?

Right.  But we shouldn't be checking the mark state of unallocated cells so nothing should see this.

> That's what Arena::auxNextLink [1] is for, right? 

Yes, we already have them in a list.

Having said all this... I remember mentioning something like this to Bill ages ago and he was pretty sceptical about setting the mark bits on cells outside of marking.
Keywords: triage-deferred
Priority: -- → P3
Assignee: nobody → allstars.chh
Hi Jonco
I'd like to help to fix this bug, but can you give me more information here?

I've read the comment that what the flag is for in its original bug,
https://bugzilla.mozilla.org/show_bug.cgi?id=641025#c7, rule 2
"2. Any object that is allocated during incremental GC must start out marked."

but not exactly sure how we plan to remove this flag.

(In reply to Jon Coppeard (:jonco) from comment #0)
>   !thing->isMarked() && !thing->arena()->allocatedDuringIncremental
> 
This is from the IsMarkedInternalCommon, right?
https://searchfox.org/mozilla-central/source/js/src/gc/Marking.cpp#3195

> We could simplify this if we set all the mark flags for any arenas allocated
> during an incremental GC when the arena is allocated.
But don't we already set those flags if the arena allocation happens in incremental GC?
https://searchfox.org/mozilla-central/source/js/src/gc/Allocator.cpp#390

> We'd have make sure
> this doesn't affect what happens when we mark the contents of the arena
> later though.
Can you be more specific on this part?

Thanks
(In reply to Yoshi Huang [:allstars.chh] from comment #4)
> Hi Jonco
> I'd like to help to fix this bug, but can you give me more information here?
> 
> I've read the comment that what the flag is for in its original bug,
> https://bugzilla.mozilla.org/show_bug.cgi?id=641025#c7, rule 2
> "2. Any object that is allocated during incremental GC must start out
> marked."
> 
> but not exactly sure how we plan to remove this flag.
> 
> (In reply to Jon Coppeard (:jonco) from comment #0)
> >   !thing->isMarked() && !thing->arena()->allocatedDuringIncremental
> > 
> This is from the IsMarkedInternalCommon, right?
> https://searchfox.org/mozilla-central/source/js/src/gc/Marking.cpp#3195

I think there's another one in js::gc::IsAboutToBeFinalizedDuringSweep.

> > We could simplify this if we set all the mark flags for any arenas allocated
> > during an incremental GC when the arena is allocated.
> But don't we already set those flags if the arena allocation happens in
> incremental GC?
> https://searchfox.org/mozilla-central/source/js/src/gc/Allocator.cpp#390

That's setting the single flag on the arena. Jon's talking about bulk-setting all of the mark bits black when an arena is allocated during incremental GC.

> > We'd have make sure
> > this doesn't affect what happens when we mark the contents of the arena
> > later though.
> Can you be more specific on this part?

Because all of these cells would be "pre-marked", there is a concern that we might skip some necessary operation that happens during marking because we'd see that it is marked already. Every other live cell would be marked once (as in, its mark bit would transition from 0->1); these live cells would not (they would start out at 1, so would never have a 0->1 transition.)
(In reply to Yoshi Huang [:allstars.chh] from comment #4)
I wrote a reply to this yesterday but I seems I didn't post it... sorry about that!

As Steve said, the idea is to pre-mark every cell in an arena allocated during incremental GC when the arena is allocated.  If we do this we should be able to get rid of the flag on the Arena.  But, I'm not 100% sure this will work.  You're welcome to try it out though.
Thanks for Steve's and Jon's comments, I am trying your suggestions now, will request for your suggestions if I have further questions.
Status: NEW → ASSIGNED
So far I have to use 
arena->chunk()->bitmap.markBlack(reinterpret_cast<TenuredCell*>(arena->address()));
to mark the bit,

and use 
arena->chunk()->bitmap.isMarkedBlack(reinterpret_cast<TenuredCell*>(arena->address()))
to check the area is marked or not.

Is there a simpler way to do this?
(In reply to Yoshi Huang [:allstars.chh] from comment #8)
You need to iterate over all the cells in an arena and use TenuredCell::markBlack() to mark each of them black.

You can do this by starting at an offset of Arena::firstThingOffset() from the start of the arena and then incrementing your pointer by Arena::thingSize() bytes.  ArenaCellIterImpl does this for example.
(In reply to Jon Coppeard (:jonco) from comment #9)
Thanks, Jonco.

Now I have another question,

For example I've marked the cells in the area with Black in GCRuntime::arenaAllocatedDuringGC
https://searchfox.org/mozilla-central/source/js/src/gc/Allocator.cpp#387

But later in GCMarker::markDelayedChildren
https://searchfox.org/mozilla-central/source/js/src/gc/Marking.cpp#2563
or 
GCRuntime::endSweepingSweepGroup
https://searchfox.org/mozilla-central/source/js/src/jsgc.cpp#5691

original code will reset the flag

What should I do here?

Because now I got assertion 

1307303 GECKO(31528) | Found black to gray edge to gray ObjectGroup 0x7fd00cdefa30, arena 0x7fd00cdef000$                                                                                                    
1307304 GECKO(31528) |   from black Object PageTransitionEvent 0x7fd00cb564c0, arena 0x7fd00cb56000 (compartment 0x7fd00e184800) group edge$$
1307305 GECKO(31528) |   from gray Object PageTransitionEventPrototype 0x7fd00cd7fc40, arena 0x7fd00cd7f000 (compartment 0x7fd00e184800) object slot edge$$
1307306 GECKO(31528) |   from gray Object Window 0x7fd00cea06a0, arena 0x7fd00cea0000 (compartment 0x7fd00e184800) protoAndIfaceCache[i] edge$$
1307307 GECKO(31528) |   from gray ObjectGroup 0x7fd00cdef0a0, arena 0x7fd00cdef000 group_global edge$$
1307308 GECKO(31528) |   from gray Object Function 0x7fd00cb04180, arena 0x7fd00cb04000 (compartment 0x7fd00e184800) group edge$$
1307309 GECKO(31528) |   from gray Shape 0x7fd00cb01ee8, arena 0x7fd00cb01000 setter edge$$
1307310 GECKO(31528) |   from gray Object Object 0x7fd00cb649a0, arena 0x7fd00cb64000 (compartment 0x7fd00e184800) shape edge$$
1307311 GECKO(31528) |   from gray Object HTMLDocument 0x7fd00cdebe20, arena 0x7fd00cdeb000 (compartment 0x7fd00e184800) Shadowing DOM proxy expando edge$$
1307312 GECKO(31528) |   from root Preserved wrapper
....
GECKO(31528) | Assertion failure: js::CheckGrayMarkingState(mJSRuntime), at /home/allstars/src/gecko-dev/xpcom/base/CycleCollectedJSRuntime.cpp:1212

The black cells ObjectGroup(0x7fd00cdefa30) and PageTransitionEvent(0x7fd00cb564c0) have been marked black in GCRuntime::arenaAllocatedDuringGC
The question is now should I clear the color of the cell in GCMarker::markDelayedChildren/GCRuntime::endSweepingSweepGroup ?

Thanks
(In reply to Yoshi Huang [:allstars.chh] from comment #10)
Can you post your patch?

> But later in GCMarker::markDelayedChildren
> https://searchfox.org/mozilla-central/source/js/src/gc/Marking.cpp#2563
> or 
> GCRuntime::endSweepingSweepGroup
> https://searchfox.org/mozilla-central/source/js/src/jsgc.cpp#5691
> 
> original code will reset the flag

We shouldn't set this flag now, so we can remove the code to reset it.

> The black cells ObjectGroup(0x7fd00cdefa30) and
> PageTransitionEvent(0x7fd00cb564c0) have been marked black in
> GCRuntime::arenaAllocatedDuringGC
> The question is now should I clear the color of the cell in
> GCMarker::markDelayedChildren/GCRuntime::endSweepingSweepGroup ?

No, we shouldn't change the color of the cell again.

Can you work out where this object group is coming from?  We shouldn't be creating a new object with a gray group.
(In reply to Jon Coppeard (:jonco) from comment #11)
> (In reply to Yoshi Huang [:allstars.chh] from comment #10)
> Can you post your patch?
>
I've uploaded my current WIP patch.

> > The black cells ObjectGroup(0x7fd00cdefa30) and
> > PageTransitionEvent(0x7fd00cb564c0) have been marked black in
> > GCRuntime::arenaAllocatedDuringGC
> > The question is now should I clear the color of the cell in
> > GCMarker::markDelayedChildren/GCRuntime::endSweepingSweepGroup ?
> 
> Can you work out where this object group is coming from?  We shouldn't be
> creating a new object with a gray group.

The marking is done when allocating the 'arena', I think when the time the marking is done, the objects are not allocated yet.
Comment on attachment 8936774 [details] [diff] [review]
WIP patch

Review of attachment 8936774 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a great start.

I think you should leave all the code related to manipulating the Arena::allocatedDuringIncremental flag, and remove it in a separate patch.  It just won't be used any more.

If something is marked gray it must have lived through at least one GC, since GC is the only thing that can mark soemthing gray.

::: js/src/gc/Allocator.cpp
@@ +395,1 @@
>          marker.delayMarkingArena(arena);

Please remove this call to delayMarkingArena() now we're marking the cells black here.

@@ +401,2 @@
>          arena->setNextAllocDuringSweep(arenasAllocatedDuringSweep);
>          arenasAllocatedDuringSweep = arena;

Ditto, please remove this code to push the arena onto the areansAllocatedDuringSweep list.
Attachment #8936774 - Flags: feedback+
Hi!

I received some log parsing backlog alerts for Treeherder this morning. Looking into New Relic slow transaction traces I see this try push created logs that were in the order of 490MB compressed, 12 GB uncompressed!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9627aa74b1083fd2dab6f4f39fae24fcd9ebbd2

In the future Treeherder plans to skip parsing logs that are so large, however in the meantime if it was possible to reduce the number of such jobs triggered when using verbose logging, it would help reduce the impact on other users of Treeherder :-)
Comment on attachment 8936774 [details] [diff] [review]
WIP patch

Review of attachment 8936774 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Jonco

::: js/src/gc/Allocator.cpp
@@ +395,4 @@
>          marker.delayMarkingArena(arena);
>      } else if (zone->isGCSweeping()) {
> +        for (ArenaFreeSpanCellIter iter(arena); !iter.done(); iter.next()) {
> +            iter.getCell()->markBlack();

When the arena is coming from ArenaLists
https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/js/src/gc/Allocator.cpp#348

In this case, the arena is not empty, and even more, the cells in this area sometimes have already been marked black or gray.

I shouldn't mark those cells which are already marked, especially those gray cells, am I right?

@@ +401,2 @@
>          arena->setNextAllocDuringSweep(arenasAllocatedDuringSweep);
>          arenasAllocatedDuringSweep = arena;

Also, do we really need to remove marker.delayMarkingArena(arena); and arena->setNextAllocDuringSweep ?

Because later when GCMarker::markDelayedChildren or GCRuntime::endSweepingSweepGroup.
The original code resets the flag to 0. 
So later if there's a cell allocated in this arena, since the flag is 0, the cell won't be marked.
However, now with my patch, the cell will be marked even it's allocated after GCMarker::markDelayedChildren or GCRuntime::endSweepingSweepGroup is called.

I am not sure if it's safe to remove these code here.
(In reply to Yoshi Huang [:allstars.chh] from comment #16)
> When the arena is coming from ArenaLists
> https://searchfox.org/mozilla-central/rev/
> 03877052c151a8f062eea177f684a2743cd7b1d5/js/src/gc/Allocator.cpp#348
> 
> In this case, the arena is not empty, and even more, the cells in this area
> sometimes have already been marked black or gray.
>
> I shouldn't mark those cells which are already marked, especially those gray
> cells, am I right?

That's right, don't mark anything that's already been marked.  You should probably assert that the cell is unmarked.

Oh, the problem is that ArenaFreeSpanCellIter iterates over all cells in the arena, not just the free ones.  Can you make an iterator that just iterates the free cells instead?

> @@ +401,2 @@
> >          arena->setNextAllocDuringSweep(arenasAllocatedDuringSweep);
> >          arenasAllocatedDuringSweep = arena;
> 
> Also, do we really need to remove marker.delayMarkingArena(arena); and
> arena->setNextAllocDuringSweep ?

I think so.

> Because later when GCMarker::markDelayedChildren or
> GCRuntime::endSweepingSweepGroup.
> The original code resets the flag to 0. 
> So later if there's a cell allocated in this arena, since the flag is 0, the
> cell won't be marked.

I don't follow this part.  Why does marking the cell depend on this flag?  This should depend on the zone's GC state.  We shouldn't need to set the Arena::allocatedDuringIncremental flag now at all.
(In reply to Jon Coppeard (:jonco) from comment #17)
> Oh, the problem is that ArenaFreeSpanCellIter iterates over all cells in
> the arena, not just the free ones.  Can you make an iterator that just
> iterates the free cells instead?
> 
I'd like to confirm the naming, actually I am not sure the naming is correct or not.
When we say 'Cell', it means the cell is already allocated, right?
will 'free cell' cause some confusion? or there's a better name for this?

> > Because later when GCMarker::markDelayedChildren or
> > GCRuntime::endSweepingSweepGroup.
> > The original code resets the flag to 0. 
> > So later if there's a cell allocated in this arena, since the flag is 0, the
> > cell won't be marked.
> 
> I don't follow this part.  Why does marking the cell depend on this flag? 
> This should depend on the zone's GC state.  We shouldn't need to set the
> Arena::allocatedDuringIncremental flag now at all.

For example in the function IsMarkedBlack

bool
IsMarkedBlack(JSObject* obj)
{
    return obj->isMarkedBlack() ||
           (obj->isTenured() && obj->asTenured().arena()->allocatedDuringIncremental);
}
https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/js/src/gc/Barrier.cpp#34

Suppose the obj is allocated on the arena, which is the arena passed to arenaAllocatedDuringGC, and when the obj is allocated, the flag, allocatedDuringIncremental of the arena has been reset to 0.

So in the original behavior, the function returns the same value of obj->isMarkedBlack();

But now with my patch applied, the same object, will be allocated in the 'free' cell, and hence will be marked black.
So right this function will always return true.

It looks to me that it causes different behavior _after_ GCMarker::markDelayedChildren or GCRuntime::endSweepingSweepGroup is called. That's my concern of this.
(In reply to Jon Coppeard (:jonco) from comment #17)
> That's right, don't mark anything that's already been marked.  You should
> probably assert that the cell is unmarked.
> 
> Oh, the problem is that ArenaFreeSpanCellIter iterates over all cells in
> the arena, not just the free ones.  Can you make an iterator that just
> iterates the free cells instead?
> 
I didn't follow this part, 
you mean I should only iterate through 'free cell'? so if there's an allocated cell but it isn't marked yet, I should NOT mark it?

Currently I still use ArenaFreeSpanCellIter, however, I only mark the cell if it's marked yet.
But your comment seems to say I should only mark those free cells.
(In reply to Yoshi Huang [:allstars.chh] from comment #18)

> It looks to me that it causes different behavior _after_
> GCMarker::markDelayedChildren or GCRuntime::endSweepingSweepGroup is called.
> That's my concern of this.

Oh right, thanks for explaining that, I see now.  This is a good point.  Yes the behaviour is different, but I don't think it matters.  We clear the mark bits before GC and don't generally inspect them between GCs.

> I'd like to confirm the naming, actually I am not sure the naming is correct
> or not.
> When we say 'Cell', it means the cell is already allocated, right?
> will 'free cell' cause some confusion? or there's a better name for this?

I think the name is fine.

Another way thought that might be easier though could be to mark only the cells in arena->getFirstFreeSpan() in ArenaLists::allocateFromArenaInner().  You could also clear the mark bits for remaining free cells in ArenaLists::freeLists() at the end of the GC, which would fix your concern above.
Hi Jonco,
Right now I'll get crash in JIT part randomly

Can you help to give me some suggestions here?


0x00007fffe4f59ae0 in js::jit::JitCode::raw (this=0xedededededededed) at /home/allstars/src/gecko-dev/js/src/jit/IonCode.h:107
107	        return code_;
(gdb) bt
#0  0x00007fffe4f59ae0 in js::jit::JitCode::raw() const (this=0xedededededededed) at /home/allstars/src/gecko-dev/js/src/jit/IonCode.h:107
#1  0x00007fffe4f59b38 in js::jit::JitCode::FromExecutable(unsigned char*) (buffer=0x3ca6fb94df30 '\355' <repeats 199 times>, <incomplete sequence \355>...)
    at /home/allstars/src/gecko-dev/js/src/jit/IonCode.h:153
#2  0x00007fffe534174e in js::jit::Assembler::CodeFromJump(js::jit::JitCode*, unsigned char*) (code=0x7fffd0faf290, jump=0x3ca6fc7a390b "H\205\322\017\204\345\036")
    at /home/allstars/src/gecko-dev/js/src/jit/x64/Assembler-x64.cpp:299
#3  0x00007fffe53417d3 in js::jit::Assembler::TraceJumpRelocations(JSTracer*, js::jit::JitCode*, js::jit::CompactBufferReader&) (trc=0x7fffffffb7b8, code=0x7fffd0faf290, reader=...)
    at /home/allstars/src/gecko-dev/js/src/jit/x64/Assembler-x64.cpp:307
#4  0x00007fffe504b997 in js::jit::JitCode::traceChildren(JSTracer*) (this=0x7fffd0faf290, trc=0x7fffffffb7b8) at /home/allstars/src/gecko-dev/js/src/jit/Ion.cpp:809
#5  0x00007fffe5ad31f3 in TraceChildrenFunctor::operator()<js::jit::JitCode>(JSTracer*, void*) (this=0x7fffffffb660, trc=0x7fffffffb7b8, thingArg=0x7fffd0faf290)
    at /home/allstars/src/gecko-dev/js/src/gc/Tracer.cpp:119
#6  0x00007fffe5ac9280 in JS::DispatchTraceKindTyped<TraceChildrenFunctor, JSTracer*&, void*&>(TraceChildrenFunctor, JS::TraceKind, JSTracer*&, void*&) (f=..., traceKind=JS::TraceKind::JitCode, args#0=@0x7fffffffb688: 0x7fffffffb7b8, args#1=@0x7fffffffb680: 0x7fffd0faf290) at /home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/js/TraceKind.h:196
#7  0x00007fffe5ab74d2 in js::TraceChildren(JSTracer*, void*, JS::TraceKind) (trc=0x7fffffffb7b8, thing=0x7fffd0faf290, kind=JS::TraceKind::JitCode)
    at /home/allstars/src/gecko-dev/js/src/gc/Tracer.cpp:128
#8  0x00007fffe5ab7453 in JS::TraceChildren(JSTracer*, JS::GCCellPtr) (trc=0x7fffffffb7b8, thing=Python Exception <class 'KeyError'> (7,): 
) at /home/allstars/src/gecko-dev/js/src/gc/Tracer.cpp:108
#9  0x00007fffe5ab9c91 in HeapCheckTracerBase::traceHeap(js::AutoLockForExclusiveAccess&) (this=0x7fffffffb7b0, lock=...) at /home/allstars/src/gecko-dev/js/src/gc/Verifier.cpp:567
#10 0x00007fffe5aba389 in CheckGrayMarkingTracer::check(js::AutoLockForExclusiveAccess&) (this=0x7fffffffb7b0, lock=...) at /home/allstars/src/gecko-dev/js/src/gc/Verifier.cpp:723
#11 0x00007fffe5aba50f in js::CheckGrayMarkingState(JSRuntime*) (rt=0x7fffd8516000) at /home/allstars/src/gecko-dev/js/src/gc/Verifier.cpp:743
#12 0x00007fffdddfd213 in mozilla::CycleCollectedJSRuntime::CheckGrayBits() const (this=0x7fffda810800) at /home/allstars/src/gecko-dev/xpcom/base/CycleCollectedJSRuntime.cpp:1231
#13 0x00007fffdde25f6e in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) (this=0x7ffff6a85ca0, aCCType=SliceCC, aManualListener=0x0)
    at /home/allstars/src/gecko-dev/xpcom/base/nsCycleCollector.cpp:3917
#14 0x00007fffdde256c4 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) (this=0x7ffff6a85ca0, aCCType=SliceCC, aBudget=..., aManualListener=0x0, aPreferShorterSlices=false) at /home/allstars/src/gecko-dev/xpcom/base/nsCycleCollector.cpp:3741
#15 0x00007fffdde2771c in nsCycleCollector_collectSlice(js::SliceBudget&, bool) (budget=..., aPreferShorterSlices=false) at /home/allstars/src/gecko-dev/xpcom/base/nsCycleCollector.cpp:4325
#16 0x00007fffdff202f9 in nsJSContext::RunCycleCollectorSlice(mozilla::TimeStamp) (aDeadline=...) at /home/allstars/src/gecko-dev/dom/base/nsJSEnvironment.cpp:156
(In reply to Yoshi Huang [:allstars.chh] from comment #21)
That 0xeded... value is due to JS_SWEPT_CODE_PATTERN and indicates that the JIT code is being accessed after it has been freed.

If you can reproduce the crash under rr you can put a watchpoint on this address and reverse-continue to see when the memory was freed.  That should help.

Otherwise you could test with the JITs disable (by passing --no-baseline --no-ion) to see if it's a JIT specific problem.

Also, can you post you current patch here and I'll have a quick look?
Attached patch 0001-bug-1359342.patch (obsolete) — Splinter Review
(In reply to Jon Coppeard (:jonco) from comment #22)
> (In reply to Yoshi Huang [:allstars.chh] from comment #21)
> That 0xeded... value is due to JS_SWEPT_CODE_PATTERN and indicates that the
> JIT code is being accessed after it has been freed.
> 
> If you can reproduce the crash under rr you can put a watchpoint on this
> address and reverse-continue to see when the memory was freed.  That should
> help.
> 
> Otherwise you could test with the JITs disable (by passing --no-baseline
> --no-ion) to see if it's a JIT specific problem.
> 
Thanks for suggestions, I'll try these.

> Also, can you post you current patch here and I'll have a quick look?
I only have one short change,
in arenaAllocatedDuringGC, zone->isGCSweeping()

+            // skip if the cell have already been marked.
+            if (iter.getCell()->isMarkedAny()) {
+                continue;
+            }
(In reply to Jon Coppeard (:jonco) from comment #22)
> If you can reproduce the crash under rr you can put a watchpoint on this
> address and reverse-continue to see when the memory was freed.  That should
> help.
> 

I reproduced it under rr, however reverse-conti still shows it was freed at the same function, js::jit::JitCode::raw.

Thread 1 received signal SIGSEGV, Segmentation fault.
0x00007f75ee61aae0 in js::jit::JitCode::raw (this=0xedededededededed) at /home/allstars/src/gecko-dev/js/src/jit/IonCode.h:107
107	        return code_;
(rr) bt
#0  0x00007f75ee61aae0 in js::jit::JitCode::raw() const (this=0xedededededededed) at /home/allstars/src/gecko-dev/js/src/jit/IonCode.h:107
#1  0x00007f75ee61ab38 in js::jit::JitCode::FromExecutable(unsigned char*) (buffer=0xa4aae291360 '\355' <repeats 199 times>, <incomplete sequence \355>...)
    at /home/allstars/src/gecko-dev/js/src/jit/IonCode.h:153
#2  0x00007f75eea0274e in js::jit::Assembler::CodeFromJump(js::jit::JitCode*, unsigned char*) (code=0x7f75d4c2b6c8, jump=0xa4aae49b532 "H\205\322\017\204\320\v")
    at /home/allstars/src/gecko-dev/js/src/jit/x64/Assembler-x64.cpp:299
#3  0x00007f75eea027d3 in js::jit::Assembler::TraceJumpRelocations(JSTracer*, js::jit::JitCode*, js::jit::CompactBufferReader&) (trc=0x7ffff93ec9b8, code=0x7f75d4c2b6c8, reader=...)
    at /home/allstars/src/gecko-dev/js/src/jit/x64/Assembler-x64.cpp:307
#4  0x00007f75ee70c997 in js::jit::JitCode::traceChildren(JSTracer*) (this=0x7f75d4c2b6c8, trc=0x7ffff93ec9b8) at /home/allstars/src/gecko-dev/js/src/jit/Ion.cpp:809
#5  0x00007f75ef1940d7 in TraceChildrenFunctor::operator()<js::jit::JitCode>(JSTracer*, void*) (this=0x7ffff93ec860, trc=0x7ffff93ec9b8, thingArg=0x7f75d4c2b6c8)
    at /home/allstars/src/gecko-dev/js/src/gc/Tracer.cpp:119
#6  0x00007f75ef18a164 in JS::DispatchTraceKindTyped<TraceChildrenFunctor, JSTracer*&, void*&>(TraceChildrenFunctor, JS::TraceKind, JSTracer*&, void*&) (f=..., traceKind=JS::TraceKind::JitCode, args#0=@0x7ffff93ec888: 0x7ffff93ec9b8, args#1=@0x7ffff93ec880: 0x7f75d4c2b6c8) at /home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/js/TraceKind.h:196
#7  0x00007f75ef1783b6 in js::TraceChildren(JSTracer*, void*, JS::TraceKind) (trc=0x7ffff93ec9b8, thing=0x7f75d4c2b6c8, kind=JS::TraceKind::JitCode)
    at /home/allstars/src/gecko-dev/js/src/gc/Tracer.cpp:128
#8  0x00007f75ef178337 in JS::TraceChildren(JSTracer*, JS::GCCellPtr) (trc=0x7ffff93ec9b8, thing=Traceback (most recent call last):
  File "/home/allstars/src/gecko-dev/js/src/gdb/mozilla/GCCellPtr.py", line 48, in to_string
    tipe = self.cache.mod_GCCellPtr.kind_to_type[int(kind)]
KeyError: 7

) at /home/allstars/src/gecko-dev/js/src/gc/Tracer.cpp:108
#9  0x00007f75ef17ab75 in HeapCheckTracerBase::traceHeap(js::AutoLockForExclusiveAccess&) (this=0x7ffff93ec9b0, lock=...) at /home/allstars/src/gecko-dev/js/src/gc/Verifier.cpp:567
#10 0x00007f75ef17b26d in CheckGrayMarkingTracer::check(js::AutoLockForExclusiveAccess&) (this=0x7ffff93ec9b0, lock=...) at /home/allstars/src/gecko-dev/js/src/gc/Verifier.cpp:723
#11 0x00007f75ef17b3f3 in js::CheckGrayMarkingState(JSRuntime*) (rt=0x7f75e1316000) at /home/allstars/src/gecko-dev/js/src/gc/Verifier.cpp:743
#12 0x00007f75e74be213 in mozilla::CycleCollectedJSRuntime::CheckGrayBits() const (this=0x7f75e3910800) at /home/allstars/src/gecko-dev/xpcom/base/CycleCollectedJSRuntime.cpp:1231
#13 0x00007f75e74e6f6e in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) (this=0x7f7600285ca0, aCCType=SliceCC, aManualListener=0x0)
    at /home/allstars/src/gecko-dev/xpcom/base/nsCycleCollector.cpp:3917
#14 0x00007f75e74e66c4 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) (this=0x7f7600285ca0, aCCType=SliceCC, aBudget=..., aManualListener=0x0, aPreferShorterSlices=false) at /home/allstars/src/gecko-dev/xpcom/base/nsCycleCollector.cpp:3741
#15 0x00007f75e74e871c in nsCycleCollector_collectSlice(js::SliceBudget&, bool) (budget=..., aPreferShorterSlices=false) at /home/allstars/src/gecko-dev/xpcom/base/nsCycleCollector.cpp:4325
#16 0x00007f75e95e12f9 in nsJSContext::RunCycleCollectorSlice(mozilla::TimeStamp) (aDeadline=...) at /home/allstars/src/gecko-dev/dom/base/nsJSEnvironment.cpp:1561
#17 0x00007f75e95e25f4 in CCRunnerFired(mozilla::TimeStamp) (aDeadline=...) at /home/allstars/src/gecko-dev/dom/base/nsJSEnvironment.cpp:1958
#18 0x00007f75e8bbe6c5 in std::_Function_handler<bool (mozilla::TimeStamp), bool (*)(mozilla::TimeStamp)>::_M_invoke(std::_Any_data const&, mozilla::TimeStamp&&) (__functor=..., __args#0=...)
    at /usr/include/c++/7/bits/std_function.h:302
#19 0x00007f75e75a50f5 in std::function<bool (mozilla::TimeStamp)>::operator()(mozilla::TimeStamp) const (this=0x7f75a219e1d8, __args#0=...) at /usr/include/c++/7/bits/std_function.h:706
#20 0x00007f75e75a41f4 in mozilla::IdleTaskRunner::Run() (this=0x7f75a219e190) at /home/allstars/src/gecko-dev/xpcom/threads/IdleTaskRunner.cpp:62
#21 0x00007f75e75d71c8 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7f75e4d563a0, aMayWait=false, aResult=0x7ffff93ed437) at /home/allstars/src/gecko-dev/xpcom/threads/nsThread.cpp:1039
#22 0x00007f75e75f8add in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7f75e4d563a0, aMayWait=false) at /home/allstars/src/gecko-dev/xpcom/threads/nsThreadUtils.cpp:510
#23 0x00007f75e7ec69ee in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7f75e4d26680, aDelegate=0x7f75e4d25200) at /home/allstars/src/gecko-dev/ipc/glue/MessagePump.cpp:97
#24 0x00007f75e7e172a2 in MessageLoop::RunInternal() (this=0x7f75e4d25200) at /home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:326
#25 0x00007f75e7e17226 in MessageLoop::RunHandler() (this=0x7f75e4d25200) at /home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:319
#26 0x00007f75e7e171ea in MessageLoop::Run() (this=0x7f75e4d25200) at /home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:299
#27 0x00007f75eb7a3e96 in nsBaseAppShell::Run() (this=0x7f75dc6583c0) at /home/allstars/src/gecko-dev/widget/nsBaseAppShell.cpp:157
#28 0x00007f75ee1b5357 in nsAppStartup::Run() (this=0x7f75e4da3bf0) at /home/allstars/src/gecko-dev/toolkit/components/startup/nsAppStartup.cpp:288
#29 0x00007f75ee30a2af in XREMain::XRE_mainRun() (this=0x7ffff93ed7e0) at /home/allstars/src/gecko-dev/toolkit/xre/nsAppRunner.cpp:4709
#30 0x00007f75ee30ad12 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) (this=0x7ffff93ed7e0, argc=5, argv=0x7ffff93eeb58, aConfig=...)
    at /home/allstars/src/gecko-dev/toolkit/xre/nsAppRunner.cpp:4871
#31 0x00007f75ee30b00c in XRE_main(int, char**, mozilla::BootstrapConfig const&) (argc=5, argv=0x7ffff93eeb58, aConfig=...) at /home/allstars/src/gecko-dev/toolkit/xre/nsAppRunner.cpp:4963
#32 0x00007f75ee31e002 in mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) (this=0x7f760024e6a0, argc=5, argv=0x7ffff93eeb58, aConfig=...)
    at /home/allstars/src/gecko-dev/toolkit/xre/Bootstrap.cpp:49
#33 0x0000560c2dd2865c in do_main(int, char**, char**) (argc=5, argv=0x7ffff93eeb58, envp=0x7ffff93eeb88) at /home/allstars/src/gecko-dev/browser/app/nsBrowserApp.cpp:231
#34 0x0000560c2dd288ca in main(int, char**, char**) (argc=5, argv=0x7ffff93eeb58, envp=0x7ffff93eeb88) at /home/allstars/src/gecko-dev/browser/app/nsBrowserApp.cpp:304
(rr) p this
$1 = (const js::jit::JitCode * const) 0xedededededededed
(rr) watch -l this
Hardware watchpoint 1: -location this
(rr) rc
Continuing.

Thread 1 received signal SIGSEGV, Segmentation fault.
0x00007f75ee61aae0 in js::jit::JitCode::raw (this=0xedededededededed) at /home/allstars/src/gecko-dev/js/src/jit/IonCode.h:107
107	        return code_;
(rr) rc
Continuing.

Thread 1 hit Hardware watchpoint 1: -location this

Old value = (const js::jit::JitCode * const) 0xedededededededed
New value = (const js::jit::JitCode * const) 0xa4aae291360
0x00007f75ee61aad8 in js::jit::JitCode::raw (this=0xa4aae291360) at /home/allstars/src/gecko-dev/js/src/jit/IonCode.h:106
106	    uint8_t* raw() const {
(rr) bt
#0  0x00007f75ee61aad8 in js::jit::JitCode::raw() const (this=0xa4aae291360) at /home/allstars/src/gecko-dev/js/src/jit/IonCode.h:106
#1  0x00007f75ee61ab38 in js::jit::JitCode::FromExecutable(unsigned char*) (buffer=0xa4aae291360 '\355' <repeats 199 times>, <incomplete sequence \355>...)
    at /home/allstars/src/gecko-dev/js/src/jit/IonCode.h:153
#2  0x00007f75eea0274e in js::jit::Assembler::CodeFromJump(js::jit::JitCode*, unsigned char*) (code=0x7f75d4c2b6c8, jump=0xa4aae49b532 "H\205\322\017\204\320\v")
    at /home/allstars/src/gecko-dev/js/src/jit/x64/Assembler-x64.cpp:299
#3  0x00007f75eea027d3 in js::jit::Assembler::TraceJumpRelocations(JSTracer*, js::jit::JitCode*, js::jit::CompactBufferReader&) (trc=0x7ffff93ec9b8, code=0x7f75d4c2b6c8, reader=...)
    at /home/allstars/src/gecko-dev/js/src/jit/x64/Assembler-x64.cpp:307
#4  0x00007f75ee70c997 in js::jit::JitCode::traceChildren(JSTracer*) (this=0x7f75d4c2b6c8, trc=0x7ffff93ec9b8) at /home/allstars/src/gecko-dev/js/src/jit/Ion.cpp:809
#5  0x00007f75ef1940d7 in TraceChildrenFunctor::operator()<js::jit::JitCode>(JSTracer*, void*) (this=0x7ffff93ec860, trc=0x7ffff93ec9b8, thingArg=0x7f75d4c2b6c8)
    at /home/allstars/src/gecko-dev/js/src/gc/Tracer.cpp:119
#6  0x00007f75ef18a164 in JS::DispatchTraceKindTyped<TraceChildrenFunctor, JSTracer*&, void*&>(TraceChildrenFunctor, JS::TraceKind, JSTracer*&, void*&) (f=..., traceKind=JS::TraceKind::JitCode, args#0=@0x7ffff93ec888: 0x7ffff93ec9b8, args#1=@0x7ffff93ec880: 0x7f75d4c2b6c8) at /home/allstars/src/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/js/TraceKind.h:196
#7  0x00007f75ef1783b6 in js::TraceChildren(JSTracer*, void*, JS::TraceKind) (trc=0x7ffff93ec9b8, thing=0x7f75d4c2b6c8, kind=JS::TraceKind::JitCode)
    at /home/allstars/src/gecko-dev/js/src/gc/Tracer.cpp:128
#8  0x00007f75ef178337 in JS::TraceChildren(JSTracer*, JS::GCCellPtr) (trc=0x7ffff93ec9b8, thing=Traceback (most recent call last):
  File "/home/allstars/src/gecko-dev/js/src/gdb/mozilla/GCCellPtr.py", line 48, in to_string
    tipe = self.cache.mod_GCCellPtr.kind_to_type[int(kind)]
KeyError: 7

) at /home/allstars/src/gecko-dev/js/src/gc/Tracer.cpp:108
#9  0x00007f75ef17ab75 in HeapCheckTracerBase::traceHeap(js::AutoLockForExclusiveAccess&) (this=0x7ffff93ec9b0, lock=...) at /home/allstars/src/gecko-dev/js/src/gc/Verifier.cpp:567
#10 0x00007f75ef17b26d in CheckGrayMarkingTracer::check(js::AutoLockForExclusiveAccess&) (this=0x7ffff93ec9b0, lock=...) at /home/allstars/src/gecko-dev/js/src/gc/Verifier.cpp:723
#11 0x00007f75ef17b3f3 in js::CheckGrayMarkingState(JSRuntime*) (rt=0x7f75e1316000) at /home/allstars/src/gecko-dev/js/src/gc/Verifier.cpp:743
#12 0x00007f75e74be213 in mozilla::CycleCollectedJSRuntime::CheckGrayBits() const (this=0x7f75e3910800) at /home/allstars/src/gecko-dev/xpcom/base/CycleCollectedJSRuntime.cpp:1231
#13 0x00007f75e74e6f6e in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) (this=0x7f7600285ca0, aCCType=SliceCC, aManualListener=0x0)
    at /home/allstars/src/gecko-dev/xpcom/base/nsCycleCollector.cpp:3917
#14 0x00007f75e74e66c4 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) (this=0x7f7600285ca0, aCCType=SliceCC, aBudget=..., aManualListener=0x0, aPreferShorterSlices=false) at /home/allstars/src/gecko-dev/xpcom/base/nsCycleCollector.cpp:3741
#15 0x00007f75e74e871c in nsCycleCollector_collectSlice(js::SliceBudget&, bool) (budget=..., aPreferShorterSlices=false) at /home/allstars/src/gecko-dev/xpcom/base/nsCycleCollector.cpp:4325
#16 0x00007f75e95e12f9 in nsJSContext::RunCycleCollectorSlice(mozilla::TimeStamp) (aDeadline=...) at /home/allstars/src/gecko-dev/dom/base/nsJSEnvironment.cpp:1561
#17 0x00007f75e95e25f4 in CCRunnerFired(mozilla::TimeStamp) (aDeadline=...) at /home/allstars/src/gecko-dev/dom/base/nsJSEnvironment.cpp:1958
#18 0x00007f75e8bbe6c5 in std::_Function_handler<bool (mozilla::TimeStamp), bool (*)(mozilla::TimeStamp)>::_M_invoke(std::_Any_data const&, mozilla::TimeStamp&&) (__functor=..., __args#0=...)
    at /usr/include/c++/7/bits/std_function.h:302
#19 0x00007f75e75a50f5 in std::function<bool (mozilla::TimeStamp)>::operator()(mozilla::TimeStamp) const (this=0x7f75a219e1d8, __args#0=...) at /usr/include/c++/7/bits/std_function.h:706
#20 0x00007f75e75a41f4 in mozilla::IdleTaskRunner::Run() (this=0x7f75a219e190) at /home/allstars/src/gecko-dev/xpcom/threads/IdleTaskRunner.cpp:62
#21 0x00007f75e75d71c8 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7f75e4d563a0, aMayWait=false, aResult=0x7ffff93ed437) at /home/allstars/src/gecko-dev/xpcom/threads/nsThread.cpp:1039
#22 0x00007f75e75f8add in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7f75e4d563a0, aMayWait=false) at /home/allstars/src/gecko-dev/xpcom/threads/nsThreadUtils.cpp:510
#23 0x00007f75e7ec69ee in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7f75e4d26680, aDelegate=0x7f75e4d25200) at /home/allstars/src/gecko-dev/ipc/glue/MessagePump.cpp:97
#24 0x00007f75e7e172a2 in MessageLoop::RunInternal() (this=0x7f75e4d25200) at /home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:326
#25 0x00007f75e7e17226 in MessageLoop::RunHandler() (this=0x7f75e4d25200) at /home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:319
#26 0x00007f75e7e171ea in MessageLoop::Run() (this=0x7f75e4d25200) at /home/allstars/src/gecko-dev/ipc/chromium/src/base/message_loop.cc:299
#27 0x00007f75eb7a3e96 in nsBaseAppShell::Run() (this=0x7f75dc6583c0) at /home/allstars/src/gecko-dev/widget/nsBaseAppShell.cpp:157
#28 0x00007f75ee1b5357 in nsAppStartup::Run() (this=0x7f75e4da3bf0) at /home/allstars/src/gecko-dev/toolkit/components/startup/nsAppStartup.cpp:288
#29 0x00007f75ee30a2af in XREMain::XRE_mainRun() (this=0x7ffff93ed7e0) at /home/allstars/src/gecko-dev/toolkit/xre/nsAppRunner.cpp:4709
#30 0x00007f75ee30ad12 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) (this=0x7ffff93ed7e0, argc=5, argv=0x7ffff93eeb58, aConfig=...)
    at /home/allstars/src/gecko-dev/toolkit/xre/nsAppRunner.cpp:4871
#31 0x00007f75ee30b00c in XRE_main(int, char**, mozilla::BootstrapConfig const&) (argc=5, argv=0x7ffff93eeb58, aConfig=...) at /home/allstars/src/gecko-dev/toolkit/xre/nsAppRunner.cpp:4963
#32 0x00007f75ee31e002 in mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) (this=0x7f760024e6a0, argc=5, argv=0x7ffff93eeb58, aConfig=...)
    at /home/allstars/src/gecko-dev/toolkit/xre/Bootstrap.cpp:49
#33 0x0000560c2dd2865c in do_main(int, char**, char**) (argc=5, argv=0x7ffff93eeb58, envp=0x7ffff93eeb88) at /home/allstars/src/gecko-dev/browser/app/nsBrowserApp.cpp:231
#34 0x0000560c2dd288ca in main(int, char**, char**) (argc=5, argv=0x7ffff93eeb58, envp=0x7ffff93eeb88) at /home/allstars/src/gecko-dev/browser/app/nsBrowserApp.cpp:304
(In reply to Yoshi Huang [:allstars.chh] from comment #24)
> I reproduced it under rr, however reverse-conti still shows it was freed at
> the same function, js::jit::JitCode::raw.

Weird.  You might need to put the watchpoint on the actual address rather than |this|.  I don't know what it actually watches in that case.
Also, you need to work out where the swept code pattern is being read from, and put a watchpoint on that address.  If |this| is already 0xededed.... it's too late.  I imagine that's a few frames higher up.
Attached patch WIP patch (obsolete) — Splinter Review
This is my latest patch.

Jon, and ChiaHung
Thanks for your reviews and feedbacks.
Sorry I didn't finish this in time, and I might not have enough time to finish it.
But anyway still thanks for your great help. :D
Attachment #8942210 - Attachment is obsolete: true
Attached patch bug1359342-jit-barriers (obsolete) — Splinter Review
I found the source of the crashes above.

JitCode objects rely on the trace hook being called when they are allocated during incremental GC to mark certain stubs.  A read barrier is not used because these can be accessed off-thread.

This patch changes the way this works to the same approach taken for SIMD template objects.  We record the missing read barriers and ensure they are performed when we link the JitCode on the main thread.

This relies on the fact that we cancel Ion compliation at the beginning of a GC as well as when we start sweeping.  This ensure the read barrier happens in time - if we were to skip a barrier in the mark phase and then attempt to perform it in the sweep phase, the object in question could already have been swept.
Assignee: allstars.chh → jcoppeard
Attachment #8936774 - Attachment is obsolete: true
Attachment #8943516 - Attachment is obsolete: true
Attachment #8947922 - Flags: review?(nicolas.b.pierron)
Attached patch bug1359342-simplify-iatbf (obsolete) — Splinter Review
Work in progress patch.
Comment on attachment 8947922 [details] [diff] [review]
bug1359342-jit-barriers

I've found a simpler way of doing this that doesn't require these changes.
Attachment #8947922 - Flags: review?(nicolas.b.pierron)
This changes the way we handle allocations during incremental GC.

Currently it works like this:  Cells allocated during marking have their arena pushed onto a list and a flag set on their arena.  We later trace everything in that arena.  Cells allocated during sweeping are pushed onto a list and have a flag set on their arena.  When we want to check whether something is dying during sweeping (IsAboutToBeFinalized) we check the arena flag as well as checking whether the cell is marked.

The idea here is that everything allocated during incremental GC is marked black to start with.  Then we can remove this extra machinery and state we have to maintain.

This works because of the snapshot at the beginning approach - everything we write into a new cell must itself end up marked black, so we don't need to trace the contents of newly allocated cells.

Having said that, this bug did show up one place where we relied on this behaviour for another reason - deferred read barriers for Ion compilation.  I think it's better to have this happen explicitly where we actually need it however.

What the patch does is to pre-mark all free cells in arenas allocated during incremental GC, so they can still be allocated by the JIT or whatever.  It also unmarks any unallocated free cells at the end of GC.  I think this may be unnecessary but I'm leaving it in to start with.  There are assertions that this all happens correctly.

This should be win during marking because we don't have to trace new arenas.  It's slightly slower during sweeping because we have to mark.  It should be possible to optimise the pre-marking by using memset rather than a for-loop but I've left that for now.

I renamed ArenaLists::purge() to clearFreeLists() for clarity, and added setFreeList() and clearFreeLists().
Attachment #8953020 - Flags: review?(sphink)
Attachment #8947923 - Attachment is obsolete: true
Attachment #8947922 - Attachment is obsolete: true
I didn't find a better way of doing this.

This adds deferred read barriers to JitCompartment stubs in a similar way to that done for SIMD template objects, and does some refactoring.  So we keep a bitmask of stubs we need to barrier and do this when we link the code.
Attachment #8953136 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8953020 [details] [diff] [review]
bug1359342-simplify-iatbf v2

Review of attachment 8953020 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this feels like an improvement.

::: js/src/gc/Allocator.cpp
@@ +477,5 @@
> +        for (ArenaFreeCellIter iter(arena); !iter.done(); iter.next()) {
> +            TenuredCell* cell = iter.getCell();
> +            MOZ_ASSERT(!cell->isMarkedAny());
> +            cell->markBlack();
> +        }

I wonder what the optimizer is able to do with this. Probably not much. Oh well.

::: js/src/gc/ArenaList-inl.h
@@ +240,5 @@
> +void
> +js::gc::ArenaLists::setFreeList(AllocKind i, FreeSpan* span)
> +{
> +    auto old = freeList(i);
> +#ifdef DEBUG

The assignment to old should also be within DEBUG. (This probably comes from before you made freeList(), so you were keeping a pointer or reference to the freelist and reusing it.)

@@ +251,5 @@
> +void
> +js::gc::ArenaLists::clearFreeList(AllocKind i)
> +{
> +    auto old = freeList(i);
> +#ifdef DEBUG

same here

::: js/src/gc/GC.cpp
@@ +523,5 @@
> +Arena::unmarkFreeCells()
> +{
> +    for (ArenaFreeCellIter iter(this); !iter.done(); iter.next()) {
> +        TenuredCell* cell = iter.getCell();
> +        MOZ_ASSERT(cell->isMarkedBlack());

This assertion seems to be nonlocal. Current callers have the invariant that all of the free cells are marked, but future callers might be surprised. I would prefer a separate checkNoUnmarkedFreeCells().

@@ +2046,5 @@
> +    for (auto i : AllAllocKinds()) {
> +        FreeSpan* freeSpan = freeList(i);
> +        if (!freeSpan->isEmpty())
> +            freeSpan->getArena()->unmarkFreeCells();
> +    }

Ugh. I guess that means having ArenaLists::checkNoUnmarkedFreeCells too, then. :( I guess you could document the invariant instead, but it still seems weird to me.

@@ +3008,5 @@
>      gcObjectGroupArenasToUpdate(group, nullptr),
>      savedEmptyArenas(group, nullptr)
>  {
> +    for (auto i : AllAllocKinds()) {
> +        freeLists()[i] = &placeholder;

pre-existing: "sentinel" probably would have been a better name. Not worth changing.

@@ +4285,5 @@
>  
>      /*
> +     * In an incremental GC, clear the area free lists to ensure that subsequent
> +     * allocations refill them and end up marking new cells back. See
> +     * arenaAllocatedDuringGC().

*arena
Attachment #8953020 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #33)
> I wonder what the optimizer is able to do with this. Probably not much. Oh
> well.

I considered trying to optimise this with memset, but left that for now.  I can do that if you think it's worth it.

> The assignment to old should also be within DEBUG. (This probably comes from
> before you made freeList(), so you were keeping a pointer or reference to
> the freelist and reusing it.)

Cheers, fixed.

> This assertion seems to be nonlocal. Current callers have the invariant that
> all of the free cells are marked, but future callers might be surprised. I
> would prefer a separate checkNoUnmarkedFreeCells().

Right, I considered calling this unmarkPremarkedFreeCells or something that makes it clearer what state it's expected to be called in but I like your suggestion too.  I'll fix this.

> pre-existing: "sentinel" probably would have been a better name. Not worth
> changing.

Agreed.

> *arena

Man, after five years I still can't type 'arena' correctly 90% of the time.
This showed up a missing read barrier in TypeNewScript::maybeAnalyze().  The preliminary objects referenced by weak pointers and therefore we need a read barrier before writing anything we read out of them into the heap.

Also I fixed use of HeapPtr::init() where set() is needed.
Attachment #8953391 - Flags: review?(sphink)
Comment on attachment 8953136 [details] [diff] [review]
bug1359342-jit-compartment-barriers

Review of attachment 8953136 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/JitCompartment.h
@@ +660,5 @@
>  
> +    // Perform the necessary read barriers on stubs and SIMD template object
> +    // described by the bitmasks passed in. This function can only be called
> +    // from the active thread.
> +    void performStubReadBarriers(uint32_t stubsToBarrier) const;

nit, also mention:

The pointers should not be sweep nor dangling at this time, compared to when these bit mask were registered, because we kill all Ion compilations before starting the sweep phase (before ending the mark phase), and these objects are sweep on the active thread, before allowing any new Ion compilations.
Attachment #8953136 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8953391 - Flags: review?(sphink) → review+
Final patch: record singleton objects and object groups that require a read barrier after being read out of type sets during Ion compilation.

This adds them to vectors on the macro assembler.  I noticed there were frequent duplicates so it actually checks the tail of the list to avoid pushing the same thing many times.  I didn't do a full scan of the list to avoid N^2 behaviour.  Maybe this should use a hash table, I'm not sure.

We have to perform the read barriers in Linker::newCode as this can be called from several places, not just CodeGenerator::link.
Attachment #8954082 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8954082 [details] [diff] [review]
bug1359342-masm-barriers

Review of attachment 8954082 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MacroAssembler.cpp
@@ +3612,5 @@
> +
> +ObjectGroup*
> +MacroAssembler::getGroupAndDelayBarrier(const TypeSet* types, size_t i)
> +{
> +    ObjectGroup* group = types->getGroupNoBarrier(i);

nit: Add a comment above getGroupNoBarrier that people should use this MacroAssembler function instead.

::: js/src/jit/MacroAssembler.h
@@ +2616,5 @@
> +    // Methods to get a singleton object or object group from a type set without
> +    // a read barrier, and record the result so that we can perform the barrier
> +    // later.
> +    JSObject* getSingletonAndDelayBarrier(const TypeSet* types, size_t i);
> +    ObjectGroup* getGroupAndDelayBarrier(const TypeSet* types, size_t i);

nit: move these functions under the check_macroassembler_style TypeSet(*) section, and move their definitions to the .cpp file since they are only used there.

(*) Create the TypeSet section at the end of the check_macroassembler_style scope. (similar to https://searchfox.org/mozilla-central/rev/b469db5c90d618f4b202d5ef280e1a78224eb43b/js/src/jit/MacroAssembler.h#1602-1608 )

nit: Run "make check-masm" before pushing.
Attachment #8954082 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] {backlog: ~38} from comment #38)
> ::: js/src/jit/MacroAssembler.h
> @@ +2616,5 @@
> > +    // Methods to get a singleton object or object group from a type set without
> > +    // a read barrier, and record the result so that we can perform the barrier
> > +    // later.
> > +    JSObject* getSingletonAndDelayBarrier(const TypeSet* types, size_t i);
> > +    ObjectGroup* getGroupAndDelayBarrier(const TypeSet* types, size_t i);
> 
> nit: […] move their definitions to the .cpp file since they are only
> used there.

Sorry about that I thought I was reading the MacroAssembler-inl.h file.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07cab6799ff4
Pre-mark new allocations black during incremental GC r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d6247ece75
Add delayed read barriers for JitCompartment stubs r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/87e6042a409d
Record objects and groups that need to be barriered after being read from type sets r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/f361cfca3755
Add missing read barrier in TypeNewScript::maybeAnalyse r=sfink
Performance gains! \o/

== Change summary for alert #12090 (as of Tue, 13 Mar 2018 05:21:53 GMT) ==

Improvements:

  2%  Resident Memory android-4-2-x86 opt      155,603,347.58 -> 151,736,350.75
  2%  Resident Memory android-4-3-armv7-api16 opt 147,422,385.35 -> 144,321,324.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12090
jonco,

Can you guess why there was a memory usage improvement with this change?  I can't think of one immediately, I wouldn't expect the removed per-arena flag to be taking up that much room.. maybe..
Flags: needinfo?(jcoppeard)
(In reply to Paul Bone [:pbone] from comment #43)
What happened before was that whenever something was allocated from an arena during an incremental GC then that arena was put onto a list, and later everything in that arena was marked.  This applied to arenas that already contained some allocated cells at the start of GC, so this resulted in us marking things that might not have been marked otherwise.  So we would keep garbage alive if it shared an arena with cells allocated in an incremental GC.  The new scheme is more precise and only marks the necessary cells.
Flags: needinfo?(jcoppeard)
(In reply to Jon Coppeard (:jonco) from comment #44)

Okay, I understand.  Yeah, the current (premarking) situation is better than what we had before. So that's good.
You need to log in before you can comment on or make changes to this bug.