fix: reduce lock contention in RepartitionExec::execute
#10009
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
The state is initialized ONCE for all partitions. However this may take a short while (on a very busy system 1ms or more). It is quite likely that multiple threads call `execute` at the same time, because we have just fanned out to the number "target partitions" which is likely set to the number of CPU cores which now all try to start to execute the plan at the same time. The solution is to not waste CPU circles in some futex lock but to tell the async runtime (= tokio) that we are performing work and the other threads should rather do something useful. This mostly just moves code around, no functional change intended.
crepererum
commented
Apr 9, 2024
| }) | ||
| .collect(); | ||
|
|
||
| // TODO: metric input-output mapping is broken |
There was a problem hiding this comment.
This was broken before: the first parameter was set to partition, i.e. the first partition that initializes the state. That's clearly wrong. I now initialize it to 0 and will fix the tracking properly in a follow-up PR.
crepererum
commented
Apr 9, 2024
Comment on lines
+1297
to
+1300
| let mut background_task = JoinSet::new(); | ||
| background_task.spawn(async move { | ||
| input.wait().await; | ||
| }); |
There was a problem hiding this comment.
That goes for this and the other tests:
There is NO guarantee that the inputs to RepartitionExec are polled if you never poll any of the outputs. Hence we need to move this barrier call into a background task so it happens at the same time as the output poll below.
9 tasks
alamb
changed the title
RepartitionExec::executeRepartitionExec::execute
|
/benchmark :) |
Benchmark resultsBenchmarks comparing cb21404 (main) and 01a4468 (PR) |
alamb
approved these changes
Apr 9, 2024
There was a problem hiding this comment.
Thank you @crepererum -- I reviewed this code carefully and it makes sense to me
I think it is important to document the rationale in comments to reduce the chance of this change being accidentally undone in the future, but otherwise I think it is ready to go 🚀
| @@ -1240,7 +1294,10 @@ mod tests { | |||
| std::mem::drop(output_stream0); | |||
|
|
|||
| // Now, start sending input | |||
| input.wait().await; | |||
| let mut background_task = JoinSet::new(); | |||
There was a problem hiding this comment.
What is the purpose of this change? I tried this change without the other changes in this PR and the test still passes (I was expecting it would hang or something)
| }) | ||
| .collect(); | ||
|
|
||
| // TODO: metric input-output mapping is broken |
|
TBH a slightly slower runtime in a "repeat a single query in a hot-loop" benchmark is somewhat expected. If you have enough tokio threads to spare, you can totally futex-wait. But for any kind of server software that's a bad idea. |
crepererum
force-pushed
the
crepererum/fix_locking_1
branch
from
669cd3e
to
df7a034
Compare
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
crepererum
force-pushed
the
crepererum/fix_locking_1
branch
from
df7a034
to
a716e6c
Compare
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.
Which issue does this PR close?
Closes #10014
Rationale for this change
The state is initialized ONCE for all partitions. However this may take a short while (on a very busy system 1ms or more). It is quite likely that multiple threads call
executeat the same time, because we have just fanned out to the number "target partitions" which is likely set to the number of CPU cores which now all try to start to execute the plan at the same time.The solution is to not waste CPU circles in some futex lock but to tell the async runtime (= tokio) that we are performing work and the other threads should rather do something useful.
What changes are included in this PR?
This mostly just moves code around, no functional change intended.
Are these changes tested?
Existing tests still pass.
Are there any user-facing changes?
Faster query exec.