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

fix compilation of springtime tests and upgrade catch2 #436

Merged
merged 6 commits into from
Nov 10, 2022

Conversation

dzosz
Copy link
Collaborator

@dzosz dzosz commented Oct 5, 2022

Enables thread yielding instead of sleep for micro durations that OS probably won't schedule on time. Not sure if this change is welcome as it changes default behavior.
State before:

  • buggy DEAD that enables thread yielding with influence of previous sleep times (likely just a faulty logic that was never used). yielding logic was was dead because initial avg_latency values were set to 0.

State after:

  • yield thread instead of sleeping for very short durations that take no longer than 5 yield iterations (configurable). initial boundary set to 100us that gets smoothed to actual duration of yield
  • At this point of time there's no code in spring that attempts to sleep for such a short duration so it is potential improvement after all

@dzosz dzosz force-pushed the reenable_yield_in_springtime branch from 3b7094f to d84b37b Compare October 5, 2022 19:28
@dzosz dzosz changed the title Reenable yield in springtime Reenable yield in springtime sleep Oct 5, 2022
@lhog
Copy link
Collaborator

lhog commented Oct 6, 2022

Summoning @marcushutchings for review

@lhog lhog requested review from lhog and marcushutchings and removed request for lhog October 8, 2022 17:24
@marcushutchings
Copy link
Collaborator

Maybe I'm missing or overlooking something, but the current logic seems to check out. The initial avg latency being set to 0 seems fine - it can change over time. It seems to me that the original intent of the code was to measure the average inaccuracy of the OS sleep function when it overshoots. Then by measuring that, because it differs between hardware, OSes, and circumstances, if a request comes in that is smaller than the average overshot, then it will use a more accurate, but potentially more performance intensive, thread yield() function.

As the OP has mentioned, we don't have any sleep requests small enough to trigger this; though, I'm not sure why the current code is considered dead.

@marcushutchings
Copy link
Collaborator

Apologies for sounding dumb: I'm curious about the the change to rts/lib/catch.hpp. What was the driving factor that needed this update? GitHub is failing to show the diffs for it.

@dzosz
Copy link
Collaborator Author

dzosz commented Oct 15, 2022

Thank you Marcus. Your comment made me aware that I misread what was being stored in avgThreadSleepTimeMicroSecs.
My change is not required.
One thing I can do is benchmark original code in critical conditions (1-5 fps) to see whether avgThreadSleepTimeMicroSecs does not grow too much and does not cause unexpected yield calls.

@dzosz dzosz marked this pull request as draft October 15, 2022 07:16
@dzosz
Copy link
Collaborator Author

dzosz commented Oct 15, 2022

I will fix compilation of tests in separate PR

@marcushutchings
Copy link
Collaborator

I forgot to mention that I think the use of std::memory_order_relaxed is a good one.

@lhog lhog force-pushed the BAR105 branch 3 times, most recently from 7d55cea to 075e577 Compare October 21, 2022 20:40
@badosu
Copy link
Collaborator

badosu commented Nov 4, 2022

@dzosz Is there anything you still want to work on this PR?

dzosz and others added 5 commits November 4, 2022 19:06
* update catch2 to 2.13.5 to resolve issue with MINSIGSTKSZ being no longer usable in constexpr context
* catchorg/Catch2#2421
…t durations

* before yielding logic was depending on previous durations of sleep function.
  now it depends only on previous yield durations
* initial yield boundary set to 100 us. gets smoothed to actual values
  with consequent calls
…for short durations"

This reverts commit d84b37bb229b83192ba9a2ee2918d7e926262531.
…tion argument is nanosec or millisec type"

This reverts commit 4cd9994af3cc58b054a36697adbca68062cd9d45.
@dzosz dzosz force-pushed the reenable_yield_in_springtime branch from d84b37b to e537913 Compare November 4, 2022 18:10
@dzosz
Copy link
Collaborator Author

dzosz commented Nov 4, 2022

issue with compiling catch is described here catchorg/Catch2#2421
I have reverted all my changes except for fixing compilation of benchmark files.

I was testing the hypothesis that yielding the thread when timer is not precise enough can cause even more performance issues (imprecise timers might be the cause of high cpu workload) but I was unable to come to any conclusions on my modern hardware (I was using taskset to run spring on two threads). I've got an older laptop so I'll test it there but for now
I think we can limit the scope to this one fix and merge the PR.

@dzosz dzosz marked this pull request as ready for review November 4, 2022 18:20
@dzosz dzosz requested a review from badosu November 4, 2022 18:20
@dzosz dzosz changed the title Reenable yield in springtime sleep fix compilation of springtime tests and upgrade catch2 Nov 4, 2022
@badosu
Copy link
Collaborator

badosu commented Nov 4, 2022

@marcushutchings would you mind taking another look?

@badosu badosu removed their request for review November 4, 2022 22:57
@marcushutchings
Copy link
Collaborator

Sorry for the delay, sure I'll take another look.

@dzosz
Copy link
Collaborator Author

dzosz commented Nov 10, 2022

Sorry for the delay, sure I'll take another look.

since I reverted all functional changes i'll spare you the time. I'm commiting only fixes for test files in cmakelists

@dzosz dzosz force-pushed the reenable_yield_in_springtime branch from ce16f9a to 750ec36 Compare November 10, 2022 13:17
@dzosz dzosz merged commit f6f602b into beyond-all-reason:BAR105 Nov 10, 2022
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.

None yet

4 participants