Skip to content

WASM cache: POC - #36672

Closed
mejedi wants to merge 7 commits into
nodejs:mainfrom
mejedi:wasm-cache-POC
Closed

mejedi wants to merge 7 commits into
nodejs:mainfrom
mejedi:wasm-cache-POC

Conversation

@mejedi

@mejedi mejedi commented Dec 29, 2020 •

Copy link
Copy Markdown

This is a POC for compiled WebAssembly module cache proposal (#36671).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Dec 29, 2020
@targos targos added the wasm Issues and PRs related to WebAssembly. label Dec 29, 2020
@mejedi mejedi changed the title WASM cache: WIP WASM cache: POC Dec 29, 2020

@devsnek devsnek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this seems better suited to userland. Additionally, compileStreaming has long been a source of contention and if you wanted to use that (which there is no point in doing in this case at least, since you aren't streaming anything, so WebAssembly.compile would be fine), you should probably split that into its own PR.

@devsnek devsnek added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 29, 2020
@mejedi

mejedi commented Dec 29, 2020

Copy link
Copy Markdown
Author

@devsnek

I think this seems better suited to userland.

There are some technical difficulties. Please check the overview below. Personally, I don't see how this could be made into a standalone package (I guess this is what you are suggesting), could you please elaborate?

Short version

  1. The only way to deserialise a WASM module in public V8 API is to use the infrastructure supporting .compileStreaming. @addaleax

  2. WebAssembly.compileStreaming is not available, unless WasmStreamingCallback was registered with V8 prior to startup. Makes it challenging to implement the feature as a standalone module.

Compiled WASM module cache is paramount for a good WASM experience, I wonder why you are convinced this should be standalone. It's hard to claim that WASM is out of scope for Node.js, since WASI is already in core.

WebAssembly.compileStreaming workflow

  • Embeder registers a WASM streaming callback. This has to be done prior to V8 startup, otherwise compileStreaming won't be available.
  • compileStreaming(input) will pass the input (resolving the Promise if necessary) to the registered callback, along with WasmStreaming object.
  • The callback should verify that input is something it could stream from, V8 itself doesn't check the input.
  • The callback uses WasmStreaming object to feed the module in:
    • .SetClient(client) — request a notification once the Module is compiled and optimised
    • .SetUrl(url)
    • .SetCompiledModuleBytes(bytes) — pass the previously compiled and optimised module
    • .OnBytesReceived(bytes) — transfer a chunk
    • .Finish()
    • or .Abort()
  • Once .Finish() was called, V8 starts Tier 1 compiler.
  • When Tier 1 completes, the Promise returned from .compileStreaming resolves, and the Module becomes available. It's not optimised yet.
  • Tier 2 continues in the background.
  • Tier 2 completes, V8 transparently replaces the Module code with the optimised version.
  • A client, if registered with WasmStreaming.SetClient, is notified. It could extract the optimised compiled module bytes and save it to cache. Only optimised version is cacheable.

Concerning WebAssembly.compileStreaming (sidenote)

Node.js doesn't expose .compileStreaming at the moment. As a side effect of the proposed changes, it gets exposed. This is probably wrong. According to the spec, is is taking a Request object. There's no such thing in Node.js.

@devsnek

devsnek commented Dec 29, 2020

Copy link
Copy Markdown
Member

I would imagine V8 could be modified to support caching in WebAssembly.compile. In that case, the callback wouldn't need to be registered before the context is created, and you could do it from userland.

Comment thread src/node_wasm_streaming.cc Outdated
Comment thread src/node.h
Comment thread src/node_wasm_streaming.cc Outdated
Comment thread src/node_wasm_streaming.cc Outdated
Comment thread src/node_wasm_streaming.cc Outdated
Comment thread src/node_wasm_streaming.cc Outdated
Comment thread lib/wasm/cache.js Outdated
Comment thread lib/wasm/cache.js
break;
}

let cacheDir = path.resolve(os.homedir(), cacheSubdir);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see where @devsnek’s concerns are coming from – this is mixing some pretty specific I/O details with low-level functionality.

Ideally, we’d expose an API that doesn’t do any I/O, and lets the user fill that part in by specifying their own callback(s?) for that – how would that sound to you?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax Thank you for taking look! I have a meta question: what is the best channel to discuss the feature scope / public facing APIs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure, but probably this PR itself – although I can see that this might turn into a long discussion…

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax

Ideally, we’d expose an API that doesn’t do any I/O, and lets the user fill that part in by specifying their own callback(s?)

We could do it differently:

  1. either provide a low-level WebAssembly deserialise primitive, let the user build on top,
  2. or build a higher-level api with the caching largely taken care of,
  3. something in-between?

Personally, I believe 2 is more valuable. Pros:

  • implementing caching properly is non-trivial (have to detect if the source file was changed and handle race conditions while updating the cache to name a few challenges);
  • cache files end up in predictable locations, it's easy to reclaim disk space and troubleshoot issues;
  • doesn't expose implementation details that are likely to change.

Furthermore, sometimes the user can't be expected to write custom caching code — consider require-ing a WASI module or Emscripten-generated wrapper.

@kripken

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your first two Pros are really Pros for option 1? Implementing caching properly is non-trivial – hence, it’s best if a userland package addresses this, instead of adding the complexity to Node.js core and tying updates to our release schedule. And while cache files may end up in predictable locations, what those locations are may depend strongly on different user’s needs – also a good point for moving this into userland modules.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it’s best if a userland package addresses this, instead of adding the complexity to Node.js core and tying updates to our release schedule

These are valid points. However It looks like the core itself could benefit from the cache due to this feature
https://nodejs.org/api/all.html#esm_wasm_modules

FYI, concerning the cache there’s some prior art in Python we could borrow from.

https://www.python.org/dev/peps/pep-3147/

https://docs.python.org/3/whatsnew/3.8.html#parallel-filesystem-cache-for-compiled-bytecode-files

Personally, I’m mostly interested in Emscripten use case. Having a minimal low-level api in core will work for me. So if you feel strongly about it let’s do it this way.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax @devsnek

What if we expose WasmStreaming to userland without inventing new interfaces?

class Foo { ... }
require('v8').registerWebAssemblyStreamer(Foo, (foo, wasmStreaming) => {
  wasmStreaming.setCompiledModuleBytes(...);
  wasmStreaming.setUrl(...);
  wasmStreaming.onBytesReceived(...);
  wasmStreaming.finish();
});
WebAssembly.compileStreaming(new Foo(...));

mejedi and others added 6 commits January 4, 2021 15:36
Co-authored-by: Anna Henningsen <github@addaleax.net>
 * SHOULD_SET_WASM_STREAMING_CALLBACK flag introduced, normally unset.

 * .wasm_streaming_callback field accessed iff the flag is set, hence
   backwards-compatible.

 * Node's WasmStreamingCallback exposed so that embedder could have a
   custom callback without disabling Node's features built around the
   streaming callback.
@mejedi
mejedi requested review from addaleax and devsnek January 8, 2021 03:54
@tniessen

Copy link
Copy Markdown
Member

See #36671 (comment).

@mejedi mejedi closed this by deleting the head repository Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. semver-major PRs that contain breaking changes and should be released in the next major version. wasm Issues and PRs related to WebAssembly.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants