refactor: Allocate module dynamically #575
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
gumb0
force-pushed
the
dynamic-module
branch
2 times, most recently
from
371941d
to
0ccea9d
Compare
Codecov Report
@@ Coverage Diff @@
## master #575 +/- ##
==========================================
- Coverage 98.24% 98.23% -0.01%
==========================================
Files 62 63 +1
Lines 9039 8995 -44
==========================================
- Hits 8880 8836 -44
Misses 159 159 |
gumb0
force-pushed
the
dynamic-module
branch
2 times, most recently
from
fcee718
to
af80e3e
Compare
gumb0
commented
Oct 6, 2020
lib/fizzy/instantiate.hpp
Outdated
| @@ -87,13 +87,22 @@ struct Instance | |||
| }; | |||
|
|
|||
| // Instantiate a module. | |||
| std::unique_ptr<Instance> instantiate(Module module, | |||
| // Transfers ownership of passed module to instance. | |||
| std::unique_ptr<Instance> instantiate(std::unique_ptr<Module>&& module, | |||
There was a problem hiding this comment.
Another possible option would be to have
- for moving:
std::unique_ptr<Instance> instantiate(std::unique_ptr<Module> module, - for copying:
std::unique_ptr<Instance> instantiate(const Module& module,
but that would require even more changes to the tests to do either instantiate(std::move(module), ... or instantiate(*module, ...
I would better do it in a follower PR if it's a preferable variant.
There was a problem hiding this comment.
If the copy is to be made, better API is instantiate(Module module, ...)
gumb0
commented
Oct 6, 2020
|
Clang 10 build with LTO, mostly no difference. |
|
Now after rebase I'm getting significant slowdown. @chfast Could you try benchmarking this? |
Might be related to similar issue in #577 and other optimization PRs. |
chfast
reviewed
Oct 8, 2020
lib/fizzy/instantiate.cpp
Outdated
| @@ -373,6 +373,16 @@ std::unique_ptr<Instance> instantiate(Module module, | |||
| return instance; | |||
| } | |||
|
|
|||
| std::unique_ptr<Instance> instantiate(const std::unique_ptr<Module>& module, | |||
There was a problem hiding this comment.
This is not good idea, because it is too easy to copy Module unintentionally (as we have right now). If this is for avoid unittest modifications, move it test/.
There was a problem hiding this comment.
This is mostly for tests, but without this overload here it would be not clear why the only instantiate has unique_ptr<Module>&& parameter and not unique_ptr<Module> (the reason is rvalue-reference allows the call with lvalue module variable to be unambiguous)
Ok let me try the change suggested in #575 (comment)
There was a problem hiding this comment.
A comment explaining this would be enough to me. Later unit tests can be updated, when API is stable and we are happy with it.
There was a problem hiding this comment.
I moved second instantiate to test/instantiate_helpers.hpp and changed the main one to take unique_ptr by value, PTAL.
|
Benchmarked again after rebase. Clang with LTO GCC with LTO |
gumb0
commented
Oct 8, 2020
axic
reviewed
Oct 8, 2020
axic
reviewed
Oct 8, 2020
test/unittests/execute_call_test.cpp
Outdated
| @@ -171,7 +171,7 @@ TEST(execute_call, call_indirect_imported_table) | |||
| "0061736d01000000010a026000017f60017f017f020a01016d01740170010514030201010a0901070020001100" | |||
| "000b"); | |||
|
|
|||
| const Module module = parse(bin); | |||
| const auto module = parse(bin); | |||
There was a problem hiding this comment.
Can these thing be really const when it is a unique_ptr?
There was a problem hiding this comment.
It can be, but you will not be able to move it.
There was a problem hiding this comment.
Yeah, const unique_ptr means you can't change the wrapped pointer.
chfast
reviewed
Oct 8, 2020
test/unittests/end_to_end_test.cpp
Outdated
| @@ -31,7 +32,7 @@ TEST(end_to_end, milestone1) | |||
| const auto wasm = from_hex( | |||
| "0061736d0100000001070160027f7f017f030201000a13011101017f200020016a20026a220220006a0b"); | |||
| const auto module = parse(wasm); | |||
| auto instance = instantiate(module); | |||
| auto instance = instantiate(*module); | |||
There was a problem hiding this comment.
I think the better change is to "inline" the module variable (CLion has a refactoring for that).
Suggested change
| auto instance = instantiate(*module); | |
| auto instance = instantiate(parse(bin)); |
There was a problem hiding this comment.
Changed it everywhere, where possible, I think (i.e. where module variable is used only once in test)
|
Squashed. |
|
Changed to Also note that |
chfast
reviewed
Oct 9, 2020
lib/fizzy/parser.cpp
Outdated
|
|
||
| return module; | ||
| return std::unique_ptr<const Module>(module.release()); |
There was a problem hiding this comment.
This is not needed. See (6) in https://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr.
There was a problem hiding this comment.
I know, it was not there first, but then some warning/sanitizers comlained.
Specifically, one warning claimed that returning different, but compatible type may lead to additional copying on some older compilers, and std::move(module) was adviced to force the move.
And then another one warned against return with std::move :-)
There was a problem hiding this comment.
There was a problem hiding this comment.
Please disable -Wreturn-std-move-in-c++11 in Clang.
| module.codesec.emplace_back(Code{1, 0, {Instr::local_get, instr, Instr::end}, {0, 0, 0, 0}}); | ||
| module->typesec.emplace_back(FuncType{{ValType::i32}, {ValType::i32}}); | ||
| module->funcsec.emplace_back(TypeIdx{0}); | ||
| module->codesec.emplace_back(Code{1, 0, {Instr::local_get, instr, Instr::end}, {0, 0, 0, 0}}); | ||
|
|
||
| return execute(module, 0, {arg}); |
There was a problem hiding this comment.
Maybe don't use test helpers here.
gumb0
commented
Oct 12, 2020
| @@ -62,6 +62,7 @@ if(WEVERYTHING) | |||
| -Wno-double-promotion | |||
| -Wno-float-equal | |||
| -Wno-padded | |||
| -Wno-return-std-move-in-c++11 | |||
There was a problem hiding this comment.
@chfast is this ok or should I disable it only for clang somehow?
There was a problem hiding this comment.
You disabled it only for -Weverything. If that's intentional, then we are fine as -Weverything only make sense for Clang. I'm assuming -Wreturn-std-move-in-c++11 is not enabled in default build.
chfast
approved these changes
Oct 12, 2020
| @@ -62,6 +62,7 @@ if(WEVERYTHING) | |||
| -Wno-double-promotion | |||
| -Wno-float-equal | |||
| -Wno-padded | |||
| -Wno-return-std-move-in-c++11 | |||
There was a problem hiding this comment.
You disabled it only for -Weverything. If that's intentional, then we are fine as -Weverything only make sense for Clang. I'm assuming -Wreturn-std-move-in-c++11 is not enabled in default build.
Yes, it's fine in default build. |
axic
approved these changes
Oct 12, 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.
This will simplify C API implementation part, allowing to handle modules and instances in the same manner.