1520931 - Fully remove asm.js caching

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Luke Wagner [:luke]]

asm.js caching was making undue work for the parser utf-8 changes (bug 1516697 comment 14), so, since asm.js caching is going away before too long anyway, I r+'d turning it off for good.

This bug is about sweeping all the now-dead remains in SpiderMonkey and Gecko.

Comment 1

[Luke Wagner [:luke]]

Attachment: rm-asmjscache

Unlike bug 1469395, where we can't simply wipe out wasm IDB entries, I think we can simply wipe out 'asmjs' subdirs when we see them. I wasn't sure quite what the right cut point was in dom/quota, so I tried to pick one that would be noisy if we missed a spot.

30 files changed, 35 insertions(+), 2866 deletions(-)

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9041978 [details] [diff] [review]
rm-asmjscache

I'm redirecting review to Tom since he's most active in this area of the code right now.  (And would be the one to have to fix things if QuotaManager init broke! ;)  Tom, of course feel free to add Jan or myself as follow-up reviewers if there's anything you'd like a second or third set of eyes on!

Comment 3

[Luke Wagner [:luke]]

Attachment: rm-asmjscache

Oops, I forgot to include one local change in the last patch.

Comment 4

[Luke Wagner [:luke]]

Attachment: rm-nested-shell

nestedShell() can also go.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9042207 [details] [diff] [review]
rm-nested-shell

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

Fare thee shell.

::: js/src/shell/js.cpp
@@ -5717,4 @@
>  static Vector<const char*, 4, js::SystemAllocPolicy> sPropagatedFlags;
>  
>  #if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64)
>  static bool PropagateFlagToNestedShells(const char* flag) {

Can you rename this PropagateFlagToChildProcesses, or something like this, please? (since it's still used for wasm serialization)

::: js/src/tests/lib/jittests.py
@@ +341,5 @@
>                   "const scriptdir={}".format(js_quote(quotechar, scriptdir_var))]
>  
>          # We may have specified '-a' or '-d' twice: once via --jitflags, once
>          # via the "|jit-test|" line.  Remove dups because they are toggles.
> +        cmd = prefix + []

nit: can just be `cmd = prefix`

Comment 6

[Tom Tung [:tt, :ttung]]

Comment on attachment 9042200 [details] [diff] [review]
rm-asmjscache

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

Looks good and thank you! However, though this patch can remove the asmjs client from QuotaManager, I believe we can do that better.

So, InitializeOrigin() is executed for all origins in every initialization attempt and MaybeUpgradeClients() is executed in all the upgrades by default.

If we can do that in an upgrade, we can check and remove folders at once. On Bug 1423917, I have patches for an upgrade and current it hasn't been landed. I think we can do that togother.

Andrew, do you think it's possible to add the remove asmjs logic in QuotaManager in bug 1423917? If it might create tons of underlying issues (e.g. if either one has problem, we need to back out two of them), then I reckon maybe we can add another minor upgrade here.

::: dom/quota/ActorsParent.cpp
@@ +3917,5 @@
>        RECORD_IN_NIGHTLY(statusKeeper, NS_ERROR_UNEXPECTED);
>        CONTINUE_IN_NIGHTLY_RETURN_IN_OTHERS(NS_ERROR_UNEXPECTED);
>      }
>  
> +    if (Client::IsRemoved(leafName)) {

This function would run every time when QuotaManager initializes storage, so I would recommend removing them in an upgrade at once. Considering it's a backward-compatible change, it should be a minor upgrade. Bug 1423917 has patches for a minor upgrade, it's basically just removing the obsolete origins, and it hasn't landed. I think we could combine that with removing asmjs client together and becomes one upgrade.

[1] https://searchfox.org/mozilla-central/rev/490ab7f9b84570573a49d7fa018673ce0d5ddf22/dom/quota/ActorsParent.cpp#157-158

@@ +3920,5 @@
>  
> +    if (Client::IsRemoved(leafName)) {
> +      rv = file->Remove(/* aRecursive */ true);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        REPORT_TELEMETRY_INIT_ERR(kExternalError, Ori_Remove);

If we decide to remove the folder with a minor upgrade, then it's not needed. If we don't, this needs to update the Histogram [1] as well.

[1] https://searchfox.org/mozilla-central/rev/490ab7f9b84570573a49d7fa018673ce0d5ddf22/toolkit/components/telemetry/Histograms.json#14584-14660

@@ +6669,5 @@
>          }
>          continue;
>        }
>  
> +      if (Client::IsRemoved(leafName)) {

If we do remove the asmjs client in a minor upgrade, then this won't be needed.

@@ +8573,5 @@
>        }
>        continue;
>      }
>  
> +    if (Client::IsRemoved(leafName)) {

We run into here in every upgrade, I guess we only need to do that once (In the specific minor upgrade).

Comment 7

[Luke Wagner [:luke]]

I like the sound of your solution! By any chance, while we're cleaning house, could that same upgrade also fix bug 1469395 (by simply blowing away origins that contain wasm-in-IDB)? Then it'll be as if asm.js/wasm had never intruded into dom/quota or dom/indexedb :)

