1173756 - Update asmjscache to use PBackground

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Jan Varga [:janv]]

Attachment: patch 1

Luke, there will be another patch/patches for merging MainProcessRunnable with ParentProcessRunnable, etc.

This patch is based on directory locks which should land soon.

Comment 2

[Luke Wagner [:luke]]

Could you explain to me at a high level what runnable lives where in the single and multi-process configurations?

Comment 3

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #2)
> Could you explain to me at a high level what runnable lives where in the
> single and multi-process configurations?

There's no distinction between single and multi-process anymore, IPC is used for both.
The patch uses ChildProcessRunnable for the child side of communication and ParentProcessRunnable for the parent side. I'll rename the runnables in separate patches.
The final code should be much simpler than it is now.

If you are interested I can send you a patch for directory locks so you can apply it all and measure performance etc.

Comment 4

[Luke Wagner [:luke]]

So when you say "IPC is used for both", do you mean that, in the non-e10s case, ChildProcessRunnable is in the main/parser thread and ParentProcessRunnable is on a PBackground thread?  In the e10s case, is the ChildProcessRunnable *only* in the main/parser thread of the child process and the MainProcessRunnable *only* in the PBackground thread of the parent process?

From a perf perspective, my only question is whether this patch introduces any new roundtrips through the main thread (though I assume this or the final patch actually remove many roundtrips).

Comment 5

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #4)
> So when you say "IPC is used for both", do you mean that, in the non-e10s
> case, ChildProcessRunnable is in the main/parser thread and
> ParentProcessRunnable is on a PBackground thread?  In the e10s case, is the
> ChildProcessRunnable *only* in the main/parser thread of the child process
> and the MainProcessRunnable *only* in the PBackground thread of the parent
> process?

Yes, there are always just two runnables/actors.

> 
> From a perf perspective, my only question is whether this patch introduces
> any new roundtrips through the main thread (though I assume this or the
> final patch actually remove many roundtrips).

On the child side, SendPAsmJSCacheEntryConstructor() can't be called directly if PBackgroundDirectly hasn't been created yet. So that sometimes requires an extra hop.
On the parent side, there are two NS_DispatchToMainThread() added, but six removed. The real flow depends on type of requests (read/write/cache miss), but overall there are slightly less main thread roundtrips.
Real win should be visible with quota manager on PBackground.

I want to do this separately to make it easier to review, etc.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8620931 [details] [diff] [review]
patch 1

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

Looks great, then.

Comment 7

[Jan Varga [:janv]]

Attachment: Patch 1 - move stuff to PBackground

Comment 8

[Jan Varga [:janv]]

Attachment: Patch 2 - merge MainProcessRunnable and ParentProcessRunnable

Comment 9

[Jan Varga [:janv]]

Attachment: Patch 3 - merge File and ChildRunnable

Comment 10

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fd1c498b179

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8652285 [details] [diff] [review]
Patch 1 - move stuff to PBackground

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

::: dom/asmjscache/AsmJSCache.cpp
@@ +560,5 @@
>      MOZ_ASSERT(mOpenMode == eOpenForRead);
>  
>      mModuleIndex = aModuleIndex;
> +    mState = eDispatchToMainThread;
> +    NS_DispatchToMainThread(this);

Is the reason for dispatching to the main thread only to dispatch to the IO thread to avoid some race with calling QuotaManager::Get() and shutdown?  If that's the case, should DispatchToIOThread() assert it's on the main thread?  An alternate design could be to have a static method QuotaManager::Get(Guard*) where the given Guard both held the returning QM* and a lock that was also grabbed when attempting to null out QuotaManager::gInstance.

@@ +708,5 @@
>      eReadyToReadMetadata, // Waiting to read the metadata file on the IO thread
>      eFailedToReadMetadata, // Waiting to be dispatched to main thread after fail
>      eSendingMetadataForRead, // Waiting to send OnOpenMetadataForRead
>      eWaitingToOpenCacheFileForRead, // Waiting to hear back from child
> +    eDispatchToMainThread,

