Closed Bug 1105069 Opened 5 years ago Closed 4 years ago

Make the GC API strongly typed

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: terrence, Assigned: terrence)

References

(Depends on 1 open bug, 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.
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)
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)
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)
The same situation as the prior change.
Attachment #8528708 - Flags: review?(jcoppeard)
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)
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)
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)
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.
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 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 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+
Attachment #8528704 - Flags: review?(jcoppeard) → review+
Attachment #8528708 - Flags: review?(jcoppeard) → review+
Attachment #8528710 - Flags: review?(jcoppeard) → review+
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 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 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 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-
This is a nice improvement over all!  Passing around void* everywhere for JS things has bothered me since I first started working on Firefox.
(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.
(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.
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 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 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+
Attachment #8528714 - Flags: review- → review?(continuation)
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+
(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!
Pushing the asCell down one more level.
Attachment #8531669 - Flags: review?(continuation)
Attachment #8531670 - Flags: review?(jcoppeard)
Attachment #8531670 - Flags: review?(continuation)
Attachment #8531669 - Flags: review?(continuation) → review+
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+
Attachment #8531670 - Flags: review?(jcoppeard) → review+
Attachment #8531708 - Flags: review?(jcoppeard)
Attachment #8531708 - Flags: review?(continuation)
Attachment #8531711 - Flags: review?(jcoppeard)
Attachment #8531711 - Flags: review?(continuation)
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 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 on attachment 8531708 [details] [diff] [review]
11_unmarkgraygcthingrecursively-v0.diff

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

Nice.
Attachment #8531708 - Flags: review?(jcoppeard) → review+
Attachment #8531711 - Flags: review?(jcoppeard) → review+
Keywords: leave-open
Attachment #8528693 - Flags: checkin+
Attachment #8528702 - Flags: checkin+
Attachment #8528704 - Flags: checkin+
Attachment #8528708 - Flags: checkin+
Attachment #8528710 - Flags: checkin+
Attachment #8528711 - Flags: checkin+
Attachment #8528714 - Flags: checkin+
Attachment #8531109 - Flags: checkin+
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)
Attached patch 14_xpcisgraygcthing-v0.diff (obsolete) — Splinter Review
Strongly type xpc_IsGrayGCThing.
Attachment #8533827 - Flags: review?(continuation)
Attachment #8533827 - Flags: review?(jcoppeard)
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-
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)
Attached patch 14_notejschild-v0.diff (obsolete) — Splinter Review
Attachment #8533939 - Flags: review?(jcoppeard)
Attachment #8533939 - Flags: review?(continuation)
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-
Attached patch 14_notejschild-v1.diff (obsolete) — Splinter Review
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 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-
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 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+
Attachment #8531669 - Flags: checkin+
Attachment #8531670 - Flags: checkin+
Attachment #8531708 - Flags: checkin+
Attachment #8531711 - Flags: checkin+
Attachment #8533823 - Flags: review?(jcoppeard) → review+
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+
Finally got a try run through the (apparent) gauntlet.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cc1c67bb0573
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)
Attachment #8534469 - Flags: review?(jcoppeard) → review+
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+
(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? ;-)
This rewrites UnmarkGrayChildren and TenuredCell::readBarrier in terms of our more convenient (and inlinable) internal interfaces.
Attachment #8534597 - Flags: review?(jcoppeard)
Attached patch 17_xpcisgraygcthing-v1.diff (obsolete) — Splinter Review
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 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 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+
(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+
Attachment #8534599 - Attachment is obsolete: true
Attachment #8534599 - Flags: review?(jcoppeard)
Attachment #8534640 - Flags: review?(jcoppeard)
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)
I don't see much point keeping it.
Attachment #8534661 - Flags: review?(jcoppeard)
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+
Attachment #8534597 - Flags: review?(jcoppeard) → review+
Attachment #8534640 - Flags: review?(jcoppeard) → review+
Attachment #8534644 - Flags: review?(jcoppeard) → review+
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+
(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.
(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.
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)
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 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+
Attachment #8535310 - Flags: review?(jcoppeard) → review+
Attachment #8535215 - Flags: review?(jcoppeard) → review+
Attachment #8533823 - Flags: checkin+
Attachment #8534031 - Flags: checkin+
Attachment #8534469 - Flags: checkin+
Attachment #8534597 - Flags: checkin+
Attachment #8534640 - Flags: checkin+
Attachment #8534661 - Flags: checkin+
Attachment #8535215 - Flags: checkin+
Attachment #8535310 - Flags: checkin+
Duplicate of this bug: 1012742
Duplicate of this bug: 820233
Depends on: 1199843
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: 4 years ago
Resolution: --- → FIXED
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.