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

Decompose VM instantiation costs and add module cache #1359

Merged
merged 16 commits into from
Mar 26, 2024

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Feb 29, 2024

This is a sketch of the first step in the plan for addressing #1292 and eventually #827 and #1313, namely:

  • add a bunch of cost types that decompose the current "worst case" VM instantiation cost type
  • continue to charge the worst case on initial contract upload
  • store the decomposed cost-type inputs in the ledger, since we can observe them after the initial upload parse
  • use those decomposed cost-type inputs when doing runtime instantiation
  • add a module cache
  • populate the module cache with all modules on host startup
  • use cached modules during instantiation

This PR has accompanying changes in XDR and wasmi:

Remaining to do:

@graydon graydon force-pushed the cheaper-instantiation branch from c09eb57 to 21f4af5 Compare March 1, 2024 07:48
@graydon graydon force-pushed the cheaper-instantiation branch 5 times, most recently from a702bd5 to b325786 Compare March 9, 2024 07:39
@graydon
Copy link
Contributor Author

graydon commented Mar 9, 2024

Updated significantly:

  • Does intra-tx caching
  • Does minimal linking
  • Splits parsing from instantiation to, I think, be more ready for wasmi 0.9
  • Drives parser manually to get refined costs rather than delegating to wasmi
  • Added calibration and expanded refined costs a bit more
  • Added checks for complexity of const exprs
  • Protocol gated
  • feature="next" gated

This is getting close to where I want to merge it, I think it's a fairly good set of perf wins.

Copy link
Contributor

@dmkozh dmkozh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at the host code (not benchmarks) and it looks good to me, only left a few small comments.

I think we'll need to be able to re-observe the existing tests with "next" feature enabled and with protocol 21 enabled, but that doesn't need to belong to this PR.

soroban-env-host/src/host/data_helper.rs Outdated Show resolved Hide resolved
soroban-env-host/src/vm/parsed_module.rs Outdated Show resolved Hide resolved
soroban-env-host/src/vm/parsed_module.rs Outdated Show resolved Hide resolved
soroban-env-host/src/vm/parsed_module.rs Outdated Show resolved Hide resolved
soroban-env-host/src/budget/model.rs Show resolved Hide resolved
soroban-env-host/src/host/data_helper.rs Outdated Show resolved Hide resolved
soroban-env-host/src/budget.rs Show resolved Hide resolved
soroban-env-host/src/vm/parsed_module.rs Outdated Show resolved Hide resolved
@graydon graydon force-pushed the cheaper-instantiation branch from b325786 to 641e14c Compare March 15, 2024 05:37
@graydon
Copy link
Contributor Author

graydon commented Mar 15, 2024

Updated to address all comments (and all revisions from CAP review process today)

@graydon graydon force-pushed the cheaper-instantiation branch 5 times, most recently from bf8bcf2 to b25cdac Compare March 22, 2024 07:30
@graydon graydon force-pushed the cheaper-instantiation branch from b25cdac to 728c1be Compare March 23, 2024 05:41
@graydon graydon changed the title Sketch of cheaper instantiation change Decompose VM instantiation costs and add module cache Mar 23, 2024
@graydon graydon marked this pull request as ready for review March 23, 2024 07:24
@graydon graydon enabled auto-merge March 23, 2024 07:31
Copy link
Contributor

@jayz22 jayz22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pointed out a few minor things that are can be addressed later.

// nearly-the-same) nonzero constant term in each `CostComponent`. We
// correct this here by zeroing out the constant term in all but the first
// `CostComponent` of each set, (and attempting to confirm that they all
// have roughly-the-same constant term).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting approach. Glad it works.
I would've suggested using the new_baseline_case to account for these types of high constant overhead. Right now the baseline case is used for eliminating measurement noise, but it could be repurposed for here too.

let mut me = ModEmitter::default().add_bench_import();
let names = Vm::get_all_host_functions();
for (module, name, arity) in names.iter().take(n) {
if *module == "t" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: if you don't call the add_bench_import before, then you don't have to skip "t" here. Then you get a smooth linear increase in number of imported functions.

soroban-env-host/src/vm/parsed_module.rs Show resolved Hide resolved
soroban-env-host/src/budget.rs Show resolved Hide resolved
soroban-env-host/src/budget.rs Show resolved Hide resolved
soroban-env-host/src/budget.rs Show resolved Hide resolved
soroban-env-host/src/budget.rs Show resolved Hide resolved
#[derive(Clone)]
pub struct VmInstantiationSample {
pub id: Option<Hash>,
pub wasm: Vec<u8>,
pub module: Rc<ParsedModule>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it would be more clear to split this into VmParseSample and VmInstantiationSample. I was a bit confused seeing the Parse measurements using the instantiation sample at first glance.

soroban-env-host/src/vm/module_cache.rs Show resolved Hide resolved
@@ -383,7 +383,8 @@ fn total_amount_charged_from_random_inputs() -> Result<(), HostError> {
}

let actual = format!("{:?}", host.as_budget());
expect![[r#"
#[cfg(not(feature = "next"))]
let expected = expect![[r#"
Copy link
Contributor

@jayz22 jayz22 Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the tracker above should include new rows corresponding to the newly added cost types, such that the budget doesn't charge 0 for them (for the checksum to be nontrivial).

@graydon graydon added this pull request to the merge queue Mar 26, 2024
Merged via the queue into main with commit 41b4ee3 Mar 26, 2024
13 checks passed
@graydon graydon deleted the cheaper-instantiation branch March 26, 2024 19:47
@jayz22
Copy link
Contributor

jayz22 commented Mar 27, 2024

Added #1374 to follow up on setting up ModuleCache, calibrating its cost, and charging for it in the hit path.

@graydon graydon mentioned this pull request Mar 28, 2024
github-merge-queue bot pushed a commit that referenced this pull request Apr 2, 2024
This is a set of followups to #1359 to cover missing testing and wiring
around the module cache. It includes calibration of the
`VmCachedInstantiation` cost type, a shift to using a single shared
linker when using a module cache (this turns out to be critical -- the
existing code deadlocks on cross-contract calls) and some fairly subtle
wiring to patch up costs in recording mode, along with some tests to
check that it's mostly working.

Several tests remain broken with --feature=next, so I wouldn't say it's
quite ready yet. I tried, but am out of time for tonight.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants