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.

Sign up for GitHub

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

Jump to bottom

profiler: clamp CPU duration at profile period #1486

Merged
merged 2 commits into from Sep 29, 2022
Merged

profiler: clamp CPU duration at profile period #1486

merged 2 commits into from Sep 29, 2022

Conversation

nsrip-dd
Copy link
Contributor

If a user sets a profile period shorter than the default CPU duration,
but neglects to update the CPU duration, CPU profiling will still take
the default duration, exceeding the expected profile period. Add a check
to clamp the CPU duration at the configured period.

@nsrip-dd
profiler: clamp CPU duration at profile period
5ca9cf2 
If a user sets a profile period shorter than the default CPU duration,
but neglects to update the CPU duration, CPU profiling will still take
the default duration, exceeding the expected profile period. Add a check
to clamp the CPU duration at the configured period.
@nsrip-dd nsrip-dd requested a review from a team as a code owner September 28, 2022 15:08
@nsrip-dd nsrip-dd added this to the Triage milestone Sep 28, 2022
felixge
felixge approved these changes Sep 29, 2022
View reviewed changes
Copy link
Member

@felixge felixge left a comment •
edited

Choose a reason for hiding this comment

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

LGTM. Thanks! I guess my only question is if there was a specific issue motivating this change? Or is it just something you noticed and wanted to improve?

@nsrip-dd
Copy link
Contributor Author

Yes, I've encountered this behavior a few times when writing tests (like TestStopLatency or TestAllUploaded) where I set a much shorter profile period, and was then surprised that the tests took so long to run. I ran into it again while working on #1485 and decided to just fix it. I know some of our customers choose shorter profile periods, so they might run into this as well.

@nsrip-dd nsrip-dd merged commit 31366dd into main Sep 29, 2022
@nsrip-dd nsrip-dd deleted the nick.ripley/clamp-cpu-duration branch September 29, 2022 15:25
Hellzy pushed a commit that referenced this pull request Sep 30, 2022
@nsrip-dd @Hellzy
profiler: clamp CPU duration at profile period (#1486)
05bc673 
If a user sets a profile period shorter than the default CPU duration,
but neglects to update the CPU duration, CPU profiling will still take
the default duration, exceeding the expected profile period. Add a check
to clamp the CPU duration at the configured period.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixge felixge felixge approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Triage
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nsrip-dd @felixge