Bug 900669 added general asm.js caching support to the JS engine, but it only works in the shell at the moment. To work in the browser, Gecko needs to implement the AsmJSCacheOps in jsapi.h. After talking with Jan and Ben, the rough plan is: - implement the asm.js cache as a QuotaManager client so that we get all the quota management, LRU eviction, UI controls, etc for freeish - caching will be done per-origin, to avoid any cross-origin security issues - cache files will be stored in a new 'asmjs' subdir of the current 'storage' hierarchy, i.e., $(PROFILE)/storage/$(STORAGE_TYPE)/$(ORIGIN)/asmjs - normal web content will use the 'temporary' QuotaManager storage type; packaged apps will use 'persistent' - cache entries embed build-id and cpu information so that upgrades and weird profile-sharing will lazily invalidate old cache entries - the initial landing will only support a single-entry cache per origin which should be good enough for the current one-big-asm.js-module games we're working with. The trickier part is working properly with e10s and sandboxed content processes: - the basic scheme (which we've run by Guillaume) is to have the parent process open the file and send the file descriptor across IPDL to the child process which, even with seccomp filtering, should be able to read/write to this file directly. - the other tricky bit is synchronously calling from the child into the parent to do synchronous file IO in the parent w/o blocking the parent's main thread. Current best guess is just having the child send an async message to the parent and waiting on an async reply from the parent. Alternatives discussed in bug 925128.

Here's a basic WIP with a lot of TODOs that is able to load/cache unrealengine.com/html5 (see "loaded from cache" in Web Console message). This demonstrates the basic interaction with the QuotaManager and all the exciting runnable hopping between threads. I tried to make this look like other code in indexedDB as much as possible. I'd be happy to get any early feedback on style or approach.

Comment on attachment 820034 [details] [diff] [review] WIP 1 Review of attachment 820034 [details] [diff] [review]: ----------------------------------------------------------------- Just throwing some comments at you already :) ::: dom/dom-config.mk @@ +9,5 @@ > dom/file \ > dom/power \ > dom/push \ > dom/quota \ > + dom/jscache \ No need for this, your one header is exported. ::: dom/jscache/AsmJSCache.cpp @@ +3,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "AsmJSCache.h" Include as mozilla/dom/AsmJSCache.h @@ +21,5 @@ > +#include "nsThreadUtils.h" > +#include "nsXULAppAPI.h" > +#include "prio.h" > + > +#include "jsfriendapi.h" Batch all the includes after stdio.h together and sort them lexicographically @@ +26,5 @@ > + > +USING_JSCACHE_NAMESPACE > +USING_QUOTA_NAMESPACE > +using namespace mozilla; > +using namespace mozilla::dom; Wrap in namespace mozilla { namespace dom { ... } // namespace dom } // namespace mozilla @@ +50,5 @@ > + void* mMemory; > + // nsIOfflineStorage > + bool mSyncOnClose; > + > + OpenedCacheFile(OpenCacheMode aMode) Should be explicit @@ +52,5 @@ > + bool mSyncOnClose; > + > + OpenedCacheFile(OpenCacheMode aMode) > + : mMode(aMode), > + mFileSize(0), , to the front @@ +71,5 @@ > + mFileMap = nullptr; > + } > + if (mFileDesc) { > + if (mSyncOnClose) > + PR_Sync(mFileDesc); {} for all single-line ifs @@ +87,5 @@ > + OpenedCacheFile** aOpenedCacheFile) > +{ > + AssertIsOnIOThread(); > + > + nsresult rv; Declare when you need it @@ +124,5 @@ > + > + ScopedDeletePtr<OpenedCacheFile> openedCacheFile(new OpenedCacheFile(aMode)); > + > + openedCacheFile->mQuotaObject > + = quotaManager->GetQuotaObject(quota::PERSISTENCE_TYPE_TEMPORARY, = on the previous line @@ +157,5 @@ > + 0, // offset from begin > + openedCacheFile->mFileSize); > + NS_ENSURE_TRUE(openedCacheFile->mMemory, NS_ERROR_UNEXPECTED); > + > + *aOpenedCacheFile = openedCacheFile.forget(); Document the ownership transfer, please @@ +206,5 @@ > + }; > + EnsureOriginState mState; > + > + mozilla::Mutex mMutex; > + mozilla::CondVar mCondVar; No need for the mozilla:: @@ +352,5 @@ > +bool > +OpenCacheFile(JS::HandleObject aGlobal, > + OpenCacheMode aMode, > + size_t aSizeToWrite, > + OpenedCacheFile **aOpenedCacheFile) ** to the left @@ +443,5 @@ > + return true; > +} > + > +void > +CloseAsmJSCacheEntryForWrite(JS::HandleObject global, Don't use the typedefs outside js/ ::: dom/jscache/AsmJSCache.h @@ +3,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_dom_jscache_asmjs_h__ mozilla_dom_AsmJSCache_h @@ +17,5 @@ > + namespace mozilla { namespace dom { namespace jscache { > +#define END_JSCACHE_NAMESPACE \ > + } /* namespace jscache */ } /* namespace dom */ } /* namespace mozilla */ > +#define USING_JSCACHE_NAMESPACE \ > + using namespace mozilla::dom::jscache; Drop all this, and put your code in mozilla::dom directly ::: dom/jscache/Makefile.in @@ +4,5 @@ > + > +include $(topsrcdir)/dom/dom-config.mk > + > +LOCAL_INCLUDES = \ > + $(NULL) Don't do this @@ +7,5 @@ > +LOCAL_INCLUDES = \ > + $(NULL) > + > +include $(topsrcdir)/config/rules.mk > +include $(topsrcdir)/ipc/chromium/chromium-config.mk Check if you actually need chromium-config.mk ::: dom/jscache/moz.build @@ +5,5 @@ > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +MODULE = 'dom' > + > +EXPORTS.mozilla.dom.jscache += [ Just export as mozilla/dom/AsmJSCache.h

(In reply to :Ms2ger from comment #2) Many of these stylistic nits conflict with the style prevalent in dom/indexedDB and dom/quota and don't appear to be overruled by the Gecko style guide; I'd prefer to hear Ben/Jan's opinion.

As far as I'm concerned anything that isn't in our style guide is fair game for new code. Others can fight it out in the newsgroups. Here are my thoughts: > No need for this, your one header is exported. Until one day someone adds another non-exported header? I don't really care but this doesn't seem like a problem. > Include as mozilla/dom/AsmJSCache.h For cpp files in the same directory I think this is totally fine. Exported headers are a separate matter. > Batch all the includes after stdio.h together and sort them lexicographically I'm a fan of alphabetical ordering, just make sure your own header is included first. I tend to split mine into blocks (first interfaces, then external headers, finally my own module's headers) but that approach has caused headaches for other contributors so I don't really care in new code. > Wrap in > > namespace mozilla { > namespace dom { > ... > } // namespace dom > } // namespace mozilla I've never liked this but others seem to love it. Whatever. > Should be explicit This makes sense. > , to the front If our style guide doesn't say anything about this then I would call it a "6 vs. half-dozen" thing. Your style (same as mine) reads prettier but tends to complicate hg blame. I think readability wins, but whatever. > {} for all single-line ifs This for sure. > Declare when you need it I personally agree, but there are decent arguments that 'rv' is special. > = on the previous line Agreed. > No need for the mozilla:: If you can avoid it, sure. > ** to the left Yeah. Non-XPConnect gecko usually uses this. > Don't use the typedefs outside js/ I personally prefer to avoid typedefs when I can but last I heard we had not come to any real conclusion about whether or not we would enforce this in non-JS code. We can always change things later, so meh. > mozilla_dom_AsmJSCache_h Yeah, I would have written it this way too. However, it's an include guard and gecko is wildly inconsistent in this respect so as long as it is unique and unlikely to collide in the future I don't care too much. > Drop all this, and put your code in mozilla::dom directly You're following my example here (which I obviously like!) but others have disagreed very strongly. I'm not sure if we have any rules about new code here, but we can easily change this kind of thing later if we need to... > > +LOCAL_INCLUDES = \ > > + $(NULL) > > Don't do this Yeah, this isn't useful. > Check if you actually need chromium-config.mk If you can avoid it please do! > Just export as mozilla/dom/AsmJSCache.h If you move it to the dom namespace, yeah. Otherwise no.

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #4) Thanks for taking the time to go down the list Ben. > > Don't use the typedefs outside js/ > > I personally prefer to avoid typedefs when I can but last I heard we had not > come to any real conclusion about whether or not we would enforce this in > non-JS code. We can always change things later, so meh. To wit, the exclusive use of the Handle/Rooted typedefs has been proposed as a Gecko-wide style on m.d.platform with several supporting comments and no dissenting: https://groups.google.com/forum/#!topic/mozilla.dev.platform/DtN9842JqRo

Well the main thing is consistency. Either change all the use of Handle/Rooted to use typedefs or keep using non-typedefs. We have so much non-typedef use already that changing the style gradually doesn't quite work. Someone should run a script or so to change the style everywhere. (I don't care too much whether we use typedefs or non-typedefs, though I don't really understand what good the typedefs give us.)

We need to do a global switch to the typedefed RootedFoo things ... until that happens we've been trying to maintain consistency and use Rooted<Foo> in dom code.

