Compact the tenured heap when GCInvocationKind is GC_SHRINK

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: cdleary, Assigned: jonco)

Tracking

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

unspecified
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(68 attachments, 12 obsolete attachments)

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
bobbyholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
36.97 KB, patch
terrence
: review+
bobbyholley
: 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+
bobbyholley
: 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
bobbyholley
: 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
bhackett
: 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
bobbyholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
902 bytes, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.07 KB, patch
bhackett
: 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.
Posted 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.

Comment 9

8 years ago
(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: 619558
Summary: Add capability to relocate GCThings to new arenas → Compact the tenured heap when GCInvocationKind is GC_SHRINK
Depends on: 726687
Depends on: 723314
Duplicate of this bug: 668809
Whiteboard: [MemShrink]

Comment 16

5 years ago
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)

Updated

5 years ago
Assignee: sphink → jcoppeard
(Assignee)

Comment 18

5 years ago
Add --enable-gccompacting configure flag to the build.
Attachment #8467136 - Flags: review?(terrence)
(Assignee)

Comment 19

5 years ago
Add machinery for storing forwarding pointers in GC things.
Attachment #543999 - Attachment is obsolete: true
Attachment #8467137 - Flags: review?(terrence)
(Assignee)

Comment 20

5 years ago
Update our hash table checks for use after compacting GC as well as minor GC.
Attachment #8467138 - Flags: review?(terrence)
(Assignee)

Comment 21

5 years ago
Posted patch 4-update-object-pointers (obsolete) — Splinter Review
Code to fix up pointers to moved JSObjects.
Attachment #8467140 - Flags: review?(terrence)
(Assignee)

Comment 22

5 years ago
Posted 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)
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 27

5 years ago
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)
(Assignee)

Comment 28

5 years ago
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)
(Assignee)

Comment 29

5 years ago
Patch to update pointers in jitcode.  If you think this should land we should  get review from mjrosenb too.
Attachment #8468564 - Flags: review?(terrence)
(Assignee)

Comment 30

5 years ago
Updated patch with review comments addressed, carrying r+.
Attachment #8468566 - Flags: review+
(Assignee)

Comment 31

5 years ago
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+
(Assignee)

Comment 36

5 years ago
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**?
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 42

5 years ago
(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+
(Assignee)

Comment 46

5 years ago
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)
(Assignee)

Comment 50

5 years ago
(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.
(Assignee)

Comment 52

5 years ago
(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"...
(Assignee)

Comment 56

5 years ago
(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]
(Assignee)

Updated

5 years ago
Blocks: 1055067
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 58

5 years ago
(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)
(Assignee)

Comment 59

5 years ago
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)
(Assignee)

Comment 60

5 years ago
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)
(Assignee)

Comment 61

5 years ago
Add a moving GC callback and use it to fix up JSObject pointers in IPC tables.
Attachment #8477462 - Flags: review?(terrence)
(Assignee)

Comment 62

5 years ago
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)
(Assignee)

Comment 63

5 years ago
Posted 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.
(Assignee)

Comment 64

5 years ago
Posted patch 13-fixup-xpconnect (obsolete) — Splinter Review
Use the callback and the class hook to update more pointers in xpconnect.
(Assignee)

Comment 65

5 years ago
Posted 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+
(Assignee)

Updated

5 years ago
Attachment #8477459 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8477460 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8477462 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8477464 - Flags: checkin+
(Assignee)

Comment 76

5 years ago
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)
(Assignee)

Comment 78

5 years ago
(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)
(Assignee)

Comment 79

5 years ago
Posted 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)
(Assignee)

Comment 80

5 years ago
Posted 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?
(Assignee)

Comment 82

5 years ago
(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
(Assignee)

Comment 83

5 years ago
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)
(Assignee)

Comment 84

5 years ago
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+
(Assignee)

Updated

5 years ago
Attachment #8483513 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #8483513 - Flags: review?(bobbyholley)
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 91

5 years ago
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+
(Assignee)

Comment 93

5 years ago
Posted 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
(Assignee)

Comment 95

5 years ago
Posted 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-
(Assignee)

Comment 99

5 years ago
(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.
(Assignee)

Comment 100

5 years ago
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
(Assignee)

Updated

5 years ago
Attachment #8491588 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8484956 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8483533 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8483534 - Flags: checkin+
(Assignee)

Comment 103

5 years ago
(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)
(Assignee)

Comment 104

5 years ago
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)
(Assignee)

Updated

5 years ago
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
Duplicate of this bug: 878120
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+
(Assignee)

Comment 110

5 years ago
(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!
(Assignee)

Updated

5 years ago
Attachment #8492227 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8492230 - Flags: review+
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 114

5 years ago
(In reply to Nicholas Nethercote [:njn] from comment #113)
Thanks for the report, I'll look into it.
(Assignee)

Comment 115

5 years ago
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)
(Assignee)

Comment 116

5 years ago
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)
(Assignee)

Updated

5 years ago
Attachment #8496065 - Flags: review?(bobbyholley)
(Assignee)

Comment 117

5 years ago
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)
(Assignee)

Comment 118

5 years ago
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 :)
(Assignee)

Comment 121

5 years ago
(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+
(Assignee)

Updated

5 years ago
Attachment #8496063 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8496065 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8496067 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8496071 - Flags: checkin+
(Assignee)

Comment 124

5 years ago
Unbreak CGC builds after recent changes.
Attachment #8498932 - Flags: review?(terrence)
(Assignee)

Comment 125

5 years ago
We don't need a separate fixup method for CCWs if we make our normal mark path work properly.
Attachment #8498933 - Flags: review?(terrence)
(Assignee)

Comment 126

5 years ago
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)
(Assignee)

Comment 127

5 years ago
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)
(Assignee)

Comment 128

5 years ago
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+
(Assignee)

Updated

5 years ago
Attachment #8498932 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8498933 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8498936 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8498937 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8498942 - Flags: checkin+
(Assignee)

Comment 135

5 years ago
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+
(Assignee)

Comment 138

5 years ago
(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
(Assignee)

Updated

5 years ago
Depends on: 1079224
(Assignee)

Comment 143

5 years ago
Posted 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+
(Assignee)

Updated

5 years ago
Attachment #8499645 - Flags: checkin+
(Assignee)

Updated

5 years ago
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)
(Assignee)

Updated

5 years ago
Depends on: 1083716
(Assignee)

Comment 147

5 years ago
Posted 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)
(Assignee)

Comment 148

5 years ago
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)
(Assignee)

Comment 149

5 years ago
Don't try to relocate anything in the self-hosting zone as this can be shared by multiple runtimes.
Attachment #8506054 - Flags: review?(terrence)
(Assignee)

Comment 150

5 years ago
Remove the restriction on relocating global objects (SM changes).
Attachment #8506057 - Flags: review?(terrence)
(Assignee)

Comment 151

5 years ago
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.
(Assignee)

Comment 156

5 years ago
Fix breakage following generator changes.
Attachment #8506834 - Flags: review?(terrence)
(Assignee)

Comment 157

5 years ago
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+
(Assignee)

Updated

5 years ago
Attachment #8506052 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8506054 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8506057 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8506059 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8506834 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8506835 - Flags: checkin+
(Assignee)

Comment 162

5 years ago
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)
(Assignee)

Updated

5 years ago
Depends on: 1087361
(Assignee)

Comment 163

5 years ago
Disable gczeal for the end of xdr/lazy.js, because GC can change the expected test result.
Attachment #8509612 - Flags: review?(terrence)
(Assignee)

Comment 164

5 years ago
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)
(Assignee)

Comment 165

5 years ago
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)
(Assignee)

Comment 166

5 years ago
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+
(Assignee)

Comment 168

5 years ago
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+
(Assignee)

Comment 170

5 years ago
Backed out 36-fix-proxy-auto-config for causing ASAN LeakSanitizer failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/927503f08f55
(Assignee)

Updated

5 years ago
Attachment #8509612 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8509627 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8509630 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8510220 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8509620 - Flags: checkin+
(Assignee)

Updated

5 years ago
Depends on: 1087948
(Assignee)

Comment 174

5 years ago
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)
(Assignee)

Comment 175

5 years ago
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+
(Assignee)

Comment 179

5 years ago
Fix bug choosing which arenas to relocate when there aren't any to choose from.
Attachment #8512881 - Flags: review?(terrence)
(Assignee)

Comment 180

5 years ago
Include the gc invocation kind in the GC timer log output.
Attachment #8512882 - Flags: review?(terrence)
(Assignee)

Comment 181

5 years ago
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)
(Assignee)

Comment 182

5 years ago
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+
(Assignee)

Updated

5 years ago
Attachment #8511878 - Flags: checkin+
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 189

5 years ago
(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)
(Assignee)

Updated

5 years ago
Attachment #8508056 - Flags: review?(terrence)
(Assignee)

Comment 191

5 years ago
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+
(Assignee)

Comment 192

5 years ago
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.
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 199

4 years ago
And backed out again for causing hazards:

https://hg.mozilla.org/integration/mozilla-inbound/rev/054bd325286d
(Assignee)

Updated

4 years ago
Attachment #8508056 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #8512881 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #8512882 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #8512887 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #8512890 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #8514344 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #8514346 - Flags: checkin+
(Assignee)

Comment 202

4 years ago
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+
(Assignee)

Updated

4 years ago
Depends on: 989390
(Assignee)

Updated

4 years ago
Depends on: 1099463
(Assignee)

Updated

4 years ago
Attachment #8522270 - Flags: checkin+
(Assignee)

Comment 206

4 years ago
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
(Assignee)

Updated

4 years ago
Depends on: 1101585
(Assignee)

Comment 211

4 years ago
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)
(Assignee)

Comment 212

4 years ago
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+
(Assignee)

Updated

4 years ago
Attachment #8523981 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #8526746 - Flags: checkin+
(Assignee)

Comment 219

4 years ago
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+
Depends on: 1106784
(Assignee)

Updated

4 years ago
Attachment #8530925 - Flags: checkin+