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

allow perfmaps on any unix platform #6701

Merged
merged 2 commits into from
Jul 7, 2023

Conversation

Maaarcocr
Copy link
Contributor

why?

perfmaps are fairly simple files that are written to /tmp/ and that are read by perf. It does kind of make sense to have them only work for linux, but with more modern profilers also adopting some of the perf standards like samply, which can run both on linux and on macos, it may make sense to be able to produce these artefacts also on macos.

I know that samply does already support jitdumps, but currently the jitdump implementation here cannot run outside of linux as it uses gettid. Samply also supports perf maps, on both linux and macos, and the perf map implementation here is fairly simple and can run on both platforms.

This would be very beneficial for developers using macos to their code.

how?

change the cfg! statement to compile for all unix platforms.

@Maaarcocr Maaarcocr requested a review from a team as a code owner July 7, 2023 10:31
@Maaarcocr Maaarcocr requested review from alexcrichton and removed request for a team July 7, 2023 10:31
@@ -20,7 +20,7 @@ cfg_if::cfg_if! {
}

cfg_if::cfg_if! {
if #[cfg(target_os = "linux")] {
if #[cfg(unix)] {
Copy link
Contributor

@bjorn3 bjorn3 Jul 7, 2023

Choose a reason for hiding this comment

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

Can you also change this in cranelift/jit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@Maaarcocr Maaarcocr requested a review from a team as a code owner July 7, 2023 10:49
@Maaarcocr Maaarcocr requested review from cfallin and removed request for a team July 7, 2023 10:49
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Jul 7, 2023
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks!

@alexcrichton alexcrichton added this pull request to the merge queue Jul 7, 2023
Merged via the queue into bytecodealliance:main with commit e3d7b8a Jul 7, 2023
@jameysharp
Copy link
Contributor

Cool, thank you for this! There's no one "perfect" approach to profiling, so having more options on more platforms is great.

In case you weren't aware, I want to mention we recently merged a built-in profiler in Wasmtime that works on all platforms. It has some limitations, but in exchange, it doesn't require installing samply or perf or anything else.

It would be great if you could add some documentation for using samply with Wasmtime's perfmaps—especially if you can cover how to do it on macOS. See https://docs.wasmtime.dev/examples-profiling.html for the existing docs; the sources are in this repo in docs/examples-profiling*.md.

Even if Alex hadn't already merged this I was going to suggest that the documentation could be a separate PR, but now that it's merged it should definitely be a separate PR. 😆

@Maaarcocr
Copy link
Contributor Author

Maaarcocr commented Jul 7, 2023

Cool, thank you for this! There's no one "perfect" approach to profiling, so having more options on more platforms is great.

In case you weren't aware, I want to mention we recently merged a built-in profiler in Wasmtime that works on all platforms. It has some limitations, but in exchange, it doesn't require installing samply or perf or anything else.

It would be great if you could add some documentation for using samply with Wasmtime's perfmaps—especially if you can cover how to do it on macOS. See https://docs.wasmtime.dev/examples-profiling.html for the existing docs; the sources are in this repo in docs/examples-profiling*.md.

Even if Alex hadn't already merged this I was going to suggest that the documentation could be a separate PR, but now that it's merged it should definitely be a separate PR. 😆

yes I'm aware of the profiler, super cool work! (I also made a separate project that is very similar in https://github.com/Shopify/wasmprof)

my main use case for samply is profiling both native code (not running inside the wasm sandbox) and the wasm code, so the native wasmtime profiler (or wasmprof) does not work for me! (This is mostly because we embed wasmtime in a bigger application, so I'm interested in the whole system performance not just the code running in wasm)

And yes, I'll make a PR for docs too, thank you for the suggestion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants