Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Compact the tenured heap when GCInvocationKind is GC_SHRINK

RESOLVED FIXED in mozilla38

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
a year 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
jonco
: checkin+
Details | Diff | Splinter Review
7.33 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
23.39 KB, patch
bholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
36.97 KB, patch
terrence
: review+
bholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.27 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
7.71 KB, patch
terrence
: review+
bholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
9.93 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
5.85 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
5.74 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
7.32 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
4.78 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
953 bytes, patch
jonco
: checkin+
Details | Diff | Splinter Review
2.71 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.59 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
3.27 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.92 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
4.62 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
8.44 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
12.46 KB, patch
bholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.51 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
7.15 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
18.97 KB, patch
johns
: review+
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
946 bytes, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.89 KB, patch
sworkman
: review+
jonco
: checkin+
Details | Diff | Splinter Review
10.43 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
8.49 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
2.44 KB, patch
bhackett
: review+
jonco
: checkin+
Details | Diff | Splinter Review
2.08 KB, patch
jonco
: checkin+
Details | Diff | Splinter Review
1.86 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.15 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
7.62 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.22 KB, patch
bholley
: review+
jonco
: checkin+
Details | Diff | Splinter Review
902 bytes, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
1.07 KB, patch
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

Comment 1

6 years ago
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)?

Comment 2

6 years ago
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.

Comment 4

6 years ago
> 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.

Comment 6

6 years ago
> 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.
Created attachment 543999 [details] [diff] [review]
Folded movement patch.

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

6 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

3 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

3 years ago
Assignee: sphink → jcoppeard
(Assignee)

Comment 18

3 years ago
Created attachment 8467136 [details] [diff] [review]
1-compacting-defines

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

Comment 19

3 years ago
Created attachment 8467137 [details] [diff] [review]
2-cell-forwarding

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

Comment 20

3 years ago
Created attachment 8467138 [details] [diff] [review]
3-check-hash-tables

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

Comment 21

3 years ago
Created attachment 8467140 [details] [diff] [review]
4-update-object-pointers

Code to fix up pointers to moved JSObjects.
Attachment #8467140 - Flags: review?(terrence)
(Assignee)

Comment 22

3 years ago
Created attachment 8467144 [details] [diff] [review]
5-compacting-gc

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

3 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

3 years ago
Created attachment 8468560 [details] [diff] [review]
3.5-invocation-kind

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

3 years ago
Created attachment 8468563 [details] [diff] [review]
4-update-object-pointers v2

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

3 years ago
Created attachment 8468564 [details] [diff] [review]
4.5-update-pointers-in-jitcode

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

3 years ago
Created attachment 8468566 [details] [diff] [review]
5-compacting-gc v2

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

Comment 31

3 years ago
Created attachment 8468569 [details] [diff] [review]
6-stats-phases

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

3 years ago
Created attachment 8471645 [details] [diff] [review]
7-fix-test-failures

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

3 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

3 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 45

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/297ca183fff9
https://hg.mozilla.org/integration/mozilla-inbound/rev/c965ceb8b24a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a52d8157462f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b5fcc90664
https://hg.mozilla.org/integration/mozilla-inbound/rev/3adf62f886d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4fca818c9a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/edc768336c80
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdcd60020523
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d1e96d1eb31
(Assignee)

Comment 46

3 years ago
And fix a build error in non-unified builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/55126a7a7447
Sorry had to revert for crashes:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=8d1e96d1eb31

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aac02f95733e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/da9a761dc078
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/66edbd1cf41f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8bf29df58e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/72ce360905d2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/71a2c9d84759
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/41b062da08cf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4151652292a9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32ed678e0105
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0a194d9191bb

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 48

3 years ago
Fixed and re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3ba188627a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef2b28e788e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/321387d61359
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f65e6cb5cc7
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec9c54f256e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c1859b7cd6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcbc9f1cfb10
https://hg.mozilla.org/integration/mozilla-inbound/rev/67f0f1732193
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f351726ed58
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

3 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

3 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.
https://hg.mozilla.org/mozilla-central/rev/8f3ba188627a
https://hg.mozilla.org/mozilla-central/rev/ef2b28e788e1
https://hg.mozilla.org/mozilla-central/rev/321387d61359
https://hg.mozilla.org/mozilla-central/rev/9f65e6cb5cc7
https://hg.mozilla.org/mozilla-central/rev/2ec9c54f256e
https://hg.mozilla.org/mozilla-central/rev/2c1859b7cd6d
https://hg.mozilla.org/mozilla-central/rev/bcbc9f1cfb10
https://hg.mozilla.org/mozilla-central/rev/67f0f1732193
https://hg.mozilla.org/mozilla-central/rev/5f351726ed58
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

3 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

3 years ago
Blocks: 1055067
(Assignee)

Updated

3 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

3 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

3 years ago
Created attachment 8477459 [details] [diff] [review]
8-fix-more-test-failures

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

3 years ago
Created attachment 8477460 [details] [diff] [review]
9-fixup-cow-arrays

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

3 years ago
Created attachment 8477462 [details] [diff] [review]
10-fixup-ipc-tables

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

Comment 62

3 years ago
Created attachment 8477464 [details] [diff] [review]
11-add-object-moved-hook

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

3 years ago
Created attachment 8477469 [details] [diff] [review]
12-update-wrapper-cache WIP

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

3 years ago
Created attachment 8477471 [details] [diff] [review]
13-fixup-xpconnect

Use the callback and the class hook to update more pointers in xpconnect.
(Assignee)

Comment 65

3 years ago
Created attachment 8477485 [details] [diff] [review]
14-fixup-cycle-collector WIP

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)

Comment 74

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/35742bdc126c
https://hg.mozilla.org/integration/mozilla-inbound/rev/888e2521a277
https://hg.mozilla.org/integration/mozilla-inbound/rev/9df9289e2d8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d84eb49d0ea
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

3 years ago
Attachment #8477464 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/35742bdc126c
https://hg.mozilla.org/mozilla-central/rev/888e2521a277
https://hg.mozilla.org/mozilla-central/rev/9df9289e2d8f
https://hg.mozilla.org/mozilla-central/rev/1d84eb49d0ea
(Assignee)

Comment 76

3 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

3 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

3 years ago
Created attachment 8483513 [details] [diff] [review]
12-update-wrapper-cache

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

3 years ago
Created attachment 8483514 [details] [diff] [review]
13-fixup-xpconnect
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

3 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

3 years ago
Created attachment 8483533 [details] [diff] [review]
15-fix-shell-test-failures

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

3 years ago
Created attachment 8483534 [details] [diff] [review]
16-trigger-shrinking-gc-from-shell

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

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

Updated

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

Updated

3 years ago
Attachment #8483514 - Flags: review?(bobbyholley)

Comment 88

3 years ago
Comment on attachment 8483513 [details] [diff] [review]
12-update-wrapper-cache

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

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

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

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

>+class CGDOMJSProxyHandler_objectMoved(ClassMethod):

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Similarly, shouldn't this have already happened in XPCWrappedNativeScope::StartFinalizationPhaseOfGC?
Attachment #8483514 - Flags: review?(bobbyholley)
Blocks: 1008337
(Assignee)

Comment 91

3 years ago
Created attachment 8484956 [details] [diff] [review]
12-update-wrapper-cache v2

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 92

3 years ago
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

3 years ago
Created attachment 8484968 [details] [diff] [review]
13-fixup-xpconnect v2

(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

3 years ago
Created attachment 8490211 [details] [diff] [review]
13-fixup-xpconnect v3

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 97

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bea8bb703913
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0a56999e9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/512ef66a96b8
(In reply to Jon Coppeard (:jonco) from comment #97)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bea8bb703913
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0a56999e9b
> https://hg.mozilla.org/integration/mozilla-inbound/rev/512ef66a96b8

Reverted for rooting hazzard failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=48289174&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=48289188&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8be7ec1cca74
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3e9f9a5671ee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ff2190c3dbfd
(Assignee)

Comment 99

3 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

3 years ago
Created attachment 8491588 [details] [diff] [review]
11.5-gc-callback-assertions

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)

Comment 102

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe1887ddc95
https://hg.mozilla.org/integration/mozilla-inbound/rev/9737b23a2790
https://hg.mozilla.org/integration/mozilla-inbound/rev/44134599b738
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6e9d291380
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Comment 103

3 years ago
Created attachment 8492227 [details] [diff] [review]
13-fixup-xpconnect v4

(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

3 years ago
Created attachment 8492230 [details] [diff] [review]
17-weak-pointer-callback

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

3 years ago
Attachment #8492230 - Flags: review?(bobbyholley)
https://hg.mozilla.org/mozilla-central/rev/cbe1887ddc95
https://hg.mozilla.org/mozilla-central/rev/9737b23a2790
https://hg.mozilla.org/mozilla-central/rev/44134599b738
https://hg.mozilla.org/mozilla-central/rev/0e6e9d291380
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

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

Comment 111

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b32a3831212
https://hg.mozilla.org/integration/mozilla-inbound/rev/d63a5fe3ace7
(Assignee)

Updated

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

Updated

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

Updated

3 years ago
Attachment #8492230 - Flags: review+ → checkin+
https://hg.mozilla.org/mozilla-central/rev/0b32a3831212
https://hg.mozilla.org/mozilla-central/rev/d63a5fe3ace7
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

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

Comment 115

3 years ago
Created attachment 8496063 [details] [diff] [review]
18-dont-move-global-objects

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

3 years ago
Created attachment 8496065 [details] [diff] [review]
19-dont-need-hook-for-global-objects

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

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

Comment 117

3 years ago
Created attachment 8496067 [details] [diff] [review]
20-fixup-type-objects

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

3 years ago
Created attachment 8496071 [details] [diff] [review]
21-fixup-typed-objects

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

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

Comment 122

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fde2c699ac3
https://hg.mozilla.org/integration/mozilla-inbound/rev/97b903ea34a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/4320b712d193
https://hg.mozilla.org/integration/mozilla-inbound/rev/cae8940f0504
https://hg.mozilla.org/mozilla-central/rev/7fde2c699ac3
https://hg.mozilla.org/mozilla-central/rev/97b903ea34a1
https://hg.mozilla.org/mozilla-central/rev/4320b712d193
https://hg.mozilla.org/mozilla-central/rev/cae8940f0504
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Comment 124

3 years ago
Created attachment 8498932 [details] [diff] [review]
22-fixup-after-sweeping-changes

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

Comment 125

3 years ago
Created attachment 8498933 [details] [diff] [review]
23-simplify-ccw-fixup

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

3 years ago
Created attachment 8498936 [details] [diff] [review]
24-fix-toDictionaryMode

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

3 years ago
Created attachment 8498937 [details] [diff] [review]
25-fix-uninitialized-wrapper-update

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

3 years ago
Created attachment 8498942 [details] [diff] [review]
26-allow-get-script-filename-in-cgc

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)

Comment 133

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8b6ded7370d
https://hg.mozilla.org/integration/mozilla-inbound/rev/978df8aa78ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aade35d8093
https://hg.mozilla.org/integration/mozilla-inbound/rev/2424dd8f3da0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5be3f231f6f
https://hg.mozilla.org/mozilla-central/rev/b8b6ded7370d
https://hg.mozilla.org/mozilla-central/rev/978df8aa78ba
https://hg.mozilla.org/mozilla-central/rev/3aade35d8093
https://hg.mozilla.org/mozilla-central/rev/2424dd8f3da0
https://hg.mozilla.org/mozilla-central/rev/c5be3f231f6f
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 135

3 years ago
Created attachment 8499645 [details] [diff] [review]
27-fixup-after-jsobject-changes

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 137

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d73cc06f9fe
(Assignee)

Comment 138

3 years ago
(In reply to Nicholas Nethercote [:njn] from comment #136)
Thanks for reporting this, I'm investigating currently.
https://tbpl.mozilla.org/?tree=Try&rev=6d73cc06f9fe
(In reply to Jon Coppeard (:jonco) from comment #137)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6d73cc06f9fe

Backed out for SM(ggc) timeouts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/beed46f2f2bd

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2797910&repo=mozilla-inbound
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
https://hg.mozilla.org/mozilla-central/rev/b19c3336ea3f
(Assignee)

Updated

3 years ago
Depends on: 1079224
(Assignee)

Comment 143

3 years ago
Created attachment 8504137 [details] [diff] [review]
28-misc-fixes

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)

Comment 144

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/14db61bb8665
(Assignee)

Updated

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

Updated

3 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)
https://hg.mozilla.org/mozilla-central/rev/14db61bb8665
(Assignee)

Updated

3 years ago
Depends on: 1083716
(Assignee)

Comment 147

3 years ago
Created attachment 8506049 [details] [diff] [review]
29-zeal-mode-help

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

3 years ago
Created attachment 8506052 [details] [diff] [review]
30-fix-get-debuggees-interaction

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

3 years ago
Created attachment 8506054 [details] [diff] [review]
31-dont-relocate-self-hosting-zone

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

3 years ago
Created attachment 8506057 [details] [diff] [review]
32-relocate-global-objects

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

Comment 151

3 years ago
Created attachment 8506059 [details] [diff] [review]
33-relocate-global-objects-browser

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

3 years ago
Created attachment 8506834 [details] [diff] [review]
28.5-fix-generator-breakage

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

Comment 157

3 years ago
Created attachment 8506835 [details] [diff] [review]
29-zeal-mode-help v2

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)

Comment 159

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce272484b1dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/275d7eee096e
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b3a9ac8357
https://hg.mozilla.org/integration/mozilla-inbound/rev/c42ba3f5ac32
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c097e67bdb0
(Assignee)

Comment 160

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/62ac7c28ddae
https://hg.mozilla.org/mozilla-central/rev/ce272484b1dd
https://hg.mozilla.org/mozilla-central/rev/554d8083b8c6
https://hg.mozilla.org/mozilla-central/rev/275d7eee096e
https://hg.mozilla.org/mozilla-central/rev/79b3a9ac8357
https://hg.mozilla.org/mozilla-central/rev/c42ba3f5ac32
https://hg.mozilla.org/mozilla-central/rev/5c097e67bdb0
https://hg.mozilla.org/mozilla-central/rev/62ac7c28ddae
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 162

3 years ago
Created attachment 8508056 [details] [diff] [review]
34-fix-plugin-tracing

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

3 years ago
Depends on: 1087361
(Assignee)

Comment 163

3 years ago
Created attachment 8509612 [details] [diff] [review]
35-fix-xdr-lazy-test

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

3 years ago
Created attachment 8509620 [details] [diff] [review]
36-fix-proxy-auto-config

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

3 years ago
Created attachment 8509627 [details] [diff] [review]
37-pick-arenas-to-relocate

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

3 years ago
Created attachment 8509630 [details] [diff] [review]
38-only-update-necessary-arenas

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

3 years ago
Created attachment 8510220 [details] [diff] [review]
34.5-fix-after-typeinference-changes

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 169

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e0c3d82d831
https://hg.mozilla.org/integration/mozilla-inbound/rev/792c56b99dce
https://hg.mozilla.org/integration/mozilla-inbound/rev/305689497e3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4bb57ab1f3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/446fda5c6602
(Assignee)

Comment 170

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

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

Comment 171

3 years ago
Fixed and re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/21b64777a90e
https://hg.mozilla.org/mozilla-central/rev/1e0c3d82d831
https://hg.mozilla.org/mozilla-central/rev/792c56b99dce
https://hg.mozilla.org/mozilla-central/rev/f4bb57ab1f3f
https://hg.mozilla.org/mozilla-central/rev/446fda5c6602
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

3 years ago
Attachment #8510220 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/21b64777a90e
(Assignee)

Updated

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

Updated

3 years ago
Depends on: 1087948
(Assignee)

Comment 174

3 years ago
Created attachment 8511878 [details] [diff] [review]
39-fix-partly-initialized-dom-object

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

3 years ago
Created attachment 8511880 [details] [diff] [review]
40-fix-moving-partly-initialized-objects

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 177

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3a672d2759
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f5e9388952
https://hg.mozilla.org/mozilla-central/rev/8b3a672d2759
https://hg.mozilla.org/mozilla-central/rev/f5f5e9388952
(Assignee)

Comment 179

3 years ago
Created attachment 8512881 [details] [diff] [review]
41-fix-arena-selection-bug

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

Comment 180

3 years ago
Created attachment 8512882 [details] [diff] [review]
42-report-kind-in-stats

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

Comment 181

3 years ago
Created attachment 8512887 [details] [diff] [review]
43-partly-intialized-sandbox

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

3 years ago
Created attachment 8512890 [details] [diff] [review]
44-update-ignore-timeouts

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)

Comment 184

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f05aa42d06e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/3479b8c7cc8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0259e2c45c3d
(Assignee)

Updated

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

Updated

3 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

3 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

3 years ago
Attachment #8508056 - Flags: review?(terrence)
https://hg.mozilla.org/mozilla-central/rev/f05aa42d06e9
https://hg.mozilla.org/mozilla-central/rev/3479b8c7cc8d
https://hg.mozilla.org/mozilla-central/rev/0259e2c45c3d
(Assignee)

Comment 191

3 years ago
Created attachment 8514344 [details] [diff] [review]
45-fix-inline-typed-object

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

3 years ago
Created attachment 8514346 [details] [diff] [review]
46-keep-relocated-decommitted

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

3 years ago
Depends on: 1092168
https://hg.mozilla.org/mozilla-central/rev/19efe73ee9f7
https://hg.mozilla.org/mozilla-central/rev/fe564c4345d9
https://hg.mozilla.org/mozilla-central/rev/ab0c19ba7ab8
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 198

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2acbf923b6a8
(Assignee)

Comment 199

3 years ago
And backed out again for causing hazards:

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

Comment 200

3 years ago
Fixed and re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5273d8dcb91
https://hg.mozilla.org/mozilla-central/rev/e5273d8dcb91
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 202

3 years ago
Created attachment 8522270 [details] [diff] [review]
47-fix-expando-leak

XPCWrappedNativeScope::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)

Comment 204

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c42f7b5b39c
(Assignee)

Updated

3 years ago
Depends on: 989390
https://hg.mozilla.org/mozilla-central/rev/1c42f7b5b39c
(Assignee)

Updated

3 years ago
Depends on: 1099463
(Assignee)

Updated

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

Comment 206

3 years ago
Created attachment 8523981 [details] [diff] [review]
48-update-cells-in-parallel

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

Comment 208

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/63a296e4b6b3
https://hg.mozilla.org/mozilla-central/rev/63a296e4b6b3
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

3 years ago
Depends on: 1101585
(Assignee)

Comment 211

3 years ago
Created attachment 8526746 [details] [diff] [review]
49-allow-background-sweeping

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

3 years ago
Created attachment 8526747 [details] [diff] [review]
50-fix-non-heap-tracing

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

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

Comment 215

3 years ago
Comment on attachment 8526747 [details] [diff] [review]
50-fix-non-heap-tracing

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c0e6bce748
Attachment #8526747 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/b7c0e6bce748
(Assignee)

Comment 217

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e18a24f1298
https://hg.mozilla.org/mozilla-central/rev/2e18a24f1298
(Assignee)

Updated

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

Comment 219

3 years ago
Created attachment 8530925 [details] [diff] [review]
51-fix-choosing-arenas

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)

Comment 220

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9442d0b10d87
https://hg.mozilla.org/mozilla-central/rev/9442d0b10d87
(Assignee)

Updated

3 years ago
Attachment #8530925 - Flags: checkin+
(Assignee)

Comment 222

3 years ago
Created attachment 8533229 [details] [diff] [review]
52-disable-compacting-api

Add an API to disable compacting GC for embedders.
Attachment #8533229 - Flags: review?(terrence)
(Assignee)

Comment 223

3 years ago
Created attachment 8533232 [details] [diff] [review]
53-fix-weak-map-test

Fix a jsapi test that keeps an untraced pointer in a private value so that it works with compacting zeal mode enabled.
Attachment #8533232 - Flags: review?(terrence)
(Assignee)

Comment 224

3 years ago
Created attachment 8533233 [details] [diff] [review]
54-enable-compacting-gc

Build and enable compacting GC by default.

Do we need additional review by a build peer to land this?
Attachment #8533233 - Flags: review?(terrence)
(Assignee)

Updated

3 years ago
Depends on: 1080262
No longer depends on: 1106784
Attachment #8533229 - Flags: review?(terrence) → review+
Comment on attachment 8533232 [details] [diff] [review]
53-fix-weak-map-test

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

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

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

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

Given that build-peers missed our missing js-config.h.in symbols, or given any feedback at all really, I'd guess sfink would be a better bet for catching real issues up front. That said, this is trivial, so I'm just going to r+ it.
Attachment #8533233 - Flags: review?(terrence) → review+
(Assignee)

Comment 227

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f659e0e4632
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f72dca12ee
https://hg.mozilla.org/mozilla-central/rev/7f659e0e4632
https://hg.mozilla.org/mozilla-central/rev/a0f72dca12ee
(Assignee)

Comment 229

3 years ago
Created attachment 8537744 [details] [diff] [review]
55-reduce-parallel-udpate

I did some quick tests to see how much of an improvement we got by using multiple threads to update cells.  Here are the results for updating ~10 million objects:

  Threads:   Average time /mS
             MacOSX:    Linux64:

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

Both systems are 4 core / 8 threading Intel processors.

From this it seems we don't get any benefit from going above 4 threads, so I think we should dial back the parallelism to one thread per two reported CPUs.
Attachment #8537744 - Flags: review?(terrence)
(Assignee)

Comment 230

3 years ago
Created attachment 8537746 [details] [diff] [review]
56-assert-maximal-compaction

