Skip to content
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

Improve contract-call? implementation #468

Open
obycode opened this issue Aug 13, 2024 · 5 comments
Open

Improve contract-call? implementation #468

obycode opened this issue Aug 13, 2024 · 5 comments
Assignees

Comments

@obycode
Copy link
Collaborator

obycode commented Aug 13, 2024

Currently, calling another contract involves spinning up a separate Wasm VM to execute the callee, then return results to the caller, which is quite resource intensive. It would be much better to instead load the callee and link it into the caller's VM.

@obycode
Copy link
Collaborator Author

obycode commented Aug 13, 2024

Note: this change might help with issue #409.

@ureeves ureeves self-assigned this Nov 11, 2024
ureeves added a commit to ureeves/stacks-core that referenced this issue Nov 11, 2024
Instantiating a `wasmtime::Engine` is an expensive operation, so it is
best to do it only once for the duration of a `GlobalContext`. Cloning
an engine is, however, not an expensive operation (just an `Arc::clone`)
and we use it to avoid referring to the global context while while
instantiating a `ClarityWasmContext`.

See-also: stacks-network/clarity-wasm#468
ureeves added a commit to ureeves/stacks-core that referenced this issue Nov 11, 2024
Instantiating a `wasmtime::Engine` is an expensive operation, so it is
best to do it only once for the duration of a `GlobalContext`. Cloning
an engine is, however, not an expensive operation (just an `Arc::clone`)
and we use it to avoid referring to the global context already referred
to by the instantiated `ClarityWasmContext`.

See-also: stacks-network/clarity-wasm#468
ureeves added a commit to ureeves/clarity-wasm that referenced this issue Nov 11, 2024
@obycode
Copy link
Collaborator Author

obycode commented Nov 12, 2024

To try to help clarify this, currently we have:

+-------------------------+          +--------------------------+
|      Wasm Instance      |          |      Wasm Instance       |
|   +-----------------+   |          |   +-----------------+    |
|   |                 |   |          |   |                 |    |
|   |   Contract A    |   |          |   |   Contract B    |    |
|   |                 |   |          |   |                 |    |
|   +-----------------+   |          |   +-----------------+    |
+-------------------------+          +-------------------------+
              ^                                 ^
              |                                 |
              |                                 |
              v                                 v
           +---------------------------------------+
           |                 Rust                  |
           +---------------------------------------+

What I think we want is:

+------------------------------------------+
|               Wasm Instance              |
|                                          |
| +---------------+      +---------------+ |
| |   Contract A  | <--> |   Contract B  | |
| +---------------+      +---------------+ |
|                                          |
+------------------------------------------+

ureeves added a commit to ureeves/clarity-wasm that referenced this issue Nov 13, 2024
ureeves added a commit to ureeves/clarity-wasm that referenced this issue Nov 13, 2024
ureeves added a commit that referenced this issue Nov 14, 2024
@ureeves
Copy link
Collaborator

ureeves commented Nov 18, 2024

I've been exploring ways to achieve this, but think it is possible only under certain conditions.

As far as I can see there's two distinct cases of relevance. I mostly ignore the function name in my explainers, since it is equivalent to the contract identifier in the scope of the possible changes.

static contract identifier:

;; the contract to be called can be resolved by the compiler
(contract-call? 'ST1HTBVD3JG9C05J7HBJTHGR0GGW7KXW28M5JS8QE.counter \
                count tx-sender)

In this situation the compiler can understand which contract is being called, and emit an appropriate s-expression, such as the one below.

(import "'ST1HTBVD3JG9C05J7HBJTHGR0GGW7KXW28M5JS8QE.counter" \
        "count" (func $count (result i32)))

In the runtime, once we load the top level contract, we can then parse all of these imports and load all contracts that may be called recursively, building a tree of contracts. These can then be linked together using, for example, Linker::module starting from the bottom of the tree, and proceeding upwards.

There are two possible gotchas with this approach:

  • Failing to account for recursive links. It is possible that two contracts refer to each other in a circular fashion. If this is not taken into account by, for example, using Linker::define_unknown_imports_as_traps, then linking will fail.
  • Failing to take into account the passing of more complex arguments. Arguments are currently passed by reading from the caller's memory, and writing it into the callee's memory. This will not be possible if we link the contracts together, so we should think of a different approach. Sharing a Memory between could be one way, making use of the multi-memory proposal, where one memory would be used for passing arguments (and returns), would be another.

dynamic contract identifier:

;; the contract to be called **cannot** be resolved by the compiler
(define-trait counter ((count (int) (response int uint))))
(define-public (call-counter (contract <counter>) (amount int))
  (contract-call? contract count amount)
)

In this situation the compiler itself cannot emit the s-expression emitted in the static case, since it cannot resolve the contract variable to a single value. As such we have two options:

  1. We jump to Rust to resolve the contract identifier, function and arguments, and call the contract directly. This is what we do now and would entail no change in the code.
  2. We try to understand the contract that will be called from the top-level arguments, and recursively descend. While the compiler doesn't know the contract that will be called, under certain circumstance it may still be possible to understand given the top-level arguments. When this is the case, we can still link the appropriate function in the callee by making clever use of aliases.

If we go for option 2. one could have the compiler emit a "stub" import s-expression such as the one below, that the emitted WebAssembly for that function would only call if the linker managed to resolve the appropriate contract. Otherwise we would branch into the normal dynamic case.

;; here we would need to be careful with name collisions on "count"
(import "clarity" "count" (func $get_count (result i32)))

Thoughts on performance

Making the changes for the static case would be a clear performance win, since we don't need to jump into Rust to resolve the contract, function name, arguments, etc... or even back to Rust once the execution is complete. In the dynamic case, the gains in performance with option 2. are still very much unknown, and it may be that the linker logic required is more expensive than just performing the dynamic call itself. This is a prime candidate for exploring with a few benched.

@obycode
Copy link
Collaborator Author

obycode commented Nov 18, 2024

Thanks for this writeup! One thing that we know is true is that given the arguments to a contract call, we can know all possible contracts that could be reached through that contract call. Given that, all contracts can be loaded before entering Wasm. I believe it should be possible to implement some sort of dynamic dispatch mechanism within Wasm to call into the correct contracts based on the traits that are passed around. I do not have a full design in mind for this though. It definitely could end up being more complicated than is worth it.

I agree completely that we should definitely explore the performance of these options with benchmarks, especially if the linker option proves to be difficult to implement.

Please also include in this benchmarking the other improvement that we discussed, of simply avoiding the serialization/deserialization of the values and instead use a direct memory copy from one contract to the other. Basically switching

from: Contract A (Wasm) serialize --> Rust read from A and deserialize --> Rust serialize and write to B --> Contract B (Wasm) deserialize

to: Rust copy from contract A to contract B

@obycode
Copy link
Collaborator Author

obycode commented Nov 19, 2024

After some more thinking about this and looking into what wasm and wasmtime are capable of, I agree that our best option is to let the host handle the dynamic dispatch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: 🆕 New
Development

Successfully merging a pull request may close this issue.

3 participants