Skip to content

allow perfmaps on any unix platform#6701

Merged
alexcrichton merged 2 commits into
bytecodealliance:mainfrom
Maaarcocr:main
Jul 7, 2023
Merged

allow perfmaps on any unix platform#6701
alexcrichton merged 2 commits into
bytecodealliance:mainfrom
Maaarcocr:main

Conversation

@Maaarcocr
Copy link
Copy Markdown
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

cfg_if::cfg_if! {
if #[cfg(target_os = "linux")] {
if #[cfg(unix)] {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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