Something I meant to do before - add assertions that we are compacting as much as we should.
Attachment #8537746 - Flags: review?(terrence)
Comment on attachment 8537744 [details] [diff] [review]
55-reduce-parallel-udpate

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

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

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

Nice!
Attachment #8537746 - Flags: review?(terrence) → review+
(Assignee)

Comment 233

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6351a5baa95
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5492cfe7127
(Assignee)

Updated

3 years ago
Attachment #8533229 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8533232 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/e6351a5baa95
https://hg.mozilla.org/mozilla-central/rev/d5492cfe7127
(Assignee)

Updated

3 years ago
Attachment #8537744 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8537746 - Flags: checkin+
(Assignee)

Comment 235

3 years ago
Created attachment 8548358 [details] [diff] [review]
57-fix-reset-in-compact

Fix reset inside incremental compacting GC.  At the moment the only incremental thing we do is to poll for the end of background sweeping, so the reset code just waits for that and then and then finishes the GC as a non-shrinking GC which calls finishCollection() and ends the GC correctly.
Attachment #8548358 - Flags: review?(terrence)
(Assignee)

Comment 236

3 years ago
Created attachment 8548361 [details] [diff] [review]
58-fix-merge-zones

Zone merging doesn't take account of the fact that we may be keeping relocated arenas around and so doesn't update them.  Just release any relocated arenas if we merge.
Attachment #8548361 - Flags: review?(terrence)
(Assignee)

Comment 237

3 years ago
Created attachment 8548363 [details] [diff] [review]
59-fix-shape-listp

We don't mark whatever Shape::listp points to, and ICs can mark a shape in dictionary mode shapes without without the rest of the dictionary mode shape list (or whatever it's called) getting marked.

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

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

Ouch. Good find!
Attachment #8548363 - Flags: review?(terrence) → review+
(Assignee)

Comment 239

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/08878d0f0b33
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb9e44052e04
https://hg.mozilla.org/integration/mozilla-inbound/rev/95004a227383
(Assignee)

Comment 240

3 years ago
Created attachment 8549013 [details] [diff] [review]
60-release-arenas-first-on-OOM

Fix handling of OOM so that relocated areans are released first, otherwise empty chunks can be left triggering an assertion when decommitting.
Attachment #8549013 - Flags: review?(terrence)
(Assignee)

Comment 241

3 years ago
Created attachment 8549016 [details] [diff] [review]
61-stop-off-thread-compilation-while-compacting

Stop any off thread compilation which might be accessing things we are trying to relocate.
Attachment #8549016 - Flags: review?(terrence)
Comment on attachment 8549013 [details] [diff] [review]
60-release-arenas-first-on-OOM

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

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

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

Oh, good find.
Attachment #8549016 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/08878d0f0b33
https://hg.mozilla.org/mozilla-central/rev/fb9e44052e04
https://hg.mozilla.org/mozilla-central/rev/95004a227383
(Assignee)

Comment 245

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6df62cce2ace
https://hg.mozilla.org/integration/mozilla-inbound/rev/aea8d15e96bf
(Assignee)

Updated

3 years ago
Attachment #8548358 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8548361 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8548363 - Flags: checkin+
(Assignee)

Comment 246

3 years ago
Created attachment 8549759 [details] [diff] [review]
62-fix-misc-errors

Fix some misc errors after doing a more full try build:
 - mark some single argument constructors explicit
 - fix a build error on linux 32 due to header file includes (don't know why this doesn't fail on linux 64 builds)
 - fix a jsapi test failure that just started happening on ggc test runs
Attachment #8549759 - Flags: review?(terrence)
(Assignee)

Comment 247

3 years ago
Created attachment 8549763 [details] [diff] [review]
54-enable-compacting-gc v2

Rebased version of original patch, carrying r+.
Attachment #8533233 - Attachment is obsolete: true
Attachment #8549763 - Flags: review+
(Assignee)

Comment 248

3 years ago
Comment on attachment 8549763 [details] [diff] [review]
54-enable-compacting-gc v2

Requesting review from build peer for config change to enable compacting GC unconditionally.
Attachment #8549763 - Flags: review?(mh+mozilla)
Attachment #8549759 - Flags: review?(terrence) → review+
Depends on: 1122196
https://hg.mozilla.org/mozilla-central/rev/6df62cce2ace
https://hg.mozilla.org/mozilla-central/rev/aea8d15e96bf
No longer depends on: 1122196
Attachment #8549763 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 250

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0f5677578a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2eec4a74ea
(Assignee)

Updated

3 years ago
Attachment #8549013 - Flags: checkin+