Stream: git-wasmtime

Topic: wasmtime / issue #3821 x64: enable VTune support by default

Wasmtime GitHub notifications bot (Feb 17 2022 at 01:21):

abrown commented on issue #3821:

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!

Wasmtime GitHub notifications bot (Feb 17 2022 at 02:01):

github-actions[bot] commented on issue #3821:

Subscribe to Label Action

cc @peterhuene

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Feb 17 2022 at 15:30):

alexcrichton commented on issue #3821:

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

Wasmtime GitHub notifications bot (Feb 17 2022 at 16:56):

abrown commented on issue #3821:

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.

Wasmtime GitHub notifications bot (Feb 17 2022 at 17:26):

abrown commented on issue #3821:

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

Wasmtime GitHub notifications bot (Feb 17 2022 at 18:43):

alexcrichton commented on issue #3821:

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

Wasmtime GitHub notifications bot (Feb 17 2022 at 18:52):

abrown commented on issue #3821:

@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.

Last updated: Nov 22 2024 at 17:03 UTC