Open
Conversation
Codecov Report
@@ Coverage Diff @@
## master #739 +/- ##
==========================================
- Coverage 99.27% 99.27% -0.01%
==========================================
Files 88 88
Lines 13296 13158 -138
==========================================
- Hits 13200 13062 -138
Misses 96 96
Flags with carried forward coverage won't be shown. Click here to find out more.
|
axic
commented
Feb 24, 2021
axic
commented
Feb 24, 2021
Member
Author
chfast
reviewed
Feb 25, 2021
test/utils/wasm3_engine.cpp
Outdated
| if (result == m3Err_none) | ||
| return {false, ret_valid ? ret_value : std::optional<uint64_t>{}}; | ||
|
|
||
| std::vector<const void*> argPtrs; |
Collaborator
There was a problem hiding this comment.
If you want ultimate speed you can use stack-allocated fixed-size array. Just bail out if too much arguments provided.
Member
Author
There was a problem hiding this comment.
fizzy/test/utils/wasm3_engine.cpp:147:24: error: variable length arrays are a C99 feature [-Werror,-Wvla-extension]
const void* argPtrs[args.size()];
Or not sure what you've meant?
Member
Author
There was a problem hiding this comment.
Ah you meant like allocating an array with like 32 items, and fail if args.size() is larger.
Member
Author
There was a problem hiding this comment.
Not sure if this makes a lot of speed difference, but pushed a version.
gumb0
reviewed
Feb 25, 2021
gumb0
reviewed
Feb 25, 2021
gumb0
reviewed
Feb 25, 2021
gumb0
reviewed
Feb 25, 2021
gumb0
approved these changes
Feb 26, 2021
Member
Author
|
My plan is we release 0.8.0, then merge this and release 0.8.1. Then it is simple to show benchmarks for both, if needed. |
axic
commented
May 23, 2022
| namespace | ||
| { | ||
| const void* env_adler32(IM3Runtime /*runtime*/, uint64_t* stack, void* mem) | ||
| const void* env_adler32( |
Member
Author
There was a problem hiding this comment.
Based on https://github.com/wasm3/wasm3/blob/main/source/wasm3.h#L327-L340 this would be the correct way:
m3ApiRawFunction(env_adler32)
{
(void)runtime;
(void)_ctx;
m3ApiReturnType (uint32_t)
m3ApiGetArgMem (const uint8_t*, offset)
m3ApiGetArg (uint32_t, length)
// This would be the correct way, but it traps. Although the numbers seem to be correct.
// mem + offset + length <= mem + m3_getMemorySize
m3ApiCheckMem(offset, length);
m3ApiReturn(fizzy::test::adler32({offset, length}));
}See also an example wasi function: https://github.com/wasm3/wasm3/blob/main/source/m3_api_meta_wasi.c#L146
axic
commented
May 23, 2022
| const uint32_t length = static_cast<uint32_t>(stack[1]); | ||
| const uint32_t offset = static_cast<uint32_t>(stack[1]); | ||
| const uint32_t length = static_cast<uint32_t>(stack[2]); | ||
| stack[0] = fizzy::test::adler32({reinterpret_cast<uint8_t*>(mem) + offset, length}); |
Member
Author
There was a problem hiding this comment.
The stack order has changed. Now the return value is on the bottom.
axic
force-pushed
the
wasm3-update
branch
2 times, most recently
from
65e1677 to
4d0934d
Compare
axic
force-pushed
the
wasm3-update
branch
2 times, most recently
from
8761680 to
f449879
Compare
Draft
Member
Author
|
Merging this is still blocked on benchmarks running out of stack space or something: We did find some reason, increased something/update compiler to work it around, but it is back again. |
Member
Author
|
Apparently we run into a wasm3 bug: wasm3/wasm3#425 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
We still need a patch for the start function, see wasm3/wasm3#202. And also a memory leak issue: wasm3/wasm3#203.
At least we could get rid of wasm3/wasm3#130, wasm3/wasm3#129, and wasm3/wasm3#145.