p2p/dnsdisc: fix hot-spin when all trees are empty #22313
Merged
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
In the random sync algorithm used by the DNS node iterator, we first pick a random tree and then perform one sync action on that tree. This happens in a loop until any node is found. If no trees contain any nodes, the iterator would just keep starting over in a hot loop, spinning at 100% CPU. The fix is a bit complicated. The iterator now checks if any sync action can be performed on the tree before selecting it. If no action can be performed on any tree, it waits for the closest root record recheck time to arrive and then tries again.
fjl
commented
Feb 11, 2021
p2p/dnsdisc/client.go
Outdated
| } | ||
|
|
||
| // syncableTrees finds trees on which any meaningful sync action can be performed. | ||
| func (it *randomIterator) syncableTrees() (syncable, disabled []*clientTree) { |
There was a problem hiding this comment.
I still want optimize this to reuse the same slices over and over, but let's review the basic idea first.
|
|
Adjusted the log message a bit: |
|
Sorry, I ran the wrong code! |
holiman
approved these changes
Feb 12, 2021
holiman
reviewed
Feb 16, 2021
Comment on lines
+333
to
+336
| if len(it.syncableList) > 0 { | ||
| return true, it.syncableList | ||
| } | ||
| return false, it.disabledList |
There was a problem hiding this comment.
Maybe this totally doesn't matter (depends on how large the trees are?), but you could resize the unused one to zero here before returning
There was a problem hiding this comment.
At the moment, we only use one tree per client and I'm not worried about the memory usage. But will think about a way to avoid holding on to the trees in the future.
holiman
approved these changes
Feb 16, 2021
9 tasks
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In the random sync algorithm used by the DNS node iterator, we first pick a random
tree and then perform one sync action on that tree. This happens in a loop until any
node is found. If no trees contain any nodes, the iterator will enter a hot loop spinning
at 100% CPU.
The fix is complicated. The iterator now checks if a meaningful sync action can
be performed on any tree. If there is nothing to do, it waits for the next root record
recheck time to arrive and then tries again.
Fixes #22306