Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2705993002:
[wasm] Add JSToWasmWrapperCache to reuse generated wrapper code (Closed)
|
Created:
3 years, 10 months ago by Clemens Hammacher Modified:
3 years, 9 months ago Reviewers:
titzer CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Add JSToWasmWrapperCache to reuse generated wrapper code
The generated code for JSToWasm wrappers only depends on the signature
of the exported function. Hence, we can reuse the generated code and
just patch the reference to the called wasm code.
For the unity-wasm benchmark, we reach a hit rate of 98.07% for this
cache, and only 395 instead of 20471 wrappers are compiled. This brings
down instantiation time from 2.9s to 1.6s on a MBP.
R=titzer@chromium.org
Review-Url: https://codereview.chromium.org/2705993002
Cr-Commit-Position: refs/heads/master@{#43322}
Committed: https://chromium.googlesource.com/v8/v8/+/7a91e3c69c7fe183f1183ddc2359ade2d1460944
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address comments #Patch Set 3 : Rebase #Patch Set 4 : Avoid unused variable #Patch Set 5 : Fix comment #
Messages
Total messages: 31 (23 generated)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Awesome improvement! Comments below. https://codereview.chromium.org/2705993002/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2705993002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:3728: struct HashWasmSig { You can replace this with a signature map (signature-map.h). https://codereview.chromium.org/2705993002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:3779: JSToWasmWrapperCache* cache) { I think we should move the caching to the caller so that the compiler is "dumb" and only does what it is told. https://codereview.chromium.org/2705993002/diff/1/src/compiler/wasm-compiler.h File src/compiler/wasm-compiler.h (right): https://codereview.chromium.org/2705993002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.h:58: char storage_[sizeof(pseudo_storage)]; Nice trick, but it's undefined behavior :-) You can use a signature map, which maps signatures to linearly-increasing indexes, and a simple array.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/21306) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/21331) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2705993002/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2705993002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:3728: struct HashWasmSig { On 2017/02/20 at 10:14:11, titzer wrote: > You can replace this with a signature map (signature-map.h). Done. https://codereview.chromium.org/2705993002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:3779: JSToWasmWrapperCache* cache) { On 2017/02/20 at 10:14:11, titzer wrote: > I think we should move the caching to the caller so that the compiler is "dumb" and only does what it is told. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice! LGTM
The CQ bit was checked by clemensh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 70001, "attempt_start_ts": 1487596699717350,
"parent_rev": "d8ccbd693c69de67d53a9b856b4e81ec9d1c53ab", "commit_rev":
"7a91e3c69c7fe183f1183ddc2359ade2d1460944"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Add JSToWasmWrapperCache to reuse generated wrapper code The generated code for JSToWasm wrappers only depends on the signature of the exported function. Hence, we can reuse the generated code and just patch the reference to the called wasm code. For the unity-wasm benchmark, we reach a hit rate of 98.07% for this cache, and only 395 instead of 20471 wrappers are compiled. This brings down instantiation time from 2.9s to 1.6s on a MBP. R=titzer@chromium.org ========== to ========== [wasm] Add JSToWasmWrapperCache to reuse generated wrapper code The generated code for JSToWasm wrappers only depends on the signature of the exported function. Hence, we can reuse the generated code and just patch the reference to the called wasm code. For the unity-wasm benchmark, we reach a hit rate of 98.07% for this cache, and only 395 instead of 20471 wrappers are compiled. This brings down instantiation time from 2.9s to 1.6s on a MBP. R=titzer@chromium.org Review-Url: https://codereview.chromium.org/2705993002 Cr-Commit-Position: refs/heads/master@{#43322} Committed: https://chromium.googlesource.com/v8/v8/+/7a91e3c69c7fe183f1183ddc2359ade2d14... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001) as https://chromium.googlesource.com/v8/v8/+/7a91e3c69c7fe183f1183ddc2359ade2d14...
Message was sent while issue was closed.
could you give a link to "unity-wasm benchmark"?
Message was sent while issue was closed.
On 2017/03/02 at 02:53:17, ccyongwang wrote: > could you give a link to "unity-wasm benchmark"? You find the benchmark files here: http://files.unity3d.com/alexsuvorov/webgl/headless-benchmark/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