This is almost done, just need some mochitests, a bit more feedback in the console message, and a size threshold below which we don't attempt caching. I tested, and this works well on Epic Citadel, BananaBread, and various other games in progress on desktop and on mobile (well, the ones that run on mobile w/o OOMing).

To add to the previous comment: this version only caches in single-process Firefox, only for async scripts, with a single cache per origin, and not for workers. I think we should land this and get feedback while I work on filling in the other missing pieces in separate bugs. This patch has two dependencies: bug 932398 for a portable msync and bug 928050 to remove a potential deadlock where the main thread blocks on the parse thread. I'll start with r?janv and bent feel free to jump in if you'd like.

This patch refines the previous one by not attempting to lookup/store in the cache for small modules (of length < 10,000 chars) since these compile real fast anyway. The patch also adds a notification to the asm.js console warning to indicate when the compiled asm.js module was stored in the cache.

Small touchup: I realized that of of the runnable states was no longer necessary.

This patch minimizes the churn in the coming-e10s patch by splitting OpenCacheFileRunnable into a base that is used by child and parent runnables.

Sorry for the quick turn-around, but I just found a nice simplification for handling the always-AllowNextSynchronizedOp problem. This patch removes a chunk of code and simplifies the e10s patch. I won't obsolete the old one so you don't lose any review comments you had pending (a great frustration of mine).

Oops, the previous refactoring inadvertently moved the AllowNextSynchronizedOp call to after the entry was opened, when it needs to happen after it is closed (to protect the cache file while it is being written). In the meantime, I got e10s support working in this patch. I still need to refactor this all a bit better, so I'll just post the WIP and cancel review for now.

Alright, this version fixes previous bugs and is significantly simpler than the previous patches. Green on try, tested on single- and multi-process desktop firefox on the big asm.js demos, so this should be landable once the dependent bugs are fixed.

Rebased

Comment on attachment 831879 [details] [diff] [review] browser-caching Review of attachment 831879 [details] [diff] [review]: ----------------------------------------------------------------- Looks good so far, I still need to look at AsmJSCache.cpp/.h ::: dom/asmJSCache/Makefile.in @@ +1,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this file, > +# You can obtain one at http://mozilla.org/MPL/2.0/. > + > +include $(topsrcdir)/config/rules.mk hm, is this makefile needed ? ::: dom/indexedDB/ipc/IndexedDBChild.cpp @@ +25,5 @@ > USING_INDEXEDDB_NAMESPACE > > using namespace mozilla::dom; > using mozilla::dom::quota::QuotaManager; > +using mozilla::dom::quota::Client; Nit: put it before |using mozilla::dom::quota::QuotaManager;| ::: dom/ipc/ContentChild.cpp @@ +850,5 @@ > return true; > } > > +PAsmJSCacheEntryChild* > +ContentChild::AllocPAsmJSCacheEntryChild(const asmJSCache::OpenMode &aOpenMode, Nit: & after the type (not before the argument name) @@ +851,5 @@ > } > > +PAsmJSCacheEntryChild* > +ContentChild::AllocPAsmJSCacheEntryChild(const asmJSCache::OpenMode &aOpenMode, > + const int64_t &aSizeToWrite, dtto, and also in the .h file ::: dom/ipc/ContentChild.h @@ +166,5 @@ > > + virtual PAsmJSCacheEntryChild* AllocPAsmJSCacheEntryChild( > + const asmJSCache::OpenMode &aOpenMode, > + const int64_t &aSizeToWrite, > + const IPC::Principal& aPrincipal) MOZ_OVERRIDE; Nit: we usually align arguments to the right (80 chars line) in this case @@ +168,5 @@ > + const asmJSCache::OpenMode &aOpenMode, > + const int64_t &aSizeToWrite, > + const IPC::Principal& aPrincipal) MOZ_OVERRIDE; > + virtual bool DeallocPAsmJSCacheEntryChild( > + PAsmJSCacheEntryChild* aActor) MOZ_OVERRIDE; dtto ::: dom/ipc/ContentParent.h @@ +377,5 @@ > + const asmJSCache::OpenMode& aOpenMode, > + const int64_t& aSizeToWrite, > + const IPC::Principal& aPrincipal) MOZ_OVERRIDE; > + virtual bool DeallocPAsmJSCacheEntryParent( > + PAsmJSCacheEntryParent* aActor) MOZ_OVERRIDE; Nit: align to the right as in ContentChild.h

I tried to build with all the patches (dependent bugs + browser-caching) and I got this error: /Users/varga/Sources/Mozilla2/src/dom/base/nsJSEnvironment.cpp:2829:5: error: cannot initialize a member subobject of type 'BuildIdOp' (aka 'bool (*)(js::Vector<char> *)') with an lvalue of type 'bool (mozilla::Vector<char> *)': type mismatch at 1st parameter ('js::Vector<char> *' vs 'mozilla::Vector<char> *') asmJSCache::GetBuildId

