P23 #1511
Merged
P23 #1511
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
dmkozh
approved these changes
Jan 22, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 22, 2025
The goals of this PR are: 1. Surface the `ModuleCache` from inside soroban to its embedding environment. 2. Allow a module cache to _outlive_ the soroban `Host`, and be reused from one `Host` to the next. 3. Allow the `ModuleCache` to be _pre-populated_ from before the lifetime of a single `Host`, (eg. by stellar-core, with all the non-archived contracts currently live in the ledger, at startup or such.) These goals are accomplished by the following structural changes: 1. Factoring out error-handling / error-mapping functionality of `Host` into a separate trait `ErrorHandler` that `Host` implements, and adding a new trait `CompilationContext` that extends both `ErrorHandler + AsBudget`. This abstracts the services the `ModuleCache` _needs_ from the `Host`, such that the embedding environment can provide its own `CompilationContext` in the times pre- or post-`Host` lifetime. 2. Modifying the internal map that stores `Module`s inside the `ModuleCache` to use a refcounted-shared-mutable `std::map::BtreeMap` that can be incrementally and efficiently added-to or removed-from, and is _not_ metered. 3. `Module` storage and arguments that were `Rc<>` are made `Arc<>` and the whole `ModuleCache` is made `Send+Sync`. This is mainly because we want to work with the reusable `ModuleCache` across C++ threads, and while Rust has no idea about C++ threads it at least helps us shoot ourselves in the foot less if all the Rust code that works with it preserves the sense of "this would be threadsafe in Rust if Rust was making the threads". 4. There is also a bit of gratuitous churn where things get renamed from `foo` => `wasmi_foo` and `Foo` => `wasmi::Foo`. This is because this change was done as part of (and there'll be a followup PR including) work equipping the `Vm` and `ModuleCache` with an experimental wasmtime/winch backend. It turns out this PR's work (on just the `ModuleCache` reuse part) is high enough value, and agnostic enough to which backend it's using, that we want to split this work out, move it up in the schedule and explore getting it into as early a release as possible. But I didn't bother reverting-out the `wasmi_`-qualification since it's intertwined with other changes here and harmless to keep. This PR is a continuation of #1511 and should be merged after it. This PR implements [CAP-0065](https://github.com/stellar/stellar-protocol/blob/master/core/cap-0065.md)
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 PR strictly bumps the protocol to p23 (and re-observes tests) not changing anything else.
The commit that needs the most scrutiny is f4dc35d since it is the reobservation run: only small changes that derive from the protocol number itself should be visible (typically "the change of protocol number in the wasm metadata").