Comment 8

[Tom Tung [:tt, :ttung]]

(In reply to Luke Wagner [:luke] from comment #7)

I like the sound of your solution! By any chance, while we're cleaning house, could that same upgrade also fix bug 1469395 (by simply blowing away origins that contain wasm-in-IDB)? Then it'll be as if asm.js/wasm had never intruded into dom/quota or dom/indexedb :)

I'd say doing that in the indexedDB upgrades for its database.

It's the same concept but there is a small difference since bug 1469395 needs to access databases to see if there is a wasm in indexedDB or not. So, I guess it's better to have a lazy checking in the upgrades for the indexedDB origins itself. So that it won't block a lot of time by opening every database during a QuotaManager upgrade.

For more information, QuotaManager upgrade will block the process of initializing origins until it finishes so that we don't want to do something really long if it's possible.

The lazy checking I mentioned is that only blowing stuff until the first accessing the database for indexedDB of that origin. And, we check and blow the wasm for indexedDB only for the first time (since it's an upgrade for indexedDB database).

Comment 9

[Luke Wagner [:luke]]

Attachment: rm-asmjscache-hooks

Independent of the dom/asmjscache changes, we can still remove the JS API hooks which enables landing the rm-nested-shell patch.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Tom Tung [:tt, :ttung] from comment #6)

Andrew, do you think it's possible to add the remove asmjs logic in
QuotaManager in bug 1423917? If it might create tons of underlying issues
(e.g. if either one has problem, we need to back out two of them), then I
reckon maybe we can add another minor upgrade here.

It seems like we could do the removal as part of that upgrade, yes. It doesn't seem high-risk, but we would want some test coverage. Thankfully, I think you've already created an excellent bunch of test/test infra for this! :)

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Luke Wagner [:luke] from comment #7)

I like the sound of your solution! By any chance, while we're cleaning house, could that same upgrade also fix bug 1469395 (by simply blowing away origins that contain wasm-in-IDB)? Then it'll be as if asm.js/wasm had never intruded into dom/quota or dom/indexedb :)

But to be clear re:WASM-in-IDB cleanup, I do not think we want to do that as part of bug 1423917.

Comment 12

[Luke Wagner [:luke]]

After bug 1423917 lands, what should be the plan in this bug for full removal of dom/asmjscache?

Comment 13

[Tom Tung [:tt, :ttung]]

(In reply to Luke Wagner [:luke] from comment #12)

After bug 1423917 lands, what should be the plan in this bug for full removal of dom/asmjscache?

The attachment 9042200 [details] [diff] [review] should be modified a little bit (removing the part for deleting asmjs directory). I am going to put my comment on it so that it'd be much more clear.

Comment 14

[Tom Tung [:tt, :ttung]]

Comment on attachment 9042200 [details] [diff] [review]
rm-asmjscache

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

Bug 1423917 P9 only removes asmjs directories from users' profile and stop storing new thing by asmjs, so the same things here can be removed. And, we can remove the asmjs from the code base.

Aside from that, everything looks great and they are needed. I guess I should check the patch one more time after the new change for avoiding I explained something wrong or unclear. Thanks!

Please let me know if there is still something unclear. 

Note that removing the asmjs entry from the Client.h enum and the changes for registering client on ActorsParent.cpp are still needed in these patches.

::: dom/quota/ActorsParent.cpp
@@ +3927,5 @@
> +      }
> +
> +      continue;
> +    }
> +

This block can be removed (line 3921-3931)

@@ +6674,5 @@
> +        rv = file->Remove(/* recursive */ true);
> +        if (NS_WARN_IF(NS_FAILED(rv))) {
> +          return rv;
> +        }
> +      }

Also this block (line 6673-6678)

@@ +8575,5 @@
>      }
>  
> +    if (Client::IsRemoved(leafName)) {
> +      continue;
> +    }

Also this block (line 8577-8579)

