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

[jsapi] limit tests optimization #972

Closed
xtuc opened this issue Feb 21, 2019 · 11 comments
Closed

[jsapi] limit tests optimization #972

xtuc opened this issue Feb 21, 2019 · 11 comments

Comments

@xtuc
Copy link
Contributor

xtuc commented Feb 21, 2019

The get_buffer is slow, the generation of the Wasm module can be > 10min. Of course the execution itself is up to the engine, but here are my suggestions to optimize the test suite a bit:

  • Split it in multiple files to allow multithreading
  • Cache the buffers (min, limit and limit + 1) instead of generating them for each test.

What do you think?

@binji
Copy link
Member

binji commented Feb 22, 2019

Caching the get_buffer results sounds good to me. I'd prefer to keep it one file if we can, but if it's still really slow I'd be OK separating it out. @Ms2ger thoughts?

@Ms2ger
Copy link
Collaborator

Ms2ger commented Feb 25, 2019

Caching sounds great. It might be interesting to see if any particular subtests are slow, or if they're all equally slow

@xtuc
Copy link
Contributor Author

xtuc commented Feb 25, 2019

Ok, I will send a PR with that caching and post my benchmarks here.

@xtuc
Copy link
Contributor Author

xtuc commented Feb 25, 2019

By the way, is there a reason the limits.any.js testsuite isn't in WPT?

xtuc added a commit to xtuc/spec that referenced this issue Feb 25, 2019
Caches the generated Wasm buffer instead of re-generating
it for each test.

Refs WebAssembly#972
@xtuc
Copy link
Contributor Author

xtuc commented Feb 25, 2019

Here are the results (in ms)

types: get_buffer 1, 4.990000
types: get_buffer 1000000, 33252.488000
types: get_buffer 1000001, 42535.581000
functions: get_buffer 1, 1.442000
functions: get_buffer 1000000, 360336.039000
functions: get_buffer 1000001, 504540.306000
imports: get_buffer 1, 1.248000
imports: get_buffer 100000, 8153.456000
imports: get_buffer 100001, 1802.223000
exports: get_buffer 1, 1.441000
exports: get_buffer 100000, 16557.174000
exports: get_buffer 100001, 24683.390000
globals: get_buffer 1, 1.151000
globals: get_buffer 1000000, 83019.206000
globals: get_buffer 1000001, 160315.990000
data segments: get_buffer 1, 11.270000
data segments: get_buffer 100000, 8152.780000
data segments: get_buffer 100001, 2339.004000
initial declared memory pages: get_buffer 1, 0.432000
initial declared memory pages: get_buffer 65536, 0.089000
initial declared memory pages: get_buffer 65537, 0.051000
maximum declared memory pages: get_buffer 1, 0.128000
maximum declared memory pages: get_buffer 65536, 0.062000
maximum declared memory pages: get_buffer 65537, 0.053000
initial imported memory pages: get_buffer 1, 0.619000
initial imported memory pages: get_buffer 65536, 0.162000
initial imported memory pages: get_buffer 65537, 0.083000
maximum imported memory pages: get_buffer 1, 0.206000
maximum imported memory pages: get_buffer 65536, 0.131000
maximum imported memory pages: get_buffer 65537, 0.086000
function size: get_buffer 2, 0.384000
function size: get_buffer 7654321, 58292.331000
function size: get_buffer 7654322, 91006.932000
function locals: get_buffer 1, 1.182000
function locals: get_buffer 50000, 0.280000
function locals: get_buffer 50001, 0.132000
function params: get_buffer 1, 0.298000
function params: get_buffer 1000, 4.214000
function params: get_buffer 1001, 1645.167000
function params+locals: get_buffer 1, 0.458000
function params+locals: get_buffer 49998, 0.187000
function params+locals: get_buffer 49999, 0.141000
function returns: get_buffer 0, 0.244000
function returns: get_buffer 1, 0.099000
function returns: get_buffer 2, 0.065000
initial table size: get_buffer 1, 0.298000
initial table size: get_buffer 10000000, 0.090000
initial table size: get_buffer 10000001, 0.054000
maximum table size: get_buffer 1, 0.313000
maximum table size: get_buffer 10000000, 0.145000
maximum table size: get_buffer 10000001, 0.073000
element segments: get_buffer 1, 0.574000
element segments: get_buffer 10000000, 3321793.161000
element segments: get_buffer 10000001, 4558369.666000
tables: get_buffer 0, 4.515000
tables: get_buffer 1, 10720.861000
tables: get_buffer 2, 1.533000
memories: get_buffer 0, 0.203000
memories: get_buffer 1, 0.246000
memories: get_buffer 2, 0.167000

