-
Notifications
You must be signed in to change notification settings - Fork 39
Execution context generalization #779
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
447c545
Move "increment call depth" to execute()
chfast 91d1c85
Move ExecutionContext to execution_context.hpp
chfast a2dcb22
Move ExecutionContext logic to Guard
chfast 486d114
Make ExecutionContext::Guard non-copyable/non-movable
chfast 111f379
Rename ExecutionContext::{Guard -> LocalContext}
chfast 6095fb6
Rename ExecutionContext::{inrement_call_depth() -> create_local_conte…
chfast File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
// Fizzy: A fast WebAssembly interpreter | ||
// Copyright 2021 The Fizzy Authors. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
#pragma once | ||
|
||
namespace fizzy | ||
{ | ||
/// The storage for information shared by calls in the same execution "thread". | ||
/// Users may decide how to allocate the execution context, but some good defaults are available. | ||
class ExecutionContext | ||
{ | ||
/// Call local execution context. | ||
/// It will automatically decrement the call depth to the original value | ||
/// when going out of scope. | ||
class [[nodiscard]] LocalContext | ||
{ | ||
ExecutionContext& m_shared_ctx; ///< Reference to the shared execution context. | ||
|
||
public: | ||
LocalContext(const LocalContext&) = delete; | ||
LocalContext(LocalContext&&) = delete; | ||
LocalContext& operator=(const LocalContext&) = delete; | ||
LocalContext& operator=(LocalContext&&) = delete; | ||
|
||
explicit LocalContext(ExecutionContext& ctx) noexcept : m_shared_ctx{ctx} | ||
{ | ||
++m_shared_ctx.depth; | ||
} | ||
|
||
~LocalContext() noexcept { --m_shared_ctx.depth; } | ||
}; | ||
|
||
public: | ||
int depth = 0; ///< Current call depth. | ||
|
||
/// Increments the call depth and returns the local call context which | ||
/// decrements the call depth back to the original value when going out of scope. | ||
LocalContext create_local_context() noexcept { return LocalContext{*this}; } | ||
}; | ||
} // namespace fizzy |
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does moving this action from
invoke
toexecute
change the depth that host functions see?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We record the value in many tests:
fizzy/test/unittests/execute_call_depth_test.cpp
Line 65 in faddd97
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so, but why?
Before:
call
opcode callsinvoke_function
, it increments depth, callsexecute
, it calls host function.After:
call
opcode callsinvoke_function
, callsexecute
, it calls host function. (increment only if it's not imported)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before:
execute(depth: D)
,call
opcode,invoke_function()
: increment depth,execute(depth: D+1)
.After:
execute(depth: D)
: increment depth,call
opcode,invoke_function()
,execute(depth: D+1)
.So this changes the value of the
depth
only inside theexecute()
.This is needed because later I want to get access the
ExecutionContext
shared stack space in the same place wheredepth
is incremented.I was not able to also move the depth check to the same place easily. This may be handled later, e.g. by trapping inside the
invoke_function()
.