Closed
Bug 1105069
Opened 10 years ago
Closed 9 years ago
Make the GC API strongly typed
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
People
(Reporter: terrence, Assigned: terrence)
References
(Blocks 3 open bugs)
Details
Attachments
(21 files, 4 obsolete files)
9.47 KB,
patch
|
jonco
:
review+
mccr8
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
9.10 KB,
patch
|
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
3.08 KB,
patch
|
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
3.48 KB,
patch
|
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
2.63 KB,
patch
|
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
7.05 KB,
patch
|
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
8.68 KB,
patch
|
jonco
:
review+
mccr8
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
13.15 KB,
patch
|
jonco
:
review+
mccr8
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
2.61 KB,
patch
|
mccr8
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
8.38 KB,
patch
|
jonco
:
review+
mccr8
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
7.90 KB,
patch
|
jonco
:
review+
mccr8
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
5.05 KB,
patch
|
jonco
:
review+
mccr8
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
7.42 KB,
patch
|
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
23.12 KB,
patch
|
jonco
:
review+
mccr8
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
8.50 KB,
patch
|
mccr8
:
review+
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
7.68 KB,
patch
|
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
21.21 KB,
patch
|
terrence
:
review+
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
8.95 KB,
patch
|
mccr8
:
review+
jonco
:
review+
|
Details | Diff | Splinter Review |
12.61 KB,
patch
|
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
4.93 KB,
patch
|
jonco
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
6.75 KB,
patch
|
jonco
:
review+
mccr8
:
review+
terrence
:
checkin+
|
Details | Diff | Splinter Review |
Since JSObject, JSString, JSScript, and JS::Symbol are opaque symbols in the public API, embedders cannot see the common Cell base. Moreover, because the tag we store in the heap is only access via a slow API call currently, their only option is to not upcast or to use a tagged union. In the case of Gecko, this is either a void* or a void*/JSGCTraceKind pair. In order to make the code less verbose, make a wide variety of misuses into compile errors, and add runtime correctness assertions, I'm going to encapsulate this tagged union in a class. All external GC APIs should take either a specific concrete type, or one of these tagged unions. All internal GC APIs should be either generic enough to take a Cell directly, or do a template expansion to a concrete class if type-specific processing is required.
Assignee | ||
Comment 1•10 years ago
|
||
Andrew, could you take a look at the Gecko bits? I basically just moved the inline JSGCTraceKind->char* into an API method. Jon, the rest of this moves the trace kind definitions adjacent to the tracing API. It also assigns concrete integers to the various kinds: the bottom 3 bits are reserved for public kinds for reasons that will become clear in the next patch.
Attachment #8528693 -
Flags: review?(jcoppeard)
Attachment #8528693 -
Flags: review?(continuation)
Assignee | ||
Comment 2•10 years ago
|
||
Implement the actual GCCellPtr bits. As a slight departure from our existing scheme of storing two words, this packs the public kinds into the bottom of the pointer. I did this so that we can use this type efficiently, even in places that are currently using only a void* and relying on the Arena's tag bits behind the API. I don't expect the extra mask to have any impact on performance on a modern processor.
Attachment #8528702 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 3•10 years ago
|
||
Trivial, as it is only called from typed callers. I think eventually we should templatize the typed callers, but for now we should move slowly.
Attachment #8528704 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 4•10 years ago
|
||
The same situation as the prior change.
Attachment #8528708 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 5•10 years ago
|
||
Ditto, modulo the fact that this API only has a single caller. We should probably consider making the caller do the work here.
Attachment #8528710 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 6•10 years ago
|
||
Ditto again, although this time IncrementalReferenceBarrier is a bit more complex so the new wrapper allows us to make the code a bit nicer.
Attachment #8528711 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 7•10 years ago
|
||
Reduce the weakmap tracer callback from 6 args to 4 args and clean up the innards of the two users using the new wrapper methods.
Attachment #8528714 -
Flags: review?(jcoppeard)
Attachment #8528714 -
Flags: review?(continuation)
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8528714 [details] [diff] [review] 7_weakmaptracer-v0.diff Review of attachment 8528714 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +214,5 @@ > tracer->mCb.NoteWeakMapping(aMap, aKey, kdelegate, aValue); > } else { > tracer->mChildTracer.mTracedAny = false; > tracer->mChildTracer.mMap = aMap; > tracer->mChildTracer.mKey = aKey; Erk. I think |mach build binaries| must be messing with me, because I don't see how this could compile. Doing a clobber build now.
Assignee | ||
Comment 9•10 years ago
|
||
Zarro boogs found. This must be going through Cell* to void* and the API at the other end is still taking void*. I think I'd prefer not to have this hole: thoughts on changing the implicit conversion to an asCell?
Comment 10•10 years ago
|
||
Comment on attachment 8528693 [details] [diff] [review] 1_move_tracekind-v0.diff Review of attachment 8528693 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Marking.cpp @@ +712,5 @@ > case JSTRACE_JITCODE: > MarkInternal(trc, reinterpret_cast<jit::JitCode **>(thingp)); > break; > + case JSTRACE_NULL: > + case JSTRACE_OUTOFLINE: We should add a default case to catch bad trace kinds too, and also in PushArena() and TraceChildren(). ::: js/src/gc/Tracer.cpp @@ +218,5 @@ > + case JSTRACE_NULL: > + name = "nullptr"; > + break; > + > + case JSTRACE_OUTOFLINE: We could also add default case here and below in this function.
Attachment #8528693 -
Flags: review?(jcoppeard) → review+
Comment 11•10 years ago
|
||
Comment on attachment 8528702 [details] [diff] [review] 2_impl_cellptr-v0.diff Review of attachment 8528702 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: js/public/HeapAPI.h @@ +258,5 @@ > + } > + > + private: > + uintptr_t checkedCast(void *p, JSGCTraceKind traceKind) { > + js::gc::Cell *cell = reinterpret_cast<js::gc::Cell *>(p); I think you can use static_cast here. ::: js/src/jsgc.cpp @@ +7001,5 @@ > + ptr = checkedCast(v.toString(), JSTRACE_STRING); > + else if (v.isObject()) > + ptr = checkedCast(&v.toObject(), JSTRACE_OBJECT); > + else > + ptr = checkedCast(nullptr, JSTRACE_NULL); We also need to handle symbols here.
Attachment #8528702 -
Flags: review?(jcoppeard) → review+
Updated•10 years ago
|
Attachment #8528704 -
Flags: review?(jcoppeard) → review+
Updated•10 years ago
|
Attachment #8528708 -
Flags: review?(jcoppeard) → review+
Updated•10 years ago
|
Attachment #8528710 -
Flags: review?(jcoppeard) → review+
Comment 12•10 years ago
|
||
Comment on attachment 8528711 [details] [diff] [review] 6_incrementalreferencebarrier-v0.diff Review of attachment 8528711 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsfriendapi.cpp @@ +1209,5 @@ > + return BaseShape::writeBarrierPre(static_cast<BaseShape*>(cell)); > + case JSTRACE_TYPE_OBJECT: > + return types::TypeObject::writeBarrierPre(static_cast<types::TypeObject *>(cell)); > + case JSTRACE_NULL: > + case JSTRACE_OUTOFLINE: Maybe add default case here too.
Attachment #8528711 -
Flags: review?(jcoppeard) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8528714 [details] [diff] [review] 7_weakmaptracer-v0.diff Review of attachment 8528714 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsfriendapi.h @@ +491,5 @@ > * > * m will be nullptr if the weak map is not contained in a JS Object. > */ > typedef void > +(* WeakMapTraceCallback)(WeakMapTracer *trc, JSObject *m, JS::GCCellPtr key, JS::GCCellPtr value); This is much better!
Attachment #8528714 -
Flags: review?(jcoppeard) → review+
Comment 14•10 years ago
|
||
Comment on attachment 8528693 [details] [diff] [review] 1_move_tracekind-v0.diff Review of attachment 8528693 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the CycleCollectedJSRuntime.cpp changes ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +584,3 @@ > } > } else { > + JS_snprintf(name, sizeof(name), "JS %s", JS::GCTraceKindToAscii(aTraceKind)); Yay!
Attachment #8528693 -
Flags: review?(continuation) → review+
Comment 15•10 years ago
|
||
Comment on attachment 8528714 [details] [diff] [review] 7_weakmaptracer-v0.diff Review of attachment 8528714 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, having an explicit .asCell() and stopping implicit conversions sounds like a good idea, as people should not be doing that. Ideally, we'd move the CC's internal representation of ptr+participant to some kind of thing that would not involve void* for JS and then we could get rid of the .asCell() calls.
Attachment #8528714 -
Flags: review?(continuation) → review-
Comment 16•10 years ago
|
||
This is a nice improvement over all! Passing around void* everywhere for JS things has bothered me since I first started working on Firefox.
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #15) > Comment on attachment 8528714 [details] [diff] [review] > 7_weakmaptracer-v0.diff > > Review of attachment 8528714 [details] [diff] [review]: > ----------------------------------------------------------------- > > Yeah, having an explicit .asCell() and stopping implicit conversions sounds > like a good idea, as people should not be doing that. Ideally, we'd move > the CC's internal representation of ptr+participant to some kind of thing > that would not involve void* for JS and then we could get rid of the > .asCell() calls. That's exactly what I had in mind. Finding a representation that allows both a CellPtr and a C++ pointer may be a bit of work, but I think we can use the same trick we use to treat all of Cell*, jsid, and Value as T in a template context. We should add it to the list of things to discuss in Portland. I guess the r- is because you want to see it with asCell? I had originally split that out to a separate patch on top, unfortunately. I can fold this down easily enough, but I'm going to have to wait until I have steady power: touching any of this stuff is basically a clobber and all 6 patches will need touch-ups.
Comment 18•10 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #17) > I guess the r- is because you want to see it with asCell? I had originally > split that out to a separate patch on top, unfortunately. I can fold this > down easily enough, but I'm going to have to wait until I have steady power: > touching any of this stuff is basically a clobber and all 6 patches will > need touch-ups. That's fine if it is separate patches, I'll just look at them all together when you upload them to try to get a better sense of how it all fits together.
Assignee | ||
Comment 19•10 years ago
|
||
It's only a dozen or so sites inside and out. Well worth the extra safety.
Attachment #8531109 -
Flags: review?(jcoppeard)
Attachment #8531109 -
Flags: review?(continuation)
Comment 20•10 years ago
|
||
Comment on attachment 8531109 [details] [diff] [review] 8_use_explicit_accessor-v0.diff Review of attachment 8531109 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.
Attachment #8531109 -
Flags: review?(jcoppeard) → review+
Comment 21•10 years ago
|
||
Comment on attachment 8531109 [details] [diff] [review] 8_use_explicit_accessor-v0.diff Review of attachment 8531109 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! r=me for the CC changes I'll look at the other patch at some point. ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +271,5 @@ > > if (delegateMightNeedMarking && aKey.isObject()) { > JSObject* kdelegate = js::GetWeakmapKeyDelegate(aKey.toObject()); > if (kdelegate && !xpc_IsGrayGCThing(kdelegate)) { > + if (JS::UnmarkGrayGCThingRecursively(aKey.asCell(), JSTRACE_OBJECT)) { Please file a followup bug to change the type of UmarkGrayGCThingRecursively to take a JS::GCCellPtr. Hopefully we can replace that everywhere.
Attachment #8531109 -
Flags: review?(continuation) → review+
Updated•10 years ago
|
Attachment #8528714 -
Flags: review- → review?(continuation)
Comment 22•10 years ago
|
||
Comment on attachment 8528714 [details] [diff] [review] 7_weakmaptracer-v0.diff Review of attachment 8528714 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the CC changes ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +194,5 @@ > // The cycle collector can only properly reason about weak maps if it can > // reason about the liveness of their keys, which in turn requires that > // the key can be represented in the cycle collector graph. All existing > // uses of weak maps use either objects or scripts as keys, which are okay. > + MOZ_ASSERT(AddToCCKind(aKey.kind())); It looks like AddToCCKind can be changed to something that takes a JS::GCCellPtr and is called HasCCKind(). Please file a follow up for that and I or somebody else can fix it. @@ +218,5 @@ > tracer->mChildTracer.mKey = aKey; > tracer->mChildTracer.mKeyDelegate = kdelegate; > > + if (aValue.isString()) { > + JS_TraceChildren(&tracer->mChildTracer, aValue, aValue.kind()); The two args here could be combined into a single arg, too. File a followup or something?
Attachment #8528714 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 23•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #21) > Please file a followup bug to change the type of UmarkGrayGCThingRecursively > to take a JS::GCCellPtr. Hopefully we can replace that everywhere. > (In reply to Andrew McCreight [:mccr8] from comment #22) > The two args here could be combined into a single arg, too. File a followup > or something? I've got ~10 more patches in my queue right now in this bug that handles the rest of the interface bits. (In reply to Andrew McCreight [:mccr8] from comment #22) > It looks like AddToCCKind can be changed to something that takes a > JS::GCCellPtr and is called HasCCKind(). Please file a follow up for that > and I or somebody else can fix it. Thanks for offering to help with the CC internals, I'll gladly accept the help!
Assignee | ||
Comment 24•10 years ago
|
||
Pushing the asCell down one more level.
Attachment #8531669 -
Flags: review?(continuation)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8531670 -
Flags: review?(jcoppeard)
Attachment #8531670 -
Flags: review?(continuation)
Updated•10 years ago
|
Attachment #8531669 -
Flags: review?(continuation) → review+
Comment 26•10 years ago
|
||
Comment on attachment 8531670 [details] [diff] [review] 10_noteweakmapping-v0.diff Review of attachment 8531670 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the CC parts
Attachment #8531670 -
Flags: review?(continuation) → review+
Updated•10 years ago
|
Attachment #8531670 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8531708 -
Flags: review?(jcoppeard)
Attachment #8531708 -
Flags: review?(continuation)
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8531711 -
Flags: review?(jcoppeard)
Attachment #8531711 -
Flags: review?(continuation)
Comment 29•10 years ago
|
||
Comment on attachment 8531708 [details] [diff] [review] 11_unmarkgraygcthingrecursively-v0.diff Review of attachment 8531708 [details] [diff] [review]: ----------------------------------------------------------------- r=me for CC changes
Attachment #8531708 -
Flags: review?(continuation) → review+
Comment 30•10 years ago
|
||
Comment on attachment 8531711 [details] [diff] [review] 12_visitgraywrappertargets-v0.diff Review of attachment 8531711 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the CC changes
Attachment #8531711 -
Flags: review?(continuation) → review+
Comment 31•10 years ago
|
||
Comment on attachment 8531708 [details] [diff] [review] 11_unmarkgraygcthingrecursively-v0.diff Review of attachment 8531708 [details] [diff] [review]: ----------------------------------------------------------------- Nice.
Attachment #8531708 -
Flags: review?(jcoppeard) → review+
Updated•10 years ago
|
Attachment #8531711 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 32•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/caa4ffd2f765 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b57ab0cf44f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/10a8482ed924 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ceaedf7b5c06 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f2826852f58 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2efa3122ce92 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2a54fffd77 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec983c96d034 The bustage on the try run was all on m-i at the time. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f3bff939f7b3
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Assignee | ||
Updated•10 years ago
|
Attachment #8528693 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8528702 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8528704 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8528708 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8528710 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8528711 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8528714 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8531109 -
Flags: checkin+
Assignee | ||
Comment 33•10 years ago
|
||
Move the internal HeapAPI bits into the details namespace to make extra clear that it's not supposed to be used externally and make them take uintptr_t directly, rather than casting externally. This allows us to use thing.unsafeAsUIntPtr() rather than uintptr_t(thing.asCell()), which should at least make grepping for misuse easier, since we can't hide the inlined bits.
Attachment #8533823 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 34•10 years ago
|
||
Strongly type xpc_IsGrayGCThing.
Attachment #8533827 -
Flags: review?(continuation)
Assignee | ||
Updated•10 years ago
|
Attachment #8533827 -
Flags: review?(jcoppeard)
Comment 35•10 years ago
|
||
Comment on attachment 8533827 [details] [diff] [review] 14_xpcisgraygcthing-v0.diff Review of attachment 8533827 [details] [diff] [review]: ----------------------------------------------------------------- I reviewed the DOM, XPCOM, and XPConnect changes. The changes look nice to me over all, you are just unfortunately adding some calls to GCThingTraceKind that should be avoided. ::: dom/base/nsWrapperCacheInlines.h @@ +31,5 @@ > SearchGray(void* aGCThing, const char* aName, void* aClosure) > { > bool* hasGrayObjects = static_cast<bool*>(aClosure); > + if (!*hasGrayObjects && aGCThing && > + JS::GCThingIsMarkedGray(JS::GCCellPtr(aGCThing, js::GCThingTraceKind(aGCThing)))) { This code is pretty hot, so I'd like to not add a call to GCThingTraceKind in here. I think we can avoid this call by doing a conversion similar to bug 820233 (this should probably get moved to nsWrapperCache.cpp as having it here is a little weird). Would you like me to do that? ::: js/xpconnect/src/nsXPConnect.cpp @@ +297,5 @@ > > bool > xpc_GCThingIsGrayCCThing(void *thing) > { > return AddToCCKind(js::GCThingTraceKind(thing)) && Please factor out the call to GCThingTraceKind. It is slow enough that I've seen it show up in profiles, and we might as well avoid the double call. ::: js/xpconnect/src/xpcpublic.h @@ +187,5 @@ > > +inline bool > +xpc_IsGrayObject(JSObject *obj) > +{ > + return JS::ObjectIsMarkedGray(obj); I'd be inclined to just use JS::ObjectIsMarkedGray() directly everywhere instead of this new wrapper, but it is up to you. ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +317,5 @@ > if (closure->mCycleCollectionEnabled) { > return; > } > > if (AddToCCKind(js::GCThingTraceKind(aThing)) && This adds a new call to js::GCThingTraceKind(), which is expensive enough that it shows up in profiles of the CC. I actually have a patch in bug 820233 that eliminates the call here by getting rid of the tracing indirection, so I don't want to add a new one here. Could you look at that patch and suggest how I could fix it to make this easier? I guess change the first argument of JSHolderTraceImpl to be JS::GCCellPtr? With that in place, then you should be able to fix this without any trouble.
Attachment #8533827 -
Flags: review?(continuation) → review-
Assignee | ||
Comment 36•10 years ago
|
||
Comment on attachment 8533827 [details] [diff] [review] 14_xpcisgraygcthing-v0.diff In that case, I guess our options are to temporarily inline GCThingTraceKind or to reorder the patches to get the callers converted first. I think I'll go with the second route.
Attachment #8533827 -
Attachment is obsolete: true
Attachment #8533827 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8533939 -
Flags: review?(jcoppeard)
Attachment #8533939 -
Flags: review?(continuation)
Comment 38•10 years ago
|
||
Comment on attachment 8533939 [details] [diff] [review] 14_notejschild-v0.diff Review of attachment 8533939 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for reordering this to address my concerns on the other patch. r- for my concerns about DebugWrapperTraceCallback and nsScriptObjectTracer::NoteJSChild. Am I misunderstanding something? ::: dom/base/nsWrapperCache.cpp @@ +59,5 @@ > uint64_t aCompartmentAddress) > { > } > > + NS_IMETHOD_(void) NoteJSObject(JSObject* aObject) nit: please make this aChild. @@ +66,4 @@ > mFound = true; > } > } > + NS_IMETHOD_(void) NoteJSScript(JSScript* aScript) nit: please make this aChild. @@ +67,5 @@ > } > } > + NS_IMETHOD_(void) NoteJSScript(JSScript* aScript) > + { > + MOZ_ASSERT(uintptr_t(aScript) != uintptr_t(mWrapper)); This doesn't seem useful to me, now that things are properly typed. If we're passing around a pointer value as both a JSScript* and a JSObject* we're probably in big trouble anyways! @@ +89,4 @@ > }; > > static void > +DebugWrapperTraceCallback(JS::GCCellPtr aP, const char* aName, void* aClosure) While you are here, please rename aP to aPtr. @@ +93,4 @@ > { > DebugWrapperTraversalCallback* callback = > static_cast<DebugWrapperTraversalCallback*>(aClosure); > + callback->NoteJSObject(aP.toObject()); I think this isn't right. aP could be a script, and then you'd hit an assert here on the conversion. I think you just want to guard this call to NoteJSObject on aP.isObject(). ::: js/xpconnect/src/XPCVariant.cpp @@ -82,5 @@ > NS_IMPL_CYCLE_COLLECTION_CLASS(XPCVariant) > > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(XPCVariant) > JS::Value val = tmp->GetJSValPreserveColor(); > - if (val.isObjectOrNull()) { Weird that this took null before. I wonder if that even worked. ::: xpcom/glue/nsCycleCollectionParticipant.cpp @@ +21,5 @@ > { > nsCycleCollectionTraversalCallback* cb = > static_cast<nsCycleCollectionTraversalCallback*>(aClosure); > NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(*cb, aName); > + cb->NoteJSScript(aScriptThing.toScript()); The name is confusing here, but aScriptThing isn't necessarily a script, it could be an object. I'm surprised this doesn't assert. In "script thing" and "script object tracer", the "script" means "for whatever scripting language this is from, be it JS or Ruby or Python", and thus is pretty out-of-date. It doesn't specifically refer to JSScripts. ::: xpcom/glue/nsCycleCollectionParticipant.h @@ +183,5 @@ > > NS_IMETHOD_(void) Trace(void* aPtr, const TraceCallbacks& aCb, > void* aClosure) = 0; > > + static void NoteJSChild(JS::GCCellPtr aScriptThing, const char* aName, aScriptThing should probably be renamed to aGCThing or something here and in the definition to be less confusing. ::: xpcom/glue/nsCycleCollectionTraversalCallback.h @@ +26,5 @@ > const char* aObjName, > uint64_t aCompartmentAddress = 0) = 0; > > NS_IMETHOD_(void) NoteXPCOMChild(nsISupports* aChild) = 0; > + NS_IMETHOD_(void) NoteJSObject(JSObject* aObject) = 0; nit: please name this aChild (also for NoteJSScript)
Attachment #8533939 -
Flags: review?(continuation) → review-
Assignee | ||
Comment 39•10 years ago
|
||
This got through the first 8000 mochitests, so should be in better shape now. I'm pushing this to Try too, but the push is taking forever.
Attachment #8533939 -
Attachment is obsolete: true
Attachment #8533939 -
Flags: review?(jcoppeard)
Attachment #8534003 -
Flags: review?(jcoppeard)
Attachment #8534003 -
Flags: review?(continuation)
Comment 40•10 years ago
|
||
Comment on attachment 8534003 [details] [diff] [review] 14_notejschild-v1.diff r- this patch is empty. ;)
Attachment #8534003 -
Flags: review?(jcoppeard)
Attachment #8534003 -
Flags: review?(continuation)
Attachment #8534003 -
Flags: review-
Assignee | ||
Comment 41•10 years ago
|
||
Well, it has "try: -b do -p all -u all -t all", but that's... not really helpful. Just one of those days I guess.
Attachment #8534003 -
Attachment is obsolete: true
Attachment #8534031 -
Flags: review?(jcoppeard)
Attachment #8534031 -
Flags: review?(continuation)
Comment 42•10 years ago
|
||
Comment on attachment 8534031 [details] [diff] [review] 14_notejschild-v1.diff Review of attachment 8534031 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the non-Id.h changes ::: xpcom/base/CycleCollectedJSRuntime.h @@ +291,5 @@ > // isn't one. > static CycleCollectedJSRuntime* Get(); > > + // Returns true if the JSGCTraceKind is one the cycle collector cares about. > + static bool AddToCCKind(JSGCTraceKind kind) { This should be a stand alone function in the mozilla:: namespace, not a static method. (And then have "inline" again.) nit: aKind ::: xpcom/base/nsCycleCollector.cpp @@ +2082,5 @@ > > NS_IMETHOD_(void) NoteXPCOMChild(nsISupports* aChild); > + NS_IMETHOD_(void) NoteJSObject(JSObject* aChild); > + NS_IMETHOD_(void) NoteJSScript(JSScript* aChild); > + void NoteJSChild(JS::GCCellPtr aChild); NoteJSChild should probably be private.
Attachment #8534031 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 43•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=49dc54d86874 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2c634395d8da remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/03465e8b42db remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c73fe9791999 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/08945a5b1f12
Assignee | ||
Updated•10 years ago
|
Attachment #8531669 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8531670 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8531708 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8531711 -
Flags: checkin+
Comment 44•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2c634395d8da https://hg.mozilla.org/mozilla-central/rev/03465e8b42db https://hg.mozilla.org/mozilla-central/rev/c73fe9791999 https://hg.mozilla.org/mozilla-central/rev/08945a5b1f12
Updated•10 years ago
|
Attachment #8533823 -
Flags: review?(jcoppeard) → review+
Comment 45•10 years ago
|
||
Comment on attachment 8534031 [details] [diff] [review] 14_notejschild-v1.diff Review of attachment 8534031 [details] [diff] [review]: ----------------------------------------------------------------- Id.h changes are fine. Yay to pushing types through the cycle collector!
Attachment #8534031 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 46•10 years ago
|
||
Finally got a try run through the (apparent) gauntlet. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cc1c67bb0573
Assignee | ||
Comment 47•10 years ago
|
||
Push the last js::GCThingTraceKind call up a level. Now there is only one remaining site where it is required. This also introduces xpc_ValueIsGrayCCThing -- it should be a bit faster to check the value's tag first in this case.
Attachment #8534469 -
Flags: review?(jcoppeard)
Attachment #8534469 -
Flags: review?(continuation)
Updated•10 years ago
|
Attachment #8534469 -
Flags: review?(jcoppeard) → review+
Comment 48•10 years ago
|
||
Comment on attachment 8534469 [details] [diff] [review] 15_gcthingisgrayccthing-v0.diff Review of attachment 8534469 [details] [diff] [review]: ----------------------------------------------------------------- Nice. r=me on the non-HeapAPI.h changes ::: js/xpconnect/src/nsXPConnect.cpp @@ +304,5 @@ > + > +bool > +xpc_ValueIsGrayCCThing(const JS::Value& value) > +{ > + return CycleCollectedJSRuntime::AddToCCKind(value.gcKind()) && per my comment on an earlier patch, AddToCCKind should just be in the mozilla:: namespace. Does this compile? :P ::: js/xpconnect/src/xpcpublic.h @@ +187,5 @@ > > // The cycle collector only cares about some kinds of GCthings that are > // reachable from an XPConnect root. Implemented in nsXPConnect.cpp. > extern bool > +xpc_GCThingIsGrayCCThing(JS::GCCellPtr thing); It looks like these two methods are only called in nsCycleCollector.cpp, so maybe move them into that file? You could also get rid of the xpc_ part of their names when you do that. If that's too annoying to deal with you can do that in a separate patch, or file a bug and assign it to me. ::: xpcom/base/nsCycleCollector.cpp @@ +2374,3 @@ > return AddNode(zone, mJSZoneParticipant); > } > + return AddNode(aNode.asCell(), mJSParticipant); I wonder if the CC would be faster if JS stuff was stored separately, and we were able to make direct participant calls, now that we have this type information all the way down here. I'll file a bug about that. @@ +2477,5 @@ > > NS_IMETHODIMP_(void) > ChildFinder::NoteJSObject(JSObject* aChild) > { > + if (aChild && JS::ObjectIsMarkedGray(aChild)) { Neat. @@ +2631,5 @@ > void* aClosure) const > { > JS::Value val = *aValue; > + if (aValue->isMarkable()) { > + if (xpc_ValueIsGrayCCThing(*aValue)) { just combine these two ifs via an && here now that there's no intermediate declaration.
Attachment #8534469 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 49•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #48) > Comment on attachment 8534469 [details] [diff] [review] > 15_gcthingisgrayccthing-v0.diff > > Review of attachment 8534469 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice. r=me on the non-HeapAPI.h changes > > ::: js/xpconnect/src/nsXPConnect.cpp > @@ +304,5 @@ > > + > > +bool > > +xpc_ValueIsGrayCCThing(const JS::Value& value) > > +{ > > + return CycleCollectedJSRuntime::AddToCCKind(value.gcKind()) && > > per my comment on an earlier patch, AddToCCKind should just be in the > mozilla:: namespace. Does this compile? :P Ah, I see that hunk wondered into part 16. Fixed. > ::: js/xpconnect/src/xpcpublic.h > @@ +187,5 @@ > > > > // The cycle collector only cares about some kinds of GCthings that are > > // reachable from an XPConnect root. Implemented in nsXPConnect.cpp. > > extern bool > > +xpc_GCThingIsGrayCCThing(JS::GCCellPtr thing); > > It looks like these two methods are only called in nsCycleCollector.cpp, so > maybe move them into that file? You could also get rid of the xpc_ part of > their names when you do that. If that's too annoying to deal with you can > do that in a separate patch, or file a bug and assign it to me. Oh, good spot! Done. > ::: xpcom/base/nsCycleCollector.cpp > @@ +2374,3 @@ > > return AddNode(zone, mJSZoneParticipant); > > } > > + return AddNode(aNode.asCell(), mJSParticipant); > > I wonder if the CC would be faster if JS stuff was stored separately, and we > were able to make direct participant calls, now that we have this type > information all the way down here. I'll file a bug about that. Awww, not going to let me have all the fun? ;-)
Assignee | ||
Comment 50•10 years ago
|
||
This rewrites UnmarkGrayChildren and TenuredCell::readBarrier in terms of our more convenient (and inlinable) internal interfaces.
Attachment #8534597 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 51•10 years ago
|
||
And at long last, we can now use GCCellPtr for xpc_IsGrayGCThing without needing to call js::GCThingTraceKind. Or at least we could if I didn't take Andrew's advice and kill xpc_IsGrayGCThing in favor of JS::GCThingIsMarkedGray.
Attachment #8534599 -
Flags: review?(jcoppeard)
Attachment #8534599 -
Flags: review?(continuation)
Comment 52•10 years ago
|
||
Comment on attachment 8533823 [details] [diff] [review] 13_uintptr_details-v0.diff Review of attachment 8533823 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/HeapAPI.h @@ +282,5 @@ > uint64_t unsafeAsInteger() const { > return reinterpret_cast<uint64_t>(unsafeGetUntypedPtr()); > } > + // Inline mark bitmap access requires direct pointer arithmetic. > + const uintptr_t unsafeAsUIntPtr() const { Clang noticed that the const on the return type that apparently does nothing.
Comment 53•10 years ago
|
||
Comment on attachment 8534599 [details] [diff] [review] 17_xpcisgraygcthing-v1.diff Review of attachment 8534599 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the DOM, XPConnect and CC changes. ::: js/public/HeapAPI.h @@ +405,5 @@ > +static MOZ_ALWAYS_INLINE bool > +GCThingIsMarkedGray(GCCellPtr thing) > +{ > + MOZ_ASSERT(thing); > + if (thing.isObject()) It looks like there's a lot of code duplication here, which seems bad when this is going to be inlined all over the place. Is the compiler going to be able to do something reasonable with it and factor out the MarkWordAndMask stuff etc.? If not, it might be worth a little code duplication to make this more compact. ::: js/xpconnect/src/xpcpublic.h @@ -179,5 @@ > -// The JS GC marks objects gray that are held alive directly or > -// indirectly by an XPConnect root. The cycle collector explores only > -// this subset of the JS heap. > -inline bool > -xpc_IsGrayGCThing(void *thing) There's a comment in Chunk::init in jsgc.cpp that refers to xpc_IsGrayGCThing you should fix up.
Attachment #8534599 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 54•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #52) > Comment on attachment 8533823 [details] [diff] [review] > 13_uintptr_details-v0.diff > > Review of attachment 8533823 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/public/HeapAPI.h > @@ +282,5 @@ > > uint64_t unsafeAsInteger() const { > > return reinterpret_cast<uint64_t>(unsafeGetUntypedPtr()); > > } > > + // Inline mark bitmap access requires direct pointer arithmetic. > > + const uintptr_t unsafeAsUIntPtr() const { > > Clang noticed that the const on the return type that apparently does nothing. Yup, fixed it and went to re-Try with 15 included and found that Try is closed. I guess I'll have to increase my queue depth yet again and land tomorrow. (In reply to Andrew McCreight [:mccr8] from comment #53) > Comment on attachment 8534599 [details] [diff] [review] > 17_xpcisgraygcthing-v1.diff > > Review of attachment 8534599 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me on the DOM, XPConnect and CC changes. > > ::: js/public/HeapAPI.h > @@ +405,5 @@ > > +static MOZ_ALWAYS_INLINE bool > > +GCThingIsMarkedGray(GCCellPtr thing) > > +{ > > + MOZ_ASSERT(thing); > > + if (thing.isObject()) > > It looks like there's a lot of code duplication here, which seems bad when > this is going to be inlined all over the place. Is the compiler going to be > able to do something reasonable with it and factor out the MarkWordAndMask > stuff etc.? If not, it might be worth a little code duplication to make > this more compact. Yeah, this isn't great. Let's invert it and have them call a common CellIsMarkedGray instead. > ::: js/xpconnect/src/xpcpublic.h > @@ -179,5 @@ > > -// The JS GC marks objects gray that are held alive directly or > > -// indirectly by an XPConnect root. The cycle collector explores only > > -// this subset of the JS heap. > > -inline bool > > -xpc_IsGrayGCThing(void *thing) > > There's a comment in Chunk::init in jsgc.cpp that refers to > xpc_IsGrayGCThing you should fix up. Fixed.
Attachment #8534640 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8534599 -
Attachment is obsolete: true
Attachment #8534599 -
Flags: review?(jcoppeard)
Assignee | ||
Updated•10 years ago
|
Attachment #8534640 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 55•10 years ago
|
||
And push down into MergeZone so we can fix up GetTenuredGCThingZone. With this we're done with void* in HeapAPI.
Attachment #8534644 -
Flags: review?(jcoppeard)
Attachment #8534644 -
Flags: review?(continuation)
Assignee | ||
Comment 56•10 years ago
|
||
I don't see much point keeping it.
Attachment #8534661 -
Flags: review?(jcoppeard)
Comment 57•10 years ago
|
||
Comment on attachment 8534644 [details] [diff] [review] 18_gettenuredgcthingzone_mergezone-v0.diff Review of attachment 8534644 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingUtils.h @@ -765,5 @@ > MaybeWrapStringValue(JSContext* cx, JS::MutableHandle<JS::Value> rval) > { > MOZ_ASSERT(rval.isString()); > JSString* str = rval.toString(); > - if (JS::GetTenuredGCThingZone(str) != js::GetContextZone(cx)) { I assume this new function is about as fast as the old one?
Attachment #8534644 -
Flags: review?(continuation) → review+
Updated•10 years ago
|
Attachment #8534597 -
Flags: review?(jcoppeard) → review+
Updated•10 years ago
|
Attachment #8534640 -
Flags: review?(jcoppeard) → review+
Updated•10 years ago
|
Attachment #8534644 -
Flags: review?(jcoppeard) → review+
Comment 58•10 years ago
|
||
Comment on attachment 8534661 [details] [diff] [review] 19_remove_dumpheap-v0.diff Review of attachment 8534661 [details] [diff] [review]: ----------------------------------------------------------------- We have this as well as js::DumpHeapComplete()? Doesn't make sense to keep both.
Attachment #8534661 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 59•10 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #58) > Comment on attachment 8534661 [details] [diff] [review] > 19_remove_dumpheap-v0.diff > > Review of attachment 8534661 [details] [diff] [review]: > ----------------------------------------------------------------- > > We have this as well as js::DumpHeapComplete()? Doesn't make sense to keep > both. Well, they do different things. DumpHeapComplete is something Andrew added to, um, dump the heap. JS_DumpHeap is an :igor tool from ages ago that nobody other than :igor ever understood. I believe that in at least one of its many modes it can in fact actually dump parts of the heap (maybe even the full heap?), but its primary purpose seems to have been to find edges from any root to a live thing, acting much more like Jim's shell-implemented HeapReverser class.
Comment 60•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/caa4ffd2f765 https://hg.mozilla.org/mozilla-central/rev/7b57ab0cf44f https://hg.mozilla.org/mozilla-central/rev/10a8482ed924 https://hg.mozilla.org/mozilla-central/rev/ceaedf7b5c06 https://hg.mozilla.org/mozilla-central/rev/6f2826852f58 https://hg.mozilla.org/mozilla-central/rev/2efa3122ce92 https://hg.mozilla.org/mozilla-central/rev/aa2a54fffd77 https://hg.mozilla.org/mozilla-central/rev/ec983c96d034
Assignee | ||
Comment 61•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #57) > Comment on attachment 8534644 [details] [diff] [review] > 18_gettenuredgcthingzone_mergezone-v0.diff > > Review of attachment 8534644 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bindings/BindingUtils.h > @@ -765,5 @@ > > MaybeWrapStringValue(JSContext* cx, JS::MutableHandle<JS::Value> rval) > > { > > MOZ_ASSERT(rval.isString()); > > JSString* str = rval.toString(); > > - if (JS::GetTenuredGCThingZone(str) != js::GetContextZone(cx)) { > > I assume this new function is about as fast as the old one? Should be significantly faster, actually: the majority of the work appears to be checking if we're in the nursery and strings don't currently need that check.
Assignee | ||
Comment 62•10 years ago
|
||
I've always found it odd that we use masking for inline access to all GC structure fields except for ArenaHeader::zone. I don't think that manual masking is better than the shadow struct approach, but it's probably better than a hodgepodge of both. This would seem to be the path of least resistance to standardizing.
Attachment #8535215 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 63•10 years ago
|
||
And AsCell is effectively dead except for interfaces that really want to know if ObjectIsTenured, so let's just do that.
Attachment #8535310 -
Flags: review?(jcoppeard)
Attachment #8535310 -
Flags: review?(continuation)
Comment 64•10 years ago
|
||
Comment on attachment 8535310 [details] [diff] [review] 21_remove_ascell-v0.diff Review of attachment 8535310 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the CC change
Attachment #8535310 -
Flags: review?(continuation) → review+
Updated•10 years ago
|
Attachment #8535310 -
Flags: review?(jcoppeard) → review+
Updated•10 years ago
|
Attachment #8535215 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 65•10 years ago
|
||
Try is green https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9054c3077bd9 with fixes in: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=05e0388d9dce remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee3fa1e76c6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf0e4454f0d2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d67eb145b3c2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/68386a0e19d0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d90ce714c9 Up to part 17. Part 18 got a bit rotted by the zones fixes to the CC, so I'm going to push that down and wait on the type fixes.
Assignee | ||
Updated•10 years ago
|
Attachment #8533823 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8534031 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8534469 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8534597 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8534640 -
Flags: checkin+
Assignee | ||
Comment 66•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2750cb644aca remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/26a220fed616 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f960d42ac92a
Assignee | ||
Updated•10 years ago
|
Attachment #8534661 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8535215 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Attachment #8535310 -
Flags: checkin+
Comment 67•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0ee3fa1e76c6 https://hg.mozilla.org/mozilla-central/rev/bf0e4454f0d2 https://hg.mozilla.org/mozilla-central/rev/d67eb145b3c2 https://hg.mozilla.org/mozilla-central/rev/68386a0e19d0 https://hg.mozilla.org/mozilla-central/rev/c7d90ce714c9 https://hg.mozilla.org/mozilla-central/rev/2750cb644aca https://hg.mozilla.org/mozilla-central/rev/26a220fed616 https://hg.mozilla.org/mozilla-central/rev/f960d42ac92a
Assignee | ||
Comment 70•9 years ago
|
||
With bug 1199843 done, I think all of the major GC API pieces are complete. There may be some random odds and ends, but everything a typical embedding should need is done.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 71•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•