1168734 - Optimize asmjscache origin directory locking

Status: NEW

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

Description

[Jan Varga [:janv]]

Here's a partial patch from bug 1130775

@@ -948,34 +946,26 @@ MainProcessRunnable::Run()

       rv = InitOnMainThread();
       if (NS_FAILED(rv)) {
         Fail();
         return NS_OK;
       }

       mState = eWaitingToOpenMetadata;
-      rv = QuotaManager::Get()->WaitForOpenAllowed(
-                                     OriginOrPatternString::FromOrigin(mOrigin)
,
-                                     Nullable<PersistenceType>(mPersistence),
-                                     mStorageId, this);
-      if (NS_FAILED(rv)) {
-        Fail();
-        return NS_OK;
-      }
   
-      mNeedAllowNextSynchronizedOp = true;
-      return NS_OK;
-    }
+      // XXX The exclusive lock shouldn't be needed for read operations.
+      QuotaManager::Get()->OpenDirectory(mPersistence,
+                                         mGroup,
+                                         mOrigin,
+                                         mIsApp,
+                                         quota::Client::ASMJS,
+                                         /* aExclusive */ true,
+                                         this);
   
-    case eWaitingToOpenMetadata: {
-      MOZ_ASSERT(NS_IsMainThread());
-  
-      mState = eReadyToReadMetadata;
-      DispatchToIOThread();
       return NS_OK;
     }

     case eReadyToReadMetadata: {
       AssertIsOnIOThread();

       rv = ReadMetadata();
       if (NS_FAILED(rv)) {

and the relevant comment:

> ::: dom/asmjscache/AsmJSCache.cpp
> @@ +954,2 @@
> >  
> > +      // XXX The exclusive lock shouldn't be needed for read operations.
> 
> What's this about?

WaitForOpenAllowed() always gives you exclusive access, so if two pages try to load cached asm.js for the same origin, QM will allow only one at a time to do it.
However if you pass aExclusive=false to OpenDirectory(), those two pages will both get directory locks from QM, so they can load asm.js simultaneously in theory (the load will end up on the same IO thread currently, but that's a different issue).
That's why I think asmjscache could pass aExclusive=false for mOpenMode=eOpenForRead.
I didn't want to change it in this patch. I'll file a new bug for it.

Comment 1

[Luke Wagner [:luke]]

Ah, that sounds good.  So just to make sure I understand:
 - there is basically a per-origin RW lock and cache lookups take R and stores take W
 - anything that happens on the QM's thread will still be serialized by nature of execution on QM thread so, in particular, the metadata file writing (which happens even during reads) won't race.

Btw, on the subject of optimization, we're seeing (in a few other bugs) asm.js cache loads that end up taking a long time purely b/c they block on the main thread which is doing other expensive app-init stuff, so I think the biggest improvement we can make at this point is to remove all main-thread runnables.

Comment 2

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #1)
> Ah, that sounds good.  So just to make sure I understand:
>  - there is basically a per-origin RW lock and cache lookups take R and
> stores take W
>  - anything that happens on the QM's thread will still be serialized by
> nature of execution on QM thread so, in particular, the metadata file
> writing (which happens even during reads) won't race.

Oh, that's unfortunate that even reads need to update the metadata file.
I forgot that we "Move the mModuleIndex's LRU entry to the recent end of the queue."
In that case, we can't pass aExclusive=false

> 
> Btw, on the subject of optimization, we're seeing (in a few other bugs)
> asm.js cache loads that end up taking a long time purely b/c they block on
> the main thread which is doing other expensive app-init stuff, so I think
> the biggest improvement we can make at this point is to remove all
> main-thread runnables.

Yeah I understand, we should be closer to having QM on PBackground thread once bug 1130775 lands.

Comment 3

[Luke Wagner [:luke]]

(In reply to Jan Varga [:janv] from comment #2)
> Oh, that's unfortunate that even reads need to update the metadata file.
> I forgot that we "Move the mModuleIndex's LRU entry to the recent end of the
> queue."
> In that case, we can't pass aExclusive=false

OOC, why is that?

Comment 4

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #3)
> (In reply to Jan Varga [:janv] from comment #2)
> > Oh, that's unfortunate that even reads need to update the metadata file.
> > I forgot that we "Move the mModuleIndex's LRU entry to the recent end of the
> > queue."
> > In that case, we can't pass aExclusive=false
> 
> OOC, why is that?

because two or more metadata writes could happen at the same time ?
I know the IO thread would serialize it, but in future we might want to use a thread pool.

Comment 5

[Luke Wagner [:luke]]

(In reply to Jan Varga [:janv] from comment #4)
> I know the IO thread would serialize it, but in future we might want to use
> a thread pool.

Ah, ok, that was my question.

Comment 6

[Luke Wagner [:luke]]

One idea is to break the lookup into two phases: the exclusive metadata file update and the shared read.

Btw, an independent optimization I've thought of for a while is to keep a table of live compilations (keyed by the content chars) which would coordinate compilation such that if 5 workers all tried to compile the same chars at the same time, 1 would start and the others would block until the 1 finished, and then they'd receive the results.  The use case here is N workers all running the same (pthreads C++ using SharedArrayBuffer) asm.js code.  I'm also intending to allow asm.js code to be literally shared between threads so that the N workers share 1 copy of the code (which is around 40mb, so significant for a 20 thread engine on mobile).

Comment 7

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #6)
> One idea is to break the lookup into two phases: the exclusive metadata file
> update and the shared read.

that sounds good

> 
> Btw, an independent optimization I've thought of for a while is to keep a
> table of live compilations (keyed by the content chars) which would
> coordinate compilation such that if 5 workers all tried to compile the same
> chars at the same time, 1 would start and the others would block until the 1
> finished, and then they'd receive the results.  The use case here is N
> workers all running the same (pthreads C++ using SharedArrayBuffer) asm.js
> code.  I'm also intending to allow asm.js code to be literally shared
> between threads so that the N workers share 1 copy of the code (which is
> around 40mb, so significant for a 20 thread engine on mobile).

great idea!

Comment 8

[Mohammad Moazzam]

[Blocking Requested - why for this release]: