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

vtune: smoke test profiling in CI #9185

Merged
merged 4 commits into from
Sep 6, 2024
Merged

Conversation

abrown
Copy link
Contributor

@abrown abrown commented Aug 29, 2024

This checks that Wasmtime's --profile=vtune functionality continues to work. See commit messages for more details.

@abrown abrown force-pushed the vtune-testing branch 5 times, most recently from f210c88 to c3d2e41 Compare August 29, 2024 18:52
assert!(stdout.contains("CPU Time"));
println!("> stderr:\n{stderr}");
assert!(!stderr.contains("Error"));
assert!(!stderr.contains("Warning"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for searching for this kind of string is due to error dumps like this one.

@abrown abrown force-pushed the vtune-testing branch 2 times, most recently from f9d27d2 to d054145 Compare August 29, 2024 19:25
@abrown abrown marked this pull request as ready for review August 29, 2024 22:40
@abrown abrown requested review from a team as code owners August 29, 2024 22:40
@abrown abrown requested review from pchickey and removed request for a team August 29, 2024 22:40
@abrown abrown force-pushed the vtune-testing branch 2 times, most recently from 450d531 to a85a717 Compare August 29, 2024 22:42
@abrown
Copy link
Contributor Author

abrown commented Aug 29, 2024

One thing to note here: I've tried to only install VTune where needed (when we check wasmtime-cli) because it increases the build time by at least a minute. I know we've worked in the past to reduce build times so there's a trade-off to think about: faster builds or test more features?

&std::env::temp_dir().to_string_lossy(),
// ...then run Wasmtime with profiling enabled:
&get_wasmtime_path()?.to_string_lossy(),
"--profile=vtune",
Copy link
Member

Choose a reason for hiding this comment

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

To confirm, does this test fail if this flag is left out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, one can always run vanilla Wasmtime in VTune and that should be valid whether this flag is present or not. What this flag does is inform VTune about the JIT-compiled code so it can display it appropriately in the UI. We've had problems in the past with incorrectly telling VTune about this code which did result in errors and that is what this test is checking: in effect, this is a regression test to check that we correctly tell VTune about the JIT code.

@alexcrichton
Copy link
Member

The integration path here seems like a reasonable starting point to me, thanks! I think it's fine to start here and see how it goes

@abrown
Copy link
Contributor Author

abrown commented Sep 4, 2024

The integration path here seems like a reasonable starting point to me, thanks! I think it's fine to start here and see how it goes

I worked on the install-vtune-action a bit more and v2 can now install VTune in 20 seconds using a ~700MB cache (see here). I haven't switched to that version for this PR because I'm not sure if we can afford caches that large; just saying it's available.

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.

Ok sounds good to me 👍. I think things run fast enough here we can eat the extra minute for vtune, but if that becomes an issue we can investigate caching in the future.

@alexcrichton alexcrichton added this pull request to the merge queue Sep 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 4, 2024
@abrown
Copy link
Contributor Author

abrown commented Sep 6, 2024

Not sure what is going on with CI: with the latest matrix.filter == 'linux-x64' change, VTune no longer installs in the riscv64 and s390x runs, but somehow the check that runs vtune --version is returning successfully (it should not!) and the full test attempts to run. Is the vtune --version check incorrect somehow or is there some GitHub runner cached state causing issues here? [edit: let's try a rebase...]

This adds a smoke test for checking that the VTune functionality still
works.
- if `vtune` is not on the system path, skip the test
- if it is, run Wasmtime with VTune profiling enabled
- check that it captured the CPU time and didn't fail.

This test doesn't rigorously check all the ins and outs of the VTune
functionality but is better than before.
In order to actually run the VTune CLI test, we need `vtune` on the
system. The `install-vtune-action` I created does this ([script]).

[script]: https://github.com/abrown/install-vtune-action/blob/main/install.sh
Running the VTune installation and CLI test on any `ubuntu` runner meant
that even test runs for `riscv64` (emulated on QEMU) would attempt this.
Instead, restrict this test only to `linux-x64` runs.

prtest:full
@alexcrichton
Copy link
Member

Ah I think the issue is that this needs to be skipped in qemu. Otherwise it's running a non-x64 binary in vtune which isn't going to work. We don't have a great "am I in qemu" test other than #[cfg(target_arch = "...")] so running this test only on x64 should be fine

@abrown abrown added this pull request to the merge queue Sep 6, 2024
Merged via the queue into bytecodealliance:main with commit 2129072 Sep 6, 2024
123 checks passed
@abrown abrown deleted the vtune-testing branch September 6, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants