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

Accept explicit CPU delta time in GuestProfiler #7873

Merged
merged 4 commits into from
Feb 7, 2024

Conversation

Milek7
Copy link
Contributor

@Milek7 Milek7 commented Feb 5, 2024

Currently GuestProfiler::sample always records zero CPU time delta. This makes resulting profiles hard to interpret, especially because sampling is usually performed from epoch callback, which necessarily means that no samples at all are captured when guest is not running, thus there is no distinguishable "idle" stacktrace of any sort.

While it is possible to read OS thread time counters for that, GuestProfiler is not aware whether current thread is used exclusively for running that guest. For example, I'm running multiple guests on single-threaded event loop and that would partially misattribute CPU time to neighboring guests on first/last sample. Thus I think that the embedder is better suited to measure and provide time delta to sampling function.

I left old signature intact not to break any existing users. For symmetry C bindings also got new signature, though maybe old one should be removed as it doesn't have any users yet?

@Milek7 Milek7 requested a review from a team as a code owner February 5, 2024 19:53
@Milek7 Milek7 requested review from fitzgen and removed request for a team February 5, 2024 19:53
@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. labels Feb 5, 2024
Copy link

github-actions bot commented Feb 5, 2024

Subscribe to Label Action

cc @peterhuene

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

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api, wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton
Copy link
Member

Thanks for the PR! It's ok to break the current signature and change it, so I think I would agree with you that it seems like it might be best to unconditionally take a Duration argument. Could you update the documentation to indicate that passing Duration::ZERO is ok but might have some consequences when viewing the results?

@Milek7 Milek7 changed the title Add function accepting explicit CPU delta time to GuestProfiler Accept explicit CPU delta time in GuestProfiler Feb 6, 2024
@alexcrichton alexcrichton added this pull request to the merge queue Feb 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 6, 2024
@alexcrichton alexcrichton added this pull request to the merge queue Feb 7, 2024
Merged via the queue into bytecodealliance:main with commit 7d0bdcc Feb 7, 2024
19 checks passed
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:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants