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

add feature wasmtime-jitdump #9871

Merged
merged 12 commits into from
Sep 29, 2021

Conversation

librelois
Copy link
Contributor

Expose wasmtime optional feature jitdump to be able to profile wasm execution with perf.

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.

There are a couple of notes:

  1. I am all for for the profiler. The jitdump profiler is not that heavy to my knowledge and should not be a problematic to be built by default.
  2. I am however not entirely sure about the way it is activated. The configuration IMO should not take place in ad-hoc places like deep in a crate. It should come from the CLI crate (or whatever place is more appropriate in the setting in question). However, I do not intend to block this PR because of that — I realize it is important for the stuff we are doing and block won't be productive.
  3. I am not entirely sure that setting some other value should lead to an error. I would prefer to ignore such a value, ideally printing a single warning message. That is not blocking as well.

librelois and others added 2 commits September 28, 2021 14:31
Co-authored-by: Sergei Shulepov <s.pepyakin@gmail.com>
@librelois librelois requested a review from bkchr September 28, 2021 12:47
@librelois
Copy link
Contributor Author

@pepyakin i'm applied 1. and 3. :)
For 2, it would require a lot more changes and we need jitdump quickly, are you ok that we go through an env var for the moment and that the creation of a cli option is for future PR?

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

TY

@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. labels Sep 28, 2021
@bkchr bkchr requested a review from pepyakin September 28, 2021 17:28
@bkchr bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Sep 28, 2021
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.

Otherwise, LGTM.

@bkchr bkchr merged commit 0d44d83 into paritytech:master Sep 29, 2021
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)
  ...
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.

4 participants