Only one test is very slow:

element segments: get_buffer 10000000, 3321793.161000 (55min)
element segments: get_buffer 10000001, 4558369.666000 (75min)

@Ms2ger
Copy link
Collaborator

Ms2ger commented Feb 25, 2019

Thanks for checking! That code does seem somewhat expensive; maybe we can optimize the code a bit.

The test isn't in wpt yet because it's intermittently timing out in web-platform-tests/wpt#14766; I'm hoping that your work here will help with that.

@xtuc
Copy link
Contributor Author

xtuc commented Feb 25, 2019

That's not surprising I guess, I will investigate further.

binji pushed a commit that referenced this issue Feb 26, 2019
Caches the generated Wasm buffer instead of re-generating
it for each test.

Refs #972
@xtuc
Copy link
Contributor Author

xtuc commented Mar 1, 2019

I just noticed that browers will refuse to synchronously compile the big Wasm buffers (> 4KB):

test(() => {
new WebAssembly.Module(buffer_with_min);
}, `Compile ${name} mininum`);
test(() => {
new WebAssembly.Module(buffer_with_limit);
}, `Compile ${name} limit`);
test(() => {
assert_throws(new WebAssembly.CompileError(), () => new WebAssembly.Module(buffer_with_limit_plus_1));
}, `Compile ${name} over limit`);

@lars-t-hansen
Copy link

Isn't that just a limitation in Chrome? So far as I know, Firefox will synchronously compile any size buffer.

@xtuc
Copy link
Contributor Author

xtuc commented Mar 4, 2019

@lars-t-hansen yes I think so, I believe JSC would compile the buffer too.

@xtuc
Copy link
Contributor Author

xtuc commented Mar 6, 2019

Actually, this issue might be specific to v8/Chromium. I can get the whole testsuite to run under 10min (v8's --verify-heap is slowing it down heavily).

Let's close this for now.

@xtuc xtuc closed this as completed Mar 6, 2019
Honry pushed a commit to Honry/simd that referenced this issue Oct 19, 2019
Caches the generated Wasm buffer instead of re-generating
it for each test.

Refs WebAssembly/spec#972
binji added a commit to WebAssembly/threads that referenced this issue Jan 17, 2020
* [spec] Fix definition of littleendian (#968)

* Fix some test function names that got out of sync with the recent instruction renaming (#964)

* [interpreter] Fix broken link in README (#970)

* [test] More tests for missing operand missing -Part2 (#950)

* [test] More tests for missing operands -Part3 (#971)

* [test] Move tests in typecheck.wast to appropriate files (#973)

* [jsapi] update limits.any.js (#975)

Caches the generated Wasm buffer instead of re-generating
it for each test.

Refs WebAssembly/spec#972

* [jsapi] Fix typo in limits.any.js (#978)

mininum -> minimum

* [test] Add binary test for local count of zero (#980)

* [jsapi] Update wasm-module-builder.js

This speeds up the WasmModuleBuilder enormously. It follows this V8-side
CL: https://crrev.com/c/1508352
It also adopts a few other minor changes to the wasm-module-builder.js
file from the v8 side.

* [jsapi] Fix tests after wasm-module-builder.js update

Updating the wasm-module-builder.js broke the jsapi tests. I was under
the impression that travis would run them, but it seems like this is
not the case. Rolling the updated version to v8 failed then:
https://crrev.com/c/1516684

This fixes two things:
1) {toBuffer} returns a Uint8Array as before, not an ArrayBuffer,
2) {addExplicitSection} expects a byte sequence, hence pass the
   truncated buffer instead of the {Binary} object.

* Fix store handling in 'instantiate a WebAssembly module'. (#981)

The store variable was not defined before it was used, and the result was not
saved.

* Editorial: Stop breaking lists to insert a note.

* Editorial: Extract a "read the imports" algorithm.

* Editorial: Extract a "create an instance object" algorithm.

* Editorial: Remove an unnecessary local variable in "create an instance object".

* Editorial: Extract an "instantiate the core of a WebAssembly module" algorithm.

* Editorial: Extract an "asynchronously instantiate a WebAssembly module" algorithm.

* Fix `if` instruction type in index (#987)

Fixes #986.

* Normative: Read the imports synchronously in WebAssembly.instantiate(Module).

* Update index.bs

* Update index.bs

* Update index.bs

* Update index.bs

* Update index.bs

* Update index.bs

* Update index.bs

* Reject the promise if reading the imports fails.

I missed this in #988.

* [test] Check for "unreachable" consistently (#992)

The full error message generated by the interpreter is "unreachable executed", but all of the other tests check for just "unreachable".

* [interpreter] Fix in JS conversion

* [test] Add test when start function traps (#994)

The start function is invoked after the store has been modified. So if a
start function traps, all data and element segments will have been
copied into the memory and table respectively. This means that functions
can be invoked on this instance, even though the instance is not
available to the embedder directly.

* [interpreter] Check argument types on invoke

* Update instructions.rst (#995)

use `local.get` name instead of `get_local` which isn't used elsewhere.

* [test] Add dedicated test for memory.size (#997)

* [spec] Add paragraph break for more emphasis (#1001)

* [test] Alignment and offset with overlong leb128 (#998)

* [spec] More precise Unicode terminology (#1002)

* [spec] Pre/post-conditions and some renamings in embedding interface (#1003)

* [spec] Work around Sphinx/Latex issue (#1004)

* [spec] Fix off-by-one error in table grow size limit (#1005)

* [interpreter] Group digits with '_' when printing numbers (#1006)

* [spec] Fix typo (#1011)

* Fix bikeshed build (#1014)

A recent change caused the generated html to have a sequence that looked
like `{{abkasdfadsf}}`, which bikeshed misinterprets as a WebIDL markup
shorthand. Since we never use that shorthand in the core spec, we can
disable it.

Since bikeshed thought this was a IDL reference, it inserted an
additional `<code>` tag, so the `mathjax2katex.py` script was not able
to extract the math blocks properly. The `FindMatching` function was
raising an `IndexError`, but it was never caught and instead terminated
the worker thread. The main thread would then wait forever for a worker
thread that will never finish. The fix here is to catch all exceptions
and print an error, instead of just catching `KeyboardInterrupt` and
`AssertionError`.

* [spec] Typo in type table (#1013)

* [spec] Constrain name section ordering (#1012)

* [test] More tests for non-minimal LEB128 (#1007)

* [test] More tests for overlong LEB128 (#1016)

* [spec] Fix ToC of Appendix in w3c version (#1017)

It seems Sphinx and ReST allow you to use any underline pattern for
sections, but assign their number ordinally as they're encountered.

So when PR #1003 intrdouced a new section in A.1 Embedding using "...",
it becomes `<h4>`, and changes the meaning of the other sections,
creating a strange ToC.

* + Security and Privacy Considerations on Core and JS-API (#1015)

* ~ reflect (rossberg's suggestions)[WebAssembly/spec#1015 (comment)]

* ~ reflect [WebAssembly/spec#1015 (comment)]

* ~ reflect binji's fixes

[https://github.com/WebAssembly/spec/pull/1015/files/122a9483ef1b1e7e7138a51a8711604d45ec1f4b#r283588141]
[https://github.com/WebAssembly/spec/pull/1015/files/122a9483ef1b1e7e7138a51a8711604d45ec1f4b#r283588352]

* [spec] Replace URLs with bikeshed biblio refs (#1018)

This will automatically add these entries to the normative references
section which is auto-generated at the bottom of the document.

* [test] More LEB128 tests (#1019)

* [spec][js-api] Fix some links (#1020)

Replace `init_store` with `store_init`.

Also, reword the "Security and Privacy Considerations" section, and fix
the references are using ReST syntax (which doesn't work in bikeshed).

* [spec] Tweak wording (#966)

* [spec] Address feedback on section 4 (#1022)

* [interpreter] Fix edge cases for f32_convert_i64 (#1021)

* Editorial: Remove links from Number, Object when checking types

This follows the convention followed by the JavaScript spec and HTML.

Fixes #1026

* [test/interpreter] Rounding edge cases for float literals (#1025)

* Remove extraneous copyright from bikeshed document (#1030)

* [test] Fix unintended errors in negative tests (#1034)

* [interpreter] Fix broken link (#1035)

* [test] More inconsistent lengths (#1029)

for type, import, export, table, memory, global, element/data segment,
and br_table.

* [js-api] Remove incorrect note about object caching. (#1036)

Fixes #985.

* [js-api] Use IDL conventions around ambient values. (#1037)

* [interpreter] Fix typo in comment (#1045)

* [spec] Fix header level (#1047)

* [interpreter] Downgrade to Ocaml 4.02 (#1044)

* [spec] Upgrade to IEEE 754-2019 (#1050)

* [spec] Terminology nits (#1053)

* [interpreter] Make format roundtrips perfect (#1057)

* [interpreter] Update BS support (#1058)

* [interpreter] Tweak  target

* [interpreter] Simplify wast.js build

* [interpreter] Add `assert_exhaustion` to README (#1061)

* [spec] Fix the katex build on Travis. (#1062)

It was broken due to the accidental submodule update in #1034.

* Rewrite definition of error types.

This at least gets somewhat close to defining what's actually intended.

Fixes #810.

* [js-api] Editorial: Clarify the host function algorithms a bit. (#1063)

* [js-api] Editorial: Update reference to 'resolve'. (#1065)

* [spec] Fix float text format (#1069)

* [test] Don't require the element index in "uninitialized element" trap strings. (#1076)

One test in elem.wast expects the trap message "uninitialized element 7",
which requires wasm runtimes to be able to print the element index. This
is obviously nice for humans, but can be inconvenient to implement in wasm
implementations which use signals to implement traps, as it requires special
code to preserve the element index.

Other tests eg. in imports.wast don't include the number in the expected
message.

It seems better to allow implementations to decide for themselves
whether to print the index number, rather than having it be an outright
requirement of the spec test.

* Fix error in build.py when test compilation fails

convert_wast_to_js uses multiprocessing to parallelize compilation of
wast test cases. When an error happens, the logging code expects the
result to be a process exit code and a CompletedProcess instance.

This isn't possible, so this commit updates the result to be a
CompletedProcess and modifies the code to manually check the exit code.

* [spec] Fix typo (determinsitically) (#1079)

* Add OOPSLA paper

* [build] Try to reduce output spam from Latex built

* Editorial: Update editor information in JS/Web API specifications.

I will be taking over Dan's duties as editor.

* Add tests for multiple start sections (#1092)

* [spec] fix valid tableinst and meminst premises in appendix (#1093)

* Dont expect indices in validation messages either. (#1089)

Similar to #1076, don't include index numbers in expected error messages
from validation. This allows implementations to avoid creating
dynamically formatted strings for validation error messages. Admittedly
this isn't a huge burden, but it does seem like something that shouldn't
be required to pass the spec test.

* Miscellaneous tests (#1090)

* Test that INT_MIN/0 gets a divide-by-zero trap.

* Test Unicode characters that differ with NFC, NFD, NFKC, and NFKD.

There are three characters whose normalization forms under NFC, NFD, NFKC,
and NFKD are all different. Test them.

* Add a testcase for unreachable in an if-then with no else.

* Fix some assert_malformed module tests to ONLY include malformed syntax

* [test] fix a typo (#1098)

* [spec] Fix typo (#1099)

* [interpreter] Cosmetic tweak to return reduction (#1101)

according to
https://webassembly.github.io/spec/core/exec/instructions.html#exec-return
the `Return` instruction pops the top of the stack, until the next
frame. The equivalent in the reference interpreter should be to set
the value stack to `[]`. This is what is happening for `Break` as well.
So for consistency, also do it here.

This has no visible effect: If the first instruction is `Returning`, the
stack is ignored.

* [test] Long argument lists (#1105)

* Fix cross-reference for list/is empty.

* [wasm-js-api-1] Align with Web IDL specification (#1083)

* Remove a year from the ECMAScript Language title in normative reference

The URL points to latest spec draft so it should be just ECMAScript and not ES2018. This also make it consistent with [WebAssembly JS API Normative Reference](https://www.w3.org/TR/2019/PR-wasm-js-api-1-20191001/#normative)

* [js-api] Correct 'append' cross-references.

* [interpreter] Unify assert_result* assertions (#1104)

Co-authored-by: Andreas Rossberg <[email protected]>
Co-authored-by: Andrew Scheidecker <[email protected]>
Co-authored-by: Junior Rojas <[email protected]>
Co-authored-by: Wanming Lin <[email protected]>
Co-authored-by: Sven Sauleau <[email protected]>
Co-authored-by: Clemens (Hammacher) Backes <[email protected]>
Co-authored-by: Ms2ger <[email protected]>
Co-authored-by: Daniel Ehrenberg <[email protected]>
Co-authored-by: Søren Sjørup <[email protected]>
Co-authored-by: Galaxtone <[email protected]>
Co-authored-by: Eric Prud'hommeaux <[email protected]>
Co-authored-by: Erik McClure <[email protected]>
Co-authored-by: Rikard Hjort <[email protected]>
Co-authored-by: NDTSTN <[email protected]>
Co-authored-by: Dan Gohman <[email protected]>
Co-authored-by: Ryan Hunt <[email protected]>
Co-authored-by: Xuan Huang  <[email protected]>
Co-authored-by: Petr Penzin <[email protected]>
Co-authored-by: Jeremy Ruten <[email protected]>
Co-authored-by: Joachim Breitner <[email protected]>
Co-authored-by: Rian Hunter <[email protected]>
Co-authored-by: autokagami <[email protected]>
Co-authored-by: Sergey Rubanov <[email protected]>
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

No branches or pull requests

4 participants