::: dom/quota/Client.h
@@ +64,5 @@
>      switch (aType) {
>        case IDB:
>          aText.AssignLiteral(IDB_DIRECTORY_NAME);
>          break;
>  

This one is done in bug 1423917 as well

@@ +92,5 @@
>  
> +  static bool IsRemoved(const nsAString& aText) {
> +    return aText.EqualsLiteral(REMOVED_ASMJSCACHE_DIRECTORY_NAME);
> +  }
> +

This one can be removed since we will have IsDeprecatedClient on bug 1423917

@@ +104,5 @@
>      } else if (CachedNextGenLocalStorageEnabled() &&
>                 aText.EqualsLiteral(LS_DIRECTORY_NAME)) {
>        aType = LS;
>      } else {
> +      MOZ_RELEASE_ASSERT(!IsRemoved(aText));

This one can be removed as well

Comment 15

[Luke Wagner [:luke]]

Ok, thanks! I'll wait for P9 to land and then rebase this patch.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94cf160df1de
Remove asm.js cache hooks JS API (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b01a3234d69
Remove nestedShell() testing function (r=bbouvier)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a08b6d68e70
Remove unused import from jit_tests.py (r=me)

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/94cf160df1de
https://hg.mozilla.org/mozilla-central/rev/7b01a3234d69
https://hg.mozilla.org/mozilla-central/rev/8a08b6d68e70

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aca1805a96a
Fix EXIT_FAILURE typos in JS shell main() (r=me)

Comment 20

[Luke Wagner [:luke]]

Attachment: simplify-a-bit-more

I noticed another little bit of simplification in AsmJS.cpp that lets us avoid this optional route whereby the LinkData can be extracted along with the Module (but only if !debugging). The other use of this path (shell serialization) can be modified to use the OptimizedEncodingListener, which actually (slightly) increases its testing coverage.

Some of the other stylistic changes are due to clang-format, not me. I guess we don't have automatic hooks yet so I'm just the first to touch these files with clang-format.

Comment 21

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/9aca1805a96a

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9043326 [details] [diff] [review]
simplify-a-bit-more

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

Nice trick.

::: js/src/wasm/WasmJS.cpp
@@ +456,5 @@
> +  // MOZ_STACK_CLASS means these can be nops.
> +  MozExternalRefCountType MOZ_XPCOM_ABI AddRef() override { return 0; }
> +  MozExternalRefCountType MOZ_XPCOM_ABI Release() override { return 0; }
> +
> +  DebugOnly<bool> called = false;

nit: DebugOnly is not 0 bytes in non-debug builds, so we use #ifdef DEBUG for class members.

@@ +461,5 @@
> +  Bytes* serialized;
> +  SerializeListener(Bytes* serialized) : serialized(serialized) {}
> +
> +  void storeOptimizedEncoding(const uint8_t* bytes, size_t length) override {
> +    called = true;

maybe MOZ_ASSERT(!called) before this?

@@ +483,1 @@
>    compileArgs->ionEnabled = true;

HasCachingSupport() will return true if Cranelift is supported but not Ion (we're not there yet, but some day...). Also if both optimizing compilers are supported, the user's choice (about:config pref) might not be respected in this case, because we don't have any cx hanging around. (This makes testing Cranelift a bit hard, but as soon as we display which engine has been used for compilation with the verbose pref, at least it'll be clear). So we might need something like:

compileArgs->ionEnabled = IonCanCompile();
#ifdef ENABLE_WASM_CRANELIFT
compileArgs->craneliftEnabled = !IonCanCompile() && CraneliftCanCompile(); // prefer Ion over Cranelift by default.
#endif

@@ +487,3 @@
>    UniqueChars error;
>    UniqueCharsVector warnings;
>    UniqueLinkData linkData;

This is unused and can be removed.

@@ +495,5 @@
>    }
>  
>    MOZ_ASSERT(module->code().hasTier(Tier::Serialized));
> +  MOZ_ASSERT(listener.called);
> +  return !listener.serialized->empty();

This is correct because even an empty module would have e.g. length headers for serialized vectors, so the serialized byte vector is never empty, right?

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9aca1805a96a

Comment 24

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #22)

• DebugOnly<bool> called = false;

nit: DebugOnly is not 0 bytes in non-debug builds, so we use #ifdef DEBUG
for class members.

You're right in general but given that this is a shell testing function and that it's a MOZ_STACK_CLASS (and thus may be SRA'd anyhow), it didn't seem worth the visual ugliness to me.

So we might need something like:

compileArgs->ionEnabled = IonCanCompile();
#ifdef ENABLE_WASM_CRANELIFT
compileArgs->craneliftEnabled = !IonCanCompile() && CraneliftCanCompile();
// prefer Ion over Cranelift by default.
#endif

Since IonCanCompile() will always be true when CraneliftCanCompile() is true, I think adding this code won't help. I think the real fix is to propagate the --wasm-ion / --wasm-cranelift flags from parent to child so that WasmCompileAndSerialize() in the child can use these flags to pick the right compiler. I think that would be a follow-up, though, when we want to start testing cranelift serialization.

This is correct because even an empty module would have e.g. length headers
for serialized vectors, so the serialized byte vector is never empty, right?

Yep!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10030bd8ac3d
Baldr: another follow-up simplification from asm.js caching removal (r=bbouvier)

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e772a34b885e
Baldr: fix implicit constructor warning (r=me)

Comment 27

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/10030bd8ac3d
https://hg.mozilla.org/mozilla-central/rev/e772a34b885e

Comment 28

[Luke Wagner [:luke]]

Attachment: Bug 1520931 - Remove dom/asmjscache (r=ttung)

Comment 29

[Luke Wagner [:luke]]

IIUC, the prerequisite bits in bug 1423917 have landed so we can now go ahead with removing the whole dom/asmjscache?

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e86fd84509e
Remove dom/asmjscache (r=ttung)

Comment 31

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/3e86fd84509e