Heh, yeah, that just changed on trunk.

Comment on attachment 831879 [details] [diff] [review] browser-caching Review of attachment 831879 [details] [diff] [review]: ----------------------------------------------------------------- Ok, overall this looks pretty good. I convinced myself that it's better to keep everything in just one .h/.cpp pair. Originally I wanted to suggest splitting Client and Actor implementationto into separate files. I have to take a look at the runnables one more time, posting what I have... ::: dom/asmJSCache/AsmJSCache.cpp @@ +140,5 @@ > + explicit AutoClose(File* aFile = nullptr) > + : mFile(aFile) > + { } > + > + void Init(File* aFile) Nit: return type on own line here and below @@ +182,5 @@ > + { } > + > + ~File() > + { > + MOZ_ASSERT(!mWaiting, "File shouldn't be destroyed while compilation thread is blocking"); Nit: 80 char limit @@ +277,5 @@ > + > + virtual ~MainProcessRunnable() > + { > + MOZ_ASSERT(mState == eClosed); > + MOZ_ASSERT(!mId, "Should have been released on the main thread on success/failure"); Nit: 80 char limit @@ +340,5 @@ > + > + enum State { > + eInitial, // Just created, waiting to be dispatched to main thread > + eWaitingToOpen, // Waiting to be called back from WaitForOpenAllowed > + eReadyToOpen, // Waiting to be dispatched to the IO thread to do all the work Nit: 80 char limit @@ +343,5 @@ > + eWaitingToOpen, // Waiting to be called back from WaitForOpenAllowed > + eReadyToOpen, // Waiting to be dispatched to the IO thread to do all the work > + eNotifying, // Waiting to be dispatched to main thread to notify of success > + eOpened, // Finished calling OnOpen from main thread, waiting to be closed > + eClosing, // Waiting to be dispatched to main thread to AllowNextSynchronizedOp etc Nit: 80 char limit @@ +354,5 @@ > +nsresult > +MainProcessRunnable::InitOnMainThread() > +{ > + MOZ_ASSERT(mState == eInitial); > + MOZ_ASSERT(NS_IsMainThread()); Nit: we usually put this first @@ +362,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + mId = QuotaManager::GetStorageId(quota::PERSISTENCE_TYPE_TEMPORARY, > + mOrigin, quota::Client::ASMJS, > + NS_LITERAL_STRING(ASMJSCACHE_MODULE_FILE_NAME)); Nit: 80 char limit @@ +363,5 @@ > + > + mId = QuotaManager::GetStorageId(quota::PERSISTENCE_TYPE_TEMPORARY, > + mOrigin, quota::Client::ASMJS, > + NS_LITERAL_STRING(ASMJSCACHE_MODULE_FILE_NAME)); > + NS_ENSURE_TRUE(mId, NS_ERROR_OUT_OF_MEMORY); Just NS_ERROR_FAILURE ? @@ +366,5 @@ > + NS_LITERAL_STRING(ASMJSCACHE_MODULE_FILE_NAME)); > + NS_ENSURE_TRUE(mId, NS_ERROR_OUT_OF_MEMORY); > + > + QuotaManager* qm = QuotaManager::GetOrCreate(); > + NS_ENSURE_TRUE(qm, NS_ERROR_OUT_OF_MEMORY); It can fail due to something else, just |NS_ENSURE_STATE(qm)| ? Also, I know those two static QuotaManager methods work without initializing QuotaManager, but wouldn't it be more logical to call GetOrCreate() before them ? @@ +420,5 @@ > + > + openFlags = PR_RDONLY | nsIFile::OS_READAHEAD; > + } else { > + nsRefPtr<QuotaObject> quotaObject = > + qm->GetQuotaObject(quota::PERSISTENCE_TYPE_TEMPORARY, mGroup, mOrigin, path); Nit: 80 char limit @@ +476,5 @@ > + > + mState = eWaitingToOpen; > + rv = QuotaManager::Get()->WaitForOpenAllowed( > + OriginOrPatternString::FromOrigin(mOrigin), > + Nullable<PersistenceType>(), mId, this); Nit: these can be move one char to the right :) @@ +649,5 @@ > + > +private: > + ~ParentProcessRunnable() > + { > + MOZ_ASSERT(!mPrincipalHolder, "Should have been released on the main thread on success/failure"); Nit: 80 char limit @@ +672,5 @@ > + OnOpen() MOZ_OVERRIDE > + { > + MOZ_ASSERT(NS_IsMainThread()); > + > + FileDescriptor::PlatformHandleType handle(PR_FileDesc2NativeHandle(mFileDesc)); Nit: 80 char limit @@ +713,5 @@ > +{ > + ParentProcessRunnable* runnable = > + new ParentProcessRunnable(aPrincipal, aOpenMode, aSizeToWrite); > + > + // AddRef to keep the runnable alive until ReleaseEntryParent. Nit: ReleaseEntryParent -> DeallocEntryParent @@ +855,5 @@ > + AddRef(); > + > + if (!ContentChild::GetSingleton()->SendPAsmJSCacheEntryConstructor( > + this, mOpenMode, mSizeToWrite, > + IPC::Principal(mPrincipal))) { Nit: hmm, I think you don't need two lines for these arguments @@ +856,5 @@ > + > + if (!ContentChild::GetSingleton()->SendPAsmJSCacheEntryConstructor( > + this, mOpenMode, mSizeToWrite, > + IPC::Principal(mPrincipal))) { > + // On failure, undo the AddRef (since DeallocEntryChild will not be called) Nit: 80 char limit @@ +1051,5 @@ > + > +bool > +GetBuildId(Vector<char>* aBuildID) > +{ > + nsCOMPtr<nsIXULAppInfo> appInfo = do_GetService("@mozilla.org/xre/app-info;1"); Nit: 80 char limit @@ +1092,5 @@ > + rv = directory->Append(NS_LITERAL_STRING(ASMJSCACHE_DIRECTORY_NAME)); > + NS_ENSURE_SUCCESS(rv, rv); > + > +#ifdef DEBUG > + bool exists; I think, you can use |DebugOnly<bool> exists;| and remove the |#ifdef DEBUG| stuff @@ +1160,5 @@ > + > + rv = AddUsage(directory, aUsageInfo); > + NS_ENSURE_SUCCESS(rv, rv); > + > + return NS_OK; InitOrigin() can just call/delegate to GetUsageForOrigin(), then you can also "inline" GetDirectory() and GetUsage() impl into GetUsageForOrigin() since there would be only one caller. @@ +1232,5 @@ > + NS_DECL_OWNINGTHREAD > +}; > + > +NS_IMPL_ADDREF(Client) > +NS_IMPL_RELEASE(Client) I think these need to be fully qualified since there are multiple Client objects just in different namespaces and it confuses the leak analyzer. ::: dom/asmJSCache/AsmJSCache.h @@ +8,5 @@ > +#define mozilla_dom_asmjscache_asmjscache_h > + > +#include "ipc/IPCMessageUtils.h" > +#include "js/TypeDecls.h" > +#include "mozilla/Vector.h" This is not needed anymore @@ +9,5 @@ > + > +#include "ipc/IPCMessageUtils.h" > +#include "js/TypeDecls.h" > +#include "mozilla/Vector.h" > +#include "mozilla/dom/PAsmJSCacheEntryChild.h" I think you can just forward declare PAsmJSCacheEntryChild here and move the include to the cpp file @@ +10,5 @@ > +#include "ipc/IPCMessageUtils.h" > +#include "js/TypeDecls.h" > +#include "mozilla/Vector.h" > +#include "mozilla/dom/PAsmJSCacheEntryChild.h" > +#include "mozilla/dom/PAsmJSCacheEntryParent.h" dtto @@ +11,5 @@ > +#include "js/TypeDecls.h" > +#include "mozilla/Vector.h" > +#include "mozilla/dom/PAsmJSCacheEntryChild.h" > +#include "mozilla/dom/PAsmJSCacheEntryParent.h" > +#include "mozilla/dom/quota/Client.h" dtto @@ +12,5 @@ > +#include "mozilla/Vector.h" > +#include "mozilla/dom/PAsmJSCacheEntryChild.h" > +#include "mozilla/dom/PAsmJSCacheEntryParent.h" > +#include "mozilla/dom/quota/Client.h" > +#include "nsIPrincipal.h" dtto @@ +18,5 @@ > +#define ASMJSCACHE_MODULE_FILE_NAME "module" > + > +namespace mozilla { > +namespace dom { > +namespace asmJSCache { Nit: I would use "asmjscache" as the directory name and C++ namespace @@ +27,5 @@ > + eOpenForWrite, > + NUM_OPEN_MODES > +}; > + > +// Implementation of AsmJSCacheOps: Nit: I would also mention nsJSEnvironment.cpp here @@ +31,5 @@ > +// Implementation of AsmJSCacheOps: > + > +bool > +OpenEntryForRead(JS::HandleObject aGlobal, size_t* aSize, > + const uint8_t** aMemory, intptr_t *aHandle); hm, it seems we still use JS::Handle<JSObject*> in dom module @@ +42,5 @@ > +void > +CloseEntryForWrite(JS::HandleObject aGlobal, size_t aSize, > + uint8_t* aMemory, intptr_t aHandle); > +bool > +GetBuildId(mozilla::Vector<char> *buildId); Nit: "*" after the type @@ +49,5 @@ > + > +quota::Client* > +CreateClient(); > + > +// Called from ipc/ContentParent Nit: Called from ContentParent.cpp: @@ +53,5 @@ > +// Called from ipc/ContentParent > + > +PAsmJSCacheEntryParent* > +AllocEntryParent(OpenMode aOpenMode, > + uint32_t aSizeToWrite, Nit: |uint32_t aSizeToWrite,| fits after |aOpenMode| @@ +59,5 @@ > + > +void > +DeallocEntryParent(PAsmJSCacheEntryParent* aActor); > + > +// Called from ipc/ContentChild Nit: Called from ContentChild.cpp: ::: dom/asmJSCache/PAsmJSCacheEntry.ipdl @@ +4,5 @@ > + > +include protocol PContent; > + > +namespace mozilla { > +namespace dom { shouldn't this live in mozilla::dom::asmJSCache ? @@ +11,5 @@ > +{ > + manager PContent; > + > +child: > + OnOpen(int64_t aFileSize, FileDescriptor aFileDesc); Nit: it seems we don't prefix arguments with "a" in ipdl files ::: dom/asmJSCache/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +MODULE = 'dom' this was just removed from moz.build files @@ +5,5 @@ > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +MODULE = 'dom' > + > +EXPORTS.mozilla.dom += [ EXPORTS.mozilla.dom.asmJSCache ? @@ +19,5 @@ > +] > + > +FAIL_ON_WARNINGS = True > + > +LIBXUL_LIBRARY = True dtto @@ +23,5 @@ > +LIBXUL_LIBRARY = True > + > +MSVC_ENABLE_PGO = True > + > +LIBRARY_NAME = 'domasmjscache_s' dtto @@ +25,5 @@ > +MSVC_ENABLE_PGO = True > + > +LIBRARY_NAME = 'domasmjscache_s' > + > +include('/ipc/chromium/chromium-config.mozbuild') you will need to add |FINAL_LIBRARY = 'gklayout'| here ::: dom/ipc/ContentParent.cpp @@ +2685,5 @@ > #endif > } > > +PAsmJSCacheEntryParent* > +ContentParent::AllocPAsmJSCacheEntryParent(const asmJSCache::OpenMode& aOpenMode, Nit: 80 char limit ::: dom/ipc/PContent.ipdl @@ +358,5 @@ > PBluetooth(); > > PFMRadio(); > > + PAsmJSCacheEntry(OpenMode aOpenMode, int64_t aSizeToWrite, Principal aPrincipal); Nit: 80 char limit

