Closed
Bug 965970
Opened 11 years ago
Closed 11 years ago
Add support to compile asm.js code at install/update time
Categories
(Core Graveyard :: DOM: Apps, defect)
Tracking
(tracking-b2g:backlog, firefox29 wontfix, firefox30 wontfix, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)
People
(Reporter: fabrice, Assigned: fabrice)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [games][qa-])
Attachments
(1 file, 6 obsolete files)
18.46 KB,
patch
|
marco
:
review+
praghunath
:
approval-mozilla-aurora+
praghunath
:
approval-mozilla-b2g30+
|
Details | Diff | Splinter Review |
For packaged apps shipping asm.js code, we want to compile this code at install time and when gecko or app updates happen. That provides better user experience since the first launch won't be slowed down.
Apps will opt-in to this mechanism by adding a property to their manifest listing the js resources that need to be compiled.
Comment 2•11 years ago
|
||
Will this feature work on any FF platform that supports installing packaged apps or only FFOS? (For that matter, are there any FF platforms other than FFOS on which you can install a packaged app?)
Comment 3•11 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #2)
> For that matter, are there any FF platforms other than FFOS on which you can install a packaged app?
You can install packaged apps on both desktop (Windows, Mac, Linux) and Android (although the desktop runtime doesn't yet support signed packaged apps, and most packaged apps are signed).
Comment 4•11 years ago
|
||
(In reply to Myk Melez [:myk] [@mykmelez] from comment #3)
Cool! I'm guessing this is the new "App Manager" entry in the developer tools menu.
Assignee | ||
Comment 5•11 years ago
|
||
The way we install apps is quite different on desktop - each app has its own profile, so that will need some more work here I guess. I never remember if we share the same profile on android or not, so calling Myk to the rescue on this one!
Flags: needinfo?(myk)
Comment 6•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #5)
> The way we install apps is quite different on desktop - each app has its own
> profile, so that will need some more work here I guess. I never remember if
> we share the same profile on android or not, so calling Myk to the rescue on
> this one!
Each app gets its own profile on Android, so presumably we'll need more work there too.
Flags: needinfo?(myk)
Comment 7•11 years ago
|
||
A few notes:
For the 'obj' parameter to CompileOffThread, I would create a throwaway sandbox and pass the global object of that sandbox. (That way the GC will easily clear all the resulting compiled scripts when you're done.) You should also enter the compartment of the sandbox global:
AutoCompartment ac(cx, global);
(using whatever random 'cx' you have lying around) before calling CompileOffThread.
After default-constructing a CompileOptions(cx, JSVERSION_DEFAULT), the options to set are:
- setSourcePolicy(CompileOptions::NO_SOURCE)
- asmJSOption = true
- forceAsync = true
Lastly, bug 963588 adds a new 'installedFile' flag to CompileOptions (passed to CompileOffThread). Setting this flag will place any asm.js cache entries added during compilation in persistent storage so they won't get evicted.
Assignee | ||
Comment 8•11 years ago
|
||
Luke, here's my current wip. It's crashing in the js parser:
--*-- AsmjsPreloader: Preloading app://{6c1696d4-a825-4476-9e98-7b0729726c87}
--*-- AsmjsPreloader: asmJS file: app://{6c1696d4-a825-4476-9e98-7b0729726c87}/game.js
XXX script=0x7fffb8d00008 length=13619942
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffdd8f7700 (LWP 13423)]
0x00007ffff49e162a in js::frontend::TokenStream::TokenBuf::getRawChar (this=0x7fffdd8f6780) at /home/fabrice/dev/birch/js/src/frontend/TokenStream.h:771
771 return *ptr++; // this will nullptr-crash if poisoned
(gdb) bt
#0 0x00007ffff49e162a in js::frontend::TokenStream::TokenBuf::getRawChar (this=0x7fffdd8f6780) at /home/fabrice/dev/birch/js/src/frontend/TokenStream.h:771
#1 0x00007ffff49a0c34 in js::frontend::TokenStream::getTokenInternal (this=0x7fffdd8f64b0, modifier=js::frontend::TokenStream::Operand)
at /home/fabrice/dev/birch/js/src/frontend/TokenStream.cpp:1068
#2 0x00007ffff49864a5 in js::frontend::TokenStream::peekToken (this=0x7fffdd8f64b0, modifier=js::frontend::TokenStream::Operand)
at /home/fabrice/dev/birch/js/src/frontend/TokenStream.h:523
#3 0x00007ffff4957bde in js::frontend::CompileScript (cx=0x7fffcf739380, alloc=0x7fffd0996e98, scopeChain=..., evalCaller=..., options=...,
chars=0x7fffb8d00008 <Address 0x7fffb8d00008 out of bounds>, length=13619942, source_=0x0, staticLevel=0, extraSct=0x0)
at /home/fabrice/dev/birch/js/src/frontend/BytecodeCompiler.cpp:318
#4 0x00007ffff4d1d47b in js::WorkerThread::handleParseWorkload (this=0x7fffdd6479b0) at /home/fabrice/dev/birch/js/src/jsworkers.cpp:855
#5 0x00007ffff4d1dd16 in js::WorkerThread::threadLoop (this=0x7fffdd6479b0) at /home/fabrice/dev/birch/js/src/jsworkers.cpp:1032
#6 0x00007ffff4d1cb1e in js::WorkerThread::ThreadMain (arg=0x7fffdd6479b0) at /home/fabrice/dev/birch/js/src/jsworkers.cpp:712
#7 0x00007ffff6aad0be in _pt_root (arg=0x7ffff6c60ee0) at /home/fabrice/dev/birch/nsprpub/pr/src/pthreads/ptthread.c:212
#8 0x00007ffff7bc6e0e in start_thread (arg=0x7fffdd8f7700) at pthread_create.c:311
#9 0x00007ffff6ee00fd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:113
(gdb)
Do you see anything obviously wrong in this patch?
Attachment #8385871 -
Flags: feedback?(luke)
Comment 9•11 years ago
|
||
Ah, yes, I think so: the patch decodes the chars into a 'script' string variable on the stack and then passes this to CompileOffThread() which does not make a copy before the function returns and 'script' is destroyed. You probably want to make 'script' a member variable of AsmJSCompileLoader.
Comment 10•11 years ago
|
||
Comment on attachment 8385871 [details] [diff] [review]
wip
Review of attachment 8385871 [details] [diff] [review]:
-----------------------------------------------------------------
Glad to see this shaping up, btw!
::: js/xpconnect/src/XPCComponents.cpp
@@ +3772,5 @@
> + aObserver->Observe(uri, nullptr, nullptr);
> +}
> +
> +NS_IMETHODIMP
> +nsXPCComponents_Utils::CompileAsmJS(nsIURI *aURI, nsIObserver *aObserver,
Random bikeshedding request: since this mechanism may be reused for other times of install-time caching, could you leave 'AsmJS' out of all the identifiers and just use a generic term like "PrecompileScript" or something?
Attachment #8385871 -
Flags: feedback?(luke)
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 8385871 [details] [diff] [review]
wip
Review of attachment 8385871 [details] [diff] [review]:
-----------------------------------------------------------------
Nevermind, I found the issue.
Assignee | ||
Updated•11 years ago
|
Attachment #8385871 -
Attachment is obsolete: true
Assignee | ||
Comment 12•11 years ago
|
||
With this patch I've been able to install a game with asm.js code, and everything works as expected. At first launch instead of waiting 20s (on a desktop build) we get:
JavaScript warning: app://{3adc6f9a-9f03-408f-9879-311976cd45a5}/game.js, line 0: Successfully compiled asm.js code (loaded from cache in 418ms)
Still missing from this patch:
- a couple of paths in Webapps.jsm (download retries and updates).
- what to do if a file previously compiled should not be compiled anymore after an update.
- tests.
Attachment #8386590 -
Flags: feedback?(luke)
Comment 13•11 years ago
|
||
Comment on attachment 8386590 [details] [diff] [review]
wip preload-asm-js.patch
Review of attachment 8386590 [details] [diff] [review]:
-----------------------------------------------------------------
\o/ Beautiful. Thanks a lot Fabrice.
Is it possible to emit a warning if the 'preload' option is set but the 'indexedDB-unlimited' option
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#229
is not set? The reason is that, in bug 963588 (which should land soon), even if options.installedFile is set, we only store the cache entry in persistent storage if the app has the indexedDB-unlimited storage permission (this option name will later be renamed to something that isn't indexedDB-specific) to avoid some tricky quota corner cases we'd otherwise have to deal with.
::: js/xpconnect/src/XPCComponents.cpp
@@ +3655,5 @@
> +/**
> + * Let us compile scripts from a URI off the main thread.
> + */
> +
> +class ScriptLoader : public nsIStreamLoaderObserver
Maybe name it "ScriptPreloader" or "BackgroudScriptCompiler" to distinguish from nsScriptLoader?
@@ +3726,5 @@
> + JSObject* scope =
> + JS_NewGlobalObject(cx, &sGlobalClass,
> + nsJSPrincipals::get(mPrincipal),
> + JS::FireOnNewGlobalHook);
> + JS::Rooted<JSObject*> global(cx, scope);
To avoid leaving a possibly-stale pointer in 'scope', I'd do:
JS::Rooted<JSObject*> global(cx,
JS_NewGlobalObject(...));
@@ +3742,5 @@
> + nsAutoCString spec;
> + uri->GetSpec(spec);
> + options.setFile(spec.get());
> +
> + if (!JS::CanCompileOffThread(cx, options, mScript.Length())) {
With forceAsync, the only way this can fail is if !cx->runtime()->canUseParallelParsing which is default true, even on single-core devices. Thus, if this returns false, there is some configuration error involving the different prefs involved. It's still worth checking, but, if this fails, is there any way to issue a "really loud" warning that would show up in whatever logs a FFOS dev would see? (Same for any failure in this function, actually.)
@@ +3751,5 @@
> + nsCOMPtr<nsIRunnable> runnable =
> + NS_NewRunnableMethod(this, &ScriptLoader::SendNotification);
> +
> + // This reference will be consumed by OffThreadCallback.
> + NS_ADDREF(runnable);
The !JS::CompileOffThread failure path doesn't release this. Perhaps use an nsRefPtr that is .Forget()'d after JS::ComileOffThread succeeds?
Attachment #8386590 -
Flags: feedback?(luke) → feedback+
Comment 14•11 years ago
|
||
Bug 963588 landed so now you can add options.installedFile = true to your compilation options. Right now there is, unfortunately, no way that I know of to test whether a cache file is stored in persistent or temporary storage. Probably we can just add a nsIQuotaManager.clearAllTemporaryStorages() (like clearAllStorages()) and then assert in the test that we get a cache hit after that. For now, though, you can just look in $(PROFILE)/storage/persistent/$(APP)/asmjs to see if you have an entry :)
Comment 15•11 years ago
|
||
I think we should call the preload function only on b2g, because on Firefox and Android the data stored in the main profile is useless for apps.
Comment 16•11 years ago
|
||
Per Martin Best, nom'ing this for 1.4. Without this we can't ship important tier 1 game titles that require emscripten as their startup times will be too long
blocking-b2g: --- → 1.4?
Comment 17•11 years ago
|
||
We can't block on this - we aren't accepting new features into 1.4 at this time. We can consider this for a future release though.
blocking-b2g: 1.4? → backlog
Comment 18•11 years ago
|
||
Hey guys, if you don't block on this, you are saying that you don't want to ship WMW on 1.4. We have been pushing for this for a very long time and it blocks cache on install.
Comment 19•11 years ago
|
||
Martin
Did we support MWM in 1.4? Or is it something we're developing newly?
Flags: needinfo?(mbest)
Comment 20•11 years ago
|
||
Preeti, this has been pushed out from other releases and it's going to become a bit partner issue if we don't get this in so it's likely a good candidate for an exception. Fabrice is working on the patch, says it's fairly low risk and that we should be able to get it in. I defer to him on status of the actual patch.
Comment 21•11 years ago
|
||
(In reply to Sheila Mooney from comment #20)
> Preeti, this has been pushed out from other releases and it's going to
> become a bit partner issue if we don't get this in so it's likely a good
> candidate for an exception. Fabrice is working on the patch, says it's
> fairly low risk and that we should be able to get it in. I defer to him on
> status of the actual patch.
FWIW - I think that's a case where we should consider this patch for approval, but I don't think we would hold the release for this.
Note - the last set of dev activity was over a month ago, so if this is needed to be fixed for 1.4, then we're going to need to get traction on this quickly.
Assignee | ||
Comment 22•11 years ago
|
||
That works fine on desktop builds, still need to test on a 512MB device.
Luke, please review the anything not in dom/apps, and Marco the dom/apps parts.
Attachment #8406533 -
Flags: review?(mar.castelluccio)
Attachment #8406533 -
Flags: review?(luke)
Comment 23•11 years ago
|
||
Comment on attachment 8406533 [details] [diff] [review]
preload-asm-js.patch v2
Review of attachment 8406533 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks! This is a promising start, I think I found some bugs so we'll have another iteration or two.
More importantly, I'm not an XPConnect peer, so I can't give the final r+ for XPCComponents.cpp. I think it's likely bholley will think that nsIXPCComponents_Utils/XPCComponents.cpp is the wrong place to put this. js/xpconnect/loader, on the other hand, has a lot of similar code, so probably that would be a good place, but we should ask bholley first.
::: content/base/src/nsScriptLoader.cpp
@@ +857,5 @@
> if (!JS::CompileOffThread(cx, options,
> aRequest->mScriptText.get(), aRequest->mScriptText.Length(),
> OffThreadScriptLoaderCallback,
> static_cast<void*>(runnable))) {
> + NS_RELEASE(runnable);
Good catch.
::: js/xpconnect/idl/xpccomponents.idl
@@ +574,5 @@
> + /*
> + * Compiles a JS script off the main thread and calls back the
> + * observer once it's done.
> + */
> + void compileScript(in nsIURI uri,
This sort of name tends to imply that the resulting script is returned in some way.
::: js/xpconnect/src/XPCComponents.cpp
@@ +3620,5 @@
> +/**
> + * Let us compile scripts from a URI off the main thread.
> + */
> +
> +class BackgroundScriptCompiler : public nsIStreamLoaderObserver
Perhaps we could name this ScriptPrecompiler?
@@ +3667,5 @@
> + const uint8_t* aString)
> +{
> + nsCOMPtr<nsIRequest> request;
> + aLoader->GetRequest(getter_AddRefs(request));
> + mChannel = do_QueryInterface(request);
Not my area of expertise, but could you avoid this step by passing the nsIChannel to BackgroundScriptCompiler's constructor?
@@ +3689,5 @@
> +
> + AutoJSContext cx;
> + JS::Rooted<JSObject*> global(cx,
> + JS_NewGlobalObject(cx, &sGlobalClass,
> + nsJSPrincipals::get(mPrincipal),
Can you add a little comment explaining that our goal is to cache persistently and avoid quota checks and that caching mechanisms decide the persistence type based on the principal and that this is why we create a new global with the app's principal: so we can enter its compartment and therefore compile with its principal.
@@ +3699,5 @@
> + options.asmJSOption = true;
> + options.forceAsync = true;
> + options.compileAndGo = true;
> + options.installedFile = true;
> + options.setOriginPrincipals(nsJSPrincipals::get(mPrincipal));
You can remove the setOriginPrincipals; originPrincipals() will correctly default to the compartment's current principals.
@@ +3708,5 @@
> + uri->GetSpec(spec);
> + options.setFile(spec.get());
> +
> + if (!JS::CanCompileOffThread(cx, options, mScript.Length())) {
> + printf_stderr("Can't compile script off thread! %s\n", uri.get());
I assume the right warning mechanism for this is NS_WARNING?
@@ +3738,5 @@
> +BackgroundScriptCompiler::OffThreadCallback(void *aToken, void *aData)
> +{
> + nsCOMPtr<nsIRunnable> runnable = static_cast<nsIRunnable*>(aData);
> +
> + NS_DispatchToMainThread(runnable);
This skips the important step of calling JS::FinishOffThreadScript() on the main thread. Even though you will be ignoring the return value, FinishOffThreadScript effectively releases the resources consumed by the compiled script which would otherwise be leaked.
@@ +3754,5 @@
> + mObserver->Observe(uri, nullptr, nullptr);
> +}
> +
> +NS_IMETHODIMP
> +nsXPCComponents_Utils::CompileScript(nsIURI *aURI,
* with the type
@@ +3764,5 @@
> + aURI, nullptr, nullptr, nullptr,
> + nsIRequest::LOAD_NORMAL, nullptr);
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + nsCOMPtr<nsIStreamLoader> loader;
Can you move 'loader' down to right before NS_NewStreamLoader?
@@ +3768,5 @@
> + nsCOMPtr<nsIStreamLoader> loader;
> + nsCOMPtr<BackgroundScriptCompiler> bgObserver =
> + new BackgroundScriptCompiler(aObserver, aPrincipal);
> +
> + NS_ADDREF(bgObserver);
I can't find the paired NS_RELEASE. Is this something magic to do with NS_NewStreamLoader? Whatever the case, a comment explaining would be good.
Attachment #8406533 -
Flags: review?(luke)
Comment 24•11 years ago
|
||
needinfo? bholley for opinion on comment 23 (question at the top).
Updated•11 years ago
|
Flags: needinfo?(bobbyholley)
Comment 25•11 years ago
|
||
Yeah, Cu is pretty crowded. Let's stick it on mozIJSSubscriptLoader, even though it's not entirely accurate name-wise.
Flags: needinfo?(bobbyholley)
Comment 26•11 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #23)
> > + void compileScript(in nsIURI uri,
>
> This sort of name tends to imply that the resulting script is returned in
> some way.
Oops, looks like I cut off before suggesting the name 'precompileScript'.
Assignee | ||
Comment 27•11 years ago
|
||
I addressed review comments and moved everything to mozIJSSubScriptLoader.
Works fine on a Leo with 512MB and wmw!
Attachment #8386590 -
Attachment is obsolete: true
Attachment #8406533 -
Attachment is obsolete: true
Attachment #8406533 -
Flags: review?(mar.castelluccio)
Attachment #8407113 -
Flags: review?(mar.castelluccio)
Attachment #8407113 -
Flags: review?(luke)
Attachment #8407113 -
Flags: review?(bobbyholley)
Comment 28•11 years ago
|
||
Comment on attachment 8407113 [details] [diff] [review]
preload-asm-js.patch v3
Review of attachment 8407113 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good in general. Just some small things.
::: content/base/src/nsScriptLoader.cpp
@@ +857,5 @@
> if (!JS::CompileOffThread(cx, options,
> aRequest->mScriptText.get(), aRequest->mScriptText.Length(),
> OffThreadScriptLoaderCallback,
> static_cast<void*>(runnable))) {
> + NS_RELEASE(runnable);
While you're here, can you fix this pattern to match the one suggested in mozJSSubScriptLoader?
::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
@@ +45,5 @@
> jsval loadSubScriptWithOptions(in AString url, in jsval options);
> +
> + /*
> + * Compiles a JS script off the main thread and calls back the
> + * observer once it's done.
We need more comments here explaining the impact of doing this (i.e. that the script gets cached by the JS engine on success).
@@ +50,5 @@
> + * @param uri the uri of the script to load.
> + * @param principal the principal from which we get the app id if any.
> + * @param observer this observer will be called once the script has
> + * been precompiled.
> + * the topic will be 'script-precompiled' and the subject
Nit - incomplete sentence, and probably no need for the line break.
@@ +51,5 @@
> + * @param principal the principal from which we get the app id if any.
> + * @param observer this observer will be called once the script has
> + * been precompiled.
> + * the topic will be 'script-precompiled' and the subject
> + * the uri of the script as a nsIURI.
What happens if we fail to compile the script (either due to an error in the script or an internal error)? We need to handle that, and describe what happens here.
@@ +60,4 @@
> };
> +
> +%{C++
> +#define SCRIPT_PRECOMPILED_TOPIC "script-precompiled"
This #define doesn't seem all that useful to me tbh.
::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +407,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS1(ScriptPrecompiler, nsIStreamLoaderObserver);
> +
> +const JSClass ScriptPrecompiler::sGlobalClass = {
Hm, why do we need this? Can't we just use a Sandbox (via xpc::CreateSandboxObject)?
@@ +414,5 @@
> + JS_PropertyStub, JS_DeletePropertyStub, JS_PropertyStub,
> + JS_StrictPropertyStub, JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub
> +};
> +
> +class ScriptPrecompilerRunnable : public nsRunnable
This could use a better name. NotifyPrecompilationCompleteRunnable?
@@ +499,5 @@
> + SendObserverNotification();
> + return NS_OK;
> + }
> +
> + nsCOMPtr<nsIRunnable> runnable =
This is dicey. If your runnable (or nsRunnable) ended up inheriting nsISupports twice, the cast in OffThreadCallback would be incorrect. Please use nsRefPtr<ConcreteType>, here and in OffThreadCallback.
@@ +503,5 @@
> + nsCOMPtr<nsIRunnable> runnable =
> + new ScriptPrecompilerRunnable(this);
> +
> + // This reference will be consumed by OffThreadCallback.
> + NS_ADDREF(runnable);
Instead of ADDREFing here and RELEASEing below, how about just doing runnable.forget() in the success branch?
@@ +522,5 @@
> +/* static */
> +void
> +ScriptPrecompiler::OffThreadCallback(void* aToken, void* aData)
> +{
> + nsCOMPtr<ScriptPrecompilerRunnable> runnable =
I'm pretty sure that nsCOMPtr should only be used for interface types. Please use nsRefPtr here.
@@ +546,5 @@
> + JSRuntime *rt;
> + svc->GetRuntime(&rt);
> + if (!rt) {
> + return;
> + }
Just do XPCJSRuntime::Get()->Runtime(); And really, this whole thing can just be inlined into the one caller, right?
@@ +572,5 @@
> + aURI, nullptr, nullptr, nullptr,
> + nsIRequest::LOAD_NORMAL, nullptr);
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + nsCOMPtr<ScriptPrecompiler> loadObserver =
nsRefPtr
Attachment #8407113 -
Flags: review?(bobbyholley) → review-
Comment 29•11 years ago
|
||
Comment on attachment 8407113 [details] [diff] [review]
preload-asm-js.patch v3
Review of attachment 8407113 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for addressing the last round; this is looking great! Just a few comments on top of Bobby's.
::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +414,5 @@
> + JS_PropertyStub, JS_DeletePropertyStub, JS_PropertyStub,
> + JS_StrictPropertyStub, JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub
> +};
> +
> +class ScriptPrecompilerRunnable : public nsRunnable
Alternatively, it seems like you could make ScriptPrecompiler derive nsRunnable which would reduce the number of moving parts. Then ScriptPrecompilerRunnable::Run could just hold the body of Cleanup() (with a SendObserverNotification() at the end).
@@ +482,5 @@
> + JSAutoCompartment ac(cx, global);
> +
> + JS::CompileOptions options(cx, JSVERSION_DEFAULT);
> + options.setSourcePolicy(CompileOptions::NO_SOURCE);
> + options.asmJSOption = true;
You can remove the asmJSOption. The ReadOnlyCompileOptions constructor may look like the default is 'false', but the CompileOptions constructor initializes it to cx->runtime()->options().asmJS which is controlled by the global pref (which we'd like to respect).
Attachment #8407113 -
Flags: review?(luke)
Assignee | ||
Comment 30•11 years ago
|
||
Addressed comments except Luke's "merging the runnable".
Works really good on device, where we load the cached code for wmw in 2.5s
Attachment #8407113 -
Attachment is obsolete: true
Attachment #8407113 -
Flags: review?(mar.castelluccio)
Attachment #8407220 -
Flags: review?(mar.castelluccio)
Attachment #8407220 -
Flags: review?(luke)
Attachment #8407220 -
Flags: review?(bobbyholley)
Comment 31•11 years ago
|
||
Comment on attachment 8407220 [details] [diff] [review]
preload-asm-js.patch v4
Review of attachment 8407220 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/base/src/nsScriptLoader.cpp
@@ -817,5 @@
> NotifyOffThreadScriptLoadCompletedRunnable* aRunnable =
> static_cast<NotifyOffThreadScriptLoadCompletedRunnable*>(aCallbackData);
> aRunnable->SetToken(aToken);
> NS_DispatchToMainThread(aRunnable);
> - NS_RELEASE(aRunnable);
From IRC: this change should be reverted.
::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +431,5 @@
> + MOZ_ASSERT(NS_IsMainThread());
> + MOZ_ASSERT(mPrecompiler);
> +
> + MOZ_ASSERT(mChannel && mObserver);
> + MOZ_ASSERT(NS_IsMainThread());
This assert is repeated.
@@ +435,5 @@
> + MOZ_ASSERT(NS_IsMainThread());
> +
> + if (mToken) {
> + JSRuntime *rt = XPCJSRuntime::Get()->Runtime();
> + NS_ENSURE_TRUE(rt, NS_ERROR_FAILURE);
I assume rt can never be null. If it could, then this error return path would skip the call to SendObserverNotification.
@@ +476,5 @@
> + RootedValue v(cx);
> + SandboxOptions sandboxOptions;
> + sandboxOptions.sandboxName.AssignASCII("asm.js precompilation");
> + rv = CreateSandboxObject(cx, &v, mPrincipal, sandboxOptions);
> + NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
This failure skips SendObserverNotification().
@@ +478,5 @@
> + sandboxOptions.sandboxName.AssignASCII("asm.js precompilation");
> + rv = CreateSandboxObject(cx, &v, mPrincipal, sandboxOptions);
> + NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
> +
> + JSObject* global = js::UncheckedUnwrap(&(v.toObject()));
Generally we avoid naked JSObject* on the stack, so you'd need a RootedObject. Alternatively, since this is only used by JSAutoCompartment, you could just
JSAutoCompartment ac(cx, js::UncheckedUnwrap(&v.toObject()));
(The null assert doesn't seem very helpful since we'll immediately null-crash and UncheckedUnwrap is infallible anyhow.)
Attachment #8407220 -
Flags: review?(luke) → review+
Comment 32•11 years ago
|
||
Comment on attachment 8407220 [details] [diff] [review]
preload-asm-js.patch v4
Review of attachment 8407220 [details] [diff] [review]:
-----------------------------------------------------------------
This is close, but I want to look at the resulting RAII class.
::: content/base/src/nsScriptLoader.cpp
@@ -817,5 @@
> NotifyOffThreadScriptLoadCompletedRunnable* aRunnable =
> static_cast<NotifyOffThreadScriptLoadCompletedRunnable*>(aCallbackData);
> aRunnable->SetToken(aToken);
> NS_DispatchToMainThread(aRunnable);
> - NS_RELEASE(aRunnable);
This will leak. You either need to keep this line, or cast aCallbackData to an already_AddRefed and assign that into an nsRefPtr.
::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +389,5 @@
> +
> + virtual ~ScriptPrecompiler()
> + {}
> +
> + /* The Off main thread callback */
nit: /* The off-main-thread callback */
But actually, this comment doesn't really add anything. Maybe just remove it?
@@ +392,5 @@
> +
> + /* The Off main thread callback */
> + static void OffThreadCallback(void *aToken, void *aData);
> +
> + /* Sends the "done" notification back. Main thread only */
nit - add a period.
@@ +430,5 @@
> +{
> + MOZ_ASSERT(NS_IsMainThread());
> + MOZ_ASSERT(mPrecompiler);
> +
> + MOZ_ASSERT(mChannel && mObserver);
This class has neither an mChannel nor an mObserver. How does this even compile?
@@ +435,5 @@
> + MOZ_ASSERT(NS_IsMainThread());
> +
> + if (mToken) {
> + JSRuntime *rt = XPCJSRuntime::Get()->Runtime();
> + NS_ENSURE_TRUE(rt, NS_ERROR_FAILURE);
Reply to luke - actually, it might be, in the case that this runnable runs at shutdown. Though in that case we probably don't care about leaking.
@@ +476,5 @@
> + RootedValue v(cx);
> + SandboxOptions sandboxOptions;
> + sandboxOptions.sandboxName.AssignASCII("asm.js precompilation");
> + rv = CreateSandboxObject(cx, &v, mPrincipal, sandboxOptions);
> + NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
yeah, I think this is too error prone. We should add an RAII AutoSendObserverNotification class (with a .disarm()) that we can use both here and in NotifyPrecompilationCompleteRunnable.
@@ +478,5 @@
> + sandboxOptions.sandboxName.AssignASCII("asm.js precompilation");
> + rv = CreateSandboxObject(cx, &v, mPrincipal, sandboxOptions);
> + NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
> +
> + JSObject* global = js::UncheckedUnwrap(&(v.toObject()));
Also, drop the extra parens and make that &v.toObject().
@@ +526,5 @@
> + static_cast<NotifyPrecompilationCompleteRunnable*>(aData);
> + runnable->SetToken(aToken);
> +
> + NS_DispatchToMainThread(runnable);
> + NS_RELEASE(runnable);
The refcounting here is confusing. We should stick a dont_AddRef around the static_cast and drop the NS_RELEASE.
Attachment #8407220 -
Flags: review?(bobbyholley) → review-
Assignee | ||
Comment 33•11 years ago
|
||
Attachment #8407649 -
Flags: review?(luke)
Attachment #8407649 -
Flags: review?(ferjmoreno)
Attachment #8407649 -
Flags: review?(bobbyholley)
Updated•11 years ago
|
Attachment #8407649 -
Flags: review?(luke) → review+
Comment 34•11 years ago
|
||
Comment on attachment 8407649 [details] [diff] [review]
preload-asm-js.patch v5
Review of attachment 8407649 [details] [diff] [review]:
-----------------------------------------------------------------
r=bholley on the C++ bits with those two fixes. Thanks for doing this!
::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +438,5 @@
> + }
> + }
> +
> + void Disarm() {
> + mDisarmed = true;
This could be done more simply by having disarm just null out mPrecompiler. No need for the extra piece of state.
@@ +490,5 @@
> + // We then enter its compartment to compile with its principal.
> + AutoSafeJSContext cx;
> + RootedValue v(cx);
> + SandboxOptions sandboxOptions;
> + sandboxOptions.sandboxName.AssignASCII("asm.js precompilation");
Note - given that this is an implementation detail, you should set sandboxOptions.invisibleToDebugger = true, so that the debugger won't be notified about these scopes.
Attachment #8407649 -
Flags: review?(bobbyholley) → review+
Comment 35•11 years ago
|
||
Comment on attachment 8407649 [details] [diff] [review]
preload-asm-js.patch v5
Review of attachment 8407649 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/apps/src/ScriptPreloader.jsm
@@ +6,5 @@
> +
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;
Nit: You could use destructuring assignments for clarity here (you could also avoid defining some of the constants unused in this file).
@@ +20,5 @@
> +}
> +
> +this.ScriptPreloader = {
> +
> + preload: function(aApp, aManifest) {
I think we should reject the promise in case of problems (for example if the developer didn't write the precompile field correctly), to let the developer know if they've made any error (instead now we're failing silently here).
Even if we want to complete the installation either way, we should still at least report the error.
@@ +42,5 @@
> +
> + toLoad--;
> + if (toLoad == 0) {
> + deferred.resolve();
> + }
Is precompileScript always going to call the observer for each compilation, even if there are errors?
If it isn't, then the promise could end up being always unresolved.
@@ +51,5 @@
> +
> + aManifest.precompile.forEach((aPath) => {
> + let uri = Services.io.newURI(aPath, null, origin);
> + debug("Script to compile: " + uri.spec);
> + Services.scriptloader.precompileScript(uri, principal, observer);
Nit: IIRC, even if you keep using nsIObserver, I think the observer could just be a normal function.
Services.scriptLoader.precompileScript(uri, principal, (aSubject) => {
...
});
::: dom/apps/src/Webapps.jsm
@@ +1493,5 @@
> delete app.retryingDownload;
>
> + // Update the asm.js scripts we need to compile.
> + ScriptPreloader.preload(app, aData)
> + .then(this.saveApps).then(() => {
this._saveApps
::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
@@ +59,5 @@
> + * script as a nsIURI.
> + */
> + void precompileScript(in nsIURI uri,
> + in nsIPrincipal principal,
> + in nsIObserver observer);
Nit: I'm not sure, but I think you could just use a callback function instead of an observer here.
Attachment #8407649 -
Flags: feedback+
Assignee | ||
Comment 36•11 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #35)
> Comment on attachment 8407649 [details] [diff] [review]
> preload-asm-js.patch v5
>
> Review of attachment 8407649 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/apps/src/ScriptPreloader.jsm
> @@ +6,5 @@
> > +
> > +const Cu = Components.utils;
> > +const Cc = Components.classes;
> > +const Ci = Components.interfaces;
> > +const Cr = Components.results;
>
> Nit: You could use destructuring assignments for clarity here (you could
> also avoid defining some of the constants unused in this file).
Yep, I'll clean up the ununsed one. I kind of disagree about destructuring assignment being clearer ;)
> @@ +20,5 @@
> > +}
> > +
> > +this.ScriptPreloader = {
> > +
> > + preload: function(aApp, aManifest) {
>
> I think we should reject the promise in case of problems (for example if the
> developer didn't write the precompile field correctly), to let the developer
> know if they've made any error (instead now we're failing silently here).
> Even if we want to complete the installation either way, we should still at
> least report the error.
I'm fine with reporting an error, but that should not prevent app install, so I will not reject the promise.
> @@ +42,5 @@
> > +
> > + toLoad--;
> > + if (toLoad == 0) {
> > + deferred.resolve();
> > + }
>
> Is precompileScript always going to call the observer for each compilation,
> even if there are errors?
> If it isn't, then the promise could end up being always unresolved.
Yes, the observer is always called from the c++ side.
> @@ +51,5 @@
> > +
> > + aManifest.precompile.forEach((aPath) => {
> > + let uri = Services.io.newURI(aPath, null, origin);
> > + debug("Script to compile: " + uri.spec);
> > + Services.scriptloader.precompileScript(uri, principal, observer);
>
> Nit: IIRC, even if you keep using nsIObserver, I think the observer could
> just be a normal function.
>
> Services.scriptLoader.precompileScript(uri, principal, (aSubject) => {
> ...
> });
Yes, that will work. I have no strong opinion there.
> ::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
> @@ +59,5 @@
> > + * script as a nsIURI.
> > + */
> > + void precompileScript(in nsIURI uri,
> > + in nsIPrincipal principal,
> > + in nsIObserver observer);
>
> Nit: I'm not sure, but I think you could just use a callback function
> instead of an observer here.
You can't define function parameters in xpidl unfortunately, you have to wrap them in an interface, which is just what nsIObserver does. The observe() signature is a bit overkill (we don't use data) but I think it's still better than creating a new interface for this use case.
Assignee | ||
Comment 37•11 years ago
|
||
Some changes only to the js side.
try build at https://tbpl.mozilla.org/?tree=Try&rev=eeb1bcb7472b
Attachment #8407220 -
Attachment is obsolete: true
Attachment #8407649 -
Attachment is obsolete: true
Attachment #8407220 -
Flags: review?(mar.castelluccio)
Attachment #8407649 -
Flags: review?(ferjmoreno)
Attachment #8407794 -
Flags: review?(mar.castelluccio)
Assignee | ||
Comment 38•11 years ago
|
||
updated after discussing on irc with bholley:
https://tbpl.mozilla.org/?tree=Try&rev=59261ff96ea9
Comment 39•11 years ago
|
||
Comment on attachment 8407794 [details] [diff] [review]
preload-asm-js.patch v6
Review of attachment 8407794 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Fabrice Desré [:fabrice] from comment #36)
> > @@ +20,5 @@
> > > +}
> > > +
> > > +this.ScriptPreloader = {
> > > +
> > > + preload: function(aApp, aManifest) {
> >
> > I think we should reject the promise in case of problems (for example if the
> > developer didn't write the precompile field correctly), to let the developer
> > know if they've made any error (instead now we're failing silently here).
> > Even if we want to complete the installation either way, we should still at
> > least report the error.
>
> I'm fine with reporting an error, but that should not prevent app install,
> so I will not reject the promise.
Fine by me.
> > @@ +42,5 @@
> > > +
> > > + toLoad--;
> > > + if (toLoad == 0) {
> > > + deferred.resolve();
> > > + }
> >
> > Is precompileScript always going to call the observer for each compilation,
> > even if there are errors?
> > If it isn't, then the promise could end up being always unresolved.
>
> Yes, the observer is always called from the c++ side.
Good, if it is called even if the compilation fails then we're good.
> > ::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
> > @@ +59,5 @@
> > > + * script as a nsIURI.
> > > + */
> > > + void precompileScript(in nsIURI uri,
> > > + in nsIPrincipal principal,
> > > + in nsIObserver observer);
> >
> > Nit: I'm not sure, but I think you could just use a callback function
> > instead of an observer here.
>
> You can't define function parameters in xpidl unfortunately, you have to
> wrap them in an interface, which is just what nsIObserver does. The
> observe() signature is a bit overkill (we don't use data) but I think it's
> still better than creating a new interface for this use case.
Well, we're actually not using any of the parameters (just the subject, but
only to print a debug message, that we could print anyway).
Anyway, it's just a matter of taste, I prefer a callback function in cases like
this because it feels more "logical" (you're observing just one notification,
and it's just the end of an asynchronous operation) and "coherent" (because by
defining an interface you precisely define what the arguments of the function
are).
It's OK either way.
::: dom/apps/src/ScriptPreloader.jsm
@@ +50,5 @@
> + } else {
> + // The precompile field is not an array, let the developer know.
> + // We don't want to have to enable debug for that to show.
> + if (aManifest.precompile) {
> + dump("ASM.JS compilation failed: the 'precompile' manifest " +
Cu.reportError would be better, I think web developers usually don't see what Firefox prints in the console.
::: dom/apps/src/Webapps.jsm
@@ +1493,5 @@
> delete app.retryingDownload;
>
> + // Update the asm.js scripts we need to compile.
> + ScriptPreloader.preload(app, aData)
> + .then(this._saveApps).then(() => {
The test is failing because the function is not bound here, you could do: () => this._saveApps()
Attachment #8407794 -
Flags: review?(mar.castelluccio) → review+
Comment 40•11 years ago
|
||
Comment on attachment 8407794 [details] [diff] [review]
preload-asm-js.patch v6
Review of attachment 8407794 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/apps/src/ScriptPreloader.jsm
@@ +22,5 @@
> + preload: function(aApp, aManifest) {
> + debug("Preloading " + aApp.origin);
> + let deferred = Promise.defer();
> +
> +#ifdef MOZ_B2G
For testing purposes, it could be useful to make this a pref or
something configurable (otherwise we'll be able to test only on b2g).
For example, to avoid yet another pref, you could define a |_isEnabled|
property (|true| by default if MOZ_B2G is defined, |false| otherwise)
that we can override during tests.
Comment 41•11 years ago
|
||
Filed bug 997886 for tests.
Assignee | ||
Comment 42•11 years ago
|
||
Hopefully last try build before landing:
https://tbpl.mozilla.org/?tree=Try&rev=f208fa849ca1
Comment 43•11 years ago
|
||
Last thing, |precompileScript| could throw (for example if the js file doesn't exist), you should wrap the call in a try block and resolve the promise in case of exceptions.
Assignee | ||
Comment 44•11 years ago
|
||
Comment 45•11 years ago
|
||
\o/
Comment 46•11 years ago
|
||
This patch has landed in in-bound and requesting a nom for 1.4
Per results from QA will decide if safe for 1.4 or move to backlog.
Tony,
Please help verify the same.
Comment 47•11 years ago
|
||
(In reply to Preeti Raghunath(:Preeti) from comment #46)
> This patch has landed in in-bound and requesting a nom for 1.4
>
> Per results from QA will decide if safe for 1.4 or move to backlog.
>
> Tony,
>
> Please help verify the same.
This is not a blocker - the only option we can consider this for is an approval request.
Please have an approval form filled out & we'll consider this for approval.
blocking-b2g: 1.4? → backlog
Updated•11 years ago
|
QA Contact: jsmith
Comment 48•11 years ago
|
||
[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined:
Testing completed:
Risk to taking this patch (and alternatives if risky):
String or UUID changes made by this patch:
Luke,
Please request aurora-approval on the same. Form attached on top.
blocking-b2g: backlog → 1.4?
Flags: needinfo?(luke)
QA Contact: jsmith
Updated•11 years ago
|
blocking-b2g: 1.4? → backlog
Comment 49•11 years ago
|
||
Comment on attachment 8407794 [details] [diff] [review]
preload-asm-js.patch v6
[Approval Request Comment]
User impact if declined: asm.js games will sometimes (esp. on first run) take a really long time to load (like 1 minute)
Testing completed (on m-c, etc.): b2g-inbound
Risk to taking this patch (and alternatives if risky): very low: this whole path is predicated on a new manifest option that isn't set by any existing apps
String or IDL/UUID changes made by this patch: mozIJSSubScriptLoader
Attachment #8407794 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(luke)
Comment 50•11 years ago
|
||
Flags got overwritten. Moving to backlog so approval can be requested.
blocking-b2g: backlog → -
Updated•11 years ago
|
blocking-b2g: - → backlog
Comment 51•11 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #49)
> Comment on attachment 8407794 [details] [diff] [review]
> preload-asm-js.patch v6
>
> [Approval Request Comment]
> User impact if declined: asm.js games will sometimes (esp. on first run)
> take a really long time to load (like 1 minute)
> Testing completed (on m-c, etc.): b2g-inbound
> Risk to taking this patch (and alternatives if risky): very low: this whole
> path is predicated on a new manifest option that isn't set by any existing
> apps
> String or IDL/UUID changes made by this patch: mozIJSSubScriptLoader
To amplify Luke's notes -- a high profile partner we've been cultivating won't submit their game to marketplace without this patch (nor should they), see Martin's comment 18 above.
Comment 52•11 years ago
|
||
(In reply to Preeti Raghunath(:Preeti) from comment #46)
> This patch has landed in in-bound and requesting a nom for 1.4
>
> Per results from QA will decide if safe for 1.4 or move to backlog.
>
> Tony,
>
> Please help verify the same.
(In reply to Luke Wagner [:luke] from comment #49)
> Comment on attachment 8407794 [details] [diff] [review]
> preload-asm-js.patch v6
>
> [Approval Request Comment]
> User impact if declined: asm.js games will sometimes (esp. on first run)
> take a really long time to load (like 1 minute)
> Testing completed (on m-c, etc.): b2g-inbound
> Risk to taking this patch (and alternatives if risky): very low: this whole
> path is predicated on a new manifest option that isn't set by any existing
> apps
> String or IDL/UUID changes made by this patch: mozIJSSubScriptLoader
Looking for a little more Dev guidance here to help with inbound testing. Some questions i have:
- regression risks to the rest of the platform when this lands? (specifically, affected APIs around app installation, removal, etc.., other packaged apps that are already on the device)
- is WMW the only affected app that we know of for this work on 1.4? or are there other apps that need to be considered for 1.4?
- if QA needs to test WMW from marketplace, is there a sandboxed environment that we can pull this from? (we can certainly test sideloading, but the correct way is to go through the full marketplace userflow)
- Performance risks: do you expect this to touch any number of cold/warm launch hits on the app and other core apps?
- if something goes awry, can we cleanly back this patch out in isolation?
Flags: needinfo?(tchung)
Comment 53•11 years ago
|
||
Luke,
Can you please help Tony here? Looks like risk analysis is being asked here.
Flags: needinfo?(luke)
Assignee | ||
Comment 54•11 years ago
|
||
I'm not Luke, but I htink I can answer!
(In reply to Tony Chung [:tchung] from comment #52)
> (In reply to Luke Wagner [:luke] from comment #49)
> > Comment on attachment 8407794 [details] [diff] [review]
> > preload-asm-js.patch v6
> >
> > [Approval Request Comment]
> > User impact if declined: asm.js games will sometimes (esp. on first run)
> > take a really long time to load (like 1 minute)
> > Testing completed (on m-c, etc.): b2g-inbound
> > Risk to taking this patch (and alternatives if risky): very low: this whole
> > path is predicated on a new manifest option that isn't set by any existing
> > apps
> > String or IDL/UUID changes made by this patch: mozIJSSubScriptLoader
>
> Looking for a little more Dev guidance here to help with inbound testing.
> Some questions i have:
> - regression risks to the rest of the platform when this lands?
> (specifically, affected APIs around app installation, removal, etc.., other
> packaged apps that are already on the device)
Very low, and covered by existing install/update tests. We also have new tests coming up in bug 997886.
> - is WMW the only affected app that we know of for this work on 1.4? or are
> there other apps that need to be considered for 1.4?
It's partner dependant, but could be used by other apps for sure.
> - if QA needs to test WMW from marketplace, is there a sandboxed environment
> that we can pull this from? (we can certainly test sideloading, but the
> correct way is to go through the full marketplace userflow)
You don't need to test with wmw specifically. Any app the include asm.js code will be good enough. I'll provide one to QA that we can share.
> - Performance risks: do you expect this to touch any number of cold/warm
> launch hits on the app and other core apps?
No.
> - if something goes awry, can we cleanly back this patch out in isolation?
Yes.
Comment 55•11 years ago
|
||
(I thought most of these questions should be going to you anyway :)
Flags: needinfo?(luke)
Comment 56•11 years ago
|
||
sounds good. Fabrice, can you attach that asm.js test app to this bug? then anyone watching this can help out too.
when this patch a-plusses, qa will organize some folks to test the scenarios out on 1.4. Jason is already working on a testplan so we can hammer through different usecases.
Flags: needinfo?(fabrice)
Comment 57•11 years ago
|
||
(In reply to Tony Chung [:tchung] from comment #56)
> sounds good. Fabrice, can you attach that asm.js test app to this bug?
> then anyone watching this can help out too.
I think Fabrice mentioned Alon had some sample asm.js test apps I could use.
>
> when this patch a-plusses, qa will organize some folks to test the scenarios
> out on 1.4. Jason is already working on a testplan so we can hammer through
> different usecases.
Actually we should test this before uplifting. That means we should test this on trunk first.
Comment 58•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•11 years ago
|
QA Contact: jsmith
Updated•11 years ago
|
Flags: needinfo?(mbest)
Comment 59•11 years ago
|
||
Test plan is written up here:
https://docs.google.com/a/mozilla.com/spreadsheets/d/1qFct4EDx5mCt3G0n-KHxpeZztxwsNmD7Z5dmAFUs1mY/edit#gid=1588926924
I'm currently waiting on getting a reference app.
Comment 60•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #59)
> Test plan is written up here:
>
> https://docs.google.com/a/mozilla.com/spreadsheets/d/1qFct4EDx5mCt3G0n-
> KHxpeZztxwsNmD7Z5dmAFUs1mY/edit#gid=1588926924
>
> I'm currently waiting on getting a reference app.
Nick (cc'ed) has volunteered to help get you a reference app
Flags: needinfo?(nick)
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 61•11 years ago
|
||
works when installing using `navigator.mozApps.installPackage`. Boot time cut down to 20s from 50s! Doesn't work when side loading from App Manager.
Flags: needinfo?(nick)
Updated•11 years ago
|
Flags: needinfo?(fabrice)
Comment 62•11 years ago
|
||
Triage update : waiting on QA testing to be completed before uplifting on 1.4
Comment 63•11 years ago
|
||
Update on testing:
* Nick and I worked to get WMW working with the precompile manifest flag. We verified that it does work with this patch.
* I also developed two custom packaged apps making use of the precompile flag with some of Enscripten benchmarks & verified that precompilation worked correctly on install.
* I worked on trying to get a customization working with an app using the precompile manifest flag, but got blocked by bug 1001209 in the process
Next Steps
* Run a couple more tests with install flows for error cases & variant file paths
* Run a couple tests doing app updates for precompliation of JS files
I'll plan on finishing this up tomorrow.
Comment 64•11 years ago
|
||
Sorry for taking too long to finish this. App update tests look fine - I'm seeing the right files getting compiled on an update. I still haven't been able to test preinstalled apps with asm.js due to being blocked on bug 1001209, but I don't think that needs to block an uplift. I'll just test it post uplift.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Updated•11 years ago
|
blocking-b2g: backlog → 1.4?
Comment 65•11 years ago
|
||
Comment on attachment 8407794 [details] [diff] [review]
preload-asm-js.patch v6
Taking asm.js in 1.4
Attachment #8407794 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
blocking-b2g: 1.4? → backlog
Comment 66•11 years ago
|
||
FYI - Just verified that preinstalling apps with a precompile flag also works as expected.
Comment 67•11 years ago
|
||
Comment on attachment 8407794 [details] [diff] [review]
preload-asm-js.patch v6
Review of attachment 8407794 [details] [diff] [review]:
-----------------------------------------------------------------
For 1.4 marking b2g30 as approved.
Attachment #8407794 -
Flags: approval-mozilla-b2g30+
Comment 68•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/1a35ec54b068
Should we uplift bug 997886 as well?
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
status-firefox29:
--- → wontfix
status-firefox30:
--- → wontfix
status-firefox31:
--- → fixed
Flags: needinfo?(fabrice)
Comment 69•11 years ago
|
||
Unfortunately, my attempts to rebase this didn't go so well. Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4e9928bfccff
https://tbpl.mozilla.org/php/getParsedLog.php?id=38882336&tree=Mozilla-B2g30-v1.4
Keywords: branch-patch-needed
Assignee | ||
Comment 70•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #68)
> https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/1a35ec54b068
>
> Should we uplift bug 997886 as well?
That would be a nice touch, but I won't fight for that.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #69)
> Unfortunately, my attempts to rebase this didn't go so well. Backed out for
> bustage.
> https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4e9928bfccff
>
> https://tbpl.mozilla.org/php/getParsedLog.php?id=38882336&tree=Mozilla-B2g30-
> v1.4
ha... I need to clone a 1.4 tree to help :(
Flags: needinfo?(fabrice)
Assignee | ||
Comment 71•11 years ago
|
||
Updated•11 years ago
|
Keywords: branch-patch-needed
Comment 72•11 years ago
|
||
Fabrice backed this out for m-oth failures on all platforms.
Comment 73•11 years ago
|
||
Nevermind me, I mixed up the changesets. This is still landed.
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•