Fully remove asm.js caching

RESOLVED FIXED in Firefox 68

Status

()

enhancement
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Assignee

Description

4 months ago

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.

Assignee

Updated

4 months ago
Depends on: 1516697
Assignee

Comment 1

4 months ago
Posted patch rm-asmjscache (obsolete) — Splinter Review

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(-)

Assignee: nobody → luke
Attachment #9041978 - Flags: review?(jvarga)
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!
Attachment #9041978 - Flags: review?(jvarga) → review?(shes050117)
Assignee

Comment 3

4 months ago
Posted patch rm-asmjscacheSplinter Review

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

Attachment #9041978 - Attachment is obsolete: true
Attachment #9041978 - Flags: review?(shes050117)
Attachment #9042200 - Flags: review?(shes050117)
Assignee

Comment 4

4 months ago

nestedShell() can also go.

Attachment #9042207 - Flags: review?(bbouvier)
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`
Attachment #9042207 - Flags: review?(bbouvier) → review+
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).
Attachment #9042200 - Flags: feedback?(bugmail)
Assignee

Comment 7

4 months ago

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 :)

Assignee

Updated

4 months ago
Attachment #9042200 - Flags: review?(shes050117)

(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).

Assignee

Comment 9

4 months ago

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

Attachment #9042525 - Flags: review?(bbouvier)

(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! :)

(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.

Attachment #9042525 - Flags: review?(bbouvier) → review+
Assignee

Comment 12

3 months ago

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

Flags: needinfo?(shes050117)

(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.

Flags: needinfo?(shes050117)
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
Attachment #9042200 - Flags: feedback?(bugmail) → feedback+
Assignee

Comment 15

3 months ago

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

Depends on: 1423917
Keywords: leave-open

Comment 16

3 months ago
Pushed by lwagner@mozilla.com:
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

3 months ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a08b6d68e70
Remove unused import from jit_tests.py (r=me)

Comment 19

3 months ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aca1805a96a
Fix EXIT_FAILURE typos in JS shell main() (r=me)
Assignee

Comment 20

3 months ago

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.

Attachment #9043326 - Flags: review?(bbouvier)
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?
Attachment #9043326 - Flags: review?(bbouvier) → review+
Assignee

Comment 24

3 months ago

(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

3 months ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10030bd8ac3d
Baldr: another follow-up simplification from asm.js caching removal (r=bbouvier)

Comment 26

3 months ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e772a34b885e
Baldr: fix implicit constructor warning (r=me)
Assignee

Comment 29

2 months ago

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

Assignee

Updated

2 months ago
Keywords: leave-open

Comment 31

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.