-
Notifications
You must be signed in to change notification settings - Fork 43
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
Conversation
c09eb57
to
21f4af5
Compare
a702bd5
to
b325786
Compare
Updated significantly:
This is getting close to where I want to merge it, I think it's a fairly good set of perf wins. |
There was a problem hiding this 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.
b325786
to
641e14c
Compare
Updated to address all comments (and all revisions from CAP review process today) |
bf8bcf2
to
b25cdac
Compare
b25cdac
to
728c1be
Compare
There was a problem hiding this 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). |
There was a problem hiding this comment.
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" { |
There was a problem hiding this comment.
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.
#[derive(Clone)] | ||
pub struct VmInstantiationSample { | ||
pub id: Option<Hash>, | ||
pub wasm: Vec<u8>, | ||
pub module: Rc<ParsedModule>, |
There was a problem hiding this comment.
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.
@@ -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#" |
There was a problem hiding this comment.
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).
Added #1374 to follow up on setting up |
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.
This is a sketch of the first step in the plan for addressing #1292 and eventually #827 and #1313, namely:
This PR has accompanying changes in XDR and wasmi:
Remaining to do:
add more code to wasmi to enable observing more inputs