Conversation
github-actions
Bot
added
core
execution
validation
tests
priority-high
priority-medium
labels
Codecov ReportAttention: Patch coverage is
|
| if imported_memories.len() > 1 { | ||
| return Err(Error::MoreThanOneMemory); | ||
| } |
There was a problem hiding this comment.
| if imported_memories.len() > 1 { | |
| return Err(Error::MoreThanOneMemory); | |
| } | |
| #[cfg(not(multiple_memories))] // maybe without this | |
| if local_memories.len() + imported_memories.len() > 1 { | |
| return Err(Error::MoreThanOneMemory); | |
| } |
There was a problem hiding this comment.
In the multiple memories proposal, can you have more than 1 local memory? Or is it multiple local, multiple imported?
There was a problem hiding this comment.
Looking through the code, I disagree. We have bits in execution_info that MUST have a single memory otherwise it panics.
There was a problem hiding this comment.
@george-cosma Just to be sure that I understand correctly:
Our code assumes that there must be at least on local memory? Or do you mean there must bet exactly one local memory?
There was a problem hiding this comment.
Multiple memories supports more than 1 local memory, in particular I quote:
Add a memory index to all memory-related instructions.
[1]
| if local_memories.len() > 1 { | ||
| return Err(Error::MoreThanOneMemory); | ||
| } |
There was a problem hiding this comment.
| if local_memories.len() > 1 { | |
| return Err(Error::MoreThanOneMemory); | |
| } |
There was a problem hiding this comment.
Looking through the code, I disagree. We have bits in execution_info that MUST have a single memory otherwise it panics.
| code_expr, | ||
| // TODO fix this ugly clone | ||
| // TODO: Not used any more. Should we remove it? | ||
| sidetable: sidetable.clone(), |
There was a problem hiding this comment.
we should remove this, otherwise there might be confusion
There was a problem hiding this comment.
yes, looks good, you can completely remove the field and the comment
| .insert(module_name.to_string(), self.modules.len()); | ||
| self.modules.push(exec_info); | ||
|
|
||
| let local_sidetables = validation_info |
There was a problem hiding this comment.
Since Sidetables are not supposed to be modified after initialization, would it be better if we'd attach them to the FunctionDeclarations?
Mr. Titzer does the exact same thing where the Sidetable is attached to its corresponding FuncDecl.
There was a problem hiding this comment.
Letting them attatched to FunctionInst gives us headaches in terms of rust optimizations
What I am doing there is not to modify the sidetables, I am just shoving them in an array (and syncing it with function indexing by inserting dummy tables for imported functions which are not used anyway)
There was a problem hiding this comment.
I think the sidetable could be a good candidate for an Arc, in theory. No one needs to alter them past parsing a wasm module, but we need them in various places. If we have multiple instances of a given wasm module, each needs the same sidetable. Alternatively, a shared reference is also no bad option, but of course than it leaks to the user, which then needs to store the sidetable in a way that allows creation of sufficiently long lived values to hand out the references.
There was a problem hiding this comment.
Eventually we will have a per module sidetable. In terms of interface, my intuition is that the sidetable per module and the wasm binary per module should essentially be always together within the interpreter loop, as the instruction pointer and stp is. We use WasmReader object for purposes other than having a context for what module we are executing and where the ip is (for example for parsing during validation). If this wasn't the case, like if there was an object, functionally the same as WasmReader but just used within the interpreter loop, i would chuck the sidetable within this object and sort of force the programmer to update stp and pc together and not individually, since everytime, especially when doing big jumps at pc we also need to update the stp so that it points to the sidetable entry pertaining to the next branching instruction that would be hit. In the case of who has references to it, we would essentially treat it the same as we treat the WasmReader object in the interpreter loop.
| .store | ||
| .mems | ||
| .first() | ||
| .first_mut() |
There was a problem hiding this comment.
I still think we should shrink down the interpreter loop massively through the use of macros
There was a problem hiding this comment.
I thought you loved +4000 lines of code changed D:
Jokes aside, I agree that we should abstract away some parts of the interpreter loop, though I'm not sure I agree with macros.
There was a problem hiding this comment.
check element / look at the get_mem function. Abstracting it any more isn't worthwhile imho. ctrl+f works fast enough
There was a problem hiding this comment.
I agree that we should investigate optimizing the interpreter loop. However, I vow to postpone this for the next phase of erasing technical debts. Lets not implement new features and refactor at the same time :)
Signed-off-by: George Cosma <george.cosma@oxidos.io>
Signed-off-by: George Cosma <george.cosma@oxidos.io>
Signed-off-by: George Cosma <george.cosma@oxidos.io>
eva-cosma
force-pushed
the
dev/global_store
branch
from
3566104 to
a989fcf
Compare
Signed-off-by: Cem Onem <cem.oenem@dlr.de>
|
closing for now since we are going with global store approach. |
4 participants
Pull Request Overview
This pull request adds/changes/fixes...
Testing Strategy
This pull request was tested by...
TODO or Help Wanted
This pull request still needs...
Formatting
cargo fmtcargo checkcargo buildcargo docnix fmtGithub Issue
This pull request closes <GITHUB_ISSUE>