Pass function args by reference using span<> #359
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
chfast
force-pushed
the
args_span
branch
2 times, most recently
from
d8fe1cd
to
dcf6c97
Compare
Codecov Report
@@ Coverage Diff @@
## master #359 +/- ##
=======================================
Coverage 99.17% 99.17%
=======================================
Files 49 49
Lines 13232 13236 +4
=======================================
+ Hits 13123 13127 +4
Misses 109 109 |
axic
requested changes
May 28, 2020
axic
reviewed
May 28, 2020
lib/fizzy/execute.cpp
Outdated
| std::vector<uint64_t> locals = std::move(args); | ||
| std::vector<uint64_t> locals; | ||
| locals.reserve(args.size() + code.local_count); | ||
| std::copy_n(args.begin(), args.size(), std::back_inserter(locals)); | ||
| locals.resize(locals.size() + code.local_count); |
There was a problem hiding this comment.
Do we need reserve + resize or can we stay with one?
There was a problem hiding this comment.
Also this seems to now expand it to args.size() + local_count + local_count.
There was a problem hiding this comment.
We could first resize here, then just copy_n without back_inserter
There was a problem hiding this comment.
This is "minimal work" version.
- Firstly we allocate memory for both args and locals (
locals.size()is still 0 at this point). - We copy args.
- We fill locals with zero.
There was a problem hiding this comment.
In the end I used the what @gumb0 suggested. Micro-benchmarks included.
gumb0
reviewed
May 29, 2020
lib/fizzy/span.hpp
Outdated
| /// https://en.cppreference.com/w/cpp/container/span | ||
| /// Only `const T` is supported. | ||
| template <typename T, typename = typename std::enable_if<std::is_const_v<T>>::type> | ||
| struct span |
There was a problem hiding this comment.
any reason why it's a struct and not class?
There was a problem hiding this comment.
As it's not a standard method, I think it should be called Span like our other classes like Stack and OperandStask
There was a problem hiding this comment.
This is from C++20: https://en.cppreference.com/w/cpp/container/span.
But it is a class so I will change to class.
There was a problem hiding this comment.
It's not, because it's not in std.
But it's just a nitpick, I don't care much.
There was a problem hiding this comment.
I will leave it as is to be a drop-in replacement for standardized "container".
gumb0
reviewed
May 29, 2020
New benchmarksHaswell / clang-10Cloud / gcc-9 |
gumb0
reviewed
Jun 3, 2020
test/unittests/span_test.cpp
Outdated
| TEST(span, vector) | ||
| { | ||
| std::vector<uint64_t> vec{1, 2, 3, 4, 5, 6}; | ||
| span<const uint64_t> s(&vec[1], 3); |
There was a problem hiding this comment.
Suggested change
| span<const uint64_t> s(&vec[1], 3); | |
| const span<const uint64_t> s(&vec[1], 3); |
gumb0
reviewed
Jun 3, 2020
test/unittests/span_test.cpp
Outdated
| stack.push(13); | ||
|
|
||
| constexpr auto num_items = 2; | ||
| span<const uint64_t> s(stack.rend() - num_items, num_items); |
There was a problem hiding this comment.
Suggested change
| span<const uint64_t> s(stack.rend() - num_items, num_items); | |
| const span<const uint64_t> s(stack.rend() - num_items, num_items); |
gumb0
approved these changes
Jun 3, 2020
|
Not happy about regressions. Can you also benchmark the |
Closed
chfast
force-pushed
the
args_span
branch
4 times, most recently
from
13f3967
to
0a2cdfd
Compare
|
Final benchmarks: The last commit alone (locals initialization micro-optimization): And the Maybe someone wants to explain why 3/8 case is outstanding? |
|
axic
reviewed
Jul 8, 2020
test/unittests/api_test.cpp
Outdated
| @@ -7,6 +7,7 @@ | |||
| #include <gtest/gtest.h> | |||
| #include <lib/fizzy/limits.hpp> | |||
| #include <test/utils/asserts.hpp> | |||
| #include <test/utils/execute_helpers.hpp> | |||
There was a problem hiding this comment.
It has the overload for execute(..., {...}).
There was a problem hiding this comment.
But no new test was added, why wasn't this needed before?
There was a problem hiding this comment.
Previously, the args were passed by std::vector which has constructor for {}.
axic
reviewed
Jul 8, 2020
test/unittests/end_to_end_test.cpp
Outdated
| @@ -33,7 +33,8 @@ TEST(end_to_end, milestone1) | |||
| const auto module = parse(wasm); | |||
| auto instance = instantiate(module); | |||
|
|
|||
| EXPECT_THAT(execute(*instance, 0, {20, 22}), Result(20 + 22 + 20)); | |||
There was a problem hiding this comment.
No, because std::span is a reference type and {20, 22} is a temporary object.
There was a problem hiding this comment.
So in the "external API" we should have a wrapper then.
There was a problem hiding this comment.
Yes, we can have this in public API. But, I'm not convinced this is super useful. In normal usage you rarely pass constant args to wasm. They come usually from other sources like CLI, etc.
For now I placed this in fizzy::test.
chfast
force-pushed
the
args_span
branch
3 times, most recently
from
27b8366
to
70a4507
Compare
|
The latest benchmark with GCC10, Haswell, 4 GHz. |
gumb0
reviewed
Jul 10, 2020
test/utils/execute_helpers.hpp
Outdated
|
|
||
| namespace fizzy::test | ||
| { | ||
| inline execution_result execute(const Module& module, FuncIdx func_idx, std::vector<uint64_t> args) | ||
| inline execution_result execute( | ||
| Instance& instance, FuncIdx func_idx, std::initializer_list<uint64_t> args) |
There was a problem hiding this comment.
Does it not help in tests where you declare args as arrays?
There was a problem hiding this comment.
I did not included this overload in end_to_end tests where it has small number of case that could be converted to std::array.
There was a problem hiding this comment.
It's 4 cases, I'd still use it.
axic
reviewed
Jul 10, 2020
| @@ -613,8 +611,8 @@ execution_result execute( | |||
| const auto& code = instance.module.codesec[code_idx]; | |||
| auto* const memory = instance.memory.get(); | |||
|
|
|||
| std::vector<uint64_t> locals = std::move(args); | |||
| locals.resize(locals.size() + code.local_count); | |||
| std::vector<uint64_t> locals(args.size() + code.local_count, 0); | |||
There was a problem hiding this comment.
Yes. First we allocate and zero for args + locals. Then we overwrite args with proper values.
axic
reviewed
Jul 10, 2020
test/unittests/execute_test.cpp
Outdated
| @@ -1003,9 +1004,10 @@ TEST(execute, reuse_args) | |||
| "0061736d01000000010b0260027e7e017e6000017e03030200010a19020e002000200180210120002001820b08" | |||
| "004217420510000b"); | |||
|
|
|||
| const std::vector<uint64_t> args{20, 3}; | |||
| const std::array<uint64_t, 2> args{20, 3}; | |||
There was a problem hiding this comment.
Not needed, but no need to use std::vector to allocate 2 args.
axic
reviewed
Jul 10, 2020
|
|
||
| [[gnu::noinline]] auto init_locals_2(fizzy::span<const uint64_t> args, uint32_t local_count) | ||
| { | ||
| std::vector<uint64_t> locals(args.size() + local_count); |
There was a problem hiding this comment.
Is this the same as locals(args.size() + local_count, 0); e.g. the one used in execute.cpp?
There was a problem hiding this comment.
Yes. The 0 value is default.
axic
approved these changes
Jul 13, 2020
gumb0
approved these changes
Jul 13, 2020
gumb0
reviewed
Jul 13, 2020
test/unittests/execute_call_test.cpp
Outdated
| Instance&, std::vector<uint64_t> args, int) -> execution_result { | ||
| return fizzy::execute(*instance1, *func_idx, std::move(args)); | ||
| Instance&, span<const uint64_t> args, int) -> execution_result { | ||
| return fizzy::execute(*instance1, *func_idx, std::vector(args.begin(), args.end())); |
There was a problem hiding this comment.
Why this?
It seems args passed to this lambda should live long enough on the caller side.
gumb0
approved these changes
Jul 14, 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 passes function arguments by reference (instead of by value with
std::vector). This eliminates one allocation per call because - previously one allocation was done to create the vector, then another to create locals.Changing
std::vectortoconst std::vector&may have been enough, butspanis more generic and easier to refactor with.The
snapdoes not have unit tests as it is trivial and not all methods are currently used after full transition.