Terrence Cole [:terrence] Assignee
	Description • 10 years ago

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.

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] Assignee
	Comment 4 • 10 years ago

The same situation as the prior change.

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] 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?

	Jon Coppeard (:jonco) (PTO until 14th September)
	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.

	Jon Coppeard (:jonco) (PTO until 14th September)
	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.

	Jon Coppeard (:jonco) (PTO until 14th September)
	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.

	Jon Coppeard (:jonco) (PTO until 14th September)
	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!

	Andrew McCreight [:mccr8]
	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!

	Andrew McCreight [:mccr8]
	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.

	Andrew McCreight [:mccr8]
	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.

	Terrence Cole [:terrence] 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.

	Andrew McCreight [:mccr8]
	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.

	Terrence Cole [:terrence] Assignee
	Comment 19 • 10 years ago

It's only a dozen or so sites inside and out. Well worth the extra safety.

	Jon Coppeard (:jonco) (PTO until 14th September)
	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.

	Andrew McCreight [:mccr8]
	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.

	Andrew McCreight [:mccr8]
	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?

	Terrence Cole [:terrence] 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!

	Terrence Cole [:terrence] Assignee
	Comment 24 • 10 years ago

Pushing the asCell down one more level.

	Andrew McCreight [:mccr8]
	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

	Andrew McCreight [:mccr8]
	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

	Andrew McCreight [:mccr8]
	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

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 31 • 10 years ago

Comment on attachment 8531708 [details] [diff] [review]
11_unmarkgraygcthingrecursively-v0.diff

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

Nice.

	Terrence Cole [:terrence] 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

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] Assignee
	Comment 34 • 10 years ago

Strongly type xpc_IsGrayGCThing.

	Andrew McCreight [:mccr8]
	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.

	Terrence Cole [:terrence] 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.

	Andrew McCreight [:mccr8]
	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)

	Terrence Cole [:terrence] 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.

	Andrew McCreight [:mccr8]
	Comment 40 • 10 years ago

Comment on attachment 8534003 [details] [diff] [review]
14_notejschild-v1.diff

r- this patch is empty. ;)

	Terrence Cole [:terrence] 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.

	Andrew McCreight [:mccr8]
	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.

	Terrence Cole [:terrence] 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

	Carsten Book [:Tomcat]
	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

	Jon Coppeard (:jonco) (PTO until 14th September)
	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!

	Terrence Cole [:terrence] 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

	Terrence Cole [:terrence] 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.

	Andrew McCreight [:mccr8]
	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.

	Terrence Cole [:terrence] 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? ;-)

	Terrence Cole [:terrence] Assignee
	Comment 50 • 10 years ago

This rewrites UnmarkGrayChildren and TenuredCell::readBarrier in terms of our more convenient (and inlinable) internal interfaces.

	Terrence Cole [:terrence] 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.

	Andrew McCreight [:mccr8]
	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.

	Andrew McCreight [:mccr8]
	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.

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] Assignee
	Comment 56 • 10 years ago

I don't see much point keeping it.

	Andrew McCreight [:mccr8]
	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?

	Jon Coppeard (:jonco) (PTO until 14th September)
	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.

	Terrence Cole [:terrence] 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.

	Ryan VanderMeulen [:RyanVM]
	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

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] 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.

	Andrew McCreight [:mccr8]
	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

	Terrence Cole [:terrence] 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.

	Terrence Cole [:terrence] 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

	Carsten Book [:Tomcat]
	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

	Terrence Cole [:terrence] 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.

	Firefox Bug Husbandry Bot
	Comment 71 • 6 years ago

Removing leave-open keyword from resolved bugs, per :sylvestre.