Thanks! Updated/rebased to address everything so far.

Rebased

Comment on attachment 8335414 [details] [diff] [review] browser-caching 2 Review of attachment 8335414 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/asmjscache/AsmJSCache.cpp @@ +734,5 @@ > + // AddRef to keep the runnable alive until DeallocEntryParent. > + runnable->AddRef(); > + > + nsresult rv = NS_DispatchToMainThread(runnable); > + NS_ENSURE_SUCCESS(rv, nullptr); hm, I think it would be cleaner if you dispatched the runnable in ContentParent::RecvPAsmJSCacheEntryConstructor() something like this: ContentParent::RecvPAsmJSCacheEntryConstructor(aActor, aOpenMode, aSizeToWrite, aPrincipal) { auto runnable = static_cast<ParentProcessRunnable*>(aActor); nsresult rv = NS_DispatchToMainThread(runnable); NS_ENSURE_SUCCESS(rv, false); return true; } Actually, you might also want to create a new method for this RecvEntryParent() and do the dispatch there.

Comment on attachment 8335414 [details] [diff] [review] browser-caching 2 Review of attachment 8335414 [details] [diff] [review]: ----------------------------------------------------------------- I like how you structured all the runnable objects and I really appreciate the comments and readability of the code, thanks! If I understood it correctly, the flow is: - WaitForOpenAllowed() called on the main thread - Dispatch to the IO thread (the IO thread is shared by all quota clients) - Unblock a JS thread and read/write data on it - AllowNextSynchronizedOp on the main thread You added a client type to GetStorageId(), so if there are two or more tabs accessing the same origin, one of them doing database stuff using IndexedDB and other loading a big asm.js file, then IDB won't be blocked until asm.js is finished. There's only one issue that should be fixed. QuotaManager calls client->ShutdownTransactionService() when it gets a shutdown notification. The client implementation in this patch doesn't implement it, so if the user quits the browser in the middle of asm.js cache reading/writing, QuotaManager may get destroyed before a JS thread is done with asm.js. This can cause various problems, so it would be better if you implemented the method. Luke also explained to me, why it's better to dispatch the runnable in AllocEntryParent(), so no need for RecvEntryParent() ::: dom/asmjscache/AsmJSCache.cpp @@ +368,5 @@ > + QuotaManager* qm = QuotaManager::GetOrCreate(); > + NS_ENSURE_STATE(qm); > + > + nsresult rv = QuotaManager::GetInfoFromPrincipal(mPrincipal, &mGroup, > + &mOrigin, NULL, NULL); NULL -> nullptr @@ +385,5 @@ > +{ > + AssertIsOnIOThread(); > + MOZ_ASSERT(mState == eReadyToOpen); > + > + nsresult rv; Nit: this can be declared when you call EnsureOriginIsInitialized() @@ +411,5 @@ > +#ifdef DEBUG > + else { > + bool isDirectory; > + NS_ASSERTION(NS_SUCCEEDED(path->IsDirectory(&isDirectory)) && > + isDirectory, "Should have caught this earlier!"); NS_ASSERTION -> MOZ_ASSERT @@ +458,5 @@ > + // Both of these operations need to happen on the main thread: > + > + if (mNeedAllowNextSynchronizedOp) { > + QuotaManager::Get()->AllowNextSynchronizedOp( > + OriginOrPatternString::FromOrigin(mOrigin), Nit: align to the right @@ +578,5 @@ > + : MainProcessRunnable(aPrincipal, aOpenMode, aSizeToWrite) > + { > + MOZ_ASSERT(IsMainProcess()); > + MOZ_ASSERT(!NS_IsMainThread()); > + MOZ_COUNT_CTOR(SingleProcessRunnable); hm, IIRC MOZ_COUNT_CTOR/DTOR is only used for non-refcounted objects @@ +950,5 @@ > + } > + > + JSCompartment* compartment = GetObjectCompartment(aGlobal); > + JSPrincipals* jsPrincipal = JS_GetCompartmentPrincipals(compartment); > + nsIPrincipal* principal = nsJSPrincipals::get(jsPrincipal); This can be replaced by |nsContentUtils::GetObjectPrincipal(aGlobal)| @@ +997,5 @@ > + // in the first word. > + // - When attempting to read a cache file, check whether the first word is > + // sAsmJSCookie. > + if (file->FileSize() < sizeof(AsmJSCookieType) || > + *(AsmJSCookieType*) file->MappedMemory() != sAsmJSCookie) { Nit: I'm not sure, but I think the extra space after the cast is not needed, here and elsewhere @@ +1079,5 @@ > + nsresult rv = info->GetPlatformBuildID(buildID); > + NS_ENSURE_SUCCESS(rv, false); > + > + if (!aBuildID->resize(buildID.Length())) > + return false; Nit: wrap in {} @@ +1118,5 @@ > + const nsACString& aGroup, > + const nsACString& aOrigin, > + UsageInfo* aUsageInfo) MOZ_OVERRIDE > + { > + nsresult rv; Nit: declare just when you need it @@ +1147,5 @@ > + int64_t fileSize; > + rv = path->GetFileSize(&fileSize); > + NS_ENSURE_SUCCESS(rv, rv); > + > + NS_ASSERTION(fileSize >= 0, "Negative size?!"); NS_ASSERTION -> MOZ_ASSERT

(In reply to Jan Varga [:janv] from comment #26) > hm, IIRC MOZ_COUNT_CTOR/DTOR is only used for non-refcounted objects The reason I wanted these is so that the leak detector would catch bugs from a missing Release() (which is not unthinkable due to the explicit AddRef/Release). Is there any hard reason you know of MOZ_COUNT_CTOR/DTOR cannot be used for ref-counted objects?

(In reply to Luke Wagner [:luke] from comment #27) > (In reply to Jan Varga [:janv] from comment #26) > > hm, IIRC MOZ_COUNT_CTOR/DTOR is only used for non-refcounted objects > > The reason I wanted these is so that the leak detector would catch bugs from > a missing Release() (which is not unthinkable due to the explicit > AddRef/Release). Is there any hard reason you know of MOZ_COUNT_CTOR/DTOR > cannot be used for ref-counted objects? hm, that sounds reasonable

Comment on attachment 8335414 [details] [diff] [review] browser-caching 2 Review of attachment 8335414 [details] [diff] [review]: ----------------------------------------------------------------- Regular refcounted things should already be hooked into the leak infrastructure, so the CTOR/DTOR thing shouldn't be needed. Note that this doesn't yet apply to the MFBT ref pointers, and I'm not sure what you are using here. ::: dom/asmjscache/AsmJSCache.cpp @@ +599,5 @@ > + return false; > + } > + > + // Now that we're open, we're guarnateed a Close() call. However, we are > + // not guarnateed someone is holding an outstanding reference until Close() guaranteed

Comment on attachment 8335423 [details] [diff] [review] only-cache-big-modules Review of attachment 8335423 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/asmjscache/AsmJSCache.cpp @@ +976,5 @@ > static const uint32_t sAsmJSCookie = 0x600d600d; > > +// Anything smaller should compile fast enough that caching will just add > +// overhead. > +static const size_t sMinCachedModuleLength = 10000; In a month or so, B2G might want to customize this.

Comment on attachment 8335414 [details] [diff] [review] browser-caching 2 Review of attachment 8335414 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/asmjscache/AsmJSCache.cpp @@ +429,5 @@ > + openFlags = PR_RDONLY | nsIFile::OS_READAHEAD; > + } else { > + nsRefPtr<QuotaObject> quotaObject = > + qm->GetQuotaObject(quota::PERSISTENCE_TYPE_TEMPORARY, mGroup, mOrigin, > + path); One more thing, QuotaObject should be created before opening the file and destroyed right after closing the file. Otherwise we loose track of opened files and the assertion in QuotaManager.cpp won't catch an "accidental" removal of an active origin: NS_ASSERTION(!originInfo->mQuotaObjects.Count(), "Inactive origin shouldn't have open files!"); See how it is done in quota/FileStreams.cpp

(In reply to Jan Varga [:janv] from comment #31) > One more thing, QuotaObject should be created before opening the file and > destroyed right after closing the file. I meant: GetQuotaObject() should be called before opening the file and quotaObject->Release() right after closing the file.

Comment on attachment 8335423 [details] [diff] [review] only-cache-big-modules Review of attachment 8335423 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/asmjscache/AsmJSCache.h @@ +34,5 @@ > > // Implementation of AsmJSCacheOps, installed by nsJSEnvironment: > > bool > +OpenEntryForRead(JS::HandleObject aGlobal, is this change intentional ? (JS:Handle<JSObject*> -> JS::HandleObject)

(In reply to Jan Varga [:janv] from comment #33) > is this change intentional ? Nope! That patch was before your request to change to Handle<JSObject*> and I missed the difference when resolving the rebase conflict. I'll switch it back.

> One more thing, QuotaObject should be created before opening the file and > destroyed right after closing the file. Done. I was able to add a nsRefPtr<QuotaObject> FileDescriptorHolder::mQuotaObject that is assigned the result of GetQuotaObject. (If I read correctly, QuotaObject::Release can be called from any thread.) Sound good? Want me to post a new patch?

sounds good, yeah I'll take a look, thanks

Here you go. This patch also fixes a race condition where mState was being assigned to its new state after calling OnFailure/OnClose/OnOpen (which wakes up the parsing thread which can observe the the pre-assignment mState value). Here's a URL to test the caching, in case you were interested: http://people.mozilla.org/~lwagner/test-caching.html

Oops, ignore the hunk switching NS_ASSERTION to MOZ_ASSERT in QuotaManager::GetInactiveTemporaryStorageOrigins; that was unintended.

Hm, it would be better to null out mQuotaObject explicitly.

(In reply to Jan Varga [:janv] from comment #39) > Hm, it would be better to null out mQuotaObject explicitly. In ~FileDescriptorHolder()? Is that the established style, because it seems a bit redundant...

(In reply to Luke Wagner [:luke] from comment #40) > (In reply to Jan Varga [:janv] from comment #39) > > Hm, it would be better to null out mQuotaObject explicitly. > > In ~FileDescriptorHolder()? Is that the established style, because it seems > a bit redundant... Oh, I forgot that PR_Close() is actually called in ~FileDescriptorHolder() So I would add |mQuotaObject = nullptr| after PR_Close(), or a commented out version of it. An extra comment would be great. It will be later easy to check if the quota object is addrefed/released correctly. Also, you need to get a quota object even for reading. Deleting an origin while something is still reading an origin file isn't great either :)

Righto. I ended up putting these in a new FileDescriptorHolder::Close() method called right before AllowNextSychronizedOp so that ~FileDescriptorHolder can assert everything is null. This patch also rebases over bug 932119 which actually simplified things a bit. Just waiting on bug 942207 to re-land...

https://hg.mozilla.org/integration/mozilla-inbound/rev/5387d9c92201 https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3d89ed5dc7

https://hg.mozilla.org/mozilla-central/rev/5387d9c92201 https://hg.mozilla.org/mozilla-central/rev/2e3d89ed5dc7

Per 942207 comment 11, if you land a patch that adds a dependency on a new NSPR feature, you should bump the NSPR version requirement in configure.in to match to avoid breaking people building --with-system-nspr.