-
-
Notifications
You must be signed in to change notification settings - Fork 1k
[Bug Fix] High Performance Plan Fix #2485
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
base: master
Are you sure you want to change the base?
Conversation
Been 1.5 years since this PR was proposed. And yet... |
I don't think the change is robust enough. If a user explicitly configures high performance power plan, this will ignore it. It should only be ignored if it's not configured. |
Sounds good @timcassell. I will take a look at this when I have some free time. |
28c02b6
to
eb2b3cf
Compare
@timcassell Ok I have pushed my changes. My understanding of the original intent in #2319 was to never downgrade to High Performance if the current plan is Ultimate. However, your recent comment makes sense that the code should always use the explicit power plan. So, I’ve updated the code to only remain on Ultimate if the High Performance plan is not configured—otherwise, the requested plan will be set as expected. Please let me know if this matches what you’re looking for. |
I can't tell what you're testing here, but it doesn't look like it's testing what a user would actually do. Users set the powerplan via The benchmark runner retrieves the value and applies it here: BenchmarkDotNet/src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs Lines 235 to 236 in daa2339
To check if the user explicitly set the value, you can do |
@timcassell Thanks for the additional insight. Looks like I cannot properly test this change as I just realized I cannot change my laptop's settings to Ultimate. I pushed my latest commit if someone else wants to test/make changes. |
if (currentPlan.HasValue && currentPlan.Value == ultimateGuid) | ||
{ | ||
logger.WriteLineInfo("Already on Ultimate Performance; not switching to High Performance since no plan explicitly set in Job."); | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work, you're skipping the entire benchmark here. You should rather invert the branches, or move it to another function.
Fixes #2319