[release/6.0] Fix EventPipe sample profiler resolution on Windows. #59056
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
If sample profiler was initially created before sampling has been enabled, happens when setting up sessions during startup, sample_profiler_load_dependecies won't get called, meaning we won't setup timeBeginPeriod/timeEndPeriod on Windows, and won't adjust default scheduling resolution to 1ms staying on default 16ms. There is a ref count check in current sample_profiler_enable checking if we should call sample_profiler_load_dependecies (_ref_count == 0), but in case where we can't start profiling _can_start_sampling == false, we won't call sample_profiler_enable but always increase _ref_count that in turn will prevent calls to sample_profiler_load_dependecies when enabling sample profiler and that in turn won't call timeBeginPeriod staying on default scheduling resolution. Fix is to always call sample_profiler_load_dependecies making sure we will setup needed dependencies when _ref_count == 0.
jeffschwMSFT
approved these changes
Sep 13, 2021
josalem
approved these changes
Sep 13, 2021
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #58997 to release/6.0
/cc @lateralusX
Customer Impact
Users on Windows that use the environment variable or startup suspension mechanisms to start an EventPipe session will only see a stack sample resolution of 16ms. Any subsequent traces after that will also have the lower resolution due to how the reference counting is handled inside the sample profiler. This patch fixes that to allow for 1ms time resolution.
Testing
@lateralusX performed local testing to validate this fixes the scenario.
Risk
Low. This patch moves the loading of Windows libraries for changing the time resolution up so that suspended scenarios and the env var have the higher resolution. The load is still gated on internal reference counting and behavior shouldn't change other than enabling the load.
(filled out by @josalem on behalf of @lateralusX)