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

⏱️ Add cross-platform ability to profile guest code in run mode #280

Merged
merged 6 commits into from
Jul 12, 2023

Conversation

itsrainy
Copy link
Contributor

This uses the GuestProfiler that is now available in wasmtime 10.0.0 to create a profile in a location specified by the user. Right now we currently just have it working for run-mode, but are planning on adding similar functionality in serve mode, likely creating one profile per-request.

To use this, run the following:

viceroy run --profile-guest mymodule.wasm

This will result in a profile being saved to the default location (guest-profile.json), which you can upload and view on https://profiler.firefox.com.

@itsrainy itsrainy force-pushed the rainyjamey/profiling branch from 963c0c6 to 07f9bd4 Compare June 21, 2023 17:39
@itsrainy itsrainy requested a review from aturon June 21, 2023 17:40
@itsrainy itsrainy force-pushed the rainyjamey/profiling branch from 07f9bd4 to ec0714d Compare July 5, 2023 21:21
@itsrainy itsrainy removed the request for review from aturon July 5, 2023 21:27
Comment on lines 434 to 436
eprintln!();
eprintln!("Profile written to: {}", path.display());
eprintln!("View this profile at https://profiler.firefox.com/.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Newbie question: why are we writing what looks to be a success message to stderr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops I don't think that was intentional

Copy link
Contributor

Choose a reason for hiding this comment

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

I had done it that way in the equivalent code in Wasmtime to reduce the chance that this message might get mixed up with regular output from the wasm guest. I don't feel strongly about whether Viceroy should do the same, where guest output is less likely to be significant.

self,
program_name: &str,
args: &[String],
guest_profile_path: Option<&PathBuf>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Does this change, introducing a new parameter to a public function, mean we will need to release this as a new major version of the crate?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm presuming "no" because it's an additive change to the exposed binary interface (i.e. a new optional flag has been added) but if the user doesn't provide that flag the existing functionality continues to work as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant for the viceroy-lib crate, not for the cli crate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think this will probably require a bump to 0.6.0

@JakeChampion JakeChampion force-pushed the rainyjamey/profiling branch from 0bfc584 to 4d77f86 Compare July 12, 2023 09:27
@JakeChampion JakeChampion force-pushed the rainyjamey/profiling branch from 4d77f86 to f6075af Compare July 12, 2023 19:35
@JakeChampion JakeChampion merged commit ff18484 into main Jul 12, 2023
@JakeChampion JakeChampion deleted the rainyjamey/profiling branch July 12, 2023 20:04
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.

4 participants