Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Update wasmtime to 0.29.0 #9552

Merged
19 commits merged into from
Sep 29, 2021
Merged

Update wasmtime to 0.29.0 #9552

19 commits merged into from
Sep 29, 2021

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Aug 12, 2021

This updates wasmtime to 0.29.0 and its new api.

  • Removes the state-holder and makes use of the data that is being attached to the state
  • Make WasmRuntime trait take mut self

Closes: #9068

polkadot companion: paritytech/polkadot#3834

@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Aug 12, 2021
@bkchr bkchr requested a review from pepyakin August 12, 2021 15:07
.and_then(|r| Ok(unpack_ptr_and_len(r)));

// Take the host state
ctx.as_context_mut().data_mut().host_state.take();
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make it clear that it's deliberate

Suggested change
ctx.as_context_mut().data_mut().host_state.take();
let _ = ctx.as_context_mut().data_mut().host_state.take();


let ret = entrypoint
.call(&mut ctx, data_ptr, data_len)
.and_then(|r| Ok(unpack_ptr_and_len(r)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that call returns a Result. If so, then and_then here seems useless and we can get away with a map?

use wasmtime::{AsContext, AsContextMut, Engine, StoreLimits};

pub(crate) struct StoreData {
limits: StoreLimits,
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is here only to be able to return this from the limiter and it is not possible to move the StoreLimits into the closure that limiter accepts. If so, it would be good to mention that here as a doc comment.


pub(crate) struct StoreData {
limits: StoreLimits,
host_state: Option<Rc<HostState>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to leave a note that this is only Some during the call into the runtime.

.data()
.host_state()
.expect(
"host functions can be called only from wasm instance;
wasm instance is always called initializing context;
therefore host_ctx cannot be None;
qed
",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we preserve vertical alignment for this change?

@gilescope
Copy link
Contributor

It's a shame the reads require &mut now, but I guess the context does get changed. I almost wonder if we could hide that away behind some internal mutability...

Copy link
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

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

I get it now: memory_as_slice_mut removed for data_mut ( https://docs.wasmtime.dev/api/wasmtime/struct.Store.html#method.data_mut ). Yeah that's much nicer.

FYI: I see the dev release of the next version of wasmtime just dropped - looks like some module loading speedups just landed (not that I know what a veneer is...)

@gilescope
Copy link
Contributor

Looks like you'll have to bump the zstd on polkadot...

bkchr added a commit to paritytech/polkadot that referenced this pull request Sep 13, 2021
@bkchr bkchr requested a review from pepyakin September 29, 2021 09:27
Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

The changes look good. Some nits left above.

@bkchr
Copy link
Member Author

bkchr commented Sep 29, 2021

The changes look good. Some nits left above.

Forgot to push my changes :D

@bkchr
Copy link
Member Author

bkchr commented Sep 29, 2021

bot merge

@ghost
Copy link

ghost commented Sep 29, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Sep 29, 2021

Merge aborted: Head SHA changed from 89b30f7 to f011fa2

@bkchr
Copy link
Member Author

bkchr commented Sep 29, 2021

bot merge

@ghost
Copy link

ghost commented Sep 29, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Sep 29, 2021

Merge aborted: Checks failed for f011fa2

@bkchr
Copy link
Member Author

bkchr commented Sep 29, 2021

bot merge

@ghost
Copy link

ghost commented Sep 29, 2021

Trying merge.

@ghost ghost merged commit 83ced6b into master Sep 29, 2021
@ghost ghost deleted the bkchr-update-wasmtime-0.29.0 branch September 29, 2021 12:30
ordian added a commit that referenced this pull request Oct 2, 2021
* master: (67 commits)
  Downstream `node-template` pull (#9915)
  Implement core::fmt::Debug for BoundedVec (#9914)
  Quickly skip invalid transactions during block authorship. (#9789)
  Add SS58 prefix for Automata (#9805)
  Clean up sc-peerset (#9806)
  Test each benchmark case in own #[test] (#9860)
  Add build with docker section to README (#9792)
  Simple Trait to Inspect Metadata (#9893)
  Pallet Assets: Create new asset classes from genesis config (#9742)
  doc: subkey usage (#9905)
  Silence alert about large-statement-fetcher (#9882)
  Fix democracy on-initialize weight (#9890)
  Fix basic authorship flaky test (#9906)
  contracts: Add event field names (#9896)
  subkey readme update on install (#9900)
  add feature wasmtime-jitdump (#9871)
  Return `target_hash` for finality_target instead of an Option (#9867)
  Update wasmtime to 0.29.0 (#9552)
  Less sleeps (#9848)
  remove unidiomatic (#9895)
  ...
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update wasmtime to 0.28
3 participants