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

Implement WakeLock so that the system does not enter sleep #2670

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

leonvandermeer
Copy link
Contributor

Implement WakeLock so that the system does not enter sleep while benchmarks are running.

As a happy user of BDN, I was annoyed by the fact that long running benchmarks were not completed when I returned to my system. All because my system enters sleep, after not being touched for a while.

The test code needs to enumerate power requests. I decided not to pinvoke undocumented api as maintenance is difficult if it changes in future windows version. It gave me a chance to learn more about parsers in general 😂. Imo, maintaining the parser when powercfg /requests output would change is less difficult.

In case you prefer usage of Superpower library to parse, I'm willing to change that.

All test code is in integration tests, since powercfg needs elevation. I imagine developers dislike failing unit tests when they run those locally.

Copy link
Collaborator

@timcassell timcassell left a comment

Choose a reason for hiding this comment

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

I appreciate all the tests! Do any other OSes besides Windows need this treatment?

src/BenchmarkDotNet/Configs/WakeLockType.cs Outdated Show resolved Hide resolved
docs/articles/samples/IntroWakeLock.md Outdated Show resolved Hide resolved
src/BenchmarkDotNet/Configs/DefaultConfig.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@timcassell timcassell left a comment

Choose a reason for hiding this comment

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

LGTM! I would like a second review before merging this new functionality, though. @adamsitnik or @AndreyAkinshin may have different opinions on what the default should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants