885866 - Rip the deferred finalization mechanism out of XPConnect so it can be used off the main thread.

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We want this for CC on workers.  Should be pretty straightforward.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Tagging mccr8 for the cycle collection and XPConnect bits.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 766063 [details] [diff] [review]
Patch

Tagging peterv for the bindings/codegen bits.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 766063 [details] [diff] [review]
Patch

And bsmedberg for the nsHashKeys.h changes.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Do we need to stuff this into nsContentUtils?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I put it there because that's where the JS holder stuff is.  We can move it somewhere else in a followup if you like.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch V2

Now in a version that works off the main thread.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 766372 [details] [diff] [review]
Patch V2

peterv for binding/codegen again.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 766372 [details] [diff] [review]
Patch V2

bsmedberg for nsHashKeys.h

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 766372 [details] [diff] [review]
Patch V2

And jorendorff for the jsapi changes.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 766372 [details] [diff] [review]
Patch V2

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

Man, this is hairy.

::: content/xbl/src/nsXBLBinding.cpp
@@ +76,1 @@
>    

Please remove this end of line whitespace while you are here.

::: js/xpconnect/idl/nsIJSRuntimeService.idl
@@ +22,1 @@
>  interface nsIJSRuntimeService : nsISupports

This is a pretty odd interface.  I wonder if we can deCOMtaminate or eliminate it.  I filed bug 886459 for that.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +598,1 @@
>      switch (status) {

Just change this switch to an |if (status == JSGC_BEGIN)|.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +1361,5 @@
>  bool
>  DeferredRelease(nsISupports *obj)
>  {
> +    nsContentUtils::DeferredFinalize(obj);
> +    return true;

Nobody is calling DeferredRelease any more, so just remove it, please.

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +91,5 @@
> +
> +  static const PRTime SliceMillis = 10; /* ms */
> +
> +  static PLDHashOperator
> +  DeferredFinalizerEnumerator(DeferredFinalizeFunction& aFunction, 

trailing whitespace

@@ +101,5 @@
> +                              nsTArray<nsISupports*>& mSupports,
> +                              DeferredFinalizerTable& aFinalizerTable);
> +  virtual ~IncrementalFinalizeRunnable();
> +
> +  void ReleaseNow(bool limited);

aLimited

@@ +1063,5 @@
> +  return items->IsEmpty();
> +}
> +
> +/* static */ PLDHashOperator
> +IncrementalFinalizeRunnable::DeferredFinalizerEnumerator(DeferredFinalizeFunction& aFunction, 

trailing whitespace

@@ +1073,5 @@
> +  DeferredFinalizeFunctionHolder* function = array->AppendElement();
> +  function->run = aFunction;
> +  function->data = aData;
> +
> +  return PL_DHASH_REMOVE;

Maybe it makes more sense to not remove anything, and then just clear the entire hashtable when you are done?  I guess you could also do EnumerateRead, then.

@@ +1079,5 @@
> +
> +IncrementalFinalizeRunnable::IncrementalFinalizeRunnable(CycleCollectedJSRuntime* aRt,
> +                                                         nsTArray<nsISupports*>& aSupports,
> +                                                         DeferredFinalizerTable& aFinalizers)
> +: mRuntime(aRt),

I think the initializers should be indented.

@@ +1081,5 @@
> +                                                         nsTArray<nsISupports*>& aSupports,
> +                                                         DeferredFinalizerTable& aFinalizers)
> +: mRuntime(aRt),
> +  mFinalizeFunctionToRun(0)
> +{

Why don't you need the nsLayoutStatics::AddRef() any more?

@@ +1097,5 @@
> +  MOZ_ASSERT(this != mRuntime->mFinalizeRunnable);
> +}
> +
> +void
> +IncrementalFinalizeRunnable::ReleaseNow(bool limited)

aLimited

@@ +1099,5 @@
> +
> +void
> +IncrementalFinalizeRunnable::ReleaseNow(bool limited)
> +{
> +  //MOZ_ASSERT(NS_IsMainThread());

Is there some kind of IsMainThreadOrWorkerThread you can use here? Failing that, just remove the commented out assert.  It would be useful to have a OnRuntimeThread() method in CycleCollectedJSRuntime to use for things like this...

@@ +1164,5 @@
> +  return NS_OK;
> +}
> +
> +void
> +CycleCollectedJSRuntime::FinalizeDeferredThings(DeferredFinalizeType aType)

Is there some reason this is a separate method from OnGC?  It also isn't really clear why you need to pass in aType rather than just test WasIncrementalGC() here.

@@ +1186,5 @@
> +
> +void
> +CycleCollectedJSRuntime::OnGC(JSGCStatus aStatus)
> +{
> +  switch (aStatus) {

Just change this to an |if (aStatus == JSGC_END)|, with maybe an assert that aStatus is either _BEGIN or _END if you want.  The callback stuff is well-structured enough it doesn't really seem like it would matter to check in a release build.

::: xpcom/base/nsCycleCollector.h
@@ +18,3 @@
>  class CycleCollectedJSRuntime;
> +
> +// Called back from DeferredFinalize.  Should add 'thing' to the array of smart

This documentation should probably be in nsContentUtils.h, as that looks like the main interface to these functions. Then in this file, put a reference to the comment in nsContentUtils.

@@ +18,4 @@
>  class CycleCollectedJSRuntime;
> +
> +// Called back from DeferredFinalize.  Should add 'thing' to the array of smart
> +// pointers in 'pointers', creating the array if it does not already exist, and

Maybe say "if 'pointers' is null" rather than "if it does not already exist"?

@@ +90,5 @@
>  #ifdef DEBUG
>  bool TestJSHolder(void* aHolder);
>  #endif
>  
> +void DeferredFinalize(DeferredFinalizeAppendFunction aAppendFunc,

Kind of weird to have this in the cycle collector namespace, because it doesn't really have anything to do with cycle collection per se, but oh well.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 766372 [details] [diff] [review]
Patch V2

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

r=me on the JSAPI use and changes.

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 766372 [details] [diff] [review]
Patch V2

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

::: content/base/public/nsContentUtils.h
@@ +1272,5 @@
>  
> +  static void DeferredFinalize(nsISupports* aSupports);
> +  static void DeferredFinalize(mozilla::DeferredFinalizeAppendFunction aAppendFunc,
> +                               mozilla::DeferredFinalizeFunction aFunc,
> +                               void* aThing);

It doesn't make a lot of sense to me to have these in nsContentUtils, just calling the CC functions. I think it would make more sense to just only make this part of the CC API since that's effectively what you're doing. Unless the CC peers want it out of that :-).

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +1032,5 @@
> +  bool hadThingArray = mDeferredFinalizerTable.Get(aFunc, &thingArray);
> +
> +  thingArray = aAppendFunc(thingArray, aThing);
> +  if (!hadThingArray) {
> +    mDeferredFinalizerTable.Put(aFunc, thingArray);

Another way I think would work is to PutEntry and then set mData to the result of calling aAppendFunc. That only requires one lookup. Not sure it's worth it.

::: xpcom/base/CycleCollectedJSRuntime.h
@@ +206,5 @@
>    JSRuntime* mJSRuntime;
>  
>    nsDataHashtable<nsPtrHashKey<void>, nsScriptObjectTracer*> mJSHolders;
>  
> +  nsTArray<nsISupports*> mDeferredSupports;

I wonder if you could make this nsTArray<nsCOMPtr<nsISupports> >, make DeferredFinalize take an already_AddRefed and pass in a dont_AddRef :-).

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85a124822b7e