Closed
Bug 650161
Opened 13 years ago
Closed 9 years ago
Compact the tenured heap when GCInvocationKind is GC_SHRINK
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: cdleary, Assigned: jonco)
References
(Depends on 1 open bug, Blocks 2 open bugs)
Details
Attachments
(68 files, 12 obsolete files)
3.33 KB,
patch
|
terrence
:
review+
ted
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
3.35 KB,
patch
|
terrence
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
15.50 KB,
patch
|
terrence
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
8.44 KB,
patch
|
terrence
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
59.88 KB,
patch
|
terrence
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
6.25 KB,
patch
|
mjrosenb
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
43.34 KB,
patch
|
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
31.42 KB,
patch
|
terrence
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
35.54 KB,
patch
|
terrence
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
6.23 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
9.34 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
12.60 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
17.28 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
9.14 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
3.30 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
32.50 KB,
patch
|
bzbarsky
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
7.33 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
23.39 KB,
patch
|
bholley
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
36.97 KB,
patch
|
terrence
:
review+
bholley
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.27 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
7.71 KB,
patch
|
terrence
:
review+
bholley
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
9.93 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
5.85 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
5.74 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
7.32 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
4.78 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
953 bytes,
patch
|
bzbarsky
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
2.71 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.59 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
3.27 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.92 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
4.62 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
8.44 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
12.46 KB,
patch
|
bholley
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
7.15 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
18.97 KB,
patch
|
johns
:
review+
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
946 bytes,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
sworkman
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
10.43 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
8.49 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
2.44 KB,
patch
|
bhackett1024
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
2.08 KB,
patch
|
bzbarsky
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.86 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.15 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
7.62 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.22 KB,
patch
|
bholley
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
902 bytes,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.07 KB,
patch
|
bhackett1024
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
8.71 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
2.00 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
21.81 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
5.31 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
848 bytes,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
2.82 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
3.77 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
4.81 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.06 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.57 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.10 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
4.47 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.71 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
1.26 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
2.34 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
4.53 KB,
patch
|
terrence
:
review+
jonco
:
checkin+
|
Details | Diff | Splinter Review |
31.29 KB,
patch
|
jonco
:
review+
glandium
:
review+
|
Details | Diff | Splinter Review |
6.20 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
2.19 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
Basic idea: values found through conservative scanning (i.e. C stack scanning) need to be pinned at a particular memory location, but all other values *should* be capable of being moved. I'm starting out with objects. Pointers that are marked exactly (i.e. Value& where isObject and JSObject *&s) can be relocated if the pointee is unpinned. Assume that there exist no pinned things in an arena. The most basic algorithm would be: 1. move all unpinned objects within the arena to the same relative offset within a new arena; 2. mprotect the old arena segment to catch invalid references early; 3. during normal scanning update object pointer locations into the old arena segment to the new arena segment. To extend that algorithm to work with pinned objects we would have to avoid the mprotect. The death-defying excitement of this bug comes from outside use of the JSAPI. If there are places in the Mozilla code base that use implicit rooting assumptions (i.e. it's okay that I don't add this as an explicit root because I'm sure it will be rooted by the global object I'm pulling it off of) they will break. mprotect should help us figure out how breaking this change is fairly early on.
Reporter | ||
Updated•13 years ago
|
Summary: (Moving collection) Add capability to relocate GCThings to new arenas → Add capability to relocate GCThings to new arenas
Comment 1•13 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•13 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.... ;)
Reporter | ||
Comment 3•13 years ago
|
||
(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•13 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.
Reporter | ||
Comment 5•13 years ago
|
||
(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•13 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.
Reporter | ||
Comment 8•13 years ago
|
||
This is a folded version of the patches located in my tm-moving-gc-mq user repo. This is against a (very) old revision (e089a54230bf71965c9c74bb0e3aa9771fd40247) that I stuck to because there were significant GC changes going on that caused a lot of thrash. billm asked that I post the patch as is and rebase it later if possible.
(In reply to Boris Zbarsky (:bz) from comment #6) > Hmm... I guess you could run in a mode where you move on every GC or > something? Wouldn't a generational GC do that anyway? Either while compacting the old generation or while copying from the young generation.
Reporter | ||
Comment 10•13 years ago
|
||
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.
Comment 12•12 years ago
|
||
Is this bug part of the current plan or has it been replaced by others?
Comment 13•12 years ago
|
||
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.
Updated•12 years ago
|
Assignee: general → sphink
Updated•11 years ago
|
Alias: MovingGC → CompactingGC
No longer blocks: GenerationalGC
Summary: Add capability to relocate GCThings to new arenas → Compact the tenured heap when GCInvocationKind is GC_SHRINK
Updated•10 years ago
|
Whiteboard: [MemShrink]
Comment 16•10 years ago
|
||
Any estimation when this will be tackled?
Updated•10 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
Comment 17•10 years ago
|
||
(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•10 years ago
|
Assignee: sphink → jcoppeard
Assignee | ||
Comment 18•10 years ago
|
||
Add --enable-gccompacting configure flag to the build.
Attachment #8467136 -
Flags: review?(terrence)
Assignee | ||
Comment 19•10 years ago
|
||
Add machinery for storing forwarding pointers in GC things.
Attachment #543999 -
Attachment is obsolete: true
Attachment #8467137 -
Flags: review?(terrence)
Assignee | ||
Comment 20•10 years ago
|
||
Update our hash table checks for use after compacting GC as well as minor GC.
Attachment #8467138 -
Flags: review?(terrence)
Assignee | ||
Comment 21•10 years ago
|
||
Code to fix up pointers to moved JSObjects.
Attachment #8467140 -
Flags: review?(terrence)
Assignee | ||
Comment 22•10 years ago
|
||
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•10 years ago
|
Keywords: leave-open
Updated•10 years ago
|
Attachment #8467136 -
Flags: review?(terrence) → review+
Comment 23•10 years ago
|
||
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 24•10 years ago
|
||
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 25•10 years ago
|
||
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 26•10 years ago
|
||
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•10 years ago
|
||
As discussed yesterday, here's a patch to honour the invocation kind passed to the first GC slice. I guess it would also be good idea to track down the cases when the browser passes different settings to different incremental slices, but it doesn't make any difference to correctness.
Attachment #8467140 -
Attachment is obsolete: true
Attachment #8467144 -
Attachment is obsolete: true
Attachment #8468560 -
Flags: review?(terrence)
Assignee | ||
Comment 28•10 years ago
|
||
Thanks for the detailed review! The jitcode marking is necessary if we want to have the zeal mode to do periodic compacting GCs because these will be called when there is code on the stack. Unfortunately I don't see how we can be confident of our test coverage without this feature. I've split this out into a separate patch below. I've addressed your other comments.
Attachment #8468563 -
Flags: review?(terrence)
Assignee | ||
Comment 29•10 years ago
|
||
Patch to update pointers in jitcode. If you think this should land we should get review from mjrosenb too.
Attachment #8468564 -
Flags: review?(terrence)
Assignee | ||
Comment 30•10 years ago
|
||
Updated patch with review comments addressed, carrying r+.
Attachment #8468566 -
Flags: review+
Assignee | ||
Comment 31•10 years ago
|
||
Patch to add stats phases for compacting GC.
Attachment #8468569 -
Flags: review?(terrence)
Comment 32•10 years ago
|
||
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 33•10 years ago
|
||
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 34•10 years ago
|
||
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 35•10 years ago
|
||
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•10 years ago
|
||
Some updates to fix test failures including: - add an RAII guard to temporarily disable compacting GC - updats gray roots - fix sweeping of compartments' debuggee set - fix some more cases related to array buffers and typed array objects
Attachment #8471645 -
Flags: review?(terrence)
Comment 37•10 years ago
|
||
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 38•10 years ago
|
||
Comment on attachment 8467136 [details] [diff] [review] 1-compacting-defines Review of attachment 8467136 [details] [diff] [review]: ----------------------------------------------------------------- Needs build peer review.
Comment 39•10 years ago
|
||
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 40•10 years ago
|
||
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•10 years ago
|
Attachment #8467136 -
Flags: review?(ted)
Comment 41•10 years ago
|
||
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•10 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?
Comment 43•10 years ago
|
||
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 44•10 years ago
|
||
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•10 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•10 years ago
|
||
And fix a build error in non-unified builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/55126a7a7447
Comment 47•10 years ago
|
||
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•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 48•10 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
Comment 49•10 years ago
|
||
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•10 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)
Comment 51•10 years ago
|
||
Since the whole point of compacting GC is to (AIUI) reduce memory usage, some measurements would be helpful.
Assignee | ||
Comment 52•10 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.
Comment 53•10 years ago
|
||
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
Comment 54•10 years ago
|
||
Oh, I didn't realize it was hidden behind a build flag. That makes more sense!
Comment 55•10 years ago
|
||
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•10 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]
Comment 57•10 years ago
|
||
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•10 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•10 years ago
|
||
Fixes some more jit-test failures with compacting GC, adds an assertion that we don't find forwarded things when marking and fixes issue where we traced the gray roots with the wrong tracer.
Attachment #8477459 -
Flags: review?(terrence)
Assignee | ||
Comment 60•10 years ago
|
||
Fix issues relating to copy-on-write arrays. We may need to update the owner object pointer at the end of shared elements, and also the elements pointer itself if it points into inline elements in an object that was moved.
Attachment #8477460 -
Flags: review?(terrence)
Assignee | ||
Comment 61•10 years ago
|
||
Add a moving GC callback and use it to fix up JSObject pointers in IPC tables.
Attachment #8477462 -
Flags: review?(terrence)
Assignee | ||
Comment 62•10 years ago
|
||
Add a class hook to call when an object is moved. Use this to factor out class-specific fixup from RelocateCell() for array buffers and typed arrays. Implement the hook for proxies too.
Attachment #8477464 -
Flags: review?(terrence)
Assignee | ||
Comment 63•10 years ago
|
||
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•10 years ago
|
||
Use the callback and the class hook to update more pointers in xpconnect.
Assignee | ||
Comment 65•10 years ago
|
||
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.
Comment 66•10 years ago
|
||
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 67•10 years ago
|
||
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 68•10 years ago
|
||
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 69•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8467136 -
Flags: checkin+
Comment 70•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8467138 -
Flags: checkin+
Comment 71•10 years ago
|
||
Comment on attachment 8468560 [details] [diff] [review] 3.5-invocation-kind Review of attachment 8468560 [details] [diff] [review]: ----------------------------------------------------------------- Great!
Attachment #8468560 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8468563 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8468564 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8468566 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8468569 -
Flags: checkin+
Comment 72•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8477459 -
Flags: checkin+
Comment 73•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8471645 -
Flags: checkin+
Assignee | ||
Comment 74•10 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•10 years ago
|
Attachment #8477459 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8477460 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8477462 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8477464 -
Flags: checkin+
Comment 75•10 years ago
|
||
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•10 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.
Comment 77•10 years ago
|
||
Nice! jonco, do I just need to build the browser with --enable-gccompacting to test?
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 78•10 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•10 years ago
|
||
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•10 years ago
|
||
Attachment #8483514 -
Flags: review?(terrence)
Comment 81•10 years ago
|
||
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•10 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•10 years ago
|
||
Fix more jit-test failures in compacting zeal mode: - Don't actually delete the RegExpShared in RegExpCompartment::sweep if we are compacting - Clear Shape::listp in JSObject finalizer if it points into a dying object - Clear the mark bits for Arenas whose contents have been relocated - Trace through WeakMap::memberOf to ensure this is updated
Attachment #8483533 -
Flags: review?(terrence)
Assignee | ||
Comment 84•10 years ago
|
||
Allow passing a second 'shrinking' parameter to our shell gc() function to trigger a shrinking GC.
Attachment #8483534 -
Flags: review?(terrence)
Comment 85•10 years ago
|
||
(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 86•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8483514 -
Flags: review?(terrence) → review+
Comment 87•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8483534 -
Flags: review?(terrence) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8483513 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8483513 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Attachment #8483514 -
Flags: review?(bobbyholley)
Comment 88•10 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 89•10 years ago
|
||
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 90•10 years ago
|
||
Comment on attachment 8483514 [details] [diff] [review] 13-fixup-xpconnect Review of attachment 8483514 [details] [diff] [review]: ----------------------------------------------------------------- So we can avoid dealing with mGlobalJSObject, mContentXBLScope, and mAddonScopes because we never relocate global objects? If so, please add comments to that effect. ::: js/xpconnect/src/XPCMaps.cpp @@ +105,5 @@ > +{ > + for (Map::Enum e(mTable); !e.empty(); e.popFront()) { > + nsXPCWrappedJS* wrapper = e.front().value(); > + MOZ_ASSERT(wrapper, "found a null JS wrapper!"); > + MOZ_ALWAYS_FALSE(wrapper->IsObjectAboutToBeFinalized()); Why do we need to explicitly invoke this here? Don't we do do this already in the finalize callback? Also, aren't JS::Heap pointers automatically relocated? ::: js/xpconnect/src/XPCWrappedNativeScope.cpp @@ +606,5 @@ > +XPCWrappedNativeScope::FixupAfterMovingGC() > +{ > + for (XPCWrappedNativeScope* cur = gScopes; cur; cur = cur->mNext) { > + if (cur->mWaiverWrapperMap) > + cur->mWaiverWrapperMap->Sweep(); Similarly, shouldn't this have already happened in XPCWrappedNativeScope::StartFinalizationPhaseOfGC?
Attachment #8483514 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 91•10 years ago
|
||
Thanks for the comments. Yes, the issue is that if a wrapper is not preserved then mWrapper is not traced and so cannot be discovered and updated by the GC by the usual means. The purpose of Heap<T> is to track pointers from the tenured heap into the nursery (which it does by adding entries to the store buffer when its contents are changed to point to a nursery object). This allows us to perform fast minor GCs without tracing the entire heap. But it doesn't track all pointers in the system as this would be too costly to maintain and also not useful for generational GC. For compacting GC, we must potentially update every pointer in the system. In theory we can just trace the heap to find these pointers, but in our case we also have a lot of weak pointers which are not traced and so must be updated separately -- for example the wrapper cache. The objectMoved class extension allows classes that maintain weak pointers to their objects to update them appropriately. The motivation for making a proxy trap for objectMoved was first by analogy with the finalize trap, and secondly so that proxy handlers could deal with this without also having to provide their own class. I now realise that DOM proxies already implement their own class anyway though so this is moot. I've removed this as you suggested. I've fixed the issues with Codegen.py and the outer window wrapper cache.
Attachment #8483513 -
Attachment is obsolete: true
Attachment #8484956 -
Flags: review?(bzbarsky)
Comment 92•10 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•10 years ago
|
||
(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 94•10 years ago
|
||
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-
Assignee | ||
Comment 95•10 years ago
|
||
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 96•10 years ago
|
||
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•10 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
Comment 98•10 years ago
|
||
(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•10 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•10 years ago
|
||
This patch adds AutoAssertGCCallback, which asserts that something is only ever called from a GC callback. In addition it suppresses static hazard checking and turns on assertions on GC thing allocation in its lifetime (not just on GC). The reason is so that we can tell the analysis to ignore what it thinks are hazards in object moved hooks but are really just unknown field calls. I also changed AutoSuppressGCAnalysis to assert on allocation not just GC as this is a stronger test that no GC is possible. An initial try run was green. Finally I changed a couple of assertions inside the GC to assert in release builds too since the overhead is negligible here and this should give us more confidence that nothing is going awry.
Attachment #8491588 -
Flags: review?(terrence)
Comment 101•10 years ago
|
||
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+
Assignee | ||
Comment 102•10 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•10 years ago
|
Attachment #8491588 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8484956 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8483533 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8483534 -
Flags: checkin+
Assignee | ||
Comment 103•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #96) > @@ +102,5 @@ > > + if (!wrapper->IsSubjectToFinalization()) { > > + JSObject *obj = wrapper->GetJSObjectPreserveColor(); > > + JSObject *prior = obj; > > + MOZ_ASSERT(!JS_IsAboutToBeFinalizedUnbarriered(&obj)); > > + MOZ_ASSERT(obj == prior); > > I don't understand - why can't the JSObject of a strongly-held XPCWrappedJS > be moved? They can, but they will have already been updated when they were marked. I've added a comment. > ::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp > @@ +1334,5 @@ > > p->JSProtoObjectFinalized(fop, obj); > > } > > > > static void > > +XPC_WN_Shared_Proto_ObjectMoved(JSObject *obj, const JSObject *old) > > Don't we also need to do something similar for the XPC_WN_{No,}Helper > classes and XPC_WN_Tearoff_JSClass? XPC_WN_NoHelper_JSClass specifies an objectMovedOp hook and XPCNativeScriptableShared::PopulateJSClass() does this for XPC_WN_Helper classes. But I missed the tearoffs, which I've fixed now. Other comments addressed.
Attachment #8490211 -
Attachment is obsolete: true
Attachment #8492227 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 104•10 years ago
|
||
Here's a patch to replace the moving GC callback with a weak pointer update callback that also gets called during finalization. Unfortunately we need to keep the finalization callback too because that's used for more than just updating weak pointers. This also replaces the public IsAboutToBeFinalized functions with their UpdatePointerAfterGC equivalent.
Attachment #8492230 -
Flags: review?(terrence)
Assignee | ||
Updated•10 years ago
|
Attachment #8492230 -
Flags: review?(bobbyholley)
Comment 105•10 years ago
|
||
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 106•10 years ago
|
||
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+
Comment 108•10 years ago
|
||
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 109•10 years ago
|
||
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•10 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•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b32a3831212 https://hg.mozilla.org/integration/mozilla-inbound/rev/d63a5fe3ace7
Assignee | ||
Updated•10 years ago
|
Attachment #8492227 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8492230 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8492230 -
Flags: review+ → checkin+
Comment 112•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0b32a3831212 https://hg.mozilla.org/mozilla-central/rev/d63a5fe3ace7
Comment 113•10 years ago
|
||
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•10 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #113) Thanks for the report, I'll look into it.
Assignee | ||
Comment 115•10 years ago
|
||
In the general case we can't move global objects because their address may be baked into JIT code, and so assume they can't move in a bunch of places. Therefore it's not safe to move then at any time, even if the JITs are disabled.
Attachment #8496063 -
Flags: review?(terrence)
Assignee | ||
Comment 116•10 years ago
|
||
Since we never move global objects, they don't need to implement the objectMovedOp hook. This means those changes to Sandbox weren't necessary and can come out.
Attachment #8496065 -
Flags: review?(terrence)
Assignee | ||
Updated•10 years ago
|
Attachment #8496065 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 117•10 years ago
|
||
Fixup type object pointers that aren't updated during sweeping and make sure we don't delete any data if we're not sweeping.
Attachment #8496067 -
Flags: review?(terrence)
Assignee | ||
Comment 118•10 years ago
|
||
Fix interactions between typed objects and moving GC after latest updates.
Attachment #8496071 -
Flags: review?(terrence)
Updated•10 years ago
|
Attachment #8496063 -
Flags: review?(terrence) → review+
Updated•10 years ago
|
Attachment #8496065 -
Flags: review?(terrence) → review+
Comment 119•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8496071 -
Flags: review?(terrence) → review+
Comment 120•10 years ago
|
||
> In the general case we can't move global objects because their address may
> be baked into JIT code, and so assume they can't move in a bunch of places.
> Therefore it's not safe to move then at any time, even if the JITs are
> disabled.
Does that explain the compartment leaks I saw? (I hope so :)
Assignee | ||
Comment 121•10 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.
Updated•10 years ago
|
Attachment #8496065 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 122•10 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
Comment 123•10 years ago
|
||
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•10 years ago
|
Attachment #8496063 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8496065 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8496067 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8496071 -
Flags: checkin+
Assignee | ||
Comment 124•10 years ago
|
||
Unbreak CGC builds after recent changes.
Attachment #8498932 -
Flags: review?(terrence)
Assignee | ||
Comment 125•10 years ago
|
||
We don't need a separate fixup method for CCWs if we make our normal mark path work properly.
Attachment #8498933 -
Flags: review?(terrence)
Assignee | ||
Comment 126•10 years ago
|
||
This makes ObjectImpl::toDictionaryMode() work with compacting GC. The reason this was an issue was that the last shape in the list has its listp pointer set to point to a location on the stack while the list is being created, which wasn't handled by the shape fixup code. This was the only place this happened and was fairly insane. I removed this and only set the object.shape_ <-> shape.listp link at the end when everything has been created.
Attachment #8498936 -
Flags: review?(terrence)
Assignee | ||
Comment 127•10 years ago
|
||
It's possible for a GC to happen while creating a DOM object before the wrapper cache has been initialized, so we need to allow UpdateWrapper() to handle a null pointer here.
Attachment #8498937 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 128•10 years ago
|
||
It seems that TheadHangMonitor can end up calling JS_GetScriptFilename() from another thread if it detects a hang. If this happens while we're running a compacting GC this will then crash in the hang monitor and hide the original problem :( This patch allows JS_GetScriptFilename() to be called like this to give some chance of this working (useful in case we hang while compacting), although it's obviously still racey.
Attachment #8498942 -
Flags: review?(terrence)
Comment 129•10 years ago
|
||
Comment on attachment 8498937 [details] [diff] [review] 25-fix-uninitialized-wrapper-update Ah, indeed. r=me
Attachment #8498937 -
Flags: review?(bzbarsky) → review+
Comment 130•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8498933 -
Flags: review?(terrence) → review+
Comment 131•10 years ago
|
||
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 132•10 years ago
|
||
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•10 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
Comment 134•10 years ago
|
||
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•10 years ago
|
Attachment #8498932 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8498933 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8498936 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8498937 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8498942 -
Flags: checkin+
Assignee | ||
Comment 135•10 years ago
|
||
Fix for build errors after recent JSObject changes.
Attachment #8499645 -
Flags: review?(terrence)
Comment 136•10 years ago
|
||
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/)
Updated•10 years ago
|
Attachment #8499645 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 137•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d73cc06f9fe
Assignee | ||
Comment 138•10 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #136) Thanks for reporting this, I'm investigating currently.
Comment 139•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6d73cc06f9fe
Comment 140•10 years ago
|
||
(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
Comment 141•10 years ago
|
||
Apparently the timeouts are because compacting mode was recently made the default for the SM(ggc) jobs, so I've re-landed comment 137 and will hide the job on TBPL for now per discussion on IRC with sfink and terrence. https://hg.mozilla.org/integration/mozilla-inbound/rev/b19c3336ea3f
Assignee | ||
Comment 143•10 years ago
|
||
Fix build errors due to recent changes and test failure in TypedObject/inlineopaque.js
Attachment #8504137 -
Flags: review?(terrence)
Updated•10 years ago
|
Attachment #8504137 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 144•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/14db61bb8665
Assignee | ||
Updated•10 years ago
|
Attachment #8499645 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8504137 -
Flags: checkin+
Comment 145•10 years ago
|
||
Help output for the new zeal (mode 14) appears to be missing. I guess this is sitting in your patch queue somewhere, Jon?
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 147•10 years ago
|
||
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•10 years ago
|
||
While iterating through the set of debuggees in Debugger::getDebuggees() we have to be careful not to do anything that could cause a GC since that could also mutate the set by rekeying a moved object.
Attachment #8506052 -
Flags: review?(terrence)
Assignee | ||
Comment 149•10 years ago
|
||
Don't try to relocate anything in the self-hosting zone as this can be shared by multiple runtimes.
Attachment #8506054 -
Flags: review?(terrence)
Assignee | ||
Comment 150•10 years ago
|
||
Remove the restriction on relocating global objects (SM changes).
Attachment #8506057 -
Flags: review?(terrence)
Assignee | ||
Comment 151•10 years ago
|
||
Now it's possible to relocate global objects so we do need those changes to Sandbox that I removed earlier after all. Sorry about that! Also I needed to change BackstagePass so that there is only single pointer to its global object (from the wrapped native) and the existing infrastructure to update it is sufficient if it moves. I did this by changing the JSObject pointer here to be a pointer to the wrapped native instead. This may not be the best solution so let me know if you think we should do this another way.
Attachment #8506059 -
Flags: review?(bobbyholley)
Comment 152•10 years ago
|
||
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 153•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8506052 -
Flags: review?(terrence) → review+
Updated•10 years ago
|
Attachment #8506054 -
Flags: review?(terrence) → review+
Updated•10 years ago
|
Attachment #8506057 -
Flags: review?(terrence) → review+
Comment 154•10 years ago
|
||
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 155•10 years ago
|
||
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•10 years ago
|
||
Fix breakage following generator changes.
Attachment #8506834 -
Flags: review?(terrence)
Assignee | ||
Comment 157•10 years ago
|
||
Oh, we have help text for this in two places. Let's fix that.
Attachment #8506049 -
Attachment is obsolete: true
Attachment #8506835 -
Flags: review?(terrence)
Comment 158•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8506834 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 159•10 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•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/62ac7c28ddae
Comment 161•10 years ago
|
||
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•10 years ago
|
Attachment #8506052 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8506054 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8506057 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8506059 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8506834 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8506835 -
Flags: checkin+
Assignee | ||
Comment 162•10 years ago
|
||
This replaces the current scheme of keeping wrapped JSObjects alive with an explicit tracer that marks the contents of the sJSObjWrappers table and rekeys it as necessary when objects are moved. I added an ObjectMoved hook for the JSObject wrappers of NPObjects to update their hash table entries as necessary. Also I refactored the way the wrapper tables were created so there are separate functions to create and destroy each table.
Attachment #8508056 -
Flags: review?(benjamin)
Assignee | ||
Comment 163•10 years ago
|
||
Disable gczeal for the end of xdr/lazy.js, because GC can change the expected test result.
Attachment #8509612 -
Flags: review?(terrence)
Assignee | ||
Comment 164•10 years ago
|
||
This is a patch to root JSRuntimeWrapper::mGlobal. I had to refactor how this works slightly so that JSRuntime is created before the wrapper as the JS::PersistentRooted takes a pointer to the runtime in its constructor.
Attachment #8509620 -
Flags: review?(sworkman)
Assignee | ||
Comment 165•10 years ago
|
||
Add a way to pick a sensible set of arenas to relocate. Do this by relocating as many of the least-used arenas as we can so that their contents will fit into arenas we're not relocating. Note that I made this still relocate everything when zeal mode is on, and *in debug mode on 64 bit platforms*. This is so we continue to get good test coverage of moving objects. I appreciate that this is a little unusual so if you're not happy we can change it.
Attachment #8509627 -
Flags: review?(terrence)
Assignee | ||
Comment 166•10 years ago
|
||
Don't update pointers for all thing kinds, only those that can contain object pointers that might have moved. In practice that only means skipping strings, but that's still a win.
Attachment #8509630 -
Flags: review?(terrence)
Updated•10 years ago
|
Attachment #8509612 -
Flags: review?(terrence) → review+
Comment 167•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8509620 -
Flags: review?(sworkman) → review+
Updated•10 years ago
|
Attachment #8509630 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 168•10 years ago
|
||
The incremental type information sweeping changes in bug 1072564 broke compacting GC, so this is a fix for that. This runs all the sweeping code in one shot to update weak references.
Attachment #8510220 -
Flags: review?(bhackett1024)
Updated•10 years ago
|
Attachment #8510220 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 169•10 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•10 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•10 years ago
|
||
Fixed and re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/21b64777a90e
Comment 172•10 years ago
|
||
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•10 years ago
|
Attachment #8509612 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8509627 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8509630 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8510220 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8509620 -
Flags: checkin+
Assignee | ||
Comment 174•10 years ago
|
||
This patch fixes the issue where a partly-initialized DOM objects is moved before it has had its DOM_OBJECT_SLOT has been set.
Attachment #8511878 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 175•10 years ago
|
||
Fix a couple of places where moving objects that are partly-initialized causes us to try to read private values that have not been set.
Attachment #8511880 -
Flags: review?(terrence)
Comment 176•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8511880 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 177•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3a672d2759 https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f5e9388952
Comment 178•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8b3a672d2759 https://hg.mozilla.org/mozilla-central/rev/f5f5e9388952
Assignee | ||
Comment 179•10 years ago
|
||
Fix bug choosing which arenas to relocate when there aren't any to choose from.
Attachment #8512881 -
Flags: review?(terrence)
Assignee | ||
Comment 180•10 years ago
|
||
Include the gc invocation kind in the GC timer log output.
Attachment #8512882 -
Flags: review?(terrence)
Assignee | ||
Comment 181•10 years ago
|
||
Similar to the recent issue for DOM objects, sandbox objects can also have their object moved hook called before the object has finished being initialized. In this case the private pointer will be null so we need to check that.
Attachment #8512887 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 182•10 years ago
|
||
Update the list of test timeouts to ignore to include a couple more.
Attachment #8512890 -
Flags: review?(terrence)
Comment 183•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8512881 -
Flags: review?(terrence) → review+
Updated•10 years ago
|
Attachment #8512882 -
Flags: review?(terrence) → review+
Updated•10 years ago
|
Attachment #8512890 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 184•10 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•10 years ago
|
Attachment #8511878 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8511880 -
Flags: checkin+
Comment 185•10 years ago
|
||
(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)
Comment 186•10 years ago
|
||
(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.
Comment 187•10 years ago
|
||
(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...
Comment 188•10 years ago
|
||
(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•10 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•10 years ago
|
Attachment #8508056 -
Flags: review?(terrence)
Comment 190•10 years ago
|
||
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•10 years ago
|
||
Fix for test failures caused by inline typed object changes.
Attachment #8514344 -
Flags: review?(bhackett1024)
Updated•10 years ago
|
Attachment #8512887 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8514344 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 192•10 years ago
|
||
Patch to mprotect relocated arenas and keep them around to the next GC cycle to catch anything that tries to access them. Debug only.
Attachment #8514346 -
Flags: review?(terrence)
Comment 193•10 years ago
|
||
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 194•10 years ago
|
||
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 195•10 years ago
|
||
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.
Comment 196•10 years ago
|
||
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 197•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2acbf923b6a8
Assignee | ||
Comment 199•10 years ago
|
||
And backed out again for causing hazards: https://hg.mozilla.org/integration/mozilla-inbound/rev/054bd325286d
Assignee | ||
Comment 200•10 years ago
|
||
Fixed and re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5273d8dcb91
Assignee | ||
Updated•10 years ago
|
Attachment #8508056 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8512881 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8512882 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8512887 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8512890 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8514344 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8514346 -
Flags: checkin+
Assignee | ||
Comment 202•10 years ago
|
||
PCWrappedNativeScope::mDomExpandoSet is a set of JSObject pointers and in case these objects move it is updated by JS_CallHashSetObjectTracer() which calls HashSet<T>::Enum::rekeyFront(). However rekeyFront() is currently passed the prior and new object pointers, when it expects lookup and key parameters. This means it places the new entry in hash table according to the hash of the prior key value, making it present but unfindable. XPCWrappedNativeScope::RemoveDomExpandoObject() will then fail to remove the entry from the set when it should and since these are GC roots we have a leak. Prior and new pointers are totally correct when calling the very similar method HashSet<T>::rekeyIfMoved(). The patch fixes this and also asserts that RemoveDomExpandoObject() successfully removes an entry.
Attachment #8522270 -
Flags: review?(terrence)
Comment 203•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c42f7b5b39c
Assignee | ||
Updated•10 years ago
|
Attachment #8522270 -
Flags: checkin+
Assignee | ||
Comment 206•10 years ago
|
||
Parallise updating pointers to moved objects in cells. This is the longest part of compacting and is also possible to perform in parallel. This patch uses up to 8 threads and updates in batches of 256 arenas in release builds.
Attachment #8523981 -
Flags: review?(terrence)
Comment 207•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/63a296e4b6b3
Comment 210•10 years ago
|
||
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 | ||
Comment 211•10 years ago
|
||
Use background sweeping for compacting GC. With bug 989390 this is part of the preparation for having an incremental compacting GC.
Attachment #8526746 -
Flags: review?(terrence)
Assignee | ||
Comment 212•10 years ago
|
||
Remove an assertion that things don't move :)
Attachment #8526747 -
Flags: review?(terrence)
Comment 213•10 years ago
|
||
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 214•10 years ago
|
||
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•9 years ago
|
Attachment #8523981 -
Flags: checkin+
Assignee | ||
Comment 215•9 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+
Assignee | ||
Comment 217•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e18a24f1298
Assignee | ||
Updated•9 years ago
|
Attachment #8526746 -
Flags: checkin+
Assignee | ||
Comment 219•9 years ago
|
||
Fix a bug that means that we relocated far fewer arenas than we should. followingUsedCells needs to be the count of used cells from |*arenap| onwards, so don't initialise it with the count of all cells if arenap starts at the cursor!
Attachment #8530925 -
Flags: review?(terrence)
Updated•9 years ago
|
Attachment #8530925 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 220•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9442d0b10d87
Assignee | ||
Updated•9 years ago
|
Attachment #8530925 -
Flags: checkin+
Assignee | ||
Comment 222•9 years ago
|
||
Add an API to disable compacting GC for embedders.
Attachment #8533229 -
Flags: review?(terrence)
Assignee | ||
Comment 223•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
Updated•9 years ago
|
Attachment #8533229 -
Flags: review?(terrence) → review+
Comment 225•9 years ago
|
||
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 226•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f659e0e4632 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f72dca12ee
Comment 228•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7f659e0e4632 https://hg.mozilla.org/mozilla-central/rev/a0f72dca12ee
Assignee | ||
Comment 229•9 years ago
|
||
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•9 years ago
|
||
Something I meant to do before - add assertions that we are compacting as much as we should.
Attachment #8537746 -
Flags: review?(terrence)
Comment 231•9 years ago
|
||
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 232•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6351a5baa95 https://hg.mozilla.org/integration/mozilla-inbound/rev/d5492cfe7127
Assignee | ||
Updated•9 years ago
|
Attachment #8533229 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8533232 -
Flags: checkin+
Comment 234•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e6351a5baa95 https://hg.mozilla.org/mozilla-central/rev/d5492cfe7127
Assignee | ||
Updated•9 years ago
|
Attachment #8537744 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8537746 -
Flags: checkin+
Assignee | ||
Comment 235•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8548358 -
Flags: review?(terrence) → review+
Updated•9 years ago
|
Attachment #8548361 -
Flags: review?(terrence) → review+
Comment 238•9 years ago
|
||
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•9 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•9 years ago
|
||
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•9 years ago
|
||
Stop any off thread compilation which might be accessing things we are trying to relocate.
Attachment #8549016 -
Flags: review?(terrence)
Comment 242•9 years ago
|
||
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 243•9 years ago
|
||
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+
Comment 244•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6df62cce2ace https://hg.mozilla.org/integration/mozilla-inbound/rev/aea8d15e96bf
Assignee | ||
Updated•9 years ago
|
Attachment #8548358 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8548361 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8548363 -
Flags: checkin+
Assignee | ||
Comment 246•9 years ago
|
||
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•9 years ago
|
||
Rebased version of original patch, carrying r+.
Attachment #8533233 -
Attachment is obsolete: true
Attachment #8549763 -
Flags: review+
Assignee | ||
Comment 248•9 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)
Updated•9 years ago
|
Attachment #8549759 -
Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/6df62cce2ace https://hg.mozilla.org/mozilla-central/rev/aea8d15e96bf
Updated•9 years ago
|
Attachment #8549763 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 250•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0f5677578a https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2eec4a74ea
Assignee | ||
Updated•9 years ago
|
Attachment #8549013 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8549016 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Alias: CompactingGC
Assignee | ||
Updated•9 years ago
|
Blocks: CompactingGC
Comment 251•9 years ago
|
||
And, with great mourning, backed out after 8 hours on m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/dae4958ba3bf Wider exposure turned up this rare crash: https://treeherder.mozilla.org/logviewer.html#?job_id=5563530&repo=mozilla-inbound It looks like the UpdateCellPointersTask is calling trace hooks in parallel. This is firing a ReentrancyGuard -- and more generally, probably causing corruption in the HashTable it is guarding. I guess we should only do parallel updating on arenas that are background finalizable to avoid any trace-hook weirdness.
Assignee | ||
Comment 253•9 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #251) > It looks like the UpdateCellPointersTask is calling trace hooks in parallel. > This is firing a ReentrancyGuard -- and more generally, probably causing > corruption in the HashTable it is guarding. I guess we should only do > parallel updating on arenas that are background finalizable to avoid any > trace-hook weirdness. That's a really good idea. I don't think the wrapped native proto trace hook needs to be tracing the contents of its wrapped native scope either, but we'll see what bholley thinks about that.
Assignee | ||
Comment 254•9 years ago
|
||
This patch ensures that when updating cells in parallel that all foreground finalized arenas are updated on the same thread. For simplicity it's not actually the main thread but the main thread is idle at this point.
Attachment #8551393 -
Flags: review?(terrence)
Assignee | ||
Comment 255•9 years ago
|
||
The TraceInside() method on XPCWrappedNativeProto currently calls XPCWrappedNativeScope::TraceInside(). It seems to me like it could just trace the scope's global, since the trace hook for this will cause its contents to be traced. I tested this on try and the results are good, although it wouldn't surprise me if there was some subtlety that means this doesn't work. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0aa03e61085
Attachment #8551400 -
Flags: review?(continuation)
Assignee | ||
Updated•9 years ago
|
Attachment #8549759 -
Flags: checkin+
Comment 256•9 years ago
|
||
Comment on attachment 8551400 [details] [diff] [review] 64-wrapped-native-scope-tracing Review of attachment 8551400 [details] [diff] [review]: ----------------------------------------------------------------- I'm afraid I don't know much about this. Maybe Bill remembers something about it.
Attachment #8551400 -
Flags: review?(continuation) → review?(wmccloskey)
Comment on attachment 8551400 [details] [diff] [review] 64-wrapped-native-scope-tracing Review of attachment 8551400 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! This is the way things are supposed to work.
Attachment #8551400 -
Flags: review?(wmccloskey) → review+
Updated•9 years ago
|
Attachment #8551393 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 258•9 years ago
|
||
Try build is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9fa7a743847
Assignee | ||
Comment 259•9 years ago
|
||
Re-landing: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f847d97d0a0 https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e67e34e081 https://hg.mozilla.org/integration/mozilla-inbound/rev/26d8f946a53b
https://hg.mozilla.org/mozilla-central/rev/9f847d97d0a0 https://hg.mozilla.org/mozilla-central/rev/b7e67e34e081 https://hg.mozilla.org/mozilla-central/rev/26d8f946a53b
Comment 261•9 years ago
|
||
Comment on attachment 8549016 [details] [diff] [review] 61-stop-off-thread-compilation-while-compacting Review of attachment 8549016 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsgc.cpp @@ +2234,5 @@ > MOZ_ASSERT(!zone->isPreservingCode()); > > if (CanRelocateZone(rt, zone)) { > zone->setGCState(Zone::Compact); > + StopAllOffThreadCompilations(zone); Missing #include "jit/Ion.h" jit::StopAllOffThreadCompilations(zone);
Assignee | ||
Comment 262•9 years ago
|
||
This hasn't been backed out after a week so I'm marking as fixed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Keywords: leave-open
Target Milestone: --- → mozilla38
Comment 263•9 years ago
|
||
I just looked closely at AWSY and compacting GC made absolutely no difference to the results there. Hmm. jonco, have you done any more measurements recently to see if compacting GC is having a noticeable effect?
Comment 264•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #263) > I just looked closely at AWSY and compacting GC made absolutely no > difference to the results there. Hmm. > > jonco, have you done any more measurements recently to see if compacting GC > is having a noticeable effect? Hmm, I see bug 1055067 is still open -- maybe there's another switch that needs to be flicked for the browser?
Assignee | ||
Comment 265•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #264) I think the problem is that at the moment shrinking GC is only triggered on memory pressure events and by GCRuntime::maybePeriodicFullGC(). The heuristics used by the latter may actually not trigger in practice. I'll look into it, but I suspect the answer is we need to find a better way of triggering shrinking GCs.
Updated•9 years ago
|
Blocks: MatchStartupMem
You need to log in
before you can comment on or make changes to this bug.
Description
•