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

Refactor DandelionShuffleThread to run in connection scheduler #65

Merged

Conversation

barrystyle
Copy link

This PR refactors the dandelionshuffle thread, to run as a scheduler function which gets called once a second.

@SmartArray SmartArray self-requested a review August 14, 2021 05:02
@SmartArray SmartArray added the bug Something isn't working label Aug 14, 2021
@SmartArray
Copy link

Issue validated using Instruments.

@barrystyle is 100% right about the blocking loop that causes the CPU time of that thread to spin intensely, which is definitely far too inefficient.

Bildschirmfoto 2021-08-14 um 07 09 49

The current implementation only enters DandelionShuffle() (line 1798) every once in a while. I believe that the interval of 1 second - as suggested by @barrystyle - is fairly sufficient. We may also bump that number higher.

digibyte/src/net.cpp

Lines 1791 to 1798 in 3e6a19a

void CConnman::ThreadDandelionShuffle() {
auto current_time = GetTime<std::chrono::microseconds>();
auto next_dandelion_shuffle = PoissonNextSend(current_time, DANDELION_SHUFFLE_INTERVAL);
while (!interruptNet) {
current_time = GetTime<std::chrono::microseconds>();
if (current_time > next_dandelion_shuffle) {
DandelionShuffle();

Also big thumbs up to use the node's scheduler instead of a thread, which, from the looks, should be fine as DandelionShuffle() has a low complexity. This is the only thing we will need to validate once more before ACKing this PR.

@barrystyle
Copy link
Author

just removed the actual thread definition and 'on shutdown' thread cleanup

@JohnnyLawDGB
Copy link

So basically inserting breaks between CPU requests? Genius.

@SmartArray
Copy link

SmartArray commented Aug 15, 2021

I just validated that the complexity of DandelionShuffle() is low enough to run in a scheduler. It took about 0-1ms per execution, which is insignificant.

Furthermore I pushed two more commits.
3a7b9fd renamed the function as it isn't a thread anymore.
48227d4 exported the scheduler interval to an external constant.

Copy link

@SmartArray SmartArray left a comment

Choose a reason for hiding this comment

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

ACK

@SmartArray SmartArray changed the title prevent dandelion using 100% cpu Refactor DandelionShuffleThread to run in connection scheduler Aug 15, 2021
Copy link
Member

@gto90 gto90 left a comment

Choose a reason for hiding this comment

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

ACK

Great work on this performance improvement @barrystyle . One thing I would love to start working towards is some unit test coverage around the Dandelion code, but this is great for now.

Copy link

@JaredTate JaredTate left a comment

Choose a reason for hiding this comment

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

ACK. Looks good. Tested & working. Thanks for doing this Barry!

@JaredTate JaredTate merged commit 276af5a into DigiByte-Core:feature/8.22.0 Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants