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

Support vtune profiling of trampolines too #3687

Merged
merged 7 commits into from
Jan 19, 2022

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Jan 13, 2022

This refactors the code as suggested to avoid duplication of demangling (and allows having a single crate depend on the demangling crate dependencies). Also implements supports for trampolines in vtune.

After poking at the code, my understanding was that module.trampolines() contained metadata about trampolines for exported wasm functions, while trampoline_load() would be called for trampolines into and out of imported wasm functions. Is that true @alexcrichton? Weirdly enough, even by adding some println statements to check if those trampolines were generated, I could only see the printed messages for module.trampolines(), not for the other one. Am I missing something in there?

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jan 13, 2022
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

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.

Looks great to me, thanks!

/// Unique identifier for the jitted function
method_id: HashMap<(usize, DefinedFuncIndex), u32>,
/// Unique identifier for the jitted code region.
method_id: HashMap<MethodKind, u32>,
Copy link
Member

Choose a reason for hiding this comment

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

Reading over this now, there's actually a fair bit supporting this map right here, but this map is only used for an assert that each item inserted is unique. I think that may mean perhaps that we could remove this entirely? (and not worry about MethodKind as a new enum)

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, the module id we feed ourselves and the signature index will always be different anyways, so this sanity check isn't worth the trouble.

Kept the mutex, though, because I'm unclear whether the ittapi function can be called from several threads at the same time 🤔

@alexcrichton
Copy link
Member

Oh sorry and to clarify, you're right that module.trampolines() is trampolines for everything exported from that module. This is basically a trampoline-per-type-signature for any functions which are "possibly exported", including all those in tables, exported functions, etc.

The trampoline_load() method is only called for trampolines dynamically created through the Func::new API which generates a one-off trampoline specifically for the signature of the function created. Unless Func::new is called somehow (which it ideally shouldn't be because Func::wrap is a faster embedding) then trampoline_load() won't be invoked.

@bnjbvr
Copy link
Member Author

bnjbvr commented Jan 17, 2022

Updated! Also refactored a bit how the loading of dynamic trampolines is done, so that there's less code duplication with the jitdump_linux; if that approach looks good to you too, I could go ahead and do the same for single functions and trampolines for exported signatures?

@bnjbvr bnjbvr force-pushed the profiling-trampolines branch from cbfebcf to 1d77b37 Compare January 17, 2022 16:38
@alexcrichton
Copy link
Member

Looks great to me, thanks! Did you want to do that refactoring as a follow-up or attach here? (either way is fine by me)

@bnjbvr
Copy link
Member Author

bnjbvr commented Jan 19, 2022

Did you want to do that refactoring as a follow-up or attach here? (either way is fine by me)

Can do that here! I wanted to make sure this is something we wanted first 👍

@bnjbvr
Copy link
Member Author

bnjbvr commented Jan 19, 2022

Ended up giving up on the refactoring (instead of loading a whole module at once, load function per function), because:

  • if the profiling agent is the null one, that would have done a bit more work that ends up being wasted (computing the function's pointer, address, default function debug display name, etc.).
  • this introduced a bit of infra to generate the default function debug display name on demand
  • the vtune profiling agent required a new ProfilingAgent trait method to know that we started loading a new module (so it would know what module display name to generate).

Anyhow, this added more lines of code than there was before, and made the code messier overall, so not a win in my opinion. Spotted a data structure that could be removed, though!

@bnjbvr bnjbvr requested a review from alexcrichton January 19, 2022 15:09
@alexcrichton
Copy link
Member

Seems reasonable to me!

@alexcrichton alexcrichton merged commit 2649d23 into bytecodealliance:main Jan 19, 2022
@bnjbvr bnjbvr deleted the profiling-trampolines branch January 19, 2022 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants