-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Enable jitdump profiling support by default #1310
Conversation
cc @jlb6740 I was playing around with this some more and this, for me at least, also seems to support In any case I'm curious on your thoughts on this! |
@alexcrichton I think initially this was designed to not be the case out of concern to not bring in dependent crates if profiling support wasn't desired. Specifically for a production build jitdump support should not be baked in by default I don't think. Also, when vtune support is added should that also be default? Should we just make this default for the debug build but optional for the production build? |
Oh definitely yeah, I think we want the ability to build without profiling support, but on reading all this it looks like the profiling support is pretty lightweight so it seemed reasonable to enable by default. The binaries we ship I think we're ok enabling them, and then users building their own binaries still have the option to turn this feature on/off. I'm fine going either way myself, I just found this to be so slick once it worked it seemed like we'd want it in official builds by default :) |
Subscribe to Label ActionThis issue or pull request has been labeled: "wasi", "wasmtime:api", "wasmtime:c-api", "wasmtime:docs" Users Subscribed to "wasmtime:api"Users Subscribed to "wasmtime:c-api"To subscribe or unsubscribe from this label, edit the |
@@ -0,0 +1,101 @@ | |||
# Profiling WebAssembly |
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 great. I see the link to it in SUMMAR.md, but does the SUMMARY.md get published anywhere? I could only find it on the github view of this branch by clicking through the source tree and not any official links to docs found on the opening Readme.
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.
Ah yeah these are auto-published from the master branch at https://bytecodealliance.github.io/wasmtime/
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.
In particular the SUMMARY.md
is the table of contents on the left hand side. More info in the mdbook
user guide: https://rust-lang.github.io/mdBook/format/summary.html
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.
@fitzgen .. Ahh I see. Thanks. I clicked on docs here linked to at the bottom here: https://github.com/alexcrichton/wasmtime/tree/profiling for this pr, but I didn't notice that link was going to be linking back to master. Got to be carefully following links like that.
This the result of some of the investigation I was doing for bytecodealliance#1017. I've done a number of refactorings here which culminated in a number of changes that all amount to what I think should result in jitdump support being enabled by default: * Pass in a list of finished functions instead of just a range to ensure that we're emitting jit dump data for a specific module rather than a whole `CodeMemory` which may have other modules. * Define `ProfilingStrategy` in the `wasmtime` crate to have everything locally-defined * Add support to the C API to enable profiling * Documentation added for profiling with jitdump to the book * Split out supported/unsupported files in `jitdump.rs` to avoid having lots of `#[cfg]`. * Make dependencies optional that are only used for `jitdump`. * Move initialization up-front to `JitDumpAgent::new()` instead of deferring it to the first module. * Pass around `Arc<dyn ProfilingAgent>` instead of `Option<Arc<Mutex<Box<dyn ProfilingAgent>>>>` The `jitdump` Cargo feature is now enabled by default which means that our published binaries, C API artifacts, and crates will support profiling at runtime by default. The support I don't think is fully fleshed out and working but I think it's probably in a good enough spot we can get users playing around with it!
This the result of some of the investigation I was doing for #1017. I've
done a number of refactorings here which culminated in a number of
changes that all amount to what I think should result in jitdump support being
enabled by default:
ensure that we're emitting jit dump data for a specific module rather
than a whole
CodeMemory
which may have other modules.ProfilingStrategy
in thewasmtime
crate to have everythinglocally-defined
jitdump.rs
to avoid havinglots of
#[cfg]
.jitdump
.JitDumpAgent::new()
instead ofdeferring it to the first module.
Arc<dyn ProfilingAgent>
instead ofOption<Arc<Mutex<Box<dyn ProfilingAgent>>>>
The
jitdump
Cargo feature is now enabled by default which means thatour published binaries, C API artifacts, and crates will support
profiling at runtime by default. The support I don't think is fully
fleshed out and working but I think it's probably in a good enough spot
we can get users playing around with it!