Closed Bug 650161 Opened 13 years ago Closed 9 years ago

Compact the tenured heap when GCInvocationKind is GC_SHRINK

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: cdleary, Assigned: jonco)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(68 files, 12 obsolete files)

3.33 KB, patch
terrence
: review+
ted
: review+
terrence
: checkin+
Details | Diff | Splinter Review
3.35 KB, patch
terrence
: review+
terrence
: checkin+
Details | Diff | Splinter Review
15.50 KB, patch
terrence
: review+
terrence
: checkin+
Details | Diff | Splinter Review
8.44 KB, patch
terrence
: review+
terrence
: checkin+
Details | Diff | Splinter Review
59.88 KB, patch
terrence
: review+
terrence
: checkin+
Details | Diff | Splinter Review
6.25 KB, patch
mjrosenb
: review+
terrence
: checkin+
Details | Diff | Splinter Review
43.34 KB, patch
jonco
: review+
terrence
: checkin+
Details | Diff | Splinter Review
31.42 KB, patch
terrence
: review+
terrence
: checkin+
Details | Diff | Splinter Review
35.54 KB, patch
terrence
: review+
terrence
: checkin+
Details | Diff | Splinter Review
6.23 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
9.34 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
12.60 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
17.28 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
9.14 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
3.30 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
32.50 KB, patch
bzbarsky
: review+
jonco
: checkin+
Details | Diff | Splinter Review
7.33 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
23.39 KB, patch
bholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
36.97 KB, patch
terrence
: review+
bholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.27 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
7.71 KB, patch
terrence
: review+
bholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
9.93 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
5.85 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
5.74 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
7.32 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
4.78 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
953 bytes, patch
bzbarsky
: review+
jonco
: checkin+
Details | Diff | Splinter Review
2.71 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.59 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
3.27 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.92 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
4.62 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
8.44 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
12.46 KB, patch
bholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.51 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
7.15 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
18.97 KB, patch
johns
: review+
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
946 bytes, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.89 KB, patch
sworkman
: review+
jonco
: checkin+
Details | Diff | Splinter Review
10.43 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
8.49 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
2.44 KB, patch
bhackett1024
: review+
jonco
: checkin+
Details | Diff | Splinter Review
2.08 KB, patch
bzbarsky
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.86 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.15 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
7.62 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.22 KB, patch
bholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
902 bytes, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.07 KB, patch
bhackett1024
: review+
jonco
: checkin+
Details | Diff | Splinter Review
8.71 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
2.00 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
21.81 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
5.31 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
848 bytes, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
2.82 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
3.77 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
4.81 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.06 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.57 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.10 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
4.47 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.71 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.26 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
2.34 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
4.53 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
31.29 KB, patch
jonco
: review+
glandium
: review+
Details | Diff | Splinter Review
6.20 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.19 KB, patch
billm
: review+
Details | Diff | Splinter Review
Basic idea: values found through conservative scanning (i.e. C stack scanning) need to be pinned at a particular memory location, but all other values *should* be capable of being moved.

I'm starting out with objects. Pointers that are marked exactly (i.e. Value& where isObject and JSObject *&s) can be relocated if the pointee is unpinned.

Assume that there exist no pinned things in an arena. The most basic algorithm would be: 1. move all unpinned objects within the arena to the same relative offset within a new arena; 2. mprotect the old arena segment to catch invalid references early; 3. during normal scanning update object pointer locations into the old arena segment to the new arena segment.

To extend that algorithm to work with pinned objects we would have to avoid the mprotect.

The death-defying excitement of this bug comes from outside use of the JSAPI. If there are places in the Mozilla code base that use implicit rooting assumptions (i.e. it's okay that I don't add this as an explicit root because I'm sure it will be rooted by the global object I'm pulling it off of) they will break. mprotect should help us figure out how breaking this change is fairly early on.
Summary: (Moving collection) Add capability to relocate GCThings to new arenas → Add capability to relocate GCThings to new arenas
Values that are explicitly rooted should also be capable of being moved, right?

What about things like wrapper caches (where the finalizer of the JSObject clears the non-root cached pointer)?
And fwiw, it'd be really nice to automatically CC xpconnect peers on API changes stuff like this.  They might be able to tell you what assumptions they're making.... ;)
(In reply to comment #1)
> Values that are explicitly rooted should also be capable of being moved, right?

Yes, for heap data outside the JS engine referencing JSObjects that's exactly the situation we want. The signature for JS_AddRoot takes a jsval pointer. Of course, if that value is found conservatively we still have to pin it. Also, there's a small danger from silly API usage: if a client AddRooted a jsval in a JSObject's inline slots the location of that jsval be invalidated when the JSObject relocates.

> What about things like wrapper caches (where the finalizer of the JSObject
> clears the non-root cached pointer)?

Good use case! Sounds like it's implementing weak pointer semantics. Can you point us at the code for the finalizer? I found nsWrapperCache.h but not the guts.

> it'd be really nice to automatically CC xpconnect peers

Point well taken.
> The signature for JS_AddRoot takes a jsval pointer.

Hmm.  And I guess we're assuming (or hoping) that no one also stores the JSObject* on the heap?  I think at least the mJSObject of an nsGlobalWindow (which is rooted implicitly by the fact that it's the global of the window's JSContext) is one obvious counterexample.  Not sure whether we have others except nsWrapperCache.

> Sounds like it's implementing weak pointer semantics.

Pretty much, yes.

The finalizers that are in the tree right now are XPCWrappedNative::FlatJSObjectFinalized and XPC_WN_NoHelper_Finalize and XPC_WN_Helper_Finalize (well, more precisely the finalizers are these last two, but the ClearWrapper call only happens in them for slimwrappers, while full XPCNWrappedNatives go through FlatJSObjectFinalized).  There's another finalizer in our new DOM nodelist code, but we can just adjust that as needed.  The key is knowing how to adjust it.
(In reply to comment #4)
> And I guess we're assuming (or hoping) that no one also stores the
> JSObject* on the heap? 

There are also pointer-taking rooting APIs for JSObject and friends -- we're going to have to root those heapified JS references to make them known to the moving GC.

> I think at least the mJSObject of an nsGlobalWindow

That one doesn't look explicitly rooted from a glance at MXR calls to the JS engine's rooting API.

> Not sure whether we have others
> except nsWrapperCache.

The mprotect experiment should find them pretty quickly once I get the object movement patch done.

> The finalizers that are in the tree right now are

Thanks! I'm going to look them over and make sure we have a story for them.
> That one doesn't look explicitly rooted

Right; it's rooted implicitly via the JSContext, as I said.

> The mprotect experiment should find them pretty quickly

Hmm...  I guess you could run in a mode where you move on every GC or something?
I found a few more weak pointers. XPCWrappedNativeScope has fields mGlobalJSObject, mPrototypeJSObject, mPrototypeJSFunction, and mPrototypeNoHelper. See XPCWrappedNativeScope::FinishedMarkPhaseOfGC.
Attached patch Folded movement patch. (obsolete) — Splinter Review
This is a folded version of the patches located in my tm-moving-gc-mq user repo. This is against a (very) old revision (e089a54230bf71965c9c74bb0e3aa9771fd40247) that I stuck to because there were significant GC changes going on that caused a lot of thrash. billm asked that I post the patch as is and rebase it later if possible.
(In reply to Boris Zbarsky (:bz) from comment #6)
> Hmm...  I guess you could run in a mode where you move on every GC or
> something?
Wouldn't a generational GC do that anyway? Either while compacting the old generation or while copying from the young generation.
This bug has gone stale because I switched to working on IonMonkey things. Bill's barrier work for incremental GC should cover most of the capability that we need here: Bill, should we resolve this as incomplete?
Assignee: cdleary → general
Status: ASSIGNED → NEW
I don't think the write barriers will address this problem. They only need to cover objects that are traced in the normal way. This bug is mostly about finding GC things that we don't root and don't trace through.

So let's leave this open.
Is this bug part of the current plan or has it been replaced by others?
I didn't know about it until last week, so I guess it is replaced.  I still want to take a look at the patch Chris posted, but I doubt we'll do any more work in this bug.
This is essentially what I was hoping Steve could work on.
Assignee: general → sphink
Alias: MovingGC → CompactingGC
No longer blocks: GenerationalGC
Summary: Add capability to relocate GCThings to new arenas → Compact the tenured heap when GCInvocationKind is GC_SHRINK
Depends on: 726687
Depends on: 723314
Whiteboard: [MemShrink]
Any estimation when this will be tackled?
Whiteboard: [MemShrink] → [MemShrink:P1]
(In reply to Florian Bender from comment #16)
> Any estimation when this will be tackled?

"Next quarter" is probably about all I can say right now.
Assignee: sphink → jcoppeard
Add --enable-gccompacting configure flag to the build.
Attachment #8467136 - Flags: review?(terrence)
Add machinery for storing forwarding pointers in GC things.
Attachment #543999 - Attachment is obsolete: true
Attachment #8467137 - Flags: review?(terrence)
Update our hash table checks for use after compacting GC as well as minor GC.
Attachment #8467138 - Flags: review?(terrence)
Attached patch 4-update-object-pointers (obsolete) — Splinter Review
Code to fix up pointers to moved JSObjects.
Attachment #8467140 - Flags: review?(terrence)
Attached patch 5-compacting-gc (obsolete) — Splinter Review
Compacting GC for JSObjects.  Note there are a bunch of todos in the code and not all tests pass yet (~40 jit-test failures with compacting zeal mode enabled).  Posting here so we can get in into the codeline to allow others to work on it.
Attachment #8467144 - Flags: review?(terrence)
Keywords: leave-open
Attachment #8467136 - Flags: review?(terrence) → review+
Comment on attachment 8467137 [details] [diff] [review]
2-cell-forwarding

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

Nice!

::: js/src/jsgc.cpp
@@ +1987,5 @@
> +
> +static void
> +ForwardCell(Cell *dest, Cell *src)
> +{
> +    // Mark a cell has having been relocated and astore forwarding pointer to

s/has/as/
s/astore/store a/

@@ +1991,5 @@
> +    // Mark a cell has having been relocated and astore forwarding pointer to
> +    // the new cell.
> +
> +    // Putting the values this way round is a terrible hack to make
> +    // ObjectImpl::zone() work on forwarded objects.

Let's add MOZ_ASSERT(offsetof(ObjectImpl, shape_) == 0); MOZ_ASSERT(dest->isTenured()); and MOZ_ASSERT(src->shape_->zone() == dest->zone());. This should at least make the hack reliable.
Attachment #8467137 - Flags: review?(terrence) → review+
Comment on attachment 8467138 [details] [diff] [review]
3-check-hash-tables

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

::: js/src/jsinfer.cpp
@@ +4265,5 @@
> +        return;
> +
> +    for (TypeObjectWithNewScriptSet::Enum e(table); !e.empty(); e.popFront()) {
> +        TypeObjectWithNewScriptEntry entry = e.front();
> +       CheckGCThingAfterMovingGC(entry.object.get());

Missing a space.

::: js/src/jspubtd.h
@@ +25,5 @@
>  # define JSGC_TRACK_EXACT_ROOTS
>  #endif
>  
> +#if (defined(JSGC_GENERATIONAL) && defined(JS_GC_ZEAL)) || \
> +    (defined(JSGC_COMPACTING) && defined(DEBUG))

Nice! Good idea to make this variant not require zeal modes!
Attachment #8467138 - Flags: review?(terrence) → review+
Comment on attachment 8467144 [details] [diff] [review]
5-compacting-gc

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

I reviewed this first, since I wanted more context before reading 4 of 5: I'll get to 4 tomorrow. This is really quite nice -- a solid base to build off of.

::: js/src/gc/GCRuntime.h
@@ +288,5 @@
>      void gcSlice(JSGCInvocationKind gckind, JS::gcreason::Reason reason, int64_t millis = 0);
>      void runDebugGC();
>      inline void poke();
>  
> +    void markRuntime(JSTracer *trc, bool traceForGC = false, bool useSavedRoots = false);

I like that we can get rid of IS_GC_MARKING_TRACER, but I'm not sure I like the name. How about traceForMarking? Also, we should make these enums for clarity at the call sites.

::: js/src/jsgc.cpp
@@ +939,5 @@
>          removeFromAvailableList();
>  
>      zone->usage.addGCArena();
>  
> +    if (rt->isHeapCompacting() && zone->usage.gcBytes() >= zone->threshold.gcTriggerBytes()) {

I think this is missing a |!|.

@@ +1994,5 @@
> + *
> + *  - arenas are selected for compaction
> + *  - the contents of those arenas are moved to new arenas
> + *  - all references to moved things are updated
> + */

We should put this comment up at the top below the long comment on IGC.

@@ +2025,5 @@
> +static bool
> +ArenaContainsGlobal(ArenaHeader *arena)
> +{
> +    /*
> +     * We can't currently move global objects because there address is baked

s/there/their/

@@ +2028,5 @@
> +    /*
> +     * We can't currently move global objects because there address is baked
> +     * into compiled code. We therefore skip moving the contents of any arena
> +     * containing a global.
> +     */

Please move the comment down to CanRelocateArena. I think it makes more sense explaining why we're calling the method; the implementation is pretty straightforward.

@@ +2030,5 @@
> +     * into compiled code. We therefore skip moving the contents of any arena
> +     * containing a global.
> +     */
> +    if (arena->getAllocKind() > FINALIZE_OBJECT_LAST)
> +        return false;

It looks like we should be able to just assert this, at least for now.

@@ +2130,5 @@
> +        cell->markIfUnmarked(GRAY);
> +    else if (src->isMarked(BLACK))
> +        cell->markIfUnmarked(BLACK);
> +    JS_ASSERT_IF(src->isMarked(BLACK), cell->isMarked(BLACK));
> +    JS_ASSERT_IF(src->isMarked(GRAY), cell->isMarked(GRAY));

This would probably be clearer as ChunkBitmap::swapMarkBits(src, cell).

@@ +2254,5 @@
> +    Debugger::sweepAll(fop);
> +
> +    for (ZonesIter zone(rt, SkipAtoms); !zone.done(); zone.next()) {
> +        if (zone->isCollecting()) {
> +            gcstats::AutoPhase ap(rt->gc.stats, gcstats::PHASE_SWEEP_COMPARTMENTS);

I guess we'll probably want to put all of compacting in a new phase and then split out bits of it into sub-phases as we discover pieces that are time-consuming.

@@ +2304,5 @@
> +    JS_ASSERT(rt->currentThreadHasExclusiveAccess());
> +    MovingTracer trc(rt);
> +
> +    {
> +        // todo: Maybe give compaction its own set of phases.

Yes please.

@@ +2307,5 @@
> +    {
> +        // todo: Maybe give compaction its own set of phases.
> +        gcstats::AutoPhase ap(stats, gcstats::PHASE_MARK);
> +
> +        // todo: may need to fix up other read-barriered pointers here

And all the weak pointers, I'd think. We should probably just give up and add a WeakPtr<T> type to the engine that is a ReadBarriered<T> + a linked list we can use to make assertions about them.

@@ +2323,5 @@
> +            for (JSGenerator *gen = i.get()->innermostGenerator(); gen; gen = gen->prevGenerator)
> +                gen->obj = MaybeForwarded(gen->obj.get());
> +        }
> +
> +        // Iterate through all allocated cells to update internal pointers

End with a period.

@@ +2337,5 @@
> +                }
> +            }
> +        }
> +
> +        // Mark roots to update them

Ditto.

@@ +4977,5 @@
> +    ArenaHeader *relocatedList = nullptr;
> +    for (GCZonesIter zone(rt); !zone.done(); zone.next()) {
> +        JS_ASSERT(zone->isGCFinished());
> +
> +        // Cannot move atoms (even the non-interned ones, unfortunately)

Yes, we rather depend on the addresses not changing.

@@ +4978,5 @@
> +    for (GCZonesIter zone(rt); !zone.done(); zone.next()) {
> +        JS_ASSERT(zone->isGCFinished());
> +
> +        // Cannot move atoms (even the non-interned ones, unfortunately)
> +        // todo: I think we don't currently trace all references in compiled code.

This seems extremely likely. It seems to me like we'd be much better off just making a shrinking GC discard all code regardless of the "preserving" bits.
Attachment #8467144 - Flags: review?(terrence) → review+
Comment on attachment 8467140 [details] [diff] [review]
4-update-object-pointers

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

This is very nice. I'd like to better understand why we need the jitcode marking bits (and hopefully remove them) before we commit this, however.

::: js/public/HashTable.h
@@ +945,5 @@
>          // Removes the |front()| element and re-inserts it into the table with
>          // a new key at the new Lookup position.  |front()| is invalid after
>          // this operation until the next call to |popFront()|.
>          void rekeyFront(const Lookup &l, const Key &k) {
> +            JS_ASSERT(&k != &HashPolicy::getKey(this->cur->get()));

Oh, good idea!

::: js/src/gc/Marking.cpp
@@ +912,3 @@
>  
> +    JS_ASSERT_IF(trc->callback == nullptr || trc->callback == GCMarker::GrayCallback,
> +                 color == AsGCMarker(trc)->getMarkColor());

These hunks seem unrelated? Nice cleanups, but would be nice to have them split out.

::: js/src/jit/arm/Assembler-arm.cpp
@@ +809,5 @@
> +    gc::MarkGCThingUnbarriered(trc, reinterpret_cast<void **>(&ptr), "ion-masm-ptr");
> +
> +    if (ptr != prior)
> +        masm->ma_movPatchable(Imm32(int32_t(ptr)), dest, Assembler::Always, rs, ins);
> +}

I think we want to discard all code if we're doing a moving GC that can touch non-nursery things. I don't think most of this should be necessary.

::: js/src/jit/shared/Assembler-x86-shared.cpp
@@ +53,5 @@
>              jsval_layout layout;
>              layout.asBits = *word;
>              Value v = IMPL_TO_JSVAL(layout);
>              gc::MarkValueUnbarriered(trc, &v, "ion-masm-value");
> +            *word = JSVAL_TO_IMPL(v).asBits;

We should be discarding all code on shrinking GC, so I think we actually want to keep this.

::: js/src/jscntxt.cpp
@@ +87,5 @@
>  js::TraceCycleDetectionSet(JSTracer *trc, js::ObjectSet &set)
>  {
>      for (js::ObjectSet::Enum e(set); !e.empty(); e.popFront()) {
> +        JSObject *key = e.front();
> +        MarkObjectRoot(trc, &key, "cycle detector table entry");

Don't forget to trc->setTracingLocation(&e.front());

::: js/src/jscompartment.cpp
@@ +587,5 @@
> +            gcstats::AutoPhase ap(runtimeFromMainThread()->gc.stats,
> +                                  gcstats::PHASE_SWEEP_TABLES_TYPE_OBJECT);
> +            sweepNewTypeObjectTable(newTypeObjects);
> +            sweepNewTypeObjectTable(lazyTypeObjects);
> +        }

Good find!

@@ +663,5 @@
> +/*
> + * Fixup wrappers with moved keys or values. We must sweep all compartments,
> + * since string entries in the crossCompartmentWrappers table are not marked
> + * during markCrossCompartmentWrappers. FIXME: sfink needs to understand what
> + * that means.

FIXME: sfink and terrence need to understand....

::: js/src/jsinfer.cpp
@@ +4238,3 @@
>                  e.removeFront();
> +            } else {
> +                /* Any rekeying necessary is hanlded by fixupNewTypeObjectTable() below. */

s/hanlded/handled/

::: js/src/jsiter.cpp
@@ +1570,5 @@
>  
>  static void
>  MarkGeneratorFrame(JSTracer *trc, JSGenerator *gen)
>  {
> +    MarkObject(trc, &gen->obj, "Generator Object");

I don't recall exactly why this is not needed, but ISTR that adding it for GGC broke something or other. Generators have changed a bunch sense then though, so maybe it's not an issue now? In any case I think this could be a MaybeForwarded.

::: js/src/jspropertytree.cpp
@@ +270,5 @@
> +void
> +Shape::fixupUntracedChildren()
> +{
> +    if (inDictionary()) {
> +        if (listp) {

if (!listp) return and drop a level here.

@@ +284,5 @@
> +                // listp points to the shape_ field of an object.
> +                JSObject *last = reinterpret_cast<JSObject *>(uintptr_t(listp) -
> +                                                              offsetof(JSObject, shape_));
> +                listp = &gc::MaybeForwarded(last)->shape_;
> +            }

I don't even. I guess this is the cost of compacting shapes, but geez.

@@ +289,5 @@
> +        }
> +    } else {
> +        if (kids.isShape()) {
> +            if (gc::IsForwarded(kids.toShape()))
> +                kids.setShape(gc::Forwarded(kids.toShape()));

Let's make this an early-return and drop the rest a level:

if (kids.isShape() && IsForwarded(kids.toShape()) {
    kits.setShape(Forwarded(kids.toShape());
    return;
}

@@ +290,5 @@
> +    } else {
> +        if (kids.isShape()) {
> +            if (gc::IsForwarded(kids.toShape()))
> +                kids.setShape(gc::Forwarded(kids.toShape()));
> +        } else if (kids.isHash()) {

Let's handle the isNull case up top with an eager return and then JS_ASSERT(kids.isHash()) here.

@@ +294,5 @@
> +        } else if (kids.isHash()) {
> +            KidsHash *kh = kids.toHash();
> +            for (KidsHash::Enum e(*kh); !e.empty(); e.popFront()) {
> +                Shape *key = e.front();
> +                if (IsForwarded(key)) {

if (!IsForwarded(key)) continue and drop the rest a level.

@@ +311,5 @@
> +                    e.rekeyFront(lookup, key);
> +                }
> +            }
> +        }
> +    }

I'd kinda like to see this broken out into fixupDictionaryShapeAfterMovingGC and fixupShapeTreeAfterMovingGC.

::: js/src/vm/ArrayBufferObject.cpp
@@ -830,5 @@
> -#endif
> -        )
> -    {
> -        return;
> -    }

I can't image that this won't cause problems somehow.

::: js/src/vm/Debugger.h
@@ +124,5 @@
>              if (gc::IsAboutToBeFinalized(&k)) {
>                  e.removeFront();
>                  decZoneCount(k->zone());
> +            } else {
> +                // marKeys() should have done any necessary relocation.

markKeys.

::: js/src/vm/ScopeObject.cpp
@@ +1784,5 @@
>               */
>              liveScopes.remove(&(*debugScope)->scope());
>              e.removeFront();
> +        } else {
> +            // todo: How come these don't need fixing up?

We have enough code coverage to say we don't for sure?

::: js/src/vm/Shape.cpp
@@ +444,5 @@
>      JS_ASSERT(!inDictionaryMode());
>  
> +#ifdef JSGC_COMPACTING
> +    // todo: This crashes if we run a compacting GC here
> +    js::gc::AutoSuppressGC asgc(zone()->runtimeFromAnyThread());

Why are we running shrinking GC with JS on the stack?

::: js/src/vm/Shape.h
@@ +195,5 @@
>      Shape           **search(jsid id, bool adding);
> +
> +#ifdef JSGC_COMPACTING
> +    /* Update entries whose shapes have been moved */
> +    void            fixup();

|fixup| is a bit generic. Let's call this fixupAfterMovingGC() or something that will be clearer for random readers of Shape.h.
Attachment #8467140 - Flags: review?(terrence)
As discussed yesterday, here's a patch to honour the invocation kind passed to the first GC slice.  I guess it would also be good idea to track down the cases when the browser passes different settings to different incremental slices, but it doesn't make any difference to correctness.
Attachment #8467140 - Attachment is obsolete: true
Attachment #8467144 - Attachment is obsolete: true
Attachment #8468560 - Flags: review?(terrence)
Thanks for the detailed review!

The jitcode marking is necessary if we want to have the zeal mode to do periodic
compacting GCs because these will be called when there is code on the stack.
Unfortunately I don't see how we can be confident of our test coverage without
this feature.

I've split this out into a separate patch below.

I've addressed your other comments.
Attachment #8468563 - Flags: review?(terrence)
Patch to update pointers in jitcode.  If you think this should land we should  get review from mjrosenb too.
Attachment #8468564 - Flags: review?(terrence)
Updated patch with review comments addressed, carrying r+.
Attachment #8468566 - Flags: review+
Attached patch 6-stats-phasesSplinter Review
Patch to add stats phases for compacting GC.
Attachment #8468569 - Flags: review?(terrence)
Comment on attachment 8468560 [details] [diff] [review]
3.5-invocation-kind

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

Great!
Attachment #8468560 - Flags: review?(terrence) → review+
Comment on attachment 8468563 [details] [diff] [review]
4-update-object-pointers v2

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

::: js/src/vm/Shape.cpp
@@ +443,5 @@
>  {
>      JS_ASSERT(!inDictionaryMode());
>  
> +#ifdef JSGC_COMPACTING
> +    // todo: This crashes if we run a compacting GC here

Use all-caps TODO -- some editors will highlight this so it's relatively standard. Also maybe explain why we crash there, assuming we know.

@@ +444,5 @@
>      JS_ASSERT(!inDictionaryMode());
>  
> +#ifdef JSGC_COMPACTING
> +    // todo: This crashes if we run a compacting GC here
> +    js::gc::AutoSuppressGC asgc(zone()->runtimeFromAnyThread());

s/asgc/nogc/
Attachment #8468563 - Flags: review?(terrence) → review+
Comment on attachment 8468564 [details] [diff] [review]
4.5-update-pointers-in-jitcode

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

Great point! I guess we don't have a choice but to go all the way here. It also gives us the ability to add a method to TestingFunctions to trigger shrinking gc, which will be very nice for fuzzing. Please post a followup with such a patch.
Attachment #8468564 - Flags: review?(terrence) → review?(mrosenberg)
Comment on attachment 8468569 [details] [diff] [review]
6-stats-phases

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

Nice! Great idea to use MaybeAutoPhase for this.
Attachment #8468569 - Flags: review?(terrence) → review+
Some updates to fix test failures including:

 - add an RAII guard to temporarily disable compacting GC
 - updats gray roots
 - fix sweeping of compartments' debuggee set
 - fix some more cases related to array buffers and typed array objects
Attachment #8471645 - Flags: review?(terrence)
Comment on attachment 8471645 [details] [diff] [review]
7-fix-test-failures

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

This is strongly reminiscent of the sorts of things that needed attention leading up to landing GGC.

::: js/src/gc/Tracer.h
@@ +274,5 @@
>      void saveValueRanges();
>      inline void processMarkStackTop(SliceBudget &budget);
>      void processMarkStackOther(uintptr_t tag, uintptr_t addr);
>  
> +    void appendGrayRoot(void **thingp, JSGCTraceKind kind);

Nice to see this finally get fixed.
Attachment #8471645 - Flags: review?(terrence) → review+
Comment on attachment 8467136 [details] [diff] [review]
1-compacting-defines

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

Needs build peer review.
Comment on attachment 8467137 [details] [diff] [review]
2-cell-forwarding

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

::: js/src/jsgc.cpp
@@ +1993,5 @@
> +
> +    // Putting the values this way round is a terrible hack to make
> +    // ObjectImpl::zone() work on forwarded objects.
> +    uintptr_t *ptr = reinterpret_cast<uintptr_t *>(src);
> +    ptr[0] = uintptr_t(dest); // Forwarding address

reinterpret_cast

::: js/src/jsgc.h
@@ +1339,5 @@
> +template <typename T>
> +inline bool
> +IsForwarded(T *t)
> +{
> +    (void)static_cast<Cell *>(t);

I bet Waldo has some template magic to check that T is a Cell.

@@ +1349,5 @@
> +IsForwarded(const JS::Value &value)
> +{
> +    if (value.isObject())
> +        return IsForwarded(&value.toObject());
> +    else if (value.isString())

No else-after-return
Comment on attachment 8468564 [details] [diff] [review]
4.5-update-pointers-in-jitcode

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

::: js/src/jit/arm/Assembler-arm.cpp
@@ +805,5 @@
> +    const void *prior = Assembler::GetPtr32Target(iter, &dest, &rs);
> +    void *ptr = const_cast<void *>(prior);
> +
> +    // No barrier needed since these are constants.
> +    gc::MarkGCThingUnbarriered(trc, reinterpret_cast<void **>(&ptr), "ion-masm-ptr");

What is &ptr if not void**?
Attachment #8467136 - Flags: review?(ted)
Comment on attachment 8467136 [details] [diff] [review]
1-compacting-defines

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

This is fine, but is there a reason you need an actual configure argument? They're attractive nuisances to lots of developers, who add as many as they can to their mozconfigs.
Attachment #8467136 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #41)
This is still very much under development and is not stable, so we don't want it built by default, but we do want people to be able to make builds easily so they can hack on it... is there a better way to achieve this?
That's fine. Thanks for the explanation. When this feature gets enabled by default you should remove the configure option to reduce the build complexity.
Comment on attachment 8468564 [details] [diff] [review]
4.5-update-pointers-in-jitcode

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

::: js/src/jit/arm/Assembler-arm.cpp
@@ +874,5 @@
>      }
>  
> +    if (tmpDataRelocations_.length()) {
> +        ::TraceDataRelocations(trc, &m_buffer, &tmpDataRelocations_,
> +                               static_cast<MacroAssemblerARM *>(Dummy));

You should probably use this rather than Dummy, although I can't think of a reason that either one would be wrong.
Attachment #8468564 - Flags: review?(mrosenberg) → review+
And fix a build error in non-unified builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/55126a7a7447
Status: NEW → ASSIGNED
jonco: do you have any measurements on how this affects memory usage? Is it just objects that are being compacted for now?
Flags: needinfo?(jcoppeard)
(In reply to Nicholas Nethercote [:njn] from comment #49)
> jonco: do you have any measurements on how this affects memory usage? Is it
> just objects that are being compacted for now?

It's just objects so far, and no I haven't done any measurements yet.
Flags: needinfo?(jcoppeard)
Since the whole point of compacting GC is to (AIUI) reduce memory usage, some measurements would be helpful.
(In reply to Nicholas Nethercote [:njn] from comment #51)
I'm currently working on getting this stable in the browser and when it is I will measure memory usage.
Oh, I didn't realize it was hidden behind a build flag. That makes more sense!
BTW, this is marked as leave-open. Is that intentional? This bug is marked as a MemShrink:P1, but I guess we should really move that marking to some other bug with a title like "turn on compacting GC in the browser"...
(In reply to Nicholas Nethercote [:njn] from comment #55)
Yes, I was planning on landing more patches in this bug.  I'll file a new bug for enabling.
Whiteboard: [MemShrink:P1]
Blocks: 1055067
Blocks: 1055034
Jon, seems like we're having some new options for compacting GC?

--enable-gccompacting
--enable-small-chunk-size
--enable-gc-trace

Please needinfo? us when these options are ready for fuzzing.
Flags: needinfo?(jcoppeard)
(In reply to Gary Kwong [:gkw] [:nth10sd] work week Aug 18 - 22 from comment #57)
Hi Gary,

--enable-small-chunk-size is not related to compacting GC but is an improvement for B2G, and is available for fuzzing now
--enable-gc-trace is an extension to allow developers to collect data about our GC activity and is not applicable to fuzzing
--enable-gccompacting is still being worked on in this bug and is not yet ready for fuzzing, but I'll let you know when that changes

Cheers :)

Jon
Flags: needinfo?(jcoppeard)
Fixes some more jit-test failures with compacting GC, adds an assertion that we don't find forwarded things when marking and fixes issue where we traced the gray roots with the wrong tracer.
Attachment #8477459 - Flags: review?(terrence)
Fix issues relating to copy-on-write arrays.  We may need to update the owner object pointer at the end of shared elements, and also the elements pointer itself if it points into inline elements in an object that was moved.
Attachment #8477460 - Flags: review?(terrence)
Add a moving GC callback and use it to fix up JSObject pointers in IPC tables.
Attachment #8477462 - Flags: review?(terrence)
Add a class hook to call when an object is moved.  Use this to factor out class-specific fixup from RelocateCell() for array buffers and typed arrays.  Implement the hook for proxies too.
Attachment #8477464 - Flags: review?(terrence)
Attached patch 12-update-wrapper-cache WIP (obsolete) — Splinter Review
Use the object moved hook to update wrapper cache entries in the browser, and assert if we try to wrapper cache objects that don't supply this hook.

This is work in progress - I think I'm missing the hook for a couple of classes.
Attached patch 13-fixup-xpconnect (obsolete) — Splinter Review
Use the callback and the class hook to update more pointers in xpconnect.
Attached patch 14-fixup-cycle-collector WIP (obsolete) — Splinter Review
Work in progress - update purple buffer entries in the cycle collector.

This patch was necessary before - I'm not sure whether it's still necessary with the previous patches applied.
The purple buffer just contains some random set of C++ objects that have been touched, and the trace hook will just give you the JS objects held by those C++ objects, so you'll miss a bunch of them.  What you probably want is CycleCollectedJSRuntime::mJSHolders, which is where most of our rooting of JS from C++ happens.
Comment on attachment 8477459 [details] [diff] [review]
8-fix-more-test-failures

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

::: js/src/jspropertytree.cpp
@@ +276,5 @@
>  {
>      if (!listp)
>          return;
>  
> +    // It's possible that this shape is unreachable and that lisp points to the

s/lisp/listp/

@@ +277,5 @@
>      if (!listp)
>          return;
>  
> +    // It's possible that this shape is unreachable and that lisp points to the
> +    // location of a dead object in the nursery.  In this case we should never

One space between sentences.
Attachment #8477459 - Flags: review?(terrence) → review+
Comment on attachment 8477460 [details] [diff] [review]
9-fixup-cow-arrays

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

::: js/src/jsobjinlines.h
@@ +639,3 @@
>  
> +    // Use the smallest allocation kind for the array, as it can't have any
> +    // fixed slots (see assert in the above function) and will not be using its

Let's say "createArray" specifically so that code motion doesn't make this confusing.
Attachment #8477460 - Flags: review?(terrence) → review+
Comment on attachment 8477462 [details] [diff] [review]
10-fixup-ipc-tables

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

::: js/src/jsgc.cpp
@@ +1604,5 @@
> +void
> +GCRuntime::removeMovingGCCallback(JSMovingGCCallback callback)
> +{
> +    for (Callback<JSMovingGCCallback> *p = movingCallbacks.begin();
> +         p < movingCallbacks.end(); p++) {

{ on newline here.
Attachment #8477462 - Flags: review?(terrence) → review+
Attachment #8467136 - Flags: checkin+
Comment on attachment 8467137 [details] [diff] [review]
2-cell-forwarding

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

Nice!

::: js/src/jsgc.cpp
@@ +1987,5 @@
> +
> +static void
> +ForwardCell(Cell *dest, Cell *src)
> +{
> +    // Mark a cell has having been relocated and astore forwarding pointer to

s/has/as/
s/astore/store a/

@@ +1991,5 @@
> +    // Mark a cell has having been relocated and astore forwarding pointer to
> +    // the new cell.
> +
> +    // Putting the values this way round is a terrible hack to make
> +    // ObjectImpl::zone() work on forwarded objects.

Let's add MOZ_ASSERT(offsetof(ObjectImpl, shape_) == 0); MOZ_ASSERT(dest->isTenured()); and MOZ_ASSERT(src->shape_->zone() == dest->zone());. This should at least make the hack reliable.
Attachment #8467137 - Flags: checkin+
Attachment #8467138 - Flags: checkin+
Comment on attachment 8468560 [details] [diff] [review]
3.5-invocation-kind

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

Great!
Attachment #8468560 - Flags: checkin+
Attachment #8468563 - Flags: checkin+
Attachment #8468564 - Flags: checkin+
Attachment #8468566 - Flags: checkin+
Attachment #8468569 - Flags: checkin+
Comment on attachment 8477459 [details] [diff] [review]
8-fix-more-test-failures

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

::: js/src/jspropertytree.cpp
@@ +276,5 @@
>  {
>      if (!listp)
>          return;
>  
> +    // It's possible that this shape is unreachable and that lisp points to the

s/lisp/listp/

@@ +277,5 @@
>      if (!listp)
>          return;
>  
> +    // It's possible that this shape is unreachable and that lisp points to the
> +    // location of a dead object in the nursery.  In this case we should never

One space between sentences.
Attachment #8477459 - Flags: checkin+
Attachment #8477459 - Flags: checkin+
Comment on attachment 8477464 [details] [diff] [review]
11-add-object-moved-hook

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

Nice cleanup!

::: js/src/vm/ArrayBufferObject.cpp
@@ +933,5 @@
>  
>      gcLiveArrayBuffers.clear();
>  }
>  
> +

Extra \n.
Attachment #8477464 - Flags: review?(terrence) → review+
Attachment #8471645 - Flags: checkin+
Attachment #8477459 - Flags: checkin+
Attachment #8477460 - Flags: checkin+
Attachment #8477462 - Flags: checkin+
Attachment #8477464 - Flags: checkin+
Here are some test results from the browser:

Test            Run Compacting disabled        Compacting enabled     
                    Used   Unused  % Unused    Used   Unused  % Unused

guardian.co.uk  1   27.29  7.49    21.54%      28.42  5.09    15.19%
                2   30.06  9.27    23.57%      27.27  5.59    17.01%
Gmail           1   46.85  13.37   22.20%      46.49  9.14    16.43%
                2   46.34  13.91   23.09%      46.25  8.8     15.99%
                3   45.08  13.57   23.14%      44.93  8.72    16.25%

All sizes are in MB and come from about:memory js-main-runtime-gc-heap-committed/used and unused values.

This shows that in gmail we are freeing up ~5MB unused space out of a 60MB heap.
Nice! jonco, do I just need to build the browser with --enable-gccompacting to test?
Flags: needinfo?(jcoppeard)
(In reply to Nicholas Nethercote [:njn] from comment #77)
You will also need the remaining patches that I'm just about to upload.
Flags: needinfo?(jcoppeard)
Attached patch 12-update-wrapper-cache (obsolete) — Splinter Review
I'm confident that this is working now.  If you're happy with this I'll flag for additional reviews from dom and xpconnect.
Attachment #8477469 - Attachment is obsolete: true
Attachment #8477471 - Attachment is obsolete: true
Attachment #8477485 - Attachment is obsolete: true
Attachment #8483513 - Flags: review?(terrence)
Attached patch 13-fixup-xpconnect (obsolete) — Splinter Review
Attachment #8483514 - Flags: review?(terrence)
If I understand correctly, GC heap fragmentation can currently happen at 2 levels:

(1) We may hang on to multiple partially full GC chunks
(2) We may hang on to multiple partially full GC arenas

Does compacting GC as implemented here address (1), (2) or both? If both, after a compacting GC I would expect there to be at most 1 partially full arena for each GC thing type (per allocator?), and at most 1 partially full chunk (per GCRuntime?). Is that correct?
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #81)
Compacting GC addresses issue 2, partially full arenas.  Issue 1 is already addressed by decommitting unused areans on platforms where that's supported (most of them).

In an ideal world we would end up with at most one partially full arena per thing kind, however there are limitations to compacting GC at the moment:
 - we only try to relocate objects
 - we can't relocate global objects because they may have their address baked in to JIT code
Fix more jit-test failures in compacting zeal mode:
 - Don't actually delete the RegExpShared in RegExpCompartment::sweep if we are
   compacting
 - Clear Shape::listp in JSObject finalizer if it points into a dying object
 - Clear the mark bits for Arenas whose contents have been relocated
 - Trace through WeakMap::memberOf to ensure this is updated
Attachment #8483533 - Flags: review?(terrence)
Allow passing a second 'shrinking' parameter to our shell gc() function to trigger a shrinking GC.
Attachment #8483534 - Flags: review?(terrence)
(In reply to Jon Coppeard (:jonco) from comment #82)
> Compacting GC addresses issue 2, partially full arenas.  Issue 1 is already
> addressed by decommitting unused areans on platforms where that's supported
> (most of them).

Aah, I wasn't aware we did this. On 32-bit Windows it would still be nice to free up the address space taken up by these chunks if possible, though with the limitations on what arenas can be moved it sounds like we can't really reach the ideal.

Thanks for the explanation!
Comment on attachment 8483513 [details] [diff] [review]
12-update-wrapper-cache

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

LGTM.
Attachment #8483513 - Flags: review?(terrence) → review+
Attachment #8483514 - Flags: review?(terrence) → review+
Comment on attachment 8483533 [details] [diff] [review]
15-fix-shell-test-failures

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

Nice cleanups!
Attachment #8483533 - Flags: review?(terrence) → review+
Attachment #8483534 - Flags: review?(terrence) → review+
Attachment #8483513 - Flags: review?(bzbarsky)
Attachment #8483513 - Flags: review?(bobbyholley)
Attachment #8483514 - Flags: review?(bobbyholley)
Comment on attachment 8483513 [details] [diff] [review]
12-update-wrapper-cache

>+def objectMovedHook(descriptor, hookName, freeOp):

The second and third args are not used.  Please remove them.

>+    return CGIfWrapper(CGGeneric(objectMoved), "self")

Why do you need the CGIfWrapper?  "self" can't be null here, afaict. If this _is_ needed for some odd reason, that odd reason should be very clearly documented.

>+class CGDOMJSProxyHandler_objectMoved(ClassMethod):

Why do we have both a JSClass hook _and_ a proxy handler hook for this?  That seems rather excessive; seems to me like we could just use the jsclass hook for proxies too, no?

In particular, the code CGClassObjectMovedHook generates with this patch would work just fine on DOM proxies.  And then we don't need different codepaths and such.

Please document somewhere why this is all needed, by the way.  If I understand right, the key is that in a wrapper cache the mWrapper may not get marked if not preserved, but might still be live and get moved, right?  If that's what's going on, then that needs some clear spelling out somewhere, because I certainly expected a JS::Heap to get automatically updated on move, and I'm being very surprised that this is not the case.

I don't see anything obviously handling moves for the outer window's wrappercache.  That needs to be fixed.
Attachment #8483513 - Flags: review?(bzbarsky) → review-
Comment on attachment 8483513 [details] [diff] [review]
12-update-wrapper-cache

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

r=bholley on the XPConnect parts once Boris' comments are sorted out.

::: js/xpconnect/src/Sandbox.cpp
@@ +478,5 @@
>  
>  bool
>  xpc::IsSandbox(JSObject *obj)
>  {
> +    const Class *clasp = js::Valueify(GetObjectJSClass(obj));

I think js::GetObjectClass would be preferred here.
Attachment #8483513 - Flags: review?(bobbyholley) → review+
Comment on attachment 8483514 [details] [diff] [review]
13-fixup-xpconnect

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

So we can avoid dealing with mGlobalJSObject, mContentXBLScope, and mAddonScopes because we never relocate global objects? If so, please add comments to that effect.

::: js/xpconnect/src/XPCMaps.cpp
@@ +105,5 @@
> +{
> +    for (Map::Enum e(mTable); !e.empty(); e.popFront()) {
> +        nsXPCWrappedJS* wrapper = e.front().value();
> +        MOZ_ASSERT(wrapper, "found a null JS wrapper!");
> +        MOZ_ALWAYS_FALSE(wrapper->IsObjectAboutToBeFinalized());

Why do we need to explicitly invoke this here? Don't we do do this already in the finalize callback?

Also, aren't JS::Heap pointers automatically relocated?

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +606,5 @@
> +XPCWrappedNativeScope::FixupAfterMovingGC()
> +{
> +    for (XPCWrappedNativeScope* cur = gScopes; cur; cur = cur->mNext) {
> +        if (cur->mWaiverWrapperMap)
> +            cur->mWaiverWrapperMap->Sweep();

Similarly, shouldn't this have already happened in XPCWrappedNativeScope::StartFinalizationPhaseOfGC?
Attachment #8483514 - Flags: review?(bobbyholley)
Blocks: GC.size
Thanks for the comments.

Yes, the issue is that if a wrapper is not preserved then mWrapper is not traced and so cannot be discovered and updated by the GC by the usual means.

The purpose of Heap<T> is to track pointers from the tenured heap into the nursery (which it does by adding entries to the store buffer when its contents are changed to point to a nursery object).  This allows us to perform fast minor GCs without tracing the entire heap.  But it doesn't track all pointers in the system as this would be too costly to maintain and also not useful for generational GC.

For compacting GC, we must potentially update every pointer in the system.  In theory we can just trace the heap to find these pointers, but in our case we also have a lot of weak pointers which are not traced and so must be updated separately -- for example the wrapper cache.  The objectMoved class extension allows classes that maintain weak pointers to their objects to update them appropriately.

The motivation for making a proxy trap for objectMoved was first by analogy with the finalize trap, and secondly so that proxy handlers could deal with this without also having to provide their own class.  I now realise that DOM proxies already implement their own class anyway though so this is moot.  I've removed this as you suggested.

I've fixed the issues with Codegen.py and the outer window wrapper cache.
Attachment #8483513 - Attachment is obsolete: true
Attachment #8484956 - Flags: review?(bzbarsky)
Comment on attachment 8484956 [details] [diff] [review]
12-update-wrapper-cache v2

Thanks, this looks great.  r=me
Attachment #8484956 - Flags: review?(bzbarsky) → review+
Attached patch 13-fixup-xpconnect v2 (obsolete) — Splinter Review
(In reply to Bobby Holley (:bholley) from comment #90)

> So we can avoid dealing with mGlobalJSObject, mContentXBLScope, and
> mAddonScopes because we never relocate global objects? If so, please add
> comments to that effect.

That's right -- I've added a comment.

> > +        MOZ_ALWAYS_FALSE(wrapper->IsObjectAboutToBeFinalized());
> 
> Why do we need to explicitly invoke this here? Don't we do do this already
> in the finalize callback?

The compaction happens in a separate phase to finalization, so the sweep code is run twice (although it might be possible to combine these in the future).

The call to IsObjectAboutToBeFinalized() updates the pointer if it has been moved - I'm thinking about adding a new API like UpdateWeakPointerIfMoved() which would do the same thing but make it clearer what's going on.

> Also, aren't JS::Heap pointers automatically relocated?

As explained above, Heap<T> only relocates for pointers from tenured heap to nursery, to avoid having to keep track of every pointer in the system.
Attachment #8483514 - Attachment is obsolete: true
Attachment #8484968 - Flags: review?(bobbyholley)
Comment on attachment 8484968 [details] [diff] [review]
13-fixup-xpconnect v2

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

I just discussed this architecture for a while with terrence on irc [1]. In a nutshell, I think we should design the embedder interactions better so that we don't have to duplicate this code. We should have exactly one callback that lets SpiderMonkey say "yo XPConnect, I may have just collected or moved things you were pointing to. Pass them back to me and I'll fix them up". That callback (roughly based on XPCWrappedNativeScope::StartFinalizationPhaseOfGC) then iterates over the necessary stuff, and invokes JS_UpdateWeakPointerAfterGC (a renamed version of JS_IsAboutToBeFinalized) which either (a) nulls out the pointer, (b) moves it, or (c) leaves it alone. We would then invoke this both for finalization and for compaction.

In the long run, it would seem much more performant for SpiderMonkey to only invoke this callback once per GC, after both finalization and compaction have happened. In the short term, we should design the API such that such a change can happen transparently without requiring any changes in XPConnect. 

[1] http://logs.glob.uno/?c=mozilla%23jsapi&s=5+Sep+2014&e=5+Sep+2014#c469075
Attachment #8484968 - Flags: review?(bobbyholley) → review-
Depends on: 1061288
Attached patch 13-fixup-xpconnect v3 (obsolete) — Splinter Review
Thanks for the review comments.  I agree with you that it would be good to have a single callback to fixup all weak pointers but in general it's not quite so simple.  Sometimes other stuff happens in the finalization callback too and sometimes not all pointers need to be updated for finalization whereas they do for compacting.  So it's a non-trivial change.

What I've done here is to unify the sweeping and updating code inside XPConnect while leaving it with two separate entry points (finalization and moving GC callbacks).  If you're ok with it I'd like to land this as-is and make the change to a single 'update all weak pointers' callback in a following patch.

It looks like XPConnect does a bunch of other stuff in the finalize callback (e.g. deferred release of various things) so I think it will still need have both callbacks.
Attachment #8484968 - Attachment is obsolete: true
Attachment #8490211 - Flags: review?(bobbyholley)
Comment on attachment 8490211 [details] [diff] [review]
13-fixup-xpconnect v3

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

Overall this is much better - thanks.

The JS_IsObjectAboutToFinalized API is really confusing, and I don't want it to remain in the tree. Can you add an additional patch (on top of this one) that replaces:

bool JS_IsObjectAboutToBeFinalized(..) with
void JS_UpdateWeakPointerAfterGC(..)

Where the latter nulls out the inout param in the case of finalization, and callers can just do:

JSObject *old = cur;
JS_UpdateWeakPointerAfterGC(&cur);
if (!cur) {
  // finalization handling.
} else if (cur != old) {
  // move handling.
}

I recognize that these semantics differ slightly from the ones inside js/src/gc, but they should be easy enough to shim in at the jsapi.cpp level.

::: js/xpconnect/src/XPCMaps.cpp
@@ +84,5 @@
>  /***************************************************************************/
>  // implement JSObject2WrappedJSMap...
>  
>  void
> +JSObject2WrappedJSMap::UpdateWeakPointersAfterGC(nsTArray<nsXPCWrappedJS*>* dying)

Given the sharing here and that the whole "dying" bit doesn't happen when this is called from the MovingGC callback, I think this should take a reference to the XPCJSRuntime and access the map that way as-needed. We may need to make this a friend class or add a Map& accessor to the map - either is fine with me.

@@ +102,5 @@
> +            if (!wrapper->IsSubjectToFinalization()) {
> +                JSObject *obj = wrapper->GetJSObjectPreserveColor();
> +                JSObject *prior = obj;
> +                MOZ_ASSERT(!JS_IsAboutToBeFinalizedUnbarriered(&obj));
> +                MOZ_ASSERT(obj == prior);

I don't understand - why can't the JSObject of a strongly-held XPCWrappedJS be moved?

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +1334,5 @@
>          p->JSProtoObjectFinalized(fop, obj);
>  }
>  
>  static void
> +XPC_WN_Shared_Proto_ObjectMoved(JSObject *obj, const JSObject *old)

Don't we also need to do something similar for the XPC_WN_{No,}Helper classes and XPC_WN_Tearoff_JSClass?

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +491,2 @@
>  {
> +    // If this is called from the finalization callcack in JSGC_MARK_END then

s/callcack/callback/

@@ +504,5 @@
>              cur->mWaiverWrapperMap->Sweep();
>  
>          XPCWrappedNativeScope* next = cur->mNext;
>  
>          if (cur->mGlobalJSObject && cur->mGlobalJSObject.isAboutToBeFinalized()) {

Add a comment here indicating that globals can never be moved, so we don't need to worry about that here?

@@ +524,5 @@
>  }
>  
>  // static
>  void
> +XPCWrappedNativeScope::StartFinalizationPhaseOfGC(XPCJSRuntime* rt)

Can we kill this function and just inline the call to UpdateWeakPointersAfterGC(rt) directly into the callsite?
Attachment #8490211 - Flags: review?(bobbyholley) → review-
(In reply to Boris Zbarsky [:bz] from comment #88)

> >+    return CGIfWrapper(CGGeneric(objectMoved), "self")
> 
> Why do you need the CGIfWrapper?  "self" can't be null here, afaict. If this
> _is_ needed for some odd reason, that odd reason should be very clearly
> documented.

BTW I found during testing that self is sometimes null here.  I don't know why that should be, but the same test is made in the finalize hook so I assume there is some reason for it.
This patch adds AutoAssertGCCallback, which asserts that something is only ever called from a GC callback.  In addition it suppresses static hazard checking and turns on assertions on GC thing allocation in its lifetime (not just on GC).  The reason is so that we can tell the analysis to ignore what it thinks are hazards in object moved hooks but are really just unknown field calls.

I also changed AutoSuppressGCAnalysis to assert on allocation not just GC as this is a stronger test that no GC is possible.  An initial try run was green.

Finally I changed a couple of assertions inside the GC to assert in release builds too since the overhead is negligible here and this should give us more confidence that nothing is going awry.
Attachment #8491588 - Flags: review?(terrence)
Comment on attachment 8491588 [details] [diff] [review]
11.5-gc-callback-assertions

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

Great!
Attachment #8491588 - Flags: review?(terrence) → review+
Depends on: 1069655
Attachment #8491588 - Flags: checkin+
Attachment #8484956 - Flags: checkin+
Attachment #8483533 - Flags: checkin+
Attachment #8483534 - Flags: checkin+
(In reply to Bobby Holley (:bholley) from comment #96)
> @@ +102,5 @@
> > +            if (!wrapper->IsSubjectToFinalization()) {
> > +                JSObject *obj = wrapper->GetJSObjectPreserveColor();
> > +                JSObject *prior = obj;
> > +                MOZ_ASSERT(!JS_IsAboutToBeFinalizedUnbarriered(&obj));
> > +                MOZ_ASSERT(obj == prior);
> 
> I don't understand - why can't the JSObject of a strongly-held XPCWrappedJS
> be moved?

They can, but they will have already been updated when they were marked.  I've added a comment.

> ::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
> @@ +1334,5 @@
> >          p->JSProtoObjectFinalized(fop, obj);
> >  }
> >  
> >  static void
> > +XPC_WN_Shared_Proto_ObjectMoved(JSObject *obj, const JSObject *old)
> 
> Don't we also need to do something similar for the XPC_WN_{No,}Helper
> classes and XPC_WN_Tearoff_JSClass?

XPC_WN_NoHelper_JSClass specifies an objectMovedOp hook and
XPCNativeScriptableShared::PopulateJSClass() does this for XPC_WN_Helper
classes.  But I missed the tearoffs, which I've fixed now.

Other comments addressed.
Attachment #8490211 - Attachment is obsolete: true
Attachment #8492227 - Flags: review?(bobbyholley)
Here's a patch to replace the moving GC callback with a weak pointer update callback that also gets called during finalization.  Unfortunately we need to keep the finalization callback too because that's used for more than just updating weak pointers.

This also replaces the public IsAboutToBeFinalized functions with their UpdatePointerAfterGC equivalent.
Attachment #8492230 - Flags: review?(terrence)
Attachment #8492230 - Flags: review?(bobbyholley)
Comment on attachment 8492230 [details] [diff] [review]
17-weak-pointer-callback

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

This is amazing. I had some ideas kicking around for bug 878120, but this is /way/ better.
Attachment #8492230 - Flags: review?(terrence) → review+
Depends on: 877924
Comment on attachment 8492227 [details] [diff] [review]
13-fixup-xpconnect v4

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

(In reply to Jon Coppeard (:jonco) from comment #103)
> XPC_WN_NoHelper_JSClass specifies an objectMovedOp hook and
> XPCNativeScriptableShared::PopulateJSClass() does this for XPC_WN_Helper
> classes.  But I missed the tearoffs, which I've fixed now.

Oh I see, that part's already on trunk.

This looks great. Thanks for pushing this through. r=bholley with comments addressed.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +3246,5 @@
>      JS_SetDestroyCompartmentCallback(runtime, CompartmentDestroyedCallback);
>      JS_SetCompartmentNameCallback(runtime, CompartmentNameCallback);
>      mPrevGCSliceCallback = JS::SetGCSliceCallback(runtime, GCSliceCallback);
>      JS_AddFinalizeCallback(runtime, FinalizeCallback, nullptr);
> +    JS_AddMovingGCCallback(runtime, MovingGCCallback, this);

I assume there can be multiple of such callbacks? Otherwise, this should just be "Set".

::: js/xpconnect/src/xpcprivate.h
@@ +629,5 @@
>  
>      PRTime GetWatchdogTimestamp(WatchdogTimestampCategory aCategory);
>      void OnAfterProcessNextEvent() { mSlowScriptCheckpoint = mozilla::TimeStamp(); }
>  
> +    nsTArray<nsXPCWrappedJS*>& GetWrappedJSToReleaseArray() { return mWrappedJSToReleaseArray; }

Nit - Remove the "Get" from this name.
Attachment #8492227 - Flags: review?(bobbyholley) → review+
Comment on attachment 8492230 [details] [diff] [review]
17-weak-pointer-callback

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

\o/ \o/ \o/

This is awesome. Thanks for taking us to a better place!

::: js/ipc/WrapperOwner.cpp
@@ +696,5 @@
> +WrapperOwner::updatePointer(JSObject *obj, const JSObject *old)
> +{
> +    ObjectId objId = idOfUnchecked(obj);
> +    MOZ_ASSERT(findCPOWById(objId) == old);
> +    cpows_.add(objId, obj);

This just overwrites the old value, right?
Attachment #8492230 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) from comment #108)

> > +    JS_AddMovingGCCallback(runtime, MovingGCCallback, this);
>
> I assume there can be multiple of such callbacks? Otherwise, this should
> just be "Set".

Yes, there can be multiple callbacks set.

> ::: js/ipc/WrapperOwner.cpp
> @@ +696,5 @@
> > +WrapperOwner::updatePointer(JSObject *obj, const JSObject *old)
> > +{
> > +    ObjectId objId = idOfUnchecked(obj);
> > +    MOZ_ASSERT(findCPOWById(objId) == old);
> > +    cpows_.add(objId, obj);
> 
> This just overwrites the old value, right?

That's right.

Thanks for the comments!
Attachment #8492227 - Flags: checkin+
Attachment #8492230 - Flags: review+
Attachment #8492230 - Flags: review+ → checkin+
I just tried running with and without CGC on
http://www.gregor-wagner.com/tmp/mem50. Contrary to expecations, after closing
all the windows and doing "minimize memory usage" multiple times, the results
with CGC were *worse*, partly because there are some compartments not being killed:

> ├───64.38 MB (15.72%) -- window-objects
> │   ├──30.40 MB (07.42%) -- top(none)/detached/window([system])
> │   │  ├──16.06 MB (03.92%) -- (12 tiny)
> │   │  │  ├───3.62 MB (00.88%) ++ js-compartment(http://www.youku.com/)
> │   │  │  ├───2.19 MB (00.53%) ++ js-compartment(http://www.spiegel.de/)
> │   │  │  ├───2.17 MB (00.53%) ++ js-compartment(http://www.microsoft.com/en-au/default.aspx)
> │   │  │  ├───1.86 MB (00.45%) ++ js-compartment(http://www.britishairways.com/travel/home/public/en_gb)
> │   │  │  ├───1.83 MB (00.45%) ++ js-compartment(http://www.kleinezeitung.at/)
> │   │  │  ├───1.18 MB (00.29%) ++ dom
> │   │  │  ├───0.70 MB (00.17%) ++ js-compartment(https://www.youtube.com/watch?v=a1Y73sPHKxw, about:blank)
> │   │  │  ├───0.67 MB (00.16%) ++ js-compartment(http://www.bild.de/, about:blank)
> │   │  │  ├───0.65 MB (00.16%) ++ js-compartment(http://techcrunch.com/wp-content/themes/vip/techcrunch-2013/_uac/adpage.html, about:blank)
> │   │  │  ├───0.59 MB (00.14%) ++ js-compartment(http://platform.twitter.com/widgets/hub.html)
> │   │  │  ├───0.34 MB (00.08%) ++ js-compartment(http://imagesrv.adition.com/js/acb/uid.html)
> │   │  │  └───0.26 MB (00.06%) ++ js-compartment(http://www.britishairways.com/travel/home/public/en_gb, about:blank)
> │   │  ├───5.44 MB (01.33%) ++ js-compartment(https://www.youtube.com/watch?v=a1Y73sPHKxw)
> │   │  ├───4.47 MB (01.09%) ++ js-compartment(http://techcrunch.com/2011/07/07/facebook-now-lets-app-developers-see-their-spam-scores/)
> │   │  └───4.43 MB (01.08%) ++ js-compartment(http://www.bild.de/)

I checked multiple times in each configuration; this always happens with the
CGC buids, and never happens with the normal builds.
(In reply to Nicholas Nethercote [:njn] from comment #113)
Thanks for the report, I'll look into it.
In the general case we can't move global objects because their address may be baked into JIT code, and so assume they can't move in a bunch of places.  Therefore it's not safe to move then at any time, even if the JITs are disabled.
Attachment #8496063 - Flags: review?(terrence)
Since we never move global objects, they don't need to implement the objectMovedOp hook.  This means those changes to Sandbox weren't necessary and can come out.
Attachment #8496065 - Flags: review?(terrence)
Attachment #8496065 - Flags: review?(bobbyholley)
Fixup type object pointers that aren't updated during sweeping and make sure we don't delete any data if we're not sweeping.
Attachment #8496067 - Flags: review?(terrence)
Fix interactions between typed objects and moving GC after latest updates.
Attachment #8496071 - Flags: review?(terrence)
Attachment #8496063 - Flags: review?(terrence) → review+
Attachment #8496065 - Flags: review?(terrence) → review+
Comment on attachment 8496067 [details] [diff] [review]
20-fixup-type-objects

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

::: js/src/jsinfer.cpp
@@ +4846,5 @@
> +    /* preliminaryObjects are handled by sweep(). */
> +    if (templateObject_ && IsForwarded(templateObject_.get()))
> +        templateObject_ = Forwarded(templateObject_.get());
> +    if (initializedShape_ && IsForwarded(initializedShape_.get()))
> +        initializedShape_ = Forwarded(initializedShape_.get());

These should almost certainly be unbarrieredGets for performance and general sanity.

@@ +4859,5 @@
> +        singleton_ = Forwarded(singleton_.get());
> +    if (newScript_)
> +        newScript_->fixupAfterMovingGC();
> +    if (interpretedFunction && IsForwarded(interpretedFunction.get()))
> +        interpretedFunction = Forwarded(interpretedFunction.get());

Ditto.
Attachment #8496067 - Flags: review?(terrence) → review+
Attachment #8496071 - Flags: review?(terrence) → review+
> In the general case we can't move global objects because their address may
> be baked into JIT code, and so assume they can't move in a bunch of places. 
> Therefore it's not safe to move then at any time, even if the JITs are
> disabled.

Does that explain the compartment leaks I saw? (I hope so :)
(In reply to Terrence Cole [:terrence] from comment #119)
I see what you mean, but these actually aren't ReadBarriered but HeapPtr members.
Attachment #8496065 - Flags: review?(bobbyholley) → review+
Attachment #8496063 - Flags: checkin+
Attachment #8496065 - Flags: checkin+
Attachment #8496067 - Flags: checkin+
Attachment #8496071 - Flags: checkin+
Unbreak CGC builds after recent changes.
Attachment #8498932 - Flags: review?(terrence)
We don't need a separate fixup method for CCWs if we make our normal mark path work properly.
Attachment #8498933 - Flags: review?(terrence)
This makes ObjectImpl::toDictionaryMode() work with compacting GC.  The reason this was an issue was that the last shape in the list has its listp pointer set to point to a location on the stack while the list is being created, which wasn't handled by the shape fixup code.  This was the only place this happened and was fairly insane.

I removed this and only set the object.shape_ <-> shape.listp link at the end when everything has been created.
Attachment #8498936 - Flags: review?(terrence)
It's possible for a GC to happen while creating a DOM object before the wrapper cache has been initialized, so we need to allow UpdateWrapper() to handle a null pointer here.
Attachment #8498937 - Flags: review?(bzbarsky)
It seems that TheadHangMonitor can end up calling JS_GetScriptFilename() from another thread if it detects a hang.  If this happens while we're running a compacting GC this will then crash in the hang monitor and hide the original problem :(

This patch allows JS_GetScriptFilename() to be called like this to give some chance of this working (useful in case we hang while compacting), although it's obviously still racey.
Attachment #8498942 - Flags: review?(terrence)
Comment on attachment 8498937 [details] [diff] [review]
25-fix-uninitialized-wrapper-update

Ah, indeed.

r=me
Attachment #8498937 - Flags: review?(bzbarsky) → review+
Comment on attachment 8498932 [details] [diff] [review]
22-fixup-after-sweeping-changes

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

D'oh! Sorry, I totally forgot to even build test with compacting.
Attachment #8498932 - Flags: review?(terrence) → review+
Attachment #8498933 - Flags: review?(terrence) → review+
Comment on attachment 8498936 [details] [diff] [review]
24-fix-toDictionaryMode

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

Nice!
Attachment #8498936 - Flags: review?(terrence) → review+
Comment on attachment 8498942 [details] [diff] [review]
26-allow-get-script-filename-in-cgc

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

Yeah, that code has always bothered me. This seems like a reasonable workaround though, given that we have to support it.
Attachment #8498942 - Flags: review?(terrence) → review+
Attachment #8498932 - Flags: checkin+
Attachment #8498933 - Flags: checkin+
Attachment #8498936 - Flags: checkin+
Attachment #8498937 - Flags: checkin+
Attachment #8498942 - Flags: checkin+
Fix for build errors after recent JSObject changes.
Attachment #8499645 - Flags: review?(terrence)
I just tried MemBench (http://www.gregor-wagner.com/tmp/mem50) again with mozilla-inbound and the 27-fixup-after-jsobject-changes patch. I still get leaked compartments after closing all the test windows (including MemBench itself):

> ├───39.83 MB (09.76%) -- window-objects
> │   ├──30.85 MB (07.56%) -- top(none)/detached/window([system])
> │   │  ├──16.18 MB (03.96%) -- (12 tiny)
> │   │  │  ├───2.48 MB (00.61%) ++ js-compartment(http://www.msn.com/en-au/)
> │   │  │  ├───2.30 MB (00.56%) ++ js-compartment(http://www.spiegel.de/)
> │   │  │  ├───2.13 MB (00.52%) ++ js-compartment(http://www.microsoft.com/en-au/default.aspx)
> │   │  │  ├───1.89 MB (00.46%) ++ js-compartment(http://www.kleinezeitung.at/)
> │   │  │  ├───1.85 MB (00.45%) ++ js-compartment(http://www.britishairways.com/travel/home/public/en_gb)
> │   │  │  ├───1.70 MB (00.42%) ++ dom
> │   │  │  ├───0.78 MB (00.19%) ++ js-compartment(http://www.bing.com/)
> │   │  │  ├───0.71 MB (00.17%) ++ js-compartment(https://www.youtube.com/watch?v=a1Y73sPHKxw, about:blank)
> │   │  │  ├───0.70 MB (00.17%) ++ js-compartment(http://www.msn.com/en-au/, about:blank)
> │   │  │  ├───0.68 MB (00.17%) ++ js-compartment(http://techcrunch.com/wp-content/themes/vip/techcrunch-2013/_uac/adpage.html, about:blank)
> │   │  │  ├───0.67 MB (00.16%) ++ js-compartment(http://www.bild.de/, about:blank)
> │   │  │  └───0.30 MB (00.07%) ++ js-compartment(http://platform.twitter.com/widgets/hub.1c5a573e465d84666458a45e49b0a735.html)
> │   │  ├───5.41 MB (01.32%) ++ js-compartment(https://www.youtube.com/watch?v=a1Y73sPHKxw)
> │   │  ├───4.72 MB (01.16%) ++ js-compartment(http://www.bild.de/)
> │   │  └───4.54 MB (01.11%) ++ js-compartment(http://techcrunch.com/2011/07/07/facebook-now-lets-app-developers-see-their-spam-scores/)
Attachment #8499645 - Flags: review?(terrence) → review+
(In reply to Nicholas Nethercote [:njn] from comment #136)
Thanks for reporting this, I'm investigating currently.
Apparently the timeouts are because compacting mode was recently made the default for the SM(ggc) jobs, so I've re-landed comment 137 and will hide the job on TBPL for now per discussion on IRC with sfink and terrence.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b19c3336ea3f
Depends on: 1079224
Attached patch 28-misc-fixesSplinter Review
Fix build errors due to recent changes and test failure in TypedObject/inlineopaque.js
Attachment #8504137 - Flags: review?(terrence)
Attachment #8504137 - Flags: review?(terrence) → review+
Attachment #8499645 - Flags: checkin+
Attachment #8504137 - Flags: checkin+
Help output for the new zeal (mode 14) appears to be missing. I guess this is sitting in your patch queue somewhere, Jon?
Flags: needinfo?(jcoppeard)
Depends on: 1083716
Attached patch 29-zeal-mode-help (obsolete) — Splinter Review
There is already some help text for this, but it's wrong so here's an update.
Flags: needinfo?(jcoppeard)
Attachment #8506049 - Flags: review?(terrence)
While iterating through the set of debuggees in Debugger::getDebuggees() we have to be careful not to do anything that could cause a GC since that could also mutate the set by rekeying a moved object.
Attachment #8506052 - Flags: review?(terrence)
Don't try to relocate anything in the self-hosting zone as this can be shared by multiple runtimes.
Attachment #8506054 - Flags: review?(terrence)
Remove the restriction on relocating global objects (SM changes).
Attachment #8506057 - Flags: review?(terrence)
Now it's possible to relocate global objects so we do need those changes to Sandbox that I removed earlier after all.  Sorry about that!

Also I needed to change BackstagePass so that there is only single pointer to its global object (from the wrapped native) and the existing infrastructure to update it is sufficient if it moves.  I did this by changing the JSObject pointer here to be a pointer to the wrapped native instead.  This may not be the best solution so let me know if you think we should do this another way.
Attachment #8506059 - Flags: review?(bobbyholley)
Comment on attachment 8506059 [details] [diff] [review]
33-relocate-global-objects-browser

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

::: js/xpconnect/src/XPCRuntimeService.cpp
@@ +58,5 @@
> +BackstagePass::SetGlobalObject(JSObject* global)
> +{
> +    nsISupports* p = static_cast<nsISupports*>(xpc_GetJSPrivate(global));
> +    MOZ_ASSERT(p);
> +    mWrapper = static_cast<XPCWrappedNative*>(p);

This should be XPCWrappedNative::Get(global).
Attachment #8506059 - Flags: review?(bobbyholley) → review+
Comment on attachment 8506049 [details] [diff] [review]
29-zeal-mode-help

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

Ah, I meant the missing 14 in jsgc.cpp in initZeal: it's not currently listed when you do JS_GC_ZEAL=help.
Attachment #8506049 - Flags: review?(terrence)
Attachment #8506052 - Flags: review?(terrence) → review+
Attachment #8506054 - Flags: review?(terrence) → review+
Attachment #8506057 - Flags: review?(terrence) → review+
Comment on attachment 8506052 [details] [diff] [review]
30-fix-get-debuggees-interaction

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

::: js/src/vm/Debugger.cpp
@@ +2237,5 @@
> +    if (!debuggees.resize(count))
> +        return false;
> +    unsigned i = 0;
> +    for (GlobalObjectSet::Enum e(dbg->debuggees); !e.empty(); e.popFront())
> +        debuggees[i++].setObject(*e.front());

Can you wrap this in

   {
       AutoCheckCannotGC nogc;
       for (GlobalObjectSet::Enum....)
   }

not because I think it will catch anything, but because it documents exactly which part of the method is not allowed to GC. I read the comment and at first thought that the later loop must not be able to GC.
Comment on attachment 8506057 [details] [diff] [review]
32-relocate-global-objects

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

::: js/src/jsgc.cpp
@@ +2072,5 @@
>  {
>      /*
>       * We can't currently move global objects because their address can be baked
>       * into compiled code so we skip relocation of any area containing one.
>       */

Comment needs to be removed.
Fix breakage following generator changes.
Attachment #8506834 - Flags: review?(terrence)
Oh, we have help text for this in two places.  Let's fix that.
Attachment #8506049 - Attachment is obsolete: true
Attachment #8506835 - Flags: review?(terrence)
Comment on attachment 8506835 [details] [diff] [review]
29-zeal-mode-help v2

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

That also works. :-)
Attachment #8506835 - Flags: review?(terrence) → review+
Attachment #8506834 - Flags: review?(terrence) → review+
Attachment #8506052 - Flags: checkin+
Attachment #8506054 - Flags: checkin+
Attachment #8506057 - Flags: checkin+
Attachment #8506059 - Flags: checkin+
Attachment #8506834 - Flags: checkin+
Attachment #8506835 - Flags: checkin+
This replaces the current scheme of keeping wrapped JSObjects alive with an explicit tracer that marks the contents of the sJSObjWrappers table and rekeys it as necessary when objects are moved.

I added an ObjectMoved hook for the JSObject wrappers of NPObjects to update their hash table entries as necessary.

Also I refactored the way the wrapper tables were created so there are separate functions to create and destroy each table.
Attachment #8508056 - Flags: review?(benjamin)
Depends on: 1087361
Disable gczeal for the end of xdr/lazy.js, because GC can change the expected test result.
Attachment #8509612 - Flags: review?(terrence)
This is a patch to root JSRuntimeWrapper::mGlobal.  I had to refactor how this works slightly so that JSRuntime is created before the wrapper as the JS::PersistentRooted takes a pointer to the runtime in its constructor.
Attachment #8509620 - Flags: review?(sworkman)
Add a way to pick a sensible set of arenas to relocate.

Do this by relocating as many of the least-used arenas as we can so that their contents will fit into arenas we're not relocating.

Note that I made this still relocate everything when zeal mode is on, and *in debug mode on 64 bit platforms*.  This is so we continue to get good test coverage of moving objects.  I appreciate that this is a little unusual so if you're not happy we can change it.
Attachment #8509627 - Flags: review?(terrence)
Don't update pointers for all thing kinds, only those that can contain object pointers that might have moved.  In practice that only means skipping strings, but that's still a win.
Attachment #8509630 - Flags: review?(terrence)
Attachment #8509612 - Flags: review?(terrence) → review+
Comment on attachment 8509627 [details] [diff] [review]
37-pick-arenas-to-relocate

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

No, that's great! I love how this leverages :ehoogeveen's sorting work.
Attachment #8509627 - Flags: review?(terrence) → review+
Attachment #8509620 - Flags: review?(sworkman) → review+
Attachment #8509630 - Flags: review?(terrence) → review+
The incremental type information sweeping changes in bug 1072564 broke compacting GC, so this is a fix for that.

This runs all the sweeping code in one shot to update weak references.
Attachment #8510220 - Flags: review?(bhackett1024)
Attachment #8510220 - Flags: review?(bhackett1024) → review+
Backed out 36-fix-proxy-auto-config for causing ASAN LeakSanitizer failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/927503f08f55
Attachment #8509612 - Flags: checkin+
Attachment #8509627 - Flags: checkin+
Attachment #8509630 - Flags: checkin+
Attachment #8510220 - Flags: checkin+
Attachment #8509620 - Flags: checkin+
Depends on: 1087948
This patch fixes the issue where a partly-initialized DOM objects is moved before it has had its DOM_OBJECT_SLOT has been set.
Attachment #8511878 - Flags: review?(bzbarsky)
Fix a couple of places where moving objects that are partly-initialized causes us to try to read private values that have not been set.
Attachment #8511880 - Flags: review?(terrence)
Comment on attachment 8511878 [details] [diff] [review]
39-fix-partly-initialized-dom-object

Please name this function UnwrapPossiblyNotInitializedDOMObject.

Please put curlies around the if body, and add a comment there to the effect that we're getting a JSClass hook called before JS_NewObject has returned and hence we haven't had a chance to actually set our DOM_OBJECT_SLOT to anything useful yet.

r=me with that
Attachment #8511878 - Flags: review?(bzbarsky) → review+
Attachment #8511880 - Flags: review?(terrence) → review+
Fix bug choosing which arenas to relocate when there aren't any to choose from.
Attachment #8512881 - Flags: review?(terrence)
Include the gc invocation kind in the GC timer log output.
Attachment #8512882 - Flags: review?(terrence)
Similar to the recent issue for DOM objects, sandbox objects can also have their object moved hook called before the object has finished being initialized.  In this case the private pointer will be null so we need to check that.
Attachment #8512887 - Flags: review?(bobbyholley)
Update the list of test timeouts to ignore to include a couple more.
Attachment #8512890 - Flags: review?(terrence)
Comment on attachment 8508056 [details] [diff] [review]
34-fix-plugin-tracing

I'm convinced that I won't have the time to do this review properly, and I only barely understand the relevant JS APIs. So I'm going to bump this jschoenick who hopefully understand this better and/or has good review energy.
Attachment #8508056 - Flags: review?(benjamin) → review?(jschoenick)
Attachment #8512881 - Flags: review?(terrence) → review+
Attachment #8512882 - Flags: review?(terrence) → review+
Attachment #8512890 - Flags: review?(terrence) → review+
Attachment #8511878 - Flags: checkin+
Attachment #8511880 - Flags: checkin+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #183)
> Comment on attachment 8508056 [details] [diff] [review]
> 34-fix-plugin-tracing
> 
> I'm convinced that I won't have the time to do this review properly, and I
> only barely understand the relevant JS APIs. So I'm going to bump this
> jschoenick who hopefully understand this better and/or has good review
> energy.

This patch is nearly 100% managing a hash table of generic objects and handling all the moving-GC callback JSAPI magic -- which I am barely familiar with. I'm going to say someone familiar with the relevant JSAPI should review this, and ask for plugin-peer feedback if its possibly changing existing behavior.

The only plugins-specific caveat I can think of is to be aware that NPAPI calls (_createobject, _alloc*, etc) can GC, and might not be seen as such by the static analysis.
Flags: needinfo?(jcoppeard)
(In reply to John Schoenick [:johns] from comment #185)
> The only plugins-specific caveat I can think of is to be aware that NPAPI
> calls (_createobject, _alloc*, etc) can GC, and might not be seen as such by
> the static analysis.

Slander!

<sfink> can _createobject gc?
<mrgiggles> Yes, |NPObject* mozilla::plugins::parent::_createobject(_NPP*, NPClass*)| can GC: _ZN7mozilla7plugins6parent13_createobjectEP4_NPPP7NPClass|NPObject* mozilla::plugins::parent::_createobject(_NPP*, NPClass*)
GC Function: _ZN7mozilla7plugins6parent13_createobjectEP4_NPPP7NPClass|NPObject* mozilla::plugins::parent::_createobject(_NPP*, NPClass*)
FieldCall: NPClass.allocate

I suspect there are enough function pointers in NPAPI to make the analysis assume anything can GC. What are specific _alloc* function names? I couldn't see them in dom/plugins.
(In reply to Steve Fink [:sfink, :s:] from comment #186)
> I suspect there are enough function pointers in NPAPI to make the analysis
> assume anything can GC. What are specific _alloc* function names? I couldn't
> see them in dom/plugins.

I was thinking of e.g. | npobj->_class->deallocate |. You'd think that surely a function named deallocate couldn't spin the event loop, but...
(In reply to John Schoenick [:johns] from comment #187)
> (In reply to Steve Fink [:sfink, :s:] from comment #186)
> > I suspect there are enough function pointers in NPAPI to make the analysis
> > assume anything can GC. What are specific _alloc* function names? I couldn't
> > see them in dom/plugins.
> 
> I was thinking of e.g. | npobj->_class->deallocate |. You'd think that
> surely a function named deallocate couldn't spin the event loop, but...

It believes mozilla::plugins::parent::_releaseobject(NPObject*) can GC, thanks to the NPClass.deallocate function pointer.
(In reply to John Schoenick [:johns] from comment #185)
> This patch is nearly 100% managing a hash table of generic objects and
> handling all the moving-GC callback JSAPI magic -- which I am barely
> familiar with. I'm going to say someone familiar with the relevant JSAPI
> should review this, and ask for plugin-peer feedback if its possibly
> changing existing behavior.

Ok, I'll do that but it would be great if someone could look at the non-JSAPI bits (my refactoring of the way the hash tables get created/destroyed for example) and check they are ok.  I'm not that familiar with the plugin code and I don't want to accidentally break this.
Flags: needinfo?(jcoppeard)
Attachment #8508056 - Flags: review?(terrence)
Fix for test failures caused by inline typed object changes.
Attachment #8514344 - Flags: review?(bhackett1024)
Attachment #8512887 - Flags: review?(bobbyholley) → review+
Attachment #8514344 - Flags: review?(bhackett1024) → review+
Patch to mprotect relocated arenas and keep them around to the next GC cycle to catch anything that tries to access them.  Debug only.
Attachment #8514346 - Flags: review?(terrence)
Comment on attachment 8508056 [details] [diff] [review]
34-fix-plugin-tracing

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

Looks like a nice cleanup to me.

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +255,5 @@
> +    if (wrapper->mJSObj) {
> +      JS_CallObjectTracer(trc, &wrapper->mJSObj, "sJSObjWrappers wrapper object");
> +    }
> +    if (key != e.front().key())
> +      e.rekeyFront(key);

I think the local style is {} around all blocks.

@@ +288,5 @@
> +
> +  static const char rtsvc_id[] = "@mozilla.org/js/xpc/RuntimeService;1";
> +  nsCOMPtr<nsIJSRuntimeService> rtsvc(do_GetService(rtsvc_id));
> +  if (!rtsvc)
> +    return false;

Ditto.

@@ +296,5 @@
> +  MOZ_ASSERT(jsRuntime != nullptr);
> +
> +  // Register a callback to trace wrapped JSObjects.
> +  if (!JS_AddExtraGCRootsTracer(jsRuntime, TraceJSObjWrappers, nullptr))
> +    return false;

Ditto.

@@ +316,5 @@
> +  JSRuntime *jsRuntime = nullptr;
> +  sCallbackRuntime->GetRuntime(&jsRuntime);
> +  MOZ_ASSERT(jsRuntime != nullptr);
> +
> +  // Remove tracing callaback.

*callaback

@@ +337,5 @@
> +    return false;
> +  }
> +
> +  if (!sJSObjWrappers.init(16)) {
> +    NS_ERROR("Error initializing PLDHashTable!");

You should probably include the table name here, in case this shows up in OOM|small.
Attachment #8508056 - Flags: review?(terrence) → review+
Comment on attachment 8514346 [details] [diff] [review]
46-keep-relocated-decommitted

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

Nice!

::: js/src/gc/GCRuntime.h
@@ +853,5 @@
>      CallbackVector<JSTraceDataOp> blackRootTracers;
>      Callback<JSTraceDataOp> grayRootTracer;
>  
> +    /* Always preserve JIT code during GCs, for testing. */
> +    bool                  alwaysPreserveCode;

Looks like you should probably just use one space here.
Attachment #8514346 - Flags: review?(terrence) → review+
Comment on attachment 8514346 [details] [diff] [review]
46-keep-relocated-decommitted

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

Nice!

::: js/src/gc/GCRuntime.h
@@ +853,5 @@
>      CallbackVector<JSTraceDataOp> blackRootTracers;
>      Callback<JSTraceDataOp> grayRootTracer;
>  
> +    /* Always preserve JIT code during GCs, for testing. */
> +    bool                  alwaysPreserveCode;

Looks like you should probably just use one space here.

::: js/src/jsgc.cpp
@@ +2548,5 @@
> +#else  // assume Unix
> +        if (mprotect(arena, ArenaSize, PROT_READ | PROT_WRITE))
> +            MOZ_CRASH();
> +#endif
> +    }

We might want to create wrappers for these in gc/Memory, although I guess for a single instance for DEBUG-only it doesn't really matter. It would probably help out anyone porting to an incompatible unix, but we can handle that later just as easily if it ever actually comes up.
Depends on: 1092168
Comment on attachment 8508056 [details] [diff] [review]
34-fix-plugin-tracing

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

I couldn't find any issues with the non-jsapi bits
Attachment #8508056 - Flags: review?(jschoenick) → review+
And backed out again for causing hazards:

https://hg.mozilla.org/integration/mozilla-inbound/rev/054bd325286d
Attachment #8508056 - Flags: checkin+
Attachment #8512881 - Flags: checkin+
Attachment #8512882 - Flags: checkin+
Attachment #8512887 - Flags: checkin+
Attachment #8512890 - Flags: checkin+
Attachment #8514344 - Flags: checkin+
Attachment #8514346 - Flags: checkin+
PCWrappedNativeScope::mDomExpandoSet is a set of JSObject pointers and in case these objects move it is updated by JS_CallHashSetObjectTracer() which calls HashSet<T>::Enum::rekeyFront().

However rekeyFront() is currently passed the prior and new object pointers, when it expects lookup and key parameters.  This means it places the new entry in hash table according to the hash of the prior key value, making it present but unfindable.  XPCWrappedNativeScope::RemoveDomExpandoObject() will then fail to remove the entry from the set when it should and since these are GC roots we have a leak.

Prior and new pointers are totally correct when calling the very similar method HashSet<T>::rekeyIfMoved().

The patch fixes this and also asserts that RemoveDomExpandoObject() successfully removes an entry.
Attachment #8522270 - Flags: review?(terrence)
Comment on attachment 8522270 [details] [diff] [review]
47-fix-expando-leak

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

Great find!
Attachment #8522270 - Flags: review?(terrence) → review+
Depends on: 989390
Depends on: 1099463
Attachment #8522270 - Flags: checkin+
Parallise updating pointers to moved objects in cells.  This is the longest part of compacting and is also possible to perform in parallel.  This patch uses up to 8 threads and updates in batches of 256 arenas in release builds.
Attachment #8523981 - Flags: review?(terrence)
Comment on attachment 8523981 [details] [diff] [review]
48-update-cells-in-parallel

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

Woah! Amazing!

::: js/src/gc/Marking.cpp
@@ +192,5 @@
>      if (ThingIsPermanentAtom(thing))
>          return;
>  
> +    Zone *zone = thing->zoneFromAnyThread();
> +    DebugOnly<JSRuntime *> rt = trc->runtime();

I guess this used to be declared outside the #ifdef DEBUG. Might as well remove the DebugOnly now.

::: js/src/jsgc.cpp
@@ +2510,5 @@
> +
> +ArenaHeader *
> +ArenasToUpdate::next()
> +{
> +    // Find the next arena to udpate.

udpate

@@ +2513,5 @@
> +{
> +    // Find the next arena to udpate.
> +    //
> +    // Note that this uses a generator-like arrangement. The first time this is
> +    // called |initialized| is false and the for-loops below are entered in the

, after called.

@@ +2514,5 @@
> +    // Find the next arena to udpate.
> +    //
> +    // Note that this uses a generator-like arrangement. The first time this is
> +    // called |initialized| is false and the for-loops below are entered in the
> +    // normal way, returning the first arena found. In susequent invocations we

susequent

@@ +2682,5 @@
>      for (CompartmentsIter comp(rt, SkipAtoms); !comp.done(); comp.next())
>          comp->sweepCrossCompartmentWrappers();
>  
> +    // Iterate through all cells that can contain JSObject pointers to update
> +    // them.  Since updating each cell is independent we try to parallelize this

Extra space.
Attachment #8523981 - Flags: review?(terrence) → review+
I've done some measurements. It's hard to get good data because they're so noisy. I tried starting the browser, opening gmail.com, switching to valgrind.com (a much simper site; with e10s you have to keep some content page open so that child doesn't get killed) and then measuring. This is the high-level JS diff:

> -1.74 MB (100.0%) -- js-main-runtime-gc-heap-committed
> ├──-1.75 MB (100.17%) -- unused
> │  ├──-0.92 MB (52.69%) ── arenas
> │  └──-0.83 MB (47.48%) ── gc-things
> └───0.00 MB (-0.17%) -- used
>     ├──0.02 MB (-0.90%) ── chunk-admin
>     ├──-0.01 MB (00.74%) ── arena-admin
>     └──0.00 MB (-0.01%) ── gc-things
Depends on: 1101585
Use background sweeping for compacting GC.  With bug 989390 this is part of the preparation for having an incremental compacting GC.
Attachment #8526746 - Flags: review?(terrence)
Remove an assertion that things don't move :)
Attachment #8526747 - Flags: review?(terrence)
Comment on attachment 8526746 [details] [diff] [review]
49-allow-background-sweeping

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

LGTM.

::: js/src/jsgc.cpp
@@ +5849,5 @@
>          }
>          break;
>        }
>  
> +#ifdef JSGC_COMPACTING

Thinking about it a bit, I think it would make sense to have the compacting infrastructure built into the GC on tip and just have compactingDisabled=true unconditionally with a few #ifndef JSGC_COMPACTING assertions to that effect. It would clean out a bunch of this sprinkled #ifdefs and I don't think we're depending on any conditional infrastructure for this? The same thing probably goes for JSGC_INCREMENTAL, maybe even JSGC_GENERATIONAL.
Attachment #8526746 - Flags: review?(terrence) → review+
Comment on attachment 8526747 [details] [diff] [review]
50-fix-non-heap-tracing

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

:-)
Attachment #8526747 - Flags: review?(terrence) → review+
Attachment #8523981 - Flags: checkin+
Attachment #8526746 - Flags: checkin+
Fix a bug that means that we relocated far fewer arenas than we should.  

followingUsedCells needs to be the count of used cells from |*arenap| onwards, so don't initialise it with the count of all cells if arenap starts at the cursor!
Attachment #8530925 - Flags: review?(terrence)
Attachment #8530925 - Flags: review?(terrence) → review+
Attachment #8530925 - Flags: checkin+
Add an API to disable compacting GC for embedders.
Attachment #8533229 - Flags: review?(terrence)
Fix a jsapi test that keeps an untraced pointer in a private value so that it works with compacting zeal mode enabled.
Attachment #8533232 - Flags: review?(terrence)
Attached patch 54-enable-compacting-gc (obsolete) — Splinter Review
Build and enable compacting GC by default.

Do we need additional review by a build peer to land this?
Attachment #8533233 - Flags: review?(terrence)
Depends on: 1080262
No longer depends on: 1106784
Attachment #8533229 - Flags: review?(terrence) → review+
Comment on attachment 8533232 [details] [diff] [review]
53-fix-weak-map-test

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

::: js/src/jsapi-tests/testWeakMap.cpp
@@ +133,3 @@
>  }
>  
> +

Extra \n.
Attachment #8533232 - Flags: review?(terrence) → review+
Comment on attachment 8533233 [details] [diff] [review]
54-enable-compacting-gc

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

Given that build-peers missed our missing js-config.h.in symbols, or given any feedback at all really, I'd guess sfink would be a better bet for catching real issues up front. That said, this is trivial, so I'm just going to r+ it.
Attachment #8533233 - Flags: review?(terrence) → review+
I did some quick tests to see how much of an improvement we got by using multiple threads to update cells.  Here are the results for updating ~10 million objects:

  Threads:   Average time /mS
             MacOSX:    Linux64:

        1     545.3       530.6
        2     407.6       389.0
        4     375.6       343.3
        8     390.3       343.3

Both systems are 4 core / 8 threading Intel processors.

From this it seems we don't get any benefit from going above 4 threads, so I think we should dial back the parallelism to one thread per two reported CPUs.
Attachment #8537744 - Flags: review?(terrence)
Something I meant to do before - add assertions that we are compacting as much as we should.
Attachment #8537746 - Flags: review?(terrence)
Comment on attachment 8537744 [details] [diff] [review]
55-reduce-parallel-udpate

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

Hyperthreading is most effective with asymmetric workloads, so some of this might be because we're running the same tight loop and might still be effective on non-HT machines. On the other hand, memory access is probably going to dominate regardless, so HT might not matter at all here.
Attachment #8537744 - Flags: review?(terrence) → review+
Comment on attachment 8537746 [details] [diff] [review]
56-assert-maximal-compaction

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

Nice!
Attachment #8537746 - Flags: review?(terrence) → review+
Attachment #8533229 - Flags: checkin+
Attachment #8533232 - Flags: checkin+
Attachment #8537744 - Flags: checkin+
Attachment #8537746 - Flags: checkin+
Fix reset inside incremental compacting GC.  At the moment the only incremental thing we do is to poll for the end of background sweeping, so the reset code just waits for that and then and then finishes the GC as a non-shrinking GC which calls finishCollection() and ends the GC correctly.
Attachment #8548358 - Flags: review?(terrence)
Zone merging doesn't take account of the fact that we may be keeping relocated arenas around and so doesn't update them.  Just release any relocated arenas if we merge.
Attachment #8548361 - Flags: review?(terrence)
We don't mark whatever Shape::listp points to, and ICs can mark a shape in dictionary mode shapes without without the rest of the dictionary mode shape list (or whatever it's called) getting marked.

Like the way we already null out shape_->listp in JSObject::finalize() for dictionary objects, we need to null out parent->listp if appropriate in Shape::finalize() to prevent moving GC trying to access these dead objects.
Attachment #8548363 - Flags: review?(terrence)
Attachment #8548358 - Flags: review?(terrence) → review+
Attachment #8548361 - Flags: review?(terrence) → review+
Comment on attachment 8548363 [details] [diff] [review]
59-fix-shape-listp

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

Ouch. Good find!
Attachment #8548363 - Flags: review?(terrence) → review+
Fix handling of OOM so that relocated areans are released first, otherwise empty chunks can be left triggering an assertion when decommitting.
Attachment #8549013 - Flags: review?(terrence)
Stop any off thread compilation which might be accessing things we are trying to relocate.
Attachment #8549016 - Flags: review?(terrence)
Comment on attachment 8549013 [details] [diff] [review]
60-release-arenas-first-on-OOM

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

Right.
Attachment #8549013 - Flags: review?(terrence) → review+
Comment on attachment 8549016 [details] [diff] [review]
61-stop-off-thread-compilation-while-compacting

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

Oh, good find.
Attachment #8549016 - Flags: review?(terrence) → review+
Attachment #8548358 - Flags: checkin+
Attachment #8548361 - Flags: checkin+
Attachment #8548363 - Flags: checkin+
Fix some misc errors after doing a more full try build:
 - mark some single argument constructors explicit
 - fix a build error on linux 32 due to header file includes (don't know why this doesn't fail on linux 64 builds)
 - fix a jsapi test failure that just started happening on ggc test runs
Attachment #8549759 - Flags: review?(terrence)
Rebased version of original patch, carrying r+.
Attachment #8533233 - Attachment is obsolete: true
Attachment #8549763 - Flags: review+
Comment on attachment 8549763 [details] [diff] [review]
54-enable-compacting-gc v2

Requesting review from build peer for config change to enable compacting GC unconditionally.
Attachment #8549763 - Flags: review?(mh+mozilla)
Attachment #8549759 - Flags: review?(terrence) → review+
Depends on: 1122196
No longer depends on: 1122196
Attachment #8549763 - Flags: review?(mh+mozilla) → review+
Attachment #8549013 - Flags: checkin+
Attachment #8549016 - Flags: checkin+
Alias: CompactingGC
Blocks: CompactingGC
And, with great mourning, backed out after 8 hours on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae4958ba3bf

Wider exposure turned up this rare crash:
https://treeherder.mozilla.org/logviewer.html#?job_id=5563530&repo=mozilla-inbound

It looks like the UpdateCellPointersTask is calling trace hooks in parallel. This is firing a ReentrancyGuard -- and more generally, probably causing corruption in the HashTable it is guarding. I guess we should only do parallel updating on arenas that are background finalizable to avoid any trace-hook weirdness.
(In reply to Terrence Cole [:terrence] from comment #251)

> It looks like the UpdateCellPointersTask is calling trace hooks in parallel.
> This is firing a ReentrancyGuard -- and more generally, probably causing
> corruption in the HashTable it is guarding. I guess we should only do
> parallel updating on arenas that are background finalizable to avoid any
> trace-hook weirdness.

That's a really good idea.  I don't think the wrapped native proto trace hook needs to be tracing the contents of its wrapped native scope either, but we'll see what bholley thinks about that.
This patch ensures that when updating cells in parallel that all foreground finalized arenas are updated on the same thread.  For simplicity it's not actually the main thread but the main thread is idle at this point.
Attachment #8551393 - Flags: review?(terrence)
The TraceInside() method on XPCWrappedNativeProto currently calls XPCWrappedNativeScope::TraceInside().  It seems to me like it could just trace the scope's global, since the trace hook for this will cause its contents to be traced.

I tested this on try and the results are good, although it wouldn't surprise me if there was some subtlety that means this doesn't work.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0aa03e61085
Attachment #8551400 - Flags: review?(continuation)
Attachment #8549759 - Flags: checkin+
Comment on attachment 8551400 [details] [diff] [review]
64-wrapped-native-scope-tracing

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

I'm afraid I don't know much about this.  Maybe Bill remembers something about it.
Attachment #8551400 - Flags: review?(continuation) → review?(wmccloskey)
Comment on attachment 8551400 [details] [diff] [review]
64-wrapped-native-scope-tracing

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

Thanks! This is the way things are supposed to work.
Attachment #8551400 - Flags: review?(wmccloskey) → review+
Attachment #8551393 - Flags: review?(terrence) → review+
Depends on: 1123648
Comment on attachment 8549016 [details] [diff] [review]
61-stop-off-thread-compilation-while-compacting

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

::: js/src/jsgc.cpp
@@ +2234,5 @@
>          MOZ_ASSERT(!zone->isPreservingCode());
>  
>          if (CanRelocateZone(rt, zone)) {
>              zone->setGCState(Zone::Compact);
> +            StopAllOffThreadCompilations(zone);

Missing #include "jit/Ion.h"
jit::StopAllOffThreadCompilations(zone);
Depends on: 1125588
This hasn't been backed out after a week so I'm marking as fixed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Keywords: leave-open
Target Milestone: --- → mozilla38
I just looked closely at AWSY and compacting GC made absolutely no difference to the results there. Hmm.

jonco, have you done any more measurements recently to see if compacting GC is having a noticeable effect?
(In reply to Nicholas Nethercote [:njn] from comment #263)
> I just looked closely at AWSY and compacting GC made absolutely no
> difference to the results there. Hmm.
> 
> jonco, have you done any more measurements recently to see if compacting GC
> is having a noticeable effect?

Hmm, I see bug 1055067 is still open -- maybe there's another switch that needs to be flicked for the browser?
(In reply to Nicholas Nethercote [:njn] from comment #264)
I think the problem is that at the moment shrinking GC is only triggered on memory pressure events and by GCRuntime::maybePeriodicFullGC().  The heuristics used by the latter may actually not trigger in practice.

I'll look into it, but I suspect the answer is we need to find a better way of triggering shrinking GCs.
Depends on: 1135707
You need to log in before you can comment on or make changes to this bug.