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

Provide a way to set the interval for samples when profiling #340

Open
katef opened this issue Dec 19, 2023 · 1 comment
Open

Provide a way to set the interval for samples when profiling #340

katef opened this issue Dec 19, 2023 · 1 comment
Labels
feature-ux Concerning ergonomics and ease-of-use good first issue Good for newcomers

Comments

@katef
Copy link
Contributor

katef commented Dec 19, 2023

Currently the sample frequency for profiling in Viceroy is hardcoded to 50µs. For our purposes we don't need the frequency to be so low that performance is unaffected, but we do need it to be high enough to report on small things. Setting it as a CLI option seems reasonable to me.

wasmtime provides --profile=guest[,path[,interval]] which allows setting the interval between samples for profiling:
https://github.com/bytecodealliance/wasmtime/blob/main/docs/examples-profiling-guest.md?plain=1#L7-L11:

To use this profiler with the Wasmtime CLI, pass the
--profile=guest[,path[,interval]] flag.

  • path is where to write the profile, wasmtime-guest-profile.json by default
  • interval is the duration between samples, 10ms by default

I think it'd make sense to match this, Viceroy already has similar syntax for its --profile cli flag.

@katef katef added feature-ux Concerning ergonomics and ease-of-use good first issue Good for newcomers labels Dec 19, 2023
@jameysharp
Copy link
Contributor

I'm in favor of this suggestion. There's a subtlety though, which is that the EPOCH_INTERRUPTION_PERIOD is used regardless of whether the guest profiler is enabled or not. It also has an impact on how long the guest can run before async background tasks get an opportunity to make progress. As I recall, Rainy and I had some trouble with Viceroy's test suite unless we set the interval fairly short.

It's probably worth putting some thought into what the default interval should be in both cases: 10ms for Wasmtime and 50µs for Viceroy are both pretty arbitrary choices. It makes some sense that they're different if we assume that Viceroy guests are usually short-lived compared with Wasmtime guests, but the exact durations are still not well justified.

One Viceroy user reported a cost I hadn't considered of having a short interval: There's a thread that wakes up briefly on that interval, even if no guests are running at the time, and that prevents the CPU from going into deeper power-saving states. So Viceroy users on battery might need a longer interval for power management reasons. Eventually it'd be nice to pause the epoch-interruption thread while there are no requests in flight, but that's some extra complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-ux Concerning ergonomics and ease-of-use good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants