944821 - Support more than 1 cached asm.js module per origin

Assignee

Description

•

11 years ago

Bug 929236 effectively adds a per-origin size-1 cache.  However, this isn't good enough for sites that compile more than one asm.js module (BananaBread, flohofwoe.net, probably PlayCanvas soon).  It should be relatively simple to instead store a size-N LRU cache (initially, I'm thinking N = 16) per origin.  From the QuotaManager's POV, the whole LRU cache is still a single UsageInfo, so the whole thing will be evicted at once, which seems fine for now.

Luke Wagner [:luke]

Assignee

Comment 1

•

11 years ago

Attached patch WIP 1 (obsolete) — Details — Splinter Review

Here's the basic idea that appears to work.  Unfortunately, it can't be this easy because, with e10s, the parent process doesn't have access to the source chars in the child process.  I think I need to break the opening process into two messages: one to read the metadata, one to open the file.

Luke Wagner [:luke]

Assignee

Comment 2

•

11 years ago

Attached patch cache-multiple (obsolete) — Details — Splinter Review

Here's the full e10s version.  The hashing needs to happen in the child process and the metadata is opened/read in the parent, so I needed to add a roundtrip where the parent sends the metadata to the child and the child sends back which cache file to open.  This ended up fitting in fairly naturally to the existing runnable state machines.

I also moved all the asmjs mochitests into dom/asmjscache/test so that they'd start running on FFOS.

Attachment #8341798 - Attachment is obsolete: true

Attachment #8344055 - Flags: review?(Jan.Varga)

Luke Wagner [:luke]

Assignee

Comment 3

•

11 years ago

Looks green on try!

Luke Wagner [:luke]

Assignee

Comment 4

•

11 years ago

Attached patch cache-multiple (obsolete) — Details — Splinter Review

Rebased and tweaked to handle a buildid mismatch when writing cache entries correctly (it should overwrite, not fail).

Attachment #8344055 - Attachment is obsolete: true

Attachment #8344055 - Flags: review?(Jan.Varga)

Attachment #8355609 - Flags: review?(Jan.Varga)

Jan Varga [:janv]

Comment 5

•

11 years ago

ok, review in progress

Jan Varga [:janv]

Comment 6

•

11 years ago

Comment on attachment 8355609 [details] [diff] [review]
cache-multiple

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

I think PRIntn and PRInt32 shouldn't be used anymore.

Luke Wagner [:luke]

Assignee

Comment 7

•

11 years ago

Ah yes, I just saw that this morning.

Jan Varga [:janv]

Comment 8

•

11 years ago

Comment on attachment 8355609 [details] [diff] [review]
cache-multiple

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

Review comments for everything except AsmJSCache.cpp/.h

::: dom/asmjscache/AsmJSCache.cpp
@@ +50,4 @@
>  
>  namespace mozilla {
> +
> +MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedPRFileDesc, PRFileDesc, PR_Close);

Nit: Over the 80 char limit

::: dom/asmjscache/PAsmJSCacheEntry.ipdl
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  include protocol PContent;
>  
> +using mozilla::dom::asmjscache::Metadata from "mozilla/dom/asmjscache/AsmJSCache.h";

not sure how to align this nicely

::: dom/asmjscache/test/test_workers.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

this shouldn't be a new file, no ?

::: dom/ipc/ContentChild.cpp
@@ +863,5 @@
>  }
>  
>  asmjscache::PAsmJSCacheEntryChild*
>  ContentChild::AllocPAsmJSCacheEntryChild(const asmjscache::OpenMode& aOpenMode,
> +                                         const asmjscache::WriteParams& aWrite,

Nit: aWrite -> aWriteParams ?

::: dom/ipc/ContentChild.h
@@ +166,5 @@
>      virtual bool DeallocPFMRadioChild(PFMRadioChild* aActor);
>  
>      virtual PAsmJSCacheEntryChild* AllocPAsmJSCacheEntryChild(
>                                   const asmjscache::OpenMode& aOpenMode,
> +                                 const asmjscache::WriteParams& aWrite,

Nit: aWrite -> aWriteParams ?

::: dom/ipc/ContentParent.cpp
@@ +2479,5 @@
>  
>  asmjscache::PAsmJSCacheEntryParent*
>  ContentParent::AllocPAsmJSCacheEntryParent(
>                                            const asmjscache::OpenMode& aOpenMode,
> +                                          const asmjscache::WriteParams& aWrite,

Nit: aWrite -> aWriteParams ?

::: dom/ipc/ContentParent.h
@@ +380,5 @@
>      virtual bool DeallocPFMRadioParent(PFMRadioParent* aActor);
>  
>      virtual PAsmJSCacheEntryParent* AllocPAsmJSCacheEntryParent(
>                                   const asmjscache::OpenMode& aOpenMode,
> +                                 const asmjscache::WriteParams& aWrite,

Nit: aWrite -> aWriteParams ?

::: dom/ipc/PContent.ipdl
@@ +41,5 @@
>  using class IPC::Principal from "mozilla/dom/PermissionMessageUtils.h";
>  using struct mozilla::null_t from "ipc/IPCMessageUtils.h";
>  using struct mozilla::void_t from "ipc/IPCMessageUtils.h";
>  using mozilla::dom::asmjscache::OpenMode from "mozilla/dom/asmjscache/AsmJSCache.h";
> +using mozilla::dom::asmjscache::WriteParams from "mozilla/dom/asmjscache/AsmJSCache.h";

is it worth to align these two lines (80 chars) ?

@@ +372,5 @@
>      PBluetooth();
>  
>      PFMRadio();
>  
> +    PAsmJSCacheEntry(OpenMode openMode, WriteParams write, Principal principal);

Nit: write -> writeParams ?

::: js/xpconnect/tests/mochitest/test_asmjs3.html
@@ -1,4 @@
> -<!DOCTYPE HTML>
> -<html>
> -<!--
> -https://bugzilla.mozilla.org/show_bug.cgi?id=941830

Hmm, I guess this should be:
hg rename js/xpconnect/tests/mochitest/test_asmjs3.html dom/asmjscache/test/test_workers.html

Jan Varga [:janv]

Comment 9

•

11 years ago

Comment on attachment 8355609 [details] [diff] [review]
cache-multiple

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

Ok, it looks good so far, but I'll take one more look before r+

::: dom/asmjscache/AsmJSCache.cpp
@@ +124,5 @@
> +  bytesRead = PR_Read(fd, fileBuildId.begin(), length);
> +  NS_ENSURE_TRUE(bytesRead == PRInt32(length), NS_ERROR_UNEXPECTED);
> +
> +  for (uint32_t i = 0; i < length; i++) {
> +    NS_ENSURE_TRUE(currentBuildId[i] == fileBuildId[i], NS_ERROR_UNEXPECTED);

Nit: this will always happen after an upgrade, right ?
NS_ERROR_UNEXPECTED doesn't fit well here.

@@ +389,5 @@
>    // (through ref-counting or preconditions) that aPrincipal is kept alive for
>    // the lifetime of the MainProcessRunnable.
>    MainProcessRunnable(nsIPrincipal* aPrincipal,
>                        OpenMode aOpenMode,
> +                      WriteParams aWrite)

Nit: aWrite -> aWriteParams/mWrite -> mWriteParams here and elsewhere ?

@@ +613,4 @@
>      }
>  
> +    // Initialize Metadata with a valid empty state for the LRU cache.
> +    for (unsigned i = 0; i < Metadata::kNumEntries; i++) {

What about:
Metadata::Entry& entry = mMetadata.mEntries[i];
entry.mFastHash = -1;
...

@@ +667,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Move the mModuleIndex's LRU entry to the recent end of the queue.
> +  PodMove(mMetadata.mEntries + 1, mMetadata.mEntries, Metadata::kLastEntry);
> +  mMetadata.mEntries[0].mFastHash = mWrite.mFastHash;

Metadata::Entry& entry = mMetadata.mEntries[0];
entry.mFastHash = mWriteParams.mFastHash;
...

@@ +700,5 @@
> +  // Move the mModuleIndex's LRU entry to the recent end of the queue.
> +  unsigned lruIndex = 0;
> +  while (mMetadata.mEntries[lruIndex].mModuleIndex != mModuleIndex) {
> +    if (++lruIndex == Metadata::kNumEntries) {
> +      return NS_ERROR_UNEXPECTED;

Does this mean the per origin asm.js cache is full ?
If that's the case NS_ERROR_UNEXPECTED doesn't fit here either.

@@ +871,5 @@
> +  // hash allows us to avoid performing up to Metadata::kNumEntries separate
> +  // full hashes.
> +  uint32_t numChars = aRead.mLimit - aRead.mBegin;
> +  MOZ_ASSERT(numChars > sNumFastHashChars);
> +  uint32_t fastHash = HashString(aRead.mBegin, sNumFastHashChars);

What will happen if the implementation of HashString() changes again (giving different output for the same input) ?
We had to add own implementation of HashString() to prevent problems in future, bug 940315

@@ +913,5 @@
>    // the main thread.
>    SingleProcessRunnable(nsIPrincipal* aPrincipal,
>                          OpenMode aOpenMode,
> +                        WriteParams aWrite,
> +                        ReadParams aRead)

Hmm, aWrite/aRead just doesn't look good to me. Why did you want to avoid aWriteParams/aReadParams ?

@@ +1662,5 @@
> +
> +void
> +ParamTraits<Metadata>::Write(Message* aMsg, const paramType& aParam)
> +{
> +  for (unsigned i = 0; i < Metadata::kNumEntries; i++) {

again, maybe use a ref to aParam.mEntries[i]

@@ +1674,5 @@
> +bool
> +ParamTraits<Metadata>::Read(const Message* aMsg, void** aIter,
> +                            paramType* aResult)
> +{
> +  for (unsigned i = 0; i < Metadata::kNumEntries; i++) {

dtto

@@ +1689,5 @@
> +
> +void
> +ParamTraits<Metadata>::Log(const paramType& aParam, std::wstring* aLog)
> +{
> +  for (unsigned i = 0; i < Metadata::kNumEntries; i++) {

dtto

::: dom/asmjscache/AsmJSCache.h
@@ +68,5 @@
> +    mFullHash(0)
> +  { }
> +};
> +
> +// Parameters specific to opening a cache entry for writing

for *reading* ?

Luke Wagner [:luke]

Assignee

Comment 10

•

11 years ago

(In reply to Jan Varga [:janv] from comment #8)
Thanks!  New patch in a bit after I re-build and test.

> > +using mozilla::dom::asmjscache::Metadata from "mozilla/dom/asmjscache/AsmJSCache.h";
> 
> not sure how to align this nicely

There seems to be a precedent for letting these lines go past 80:
http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/ipc/PIndexedDBCursor.ipdl#12

> ::: dom/asmjscache/test/test_workers.html
> @@ +1,1 @@
> > +<!DOCTYPE HTML>
> 
> this shouldn't be a new file, no ?

For some reason, hg keeps converting the 'hg rename' into an 'hg rm; hg add'.  I'm guessing it's rebase.  I'll re-do the 'hg rename' right before landing.

(In reply to Jan Varga [:janv] from comment #9)
> > +  unsigned lruIndex = 0;
> > +  while (mMetadata.mEntries[lruIndex].mModuleIndex != mModuleIndex) {
> > +    if (++lruIndex == Metadata::kNumEntries) {
> > +      return NS_ERROR_UNEXPECTED;
> 
> Does this mean the per origin asm.js cache is full ?

It means that the child process specified a moduleIndex that doesn't exist in the metadata.  As the child chose the moduleIndex from the metadata, this indicates some sort of error in the child process.  This should never happen in practice.

> What will happen if the implementation of HashString() changes again (giving
> different output for the same input) ?

The hash matches are conservative: false negatives means we won't get a match when we should, so we'll compile the asm.js module and store a new cache entry with a new hash; fale positive means we'll attempt to open the file when we shouldn't, but this will fail in the JS engine when the source chars (stored in the cache entry) aren't exactly equal.  In any case, if HashString changes, buildid will change, so this whole scenario shouldn't happen.  Good question to ask, though!

> Hmm, aWrite/aRead just doesn't look good to me. Why did you want to avoid
> aWriteParams/aReadParams ?

To avoid wrapping a few lines ;)  I'll re-add the *Params in the next patch.

Luke Wagner [:luke]

Assignee

Comment 11

•

11 years ago

Attached patch cache-multiple v.2 — Details — Splinter Review

Attachment #8355609 - Attachment is obsolete: true

Attachment #8355609 - Flags: review?(Jan.Varga)

Attachment #8356623 - Flags: review?(Jan.Varga)

Jan Varga [:janv]

Comment 12

•

11 years ago

Comment on attachment 8356623 [details] [diff] [review]
cache-multiple v.2

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

Ok, r=me with GetQuotaObject() fixed

::: dom/asmjscache/AsmJSCache.cpp
@@ +592,5 @@
> +  // Create the QuotaObject before all file IO to get maximum assertion coverage
> +  // in QuotaManager against concurrent removal, etc.
> +  mQuotaObject = qm->GetQuotaObject(quota::PERSISTENCE_TYPE_TEMPORARY,
> +                                    mGroup, mOrigin, mDirectory);
> +  NS_ENSURE_STATE(mQuotaObject);

Hm, getting a quota object for a directory ?

The basic rule is to get (and addref) a quota object before a file is opened and then release it when the file is closed. Before writing to the file, call MaybeAllocateMoreSpace(). If you want to be precise, do it for the metadata file too.

::: dom/ipc/ContentChild.cpp
@@ +863,5 @@
>  }
>  
>  asmjscache::PAsmJSCacheEntryChild*
>  ContentChild::AllocPAsmJSCacheEntryChild(const asmjscache::OpenMode& aOpenMode,
> +                                         const asmjscache::WriteParams& aWrite,

Nit: aWrite -> aWriteParams

::: dom/ipc/ContentChild.h
@@ +166,5 @@
>      virtual bool DeallocPFMRadioChild(PFMRadioChild* aActor);
>  
>      virtual PAsmJSCacheEntryChild* AllocPAsmJSCacheEntryChild(
>                                   const asmjscache::OpenMode& aOpenMode,
> +                                 const asmjscache::WriteParams& aWrite,

Nit: aWrite -> aWriteParams

::: dom/ipc/ContentParent.cpp
@@ +2506,5 @@
>  
>  asmjscache::PAsmJSCacheEntryParent*
>  ContentParent::AllocPAsmJSCacheEntryParent(
>                                            const asmjscache::OpenMode& aOpenMode,
> +                                          const asmjscache::WriteParams& aWrite,

Nit: aWrite -> aWriteParams

::: dom/ipc/ContentParent.h
@@ +383,5 @@
>      virtual bool DeallocPFMRadioParent(PFMRadioParent* aActor);
>  
>      virtual PAsmJSCacheEntryParent* AllocPAsmJSCacheEntryParent(
>                                   const asmjscache::OpenMode& aOpenMode,
> +                                 const asmjscache::WriteParams& aWrite,

Nit: aWrite -> aWriteParams

::: dom/ipc/PContent.ipdl
@@ +372,5 @@
>      PBluetooth();
>  
>      PFMRadio();
>  
> +    PAsmJSCacheEntry(OpenMode openMode, WriteParams write, Principal principal);

Nit: write -> writeParams

Attachment #8356623 - Flags: review?(Jan.Varga) → review+

Luke Wagner [:luke]

Assignee

Comment 13

•

11 years ago

(In reply to Jan Varga [:janv] from comment #12)
> Hm, getting a quota object for a directory ?

Fixed as discussed on IRC: the quota object is created for the cache file, not metadata file, right before opening the cache file to read/write.

> Nit: aWrite -> aWriteParams

Oops, I definitely made this changes locally, I'm not sure how they got lost...

Luke Wagner [:luke]

Assignee

Comment 14

•

11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5163bf6b21b2

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 15

•

11 years ago

https://hg.mozilla.org/mozilla-central/rev/5163bf6b21b2

Status: ASSIGNED → RESOLVED

Closed: 11 years ago

Resolution: --- → FIXED

Target Milestone: --- → mozilla29

Gregor Wagner [:gwagner]

Updated

•

11 years ago

Depends on: 957921

Nobody; OK to take it and work on it

Updated

•

5 years ago

Component: DOM → DOM: Core & HTML

WIP 1 11 years ago Luke Wagner [:luke] 16.25 KB, patch		Details \| Diff \| Splinter Review
cache-multiple 11 years ago Luke Wagner [:luke] 60.45 KB, patch		Details \| Diff \| Splinter Review
cache-multiple 11 years ago Luke Wagner [:luke] 65.25 KB, patch		Details \| Diff \| Splinter Review
cache-multiple v.2 11 years ago Luke Wagner [:luke] 61.08 KB, patch	janv : review+	Details \| Diff \| Splinter Review

Bugzilla

Quick Search

Support more than 1 cached asm.js module per origin

Categories

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

Tracking

()

People

(Reporter: luke, Assigned: luke)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file, 3 obsolete files)

Description

Comment 1

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Comment 14

Comment 15

Updated

Updated

Attachment

General

Description

File Name

Content Type