[WIP] Fix subscription busy wait #1602
Closed
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
|
Please target the current default branch with your patch, currently |
|
@dirk-thomas retargeted, #1608 . |
|
Thanks, closing in favor of #1608. |
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.
PR showing a solution for bug #1545 .
This is not yet meant to be merged - treat it like I'm offering a solution open for comments.
This PR consists of two commits.
The first one solves the issue in a backwards-compatible way, however I've noticed that it's performance when calling
CallbackQueue::addCallbackrapidly drops compared to upstream (to thousands/sec compared to hundreds of thousands/sec).The second one takes it further, breaking ABI (but not API, although it adds to it). This solution doesn't suffer from the performance issues, or at least not as much (there is actually some mutex locking added, so it is probably a bit slower, but taking into account that the upstream now fully throttles the CPU...)
What should the tests show: calling
callOne()too often (as in upstream) is quite expensive, and since upstream enters a busy-wait-loop aroundcallOne, it throttles 10 cores to maximum. The fixed versions should be much more friendly to your CPU, which is shown by the fact that it does not call excessive numbers ofcallOne(roughly 2-3 times the number of subscription callbacks pushed to the queue).Another interesting metric showed to be the number of subscription callbacks that it's possible to push in the queue in the given time-frame. As I stated earlier, the version from first commit has a very strong drop in this metric. It might be interesting to test it with some real pusblisher/subscriber system, but I haven't yet had time to test it...