Could you comment this state?

@@ +1020,5 @@
>  
>        rv = ReadMetadata();
>        if (NS_FAILED(rv)) {
>          mState = eFailedToReadMetadata;
> +        MOZ_ALWAYS_TRUE(NS_SUCCEEDED(

Are both really needed?

@@ +1578,5 @@
> +    // On failure, undo the AddRef (since DeallocEntryChild will not be
> +    // called) and unblock the parsing thread with a failure. The main
> +    // thread event loop still holds an outstanding ref which will keep
> +    // 'this' alive until returning to the event loop.
> +    Release();

Preexisting, but I wonder if this Release() is necessary or whether the AddRef() could just be moved to below this send.

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8652286 [details] [diff] [review]
Patch 2 - merge MainProcessRunnable and ParentProcessRunnable

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

::: dom/asmjscache/AsmJSCache.cpp
@@ +596,5 @@
> +    mState = eFinished;
> +
> +    MOZ_ASSERT(mOpened);
> +
> +    mFinished = true;

mFinished was necessary when it was in a separate runnable, but now it seems *almost* redundant with (mState == eFinished).  The only exception seems to be Recv__delete__() which sets mFinished=true but then sets mState=eFailing and dispatches to the owner thread to set mState=eFinished.  However, since Recv__delete__() is on the owner thread *already*, it could synchronously fail (sharing code with the eFailing case) so that mFinished == (mState == eFinished).

@@ +835,5 @@
>    rv = QuotaManager::GetInfoFromPrincipal(principal, &mGroup, &mOrigin,
>                                            &mIsApp);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // XXX Could we just use the default storage?

I think no: currently, persistent storage is only set when a script is "precompiled" via explicit FFOS manifest option for a select set of .js files:
  http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSSubScriptLoader.cpp#810
  https://developer.mozilla.org/en-US/Apps/Build/Manifest#precompile
If instead we use default storage, then all asm.js for all files (or runtime compilation) will go into persistent storage if the app's default storage happens to be persistent.  This could waste several hundred MB in the worst case (esp. if we relax the 16-module cap on number of cached modules per origin).  Really, I think the direction we're going with the asm.js baseline compiler bug 1169167 will avoid the cache-or-die situation so we can completely remove this whole business and *always* use temporary storage.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8652287 [details] [diff] [review]
Patch 3 - merge File and ChildRunnable

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

::: dom/asmjscache/AsmJSCache.cpp
@@ -330,5 @@
>    void* mMappedMemory;
>  };
>  
> -// File is a base class derived by ChildRunnable that presents a single
> -// interface to the AsmJSCache ops which need to wait until the file is open.

ISTR mention of "File" in the FileDescriptorHolder class's comment that should be removed now.

@@ +1242,5 @@
> +      return mResult;
> +    }
> +
> +    // Now that we're open, we're guarnateed a Close() call. However, we are
> +    // not guarnateed someone is holding an outstanding reference until the File

Preexisting nit: guaranteed x 2

@@ +1368,5 @@
> +  JS::AsmJSCacheResult mResult;
> +
> +  bool mActorDestroyed;
> +  bool mWaiting;
> +  bool mOpened;

It seems like mWaiting/mOpened could instead be defined in terms of mState since these variables are set right before calls which set the state.

Comment 14

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #11)
> Comment on attachment 8652285 [details] [diff] [review]
> Patch 1 - move stuff to PBackground
> 
> Review of attachment 8652285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/asmjscache/AsmJSCache.cpp
> @@ +560,5 @@
> >      MOZ_ASSERT(mOpenMode == eOpenForRead);
> >  
> >      mModuleIndex = aModuleIndex;
> > +    mState = eDispatchToMainThread;
> > +    NS_DispatchToMainThread(this);
> 
> Is the reason for dispatching to the main thread only to dispatch to the IO
> thread to avoid some race with calling QuotaManager::Get() and shutdown?  If
> that's the case, should DispatchToIOThread() assert it's on the main thread?
> An alternate design could be to have a static method
> QuotaManager::Get(Guard*) where the given Guard both held the returning QM*
> and a lock that was also grabbed when attempting to null out
> QuotaManager::gInstance.

The IO thread is actually a LazyIdleThread and that class allows to dispatch
runnables from the owning thread only. The owning thread is the main thread
in this case (QM creates LazyIdleThread on the main thread).
This extra hop will go away with QM on PBackground.

> 
> @@ +708,5 @@
> >      eReadyToReadMetadata, // Waiting to read the metadata file on the IO thread
> >      eFailedToReadMetadata, // Waiting to be dispatched to main thread after fail
> >      eSendingMetadataForRead, // Waiting to send OnOpenMetadataForRead
> >      eWaitingToOpenCacheFileForRead, // Waiting to hear back from child
> > +    eDispatchToMainThread,
> 
> Could you comment this state?

Sure, fixed other comments too.

> 
> @@ +1020,5 @@
> >  
> >        rv = ReadMetadata();
> >        if (NS_FAILED(rv)) {
> >          mState = eFailedToReadMetadata;
> > +        MOZ_ALWAYS_TRUE(NS_SUCCEEDED(
> 
> Are both really needed?

Well, this is the style bent and I started using elsewhere.

> 
> @@ +1578,5 @@
> > +    // On failure, undo the AddRef (since DeallocEntryChild will not be
> > +    // called) and unblock the parsing thread with a failure. The main
> > +    // thread event loop still holds an outstanding ref which will keep
> > +    // 'this' alive until returning to the event loop.
> > +    Release();
> 
> Preexisting, but I wonder if this Release() is necessary or whether the
> AddRef() could just be moved to below this send.

Done, tests pass locally.

Comment 15

[Jan Varga [:janv]]

Attachment: Interdiff for Patch 1

Comment 16

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #12)
> Comment on attachment 8652286 [details] [diff] [review]
> Patch 2 - merge MainProcessRunnable and ParentProcessRunnable
> 
> Review of attachment 8652286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/asmjscache/AsmJSCache.cpp
> @@ +596,5 @@
> > +    mState = eFinished;
> > +
> > +    MOZ_ASSERT(mOpened);
> > +
> > +    mFinished = true;
> 
> mFinished was necessary when it was in a separate runnable, but now it seems
> *almost* redundant with (mState == eFinished).  The only exception seems to
> be Recv__delete__() which sets mFinished=true but then sets mState=eFailing
> and dispatches to the owner thread to set mState=eFinished.  However, since
> Recv__delete__() is on the owner thread *already*, it could synchronously
> fail (sharing code with the eFailing case) so that mFinished == (mState ==
> eFinished).

So do you suggest to remove mFinished but keep some kind of a check in
ActorDestroy() ?

> 
> @@ +835,5 @@
> >    rv = QuotaManager::GetInfoFromPrincipal(principal, &mGroup, &mOrigin,
> >                                            &mIsApp);
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +  // XXX Could we just use the default storage?
> 
> I think no: currently, persistent storage is only set when a script is
> "precompiled" via explicit FFOS manifest option for a select set of .js
> files:
>  
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/
> mozJSSubScriptLoader.cpp#810
>   https://developer.mozilla.org/en-US/Apps/Build/Manifest#precompile
> If instead we use default storage, then all asm.js for all files (or runtime
> compilation) will go into persistent storage if the app's default storage
> happens to be persistent.  This could waste several hundred MB in the worst
> case (esp. if we relax the 16-module cap on number of cached modules per
> origin).  Really, I think the direction we're going with the asm.js baseline
> compiler bug 1169167 will avoid the cache-or-die situation so we can
> completely remove this whole business and *always* use temporary storage.

Great!

Comment 17

[Luke Wagner [:luke]]

(In reply to Jan Varga [:janv] from comment #16)
> So do you suggest to remove mFinished but keep some kind of a check in
> ActorDestroy() ?

Right; the need for the check remains, just the independent state can go away (I think).

Comment 18

[Jan Varga [:janv]]

Ok, I now see what you meant by "it could synchronously fail". I just need to avoid the dispatch to the owning/background thread if we are on it already.

Comment 19

[Luke Wagner [:luke]]

Exactly, ideally factoring out the code called by the eClosing case and this new synchronous call (and probably adding an assert in Recv__delete__() that mState=eFinished.

Comment 20

[Jan Varga [:janv]]

Attachment: Interdiff for Patch 2

Comment 21

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #13)
> Comment on attachment 8652287 [details] [diff] [review]
> Patch 3 - merge File and ChildRunnable
> 
> Review of attachment 8652287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/asmjscache/AsmJSCache.cpp
> @@ -330,5 @@
> >    void* mMappedMemory;
> >  };
> >  
> > -// File is a base class derived by ChildRunnable that presents a single
> > -// interface to the AsmJSCache ops which need to wait until the file is open.
> 
> ISTR mention of "File" in the FileDescriptorHolder class's comment that
> should be removed now.

Fixed

> 
> @@ +1242,5 @@
> > +      return mResult;
> > +    }
> > +
> > +    // Now that we're open, we're guarnateed a Close() call. However, we are
> > +    // not guarnateed someone is holding an outstanding reference until the File
> 
> Preexisting nit: guaranteed x 2

Fixed

> 
> @@ +1368,5 @@
> > +  JS::AsmJSCacheResult mResult;
> > +
> > +  bool mActorDestroyed;
> > +  bool mWaiting;
> > +  bool mOpened;
> 
> It seems like mWaiting/mOpened could instead be defined in terms of mState
> since these variables are set right before calls which set the state.

Hm, do you mean something like this:

 MutexAutoLock lock(mMutex);
-  while (mWaiting)) {
     mCondVar.Wait();
   }
 }

 MutexAutoLock lock(mMutex);
+  while (!(mState == eOpened || mState == eFinished)) {
     mCondVar.Wait();
   }
 }

Well, for that all reads and writes of mState would have to be protected by the mutex, no ?

mOpened seems to be tricky too.

Comment 22

[Jan Varga [:janv]]

Attachment: Interdiff for Patch 3

Comment 23

[Luke Wagner [:luke]]

(In reply to Jan Varga [:janv] from comment #21)
> Well, for that all reads and writes of mState would have to be protected by
> the mutex, no ?

Ah, of course.  Perhaps then we could just assert in Notify() (next to the MOZ_ASSERT(mWaiting)) that mState is eOpened or eFinished.

> mOpened seems to be tricky too.

I believe the invariant holds 'mOpened == (mResult == AsmJSCache_Sucess)`.  Changing the `if (!mWaiting)` to `if (mResult != AsmJSCache_Success)' in BlockUntilOpen actually reads a little nicer when you read the code below it.  Anyhow, I realize this is a purely pre-existing nit, so you can ignore if you want, just nice to reduce state space.

Comment 24

[Luke Wagner [:luke]]

Comment on attachment 8653297 [details] [diff] [review]
Interdiff for Patch 2

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

Nice!

::: AsmJSCache.cpp-old
@@ -693,4 @@
>        return;
>      }
>  
> -    mFinished = true;

Can you MOZ_ASSERT(mState == eFinished) below the if/else after this?

Comment 25

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #24)
> Comment on attachment 8653297 [details] [diff] [review]
> Interdiff for Patch 2
> 
> Review of attachment 8653297 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!
> 
> ::: AsmJSCache.cpp-old
> @@ -693,4 @@
> >        return;
> >      }
> >  
> > -    mFinished = true;
> 
> Can you MOZ_ASSERT(mState == eFinished) below the if/else after this?

Sure and thanks for the reviews!

Comment 26

[Luke Wagner [:luke]]

My pleasure, and nice work.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/154ab5d65f79
https://hg.mozilla.org/integration/mozilla-inbound/rev/23d5ef182400
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1422d84e9d

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/154ab5d65f79
https://hg.mozilla.org/mozilla-central/rev/23d5ef182400
https://hg.mozilla.org/mozilla-central/rev/1f1422d84e9d