Upgrade WABT to 1.0.19 #443
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #443 +/- ##
==========================================
+ Coverage 98.61% 98.64% +0.02%
==========================================
Files 59 59
Lines 8912 8932 +20
==========================================
+ Hits 8789 8811 +22
+ Misses 123 121 -2 |
Closed
gumb0
force-pushed
the
upgrade_wabt
branch
13 times, most recently
from
77dc7c8
to
ec64b3b
Compare
axic
reviewed
Sep 15, 2020
axic
reviewed
Sep 15, 2020
|
|
||
| // Run start function (this will return ok if no start function is present) | ||
| const wabt::interp::ExecResult r = m_executor.RunStartFunction(m_module); |
There was a problem hiding this comment.
Is this now executed properly by instantiate?
There was a problem hiding this comment.
Yes, looks like so, our unit test for this passes.
axic
reviewed
Sep 15, 2020
axic
reviewed
Sep 15, 2020
| } | ||
|
|
||
| WasmEngine::Result WabtEngine::execute( | ||
| WasmEngine::FuncRef func_ref, const std::vector<uint64_t>& args) | ||
| { | ||
| const auto* e = reinterpret_cast<const wabt::interp::Export*>(func_ref); | ||
| assert(m_instance); |
There was a problem hiding this comment.
Why assert here and not in init_memory/get_memory? I think we have the "rules" described in wasm_engine.h, I can't remember myself.
There was a problem hiding this comment.
wasm_engine.hpp says for all threeRequires instantiate()., though it seems ambiguous whether it means "assumes instantiate was called" or "fails when instantiate was not called".
But I see that here it's inconsitent now, I guess I'll change it to asserts in all three, fizzy_engine also assumes m_instance is non-null there.
gumb0
force-pushed
the
upgrade_wabt
branch
2 times, most recently
from
feb9550
to
e7e2b09
Compare
gumb0
force-pushed
the
upgrade_wabt
branch
3 times, most recently
from
a03c67b
to
7ce690a
Compare
axic
reviewed
Sep 16, 2020
|
Can you also squash the commits? I think this is ready now, isn't it? |
axic
approved these changes
Sep 16, 2020
|
@chfast waiting for your review |
This does not look very impressive. |
chfast
commented
Sep 16, 2020
| wabt::interp::DefinedModule* m_module{nullptr}; | ||
|
|
||
| // WABT Executor/Thread with default options. | ||
| wabt::interp::Executor m_executor{&m_env}; |
There was a problem hiding this comment.
Previously the stack space was allocated here. Not allocating stack for every call had big impact on performance.
gumb0
reviewed
Sep 16, 2020
| })()}; | ||
| wabt::interp::Module::Ptr m_module; | ||
| wabt::interp::Instance::Ptr m_instance; | ||
| wabt::interp::Thread::Ptr m_thread{wabt::interp::Thread::New(m_store, {})}; |
There was a problem hiding this comment.
Here are the options for Thread
https://github.com/WebAssembly/wabt/blob/21350996d5bbcd9cafbaa3a5687e6fb503da060c/src/interp/interp.h#L1025-L1032
There was a problem hiding this comment.
Default values are fine provided we create it once.
There was a problem hiding this comment.
So m_store does not need to be mutable anymore, right?
There was a problem hiding this comment.
So
m_storedoes not need to be mutable anymore, right?
It still needs it for get_memory where it gets the (smart) pointer to memory, which can be only non-const.
|
I benchmarked the version with |
|
Make sure to squash the last commit :) |
|
I've got similar or worse results |
|
I propose to merge it as is, and report upstream. Depending on the feedback we may adjust to code in following PRs. |
chfast
force-pushed
the
upgrade_wabt
branch
2 times, most recently
from
f966f6c
to
33f5554
Compare
|
Updated benchmarks, now without |
gumb0
approved these changes
Sep 25, 2020
Merged
axic
approved these changes
Sep 28, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For reference, here's PR intoducing new API: WebAssembly/wabt#1330