Switch from wasmer to wasmtime #457
Merged
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
|
benchmarks please |
Centril
reviewed
Oct 20, 2023
|
There seem to be quite a few refactorings that are independent of the move to wasmtime; could these be extracted and reviewed separately? That could also make the benchmark more reliable. |
If we do end up merging this (and I will not do so without performance data that demonstrates we understand the impact), then we should break this up and make sure we document it well. Right now it's just a draft to evaluate performance I believe. |
Benchmark resultsBenchmark ReportLegend:
All throughputs are single-threaded. Empty transaction
Single-row insertions
Multi-row insertions
Full table iterate
Find unique key
Filter
Serialize
Module: invoke with large arguments
Module: print bulk
Remaining benchmarks
|
|
hmmm :/ idk what's up with this. how old are the old benchmark numbers? |
|
They should be from master. That said, this PR does quite a lot, including upgrade of a bunch of dependencies and some minor refactorings, so it's hard to attribute the performance difference to just the engine change. It would be better to split them out and keep this change isolated. |
RReverser
reviewed
Oct 24, 2023
|
@RReverser well, approve #266 and we can do that :)) |
coolreader18
force-pushed
the
noa/wasmtime
branch
from
fc53526 to
5d5b9fb
Compare
I started benchmarks on that one for now :) (also I only reread conversation above which had the same points raised already, only after leaving my comment lol) |
coolreader18
force-pushed
the
noa/wasmtime
branch
from
5d5b9fb to
6b55323
Compare
Benchmark resultsBenchmark ReportLegend:
All throughputs are single-threaded. Empty transaction
Single-row insertions
Multi-row insertions
Full table iterate
Find unique key
Filter
Serialize
Module: invoke with large arguments
Module: print bulk
Remaining benchmarks
|
Benchmark resultsBenchmark ReportLegend:
All throughputs are single-threaded. Empty transaction
Single-row insertions
Multi-row insertions
Full table iterate
Find unique key
Filter
Serialize
Module: invoke with large arguments
Module: print bulk
Remaining benchmarks
|
coolreader18
force-pushed
the
noa/wasmtime
branch
from
6b55323 to
e4f8c0f
Compare
coolreader18
force-pushed
the
noa/wasmtime
branch
3 times, most recently
from
221a604 to
3a99f58
Compare
gefjon
reviewed
Nov 2, 2023
crates/core/src/host/wasmer/mod.rs
Outdated
| let mut config = wasmtime::Config::new(); | ||
| config | ||
| .cranelift_opt_level(wasmtime::OptLevel::Speed) | ||
| .consume_fuel(true) |
There was a problem hiding this comment.
In the future, I think we should investigate whether epoch interruption is sufficient for our needs. Within this PR, fuel seems like the right thing to use, as it's the closest translation of Wasmer metering.
There was a problem hiding this comment.
Would you mind filing a ticket about this?
kulakowski
reviewed
Nov 6, 2023
There was a problem hiding this comment.
I think the code as such is fine and pretty thoroughly reviewed now so I'll say what I think we need to do to land this.
edit: added the need for comments
edit: added the task to fix the energy/fuel exchange rate based on a run of some chunky reducer
- appropriate levels of doc comments
- ensure no perf regressions
- measure and align the energy/fuel exchange rate
- ensure energy monitoring is sensible when deployed
- understand what is needed to deploy this for production
- is the "wasmer" -> "wasmtime" name change going to break any deployment scripts?
- is that same name change going to affect control db instances?
- test on all supported OSes
- linux
- macos
- windows
|
|
||
| impl WasmtimeFuel { | ||
| /// 1000 energy quanta == 1 wasmtime fuel unit | ||
| const QUANTA_MULTIPLIER: i128 = 1_000; |
There was a problem hiding this comment.
I'd like to see some explicit validation that the test deployment of this had energy consumption metered appropriately.
As discussed elsewhere, internal testing has shown mild to moderate performance improvements compared to v0.7.2-beta on one of BitCraft's hot loops. That testing was not particularly rigorous, and it's possible there are edge-case-ey regressions we haven't caught - e.g. Wasmtime might make module compilation/instantiation more expensive - but I feel comfortable saying that the part we care about, module execution, is at least no worse. |
coolreader18
force-pushed
the
noa/wasmtime
branch
3 times, most recently
from
e8d5179 to
13d6877
Compare
|
@kazimuth just pushed a commit that lets wasmtime cache compilation artifacts to the filesystem - I think you were looking for something like this for benchmarking, right? |
|
(if someone feels that's too tangential/confounding to this PR, fair, I'm happy to split it off to a follow-up) |
Centril
reviewed
Nov 8, 2023
coolreader18
force-pushed
the
noa/wasmtime
branch
4 times, most recently
from
ec83358 to
45e2837
Compare
coolreader18
force-pushed
the
noa/wasmtime
branch
from
45e2837 to
d82a5e2
Compare
kulakowski
approved these changes
Nov 17, 2023
| mut caller: Caller<'_, Self>, | ||
| func: AbiCall, | ||
| f: impl FnOnce(&mut Caller<'_, Self>) -> WasmResult<()>, | ||
| ) -> RtResult<u32> { |
There was a problem hiding this comment.
re the u16 -> u32 change throughout this file: is this ultimately stemming from a wasmtime api? like does it only let you have fn types whose return type impls some trait goop, and it bottoms out in only u32s etc because wasm only has i32s and not i16s?
I'm assuming the same for the anyhow::Result change in RtResult.
There was a problem hiding this comment.
Yeah, which was the same for wasmer and the difference is that wasmtime only lets you use {i,u,f}{32,64} for those, while wasmer was happy to truncate/zero-extend with other integer types.
|
wahoo! |
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.
Description of Changes
A draft PR to run benchmarks on wasmtime vs wasmer
API and ABI
The host_type parameter to
/publishis renamed from wasmer to wasmtime, but I don't think that it was even being checked anyway.