Reuse contract and service instances inside transaction #1506
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
ma2bd
reviewed
Jan 17, 2024
There was a problem hiding this comment.
I made a few tweaks in a branch of mine: https://github.com/ma2bd/linera-protocol/commits/reuse-app-instances-inside-transaction/
On my laptop, the wasm e2e tests are failing with Failed to initialize contract logger: SetLoggerError(()). This looks manageable. Hopefully, this is only conflicting side effect at the guest level. (Otherwise, we would have to release this a bit later)
| let mut runtime = runtime | ||
| .into_inner() | ||
| .expect("Runtime clones should have been freed by now"); | ||
| let execution_outcome = execution_result?; |
There was a problem hiding this comment.
This shouldn't matter any more because all errors are final, but sure.
There was a problem hiding this comment.
When I tested it would hang without this line on tests which returned an error.
| // Make the call to user code. | ||
| let query_context = crate::QueryContext { | ||
| chain_id: this.chain_id, | ||
| }; | ||
| this.push_application(ApplicationStatus { | ||
| id: queried_id, | ||
| parameters: description.parameters, |
There was a problem hiding this comment.
If we're going to keep the full description in the map, I would leave the field access as it is.
There was a problem hiding this comment.
I refactored to create a LoadedApplication helper type that contains just the parameters from the description. I think this makes it a bit clearer and avoids cloning the entire description when instancing an application.
jvff
force-pushed
the
reuse-app-instances-inside-transaction
branch
from
b6c3b38 to
88f8929
Compare
ma2bd
approved these changes
Jan 18, 2024
linera-execution/src/runtime.rs
Outdated
| Ok((application.instance, callee_context)) | ||
| } | ||
|
|
||
| /// Cleans-up the runtime after the execution of a call to a different contract. |
| let mut runtime = runtime | ||
| .into_inner() | ||
| .expect("Runtime clones should have been freed by now"); | ||
| let execution_outcome = execution_result?; |
Ensure consistency.
Make it easier to use the dynamically dispatched types.
Try to be closer to the Rust API naming conventions.
No need to create a separate binding for `this` if it's only going to be used once.
Create separate blocks for the contract-specific methods and the service-specific methods.
Prepare to use it to reload active contract instances without having to create a new instance. Add a `Weak` reference in `SyncRuntimeInternal` to reference the handle used to reference itself.
Place common code in new methods so that there's only one place to update.
Use a generic type parameter instead of a const parameter in order to have a single map of loaded application instances (either contracts or services).
Prepare to share the instances in the runtime.
Store the data necessary for running an application instance.
Prepare to return the value stored inside a map.
Store the loaded instances in a map, and reuse them if there are more calls to them.
Like the `load_contract_instance` method, return an `Arc<Mutex<_>>` with the instance and the parameters directly, in preparation to share the instance.
Store the loaded instances in a map, and reuse them if there are more calls to them.
The `ExecutionStateView` starts a thread to execute the application with the synchronous runtime, and in its original thread it loops listening for requests from the application. When the execution finishes, the thread ends and when the runtime is dropped it also drops the sender of requests to the `ExecutionStateView`. Therefore the `ExecutionStateView` then detects that the channel has closed and finishes the loop continuing execution. However, with the addition of the `loaded_applications` map, each loaded application has a strong reference to the runtime, so there's a circular dependency which prevents the `SyncRuntimeInternal` type from dropping. This prevents the channel from closing, and causes a deadlock. To avoid this situation, the `loaded_applications` map must be cleared at the end of the execution, independently if it succeeded or if it failed.
Prepare to allow the same Wasm guest instance to be reused for different calls inside the same transaction.
Create a separate `MockApplicationInstance` that does not share the expected calls list. This means that now the application forwards the list to the created instance, and each instance has a separate call list. This tests if the instances are reused or not. If they are not reused, session calls will always fail.
jvff
force-pushed
the
reuse-app-instances-inside-transaction
branch
from
88f8929 to
8eca794
Compare
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.
Motivation
Every time an application calls another, a new instance of the callee application is created. However, the ability to reuse an instance may increase performance and allow the application to use volatile memory that is persisted for longer than one call.
As a first step, the instance should be reused if there are multiple calls to the application inside the same transaction.
Proposal
Store a map of loaded applications in the runtime, and reuse entries if an application is called again in the same transaction.
There's a small issue with this. The
SyncRuntimeInternalcontains the map but it is placed inside anArc<Mutex<_>>that is then used as the runtime implementation. Each loaded application has a copy of theArc<Mutex<_>>to use as its runtime. This leads to a circular dependency where the runtime stores the loaded applications which themself refer back to the runtime.The solution adopted here (at least temporarily) is to ensure the map is cleared before finishing execution. Failure to do so prevents the
ExecutionStateViewfrom detecting that execution has finished (because the runtime leaks and the sender endpoint it has inside isn't dropped), which then hangs.Test Plan
The
MockApplicationhelper type was refactored to create a separateMockApplicationInstancetype. Now, when theMockApplicationis used throughUserContractModuleorUserServiceModuleto create aMockApplicationInstanceit forwards all expected calls instead of forwarding a reference to them. In practice this means that each instance has a separate list of expected calls.This allows testing if the instances are reused or not, since if they are not any session calls fail because they won't reuse the instance of the application that created them. Reapply the commit with this refactor to
mainshows that the tests with sessions fail as expected, but pass in this PR.Release Plan
Since this changes the execution behavior:
Links
Closes #488