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

x64: enable VTune support by default #3821

Merged
merged 4 commits into from
Feb 22, 2022

Conversation

abrown
Copy link
Contributor

@abrown abrown commented Feb 17, 2022

After significant work in the ittapi-rs crate, this dependency should
build without issue on Wasmtime's supported operating systems: Windows,
Linux, and macOS. The difference in the release binary is <20KB, so this
change makes vtune a default build feature. This change upgrades
ittapi-rs to v0.2.0 and updates the documentation.

After significant work in the `ittapi-rs` crate, this dependency should
build without issue on Wasmtime's supported operating systems: Windows,
Linux, and macOS. The difference in the release binary is <20KB, so this
change makes `vtune` a default build feature. This change upgrades
`ittapi-rs` to v0.2.0 and updates the documentation.
@abrown
Copy link
Contributor Author

abrown commented Feb 17, 2022

Some more details on this change:

  • I wouldn't mind a second look at what was necessary to implement the multi-OS support in ittapi-rs; e.g., right now there are bindings built for each supported OS but unsupported OS-es will cause a build failure because they export nothing (see the lib.rs file)
  • I ran cargo build --release before and after this change on a Fedora machine: without the vtune feature, the binary was 18,674,952 bytes; with vtune on by default, the binary was 18,691,768 bytes. That seemed like an appropriately-small increase.
  • Thanks to @bnjbvr and @jlb6740 for working on this as well!

@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:docs Issues related to Wasmtime's documentation labels Feb 17, 2022
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

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

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

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Thanks! Having written similar patches to test support on Windows before, this is not sufficient to enable vtune support more widely: there is also a cfg[target_os = "linux"] that need to be tweaked in profiling.rs iirc. There we should precisely enable this if the target os is linux or windows or mac, to not break compiling to other OS:es.

Then for the default feature: wonder if, in the Cargo.toml file, we can remove it from the list of dependencies (even optional), and then have per-os specific dependencies that both add the dependency and enable the feature?

@alexcrichton
Copy link
Member

In addition to what @bnjbvr mentioned to make this default I think this block and this block need updating to include vtune

@abrown
Copy link
Contributor Author

abrown commented Feb 17, 2022

cfg[target_os = "linux"] that need to be tweaked in profiling.rs iirc

This is resolved by the latest commit.

we can remove it from the list of dependencies (even optional), and then have per-os specific dependencies that both add the dependency and enable the feature

We could, yes, but it seems like we should do one or the other: either enable the vtune feature exclusively in Cargo like you suggest OR have the cfg(...) conditions in the code itself. This could be resolved in a later PR, I think.

I think this block and this block need updating to include vtune

One of those was already "defaulted" by this PR but I added a default to wasmtime-jit to be consistent.

@abrown
Copy link
Contributor Author

abrown commented Feb 17, 2022

Oh, hold on: we also need to check for x86_64 in cfg(...) as well, right?

@alexcrichton
Copy link
Member

Oh reading more closely ... personally I think it's ok to skip platform-specific checks in the #[cfg] because Wasmtime probably only really compiles on Linux/macOS/Windows anyway and other platforms likely hit compile errors elsewhere so this wouldn't be the only issue. If Wasmtime is ported to other platforms I think it'd be good to leave this in as "hey this crate probably needs an update" if possible

@abrown
Copy link
Contributor Author

abrown commented Feb 17, 2022

@bnjbvr, @alexcrichton: this removes the conditional compilation for the OS based on @alexcrichton's comment--this way, if someone ports Wasmtime to another OS but does not turn the vtune feature off, they will get a compile error in ittapi-rs which will motivate adding the OS support there.

I did add a target_arch = "x86_64" check since this feature only makes sense to be enabled on those machines anyways. ittapi-rs seems to compile fine on other architectures so there should be no problem there but this makes it more clear what environment the vtune feature is intended for.

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 good to me! I'll defer final approval to @bnjbvr though.

crates/jit/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Works fine with the wasmtime CLI binary on Linux! Thanks.

Comment on lines +35 to +36
the `ittapi` library of JIT events. But it must still be enabled at
runtime--enable runtime support based on how you use Wasmtime:
Copy link
Member

Choose a reason for hiding this comment

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

hmm, it seems the end of this comment after -- would need to be removed, as it contains : but with no extra indication thereafter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's setting up the list of ways to enable VTune at runtime (if I understand what you mean).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave it and we can fix it up later if we think of a more clear way to word this.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, i misread it as a list entry as well, my bad. Maybe can reformulate slightly then:

Suggested change
the `ittapi` library of JIT events. But it must still be enabled at
runtime--enable runtime support based on how you use Wasmtime:
the `ittapi` library of JIT events. But it must still be enabled at
runtime with one of the following:

@abrown abrown merged commit c183e93 into bytecodealliance:main Feb 22, 2022
@abrown abrown deleted the default-vtune branch February 22, 2022 16:32
mpardesh pushed a commit to avanhatt/wasmtime that referenced this pull request Mar 17, 2022
* x64: enable VTune support by default

After significant work in the `ittapi-rs` crate, this dependency should
build without issue on Wasmtime's supported operating systems: Windows,
Linux, and macOS. The difference in the release binary is <20KB, so this
change makes `vtune` a default build feature. This change upgrades
`ittapi-rs` to v0.2.0 and updates the documentation.

* review: add configuration for defaults in more places

* review: remove OS conditional compilation, add architecture

* review: do not default vtune feature in wasmtime-jit
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 wasmtime:docs Issues related to Wasmtime's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants