Closed Bug 963588 Opened 6 years ago Closed 6 years ago

asmjscache: place cache entries for app:// origins in persistent storage

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(1 file, 4 obsolete files)

Once we have app installation/upgrade messages [1], then webapps can do asm.js compilation and caching at install and upgrade time so that launching the app always gets a cache hit.  Unfortunately, the cache entries are in temporary storage and thus subject to LRU eviction, so this won't always be true.  To fix this, I'd like to store the asm.js cache entries for app:// origins in persistent storage.

[1] https://groups.google.com/forum/#!msg/mozilla.dev.webapi/aD79UxdBqxI/7iXKO6VvSWYJ
Jan: if the user doesn't explicitly clear the origin, are there any reasons why a persistent origin's storage would be cleared?  I see there is a (newUsage > mOriginInfo->mLimit) test in MaybeAllocateMoreSpace; what is the computed limit for a normal (unprivileged packaged) app?
Attached patch persistent-storage WIP (obsolete) — Splinter Review
This is the basic idea.  What's missing is of course the "is this an app://" judgement in OriginPersistenceType.

However, there is another problem: if I pass 'true' for aTrackQuota to EnsureOriginIsInitialized, then it fails on, e.g., unrealengine.com/html5, on a second visit when the app has stored >50MB (the user clicked "Allow").  It seems aTrackQuota is meant to track persistent storage up to the first 50MB, after which the user gets free reign?  If I pass 'false' to aTrackQuota, then, later, GetQuotaObject fails because the groupInfo->LockedGetOriginInfo(aOrigin) fails (presumably b/c I didn't create one earlier).  I could just not create a QuotaObject for persistent storage, but I assume that isn't intended.

Any guidance on this Jan?
I'm packing for the work week, I'll take a look at this during the travel.
Anyway, I think Jonas proposed to add a third type of storage, but I forgot details.
There's one thing, the planned asm.js caching on the main thread can only work if the storage is promptless, since for prompting we need to dispatch a runnable to the main thread.
Flags: needinfo?(jonas)
(In reply to Jan Varga [:janv] from comment #3)
On the subject of the prompt, it seems like we could forego it by restricting permanent storage to only scripts that were real scripts (came from a file, not an eval string, not a Blob-via-object-URL).  In this case, the space consumed by the cached code would be bounded and proportional to the size of the saved source, so it seems like we'd be well-justified in skipping the prompt.  This would also be good for the theoretical app that (1) had some big asm.js scripts that it wanted cache, (2) dynamically generated bunches of asm.js (like the PyPy asm.js backend does ;) that would otherwise fill up the cache and evice the important scripts.  (1) would go in persistent, (2) would go in temporary.

Perhaps this third storage class is would fit this use case?  Are there other use cases you are considering for the third storage class?
Attached patch persistent-storage (obsolete) — Splinter Review
This patch implements what we talked about at the work week.  To control when a script gets persistent storage, the JS engine adds a CompileOptions flag that lets the caller indicate "I'm compiling an installed JS file" (bug 965970 will be the only place that sets this to 'true'; in this patch it is never set).  This conservative approach should guarantee that the persistent storage consumed by a packaged app is bounded (at least for asm.js code).  However, when we run an app, we don't know whether it is installed (it might be dynamic code), so we need to search both storage/persistent and storage/temporary.  This ended up being pretty easy: on cache miss, I just restart the MainProcessRunnable state machine switching persistent to temporary.  (Bug 965970 will add tests for packaged app installation + persistent caching.)
Attachment #8365159 - Attachment is obsolete: true
Attachment #8372500 - Flags: review?(Jan.Varga)
Attached patch persistent-storage rebased (obsolete) — Splinter Review
Rebased over bug 972652.
Attachment #8372500 - Attachment is obsolete: true
Attachment #8372500 - Flags: review?(Jan.Varga)
Attachment #8382398 - Flags: review?(Jan.Varga)
(review ping)
It seems we're going to have another (similar to asmjscache) storage client for byte-code caching, so I'm now looking at this from that perspective too. It seems that it would be better to add a dedicated (internal) storage for caching and asmjscache would use only that. I'll post more details in my next comment, I might also ping you on IRC.
Comment on attachment 8382398 [details] [diff] [review]
persistent-storage rebased

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

::: dom/quota/QuotaObject.h
@@ +44,3 @@
>    bool
> +  MaybeAllocateMoreSpace(int64_t aOffset, int32_t aCount,
> +                         bool aSkipPersistentLimitCheck = false);

If we allow to skip the 50MB limit prompt then next EnsureOriginIsInitialized() will fail because of this check in InitializeOrigin():
if (totalUsageBytes > quotaMaxBytes) {
  NS_WARNING("Origin is already using more storage than allowed!");
  return NS_ERROR_UNEXPECTED;
}

This approach seems a bit fragile to me. In B2G, if the app has the "storage" permission then it also gets "indexedDB-unlimited" (we haven't changed the name to something generic), so the prompt would never show in that case. However if the app doesn't have the "storage" permission then the prompt always return DENY. Someone correct me if I'm wrong.
So if you want to compile asm.js only for apps with "storage" permission then you don't need the extra argument for MaybeAllocateMoreSpace(). But I guess you don't want to depend on the permission.
Also, if we allow storing cached asm.js in persistent storage for apps that can store only 50MB then we actually provide less space which can look strange to app developers.
Another thing is that we will never try to delete stuff from persistent storage if we get a system notification to delete unimportant data.
All in all, a third storage type intended only for caching and not exposed to apps needs to be created. It would behave like temp storage, maybe w/o the group limit.
Comment on attachment 8382398 [details] [diff] [review]
persistent-storage rebased

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

::: dom/asmjscache/AsmJSCache.cpp
@@ +902,5 @@
>        mState = eWaitingToOpenMetadata;
>        rv = QuotaManager::Get()->WaitForOpenAllowed(
>                                       OriginOrPatternString::FromOrigin(mOrigin),
> +                                     Nullable<PersistenceType>(mPersistence),
> +                                     mStorageId, this);

hmm, I missed this in the original patch for asm.js caching, we don't need to block both (persistent and temporary) storages
We talked on IRC, for now we can just make the "storage" permission (which translates to indexedDB-unlimited) to be required for install time asm.js caching. So we don't have to add hacks to MaybeAllocateMoreSpace().
Attachment #8382398 - Flags: review?(Jan.Varga)
Attached patch persistent-storage (obsolete) — Splinter Review
Attachment #8382398 - Attachment is obsolete: true
Attachment #8386328 - Flags: review?(Jan.Varga)
Comment on attachment 8386328 [details] [diff] [review]
persistent-storage

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

It's amazing how this all evolved, nice job!
Please attach one more patch, thanks.

::: dom/asmjscache/AsmJSCache.cpp
@@ +632,5 @@
>    bool mNeedAllowNextSynchronizedOp;
>    nsCString mGroup;
>    nsCString mOrigin;
>    nsCString mStorageId;
> +  quota::PersistenceType mPersistence;

Nit: I would put this before mGroup

@@ +695,5 @@
> +                                           &permission);
> +      NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
> +
> +      bool unlimitedStorage = permission == nsIPermissionManager::ALLOW_ACTION;
> +      MOZ_ASSERT_IF(unlimitedStorage, isApp);

Should we actually assert isApp if mWriteParams.mInstalled ?

@@ +696,5 @@
> +      NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
> +
> +      bool unlimitedStorage = permission == nsIPermissionManager::ALLOW_ACTION;
> +      MOZ_ASSERT_IF(unlimitedStorage, isApp);
> +

Nit: maybe put a comment that even when the app doesn't have the unlimited storage permission, we precompile to temp storage

@@ +704,5 @@
> +      mPersistence = quota::PERSISTENCE_TYPE_TEMPORARY;
> +    }
> +  } else {
> +    // For the reasons described above, apps may have cache entries in both
> +    // persistent and temporary storage. At lookup time we don't how and where

Nit: missing "know" ?

@@ +814,1 @@
>    NS_ENSURE_STATE(mQuotaObject);

This needs to be changed. GetQuotaObject() should always return a quota object if the persistence type is temporary, but in the case of persistent storage, null indicates that we don't track quota for given origin so we don't need and can't call MaybeAllocateMoreSpace().
Try to change |NS_ENSURE_STATE(mQuotaObject);| to |MOZ_ASSERT_IF(mPersistence == quota::PERSISTENCE_TYPE_TEMPORARY, mQuotaObject)|

You should also check other code, maybe we assume that mQuotaObject must always be not null elsewhere. OpenCacheForForRead() ?

@@ +816,5 @@
> +  // If we are allocating in temporary storage, ask the QuotaManager if we're
> +  // within the quota. If we are allocating in persistent storage, we've already
> +  // checked that we have the unlimited-storage permission, so there is nothing
> +  // to check.
> +  if (mPersistence == quota::PERSISTENCE_TYPE_TEMPORARY) {

This check should be changed to |if (mQuotaObject)| and maybe combined with the following one ?

@@ +1739,5 @@
>               UsageInfo* aUsageInfo) MOZ_OVERRIDE
>    {
> +    if (!aUsageInfo) {
> +      return NS_OK;
> +    }

Nit: Could you please add a todo comment to GetUsageForOrigin(), right before the "while" loop about checking aUsageInfo->Canceled() ?
Check out indexedDB::Client::GetUsageForDirectoryInternal() how it is done there.

Somebody might use asmjscache as a template for a new storage client :)
Attachment #8386328 - Flags: review?(Jan.Varga)
Clearing the need-info request, since we have decided to not add a third type of storage for now.
Flags: needinfo?(jonas)
Great find, thanks!
Attachment #8386328 - Attachment is obsolete: true
Attachment #8386883 - Flags: review?(Jan.Varga)
Attachment #8386883 - Flags: review?(Jan.Varga) → review+
https://hg.mozilla.org/mozilla-central/rev/a0c4fa6338dc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.