Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimise memory view getters #2886

Merged
merged 4 commits into from
May 4, 2022
Merged

Conversation

Liamolucko
Copy link
Collaborator

While working on RReverser/serde-wasm-bindgen#34, I noticed that a significant amount of time was spent getting memory views. Most of that time was spent in the getter for WebAssembly.Memory.buffer, so I've changed the memory view getters to check for invalidation without comparing to the wasm memory, by checking if the length is 0 (which indicates the buffer being detached).

wasm-bindgen benchmarks:

Benchmark Before After Change
Access Node.nodeType with final 364/s 383/s +5.22%
Access Node.nodeType with structural 1,166/s 1,189/s +1.97%
Call a custom JS class Foo.bar method with final 6,248/s 6,189/s -0.94%
Call a custom JS class Foo.bar method with structural 6,642/s 6,667/s +0.38%
Pass ascii_small to/from wasm-bindgen 1,281,297/s 1,823,742/s +43.04%
Pass ascii_medium to/from wasm-bindgen 1,231,065/s 1,723,498/s +40.00%
Pass ascii_number to/from wasm-bindgen 1,161,427/s 1,591,248/s +37.01%
Pass ascii_date to/from wasm-bindgen 918,950/s 1,138,144/s +23.85%
Pass ascii_url to/from wasm-bindgen 899,162/s 1,111,610/s +23.63%
Pass ascii_link to/from wasm-bindgen 864,511/s 1,105,580/s +27.89%
Pass unicode to/from wasm-bindgen 351,071/s 390,481/s +11.23%

serde-wasm-bindgen benchmarks (not including RReverser/serde-wasm-bindgen#34):

Benchmark Before After Change
Parse canada 44.01 ms/iter 34.52 ms/iter -21.56%
Parse citm_catalog 10.32 ms/iter 8.78 ms/iter -14.92%
Parse twitter 8.99 ms/iter 8.23 ms/iter -8.45%
Serialize canada 11.74 ms/iter 11.59 ms/iter -1.28%
Serialize citm_catalog 7.51 ms/iter 7.51 ms/iter -0.00%
Serialize twitter 14.54 ms/iter 14.07 ms/iter -3.23%

Getting the buffer of a `WebAssembly.Memory` seems to have some overhead to it, so instead check if the existing view is 0 bytes long, which happens when they become detached after the memory is grown.

I'm pretty sure that they'll never be zero bytes long otherwise; but even if they are, it's not the end of the world to recreate the view every time in that rare circumstance.
With the last commit, if `init` was called twice on web/no-modules, the memory views would be stuck pointing to the first copy of memory, since it never actually gets detached; this fixes that by manually resetting them.

This also allows eliminating the null check in the memory view getters, but it has practically no effect on performance.
@RReverser
Copy link
Member

Great work!

@alexcrichton
Copy link
Contributor

Nice wins, and thanks!

@alexcrichton alexcrichton merged commit f26e30b into rustwasm:main May 4, 2022
@Liamolucko Liamolucko deleted the optimise branch May 4, 2022 20:17
Liamolucko added a commit to Liamolucko/wasm-bindgen that referenced this pull request Jun 27, 2022
In the bundler target, there's a circular dependency between the bindings and the wasm module. That means that the wasm module's exports aren't available at the top level. In rustwasm#2886, I didn't realise that and made the memory views be initialised at the top level, which resulted in an error from the wasm module's memory not being available yet.

This fixes that by lazily initialising the memory views like they were before rustwasm#2886, except that they're reset to uninitialised in `init` to make sure they're updated if it's called multiple times (the reason I made them be immediately initialised in the first place).
ranile pushed a commit that referenced this pull request Jun 27, 2022
In the bundler target, there's a circular dependency between the bindings and the wasm module. That means that the wasm module's exports aren't available at the top level. In #2886, I didn't realise that and made the memory views be initialised at the top level, which resulted in an error from the wasm module's memory not being available yet.

This fixes that by lazily initialising the memory views like they were before #2886, except that they're reset to uninitialised in `init` to make sure they're updated if it's called multiple times (the reason I made them be immediately initialised in the first place).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants