-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Dataflow Streaming] Reduce contention on work submission #33687
base: master
Are you sure you want to change the base?
Conversation
R: @scwhittle |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
.../worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/BoundedQueueExecutor.java
Outdated
Show resolved
Hide resolved
.../worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/BoundedQueueExecutor.java
Outdated
Show resolved
Hide resolved
.../worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/BoundedQueueExecutor.java
Show resolved
Hide resolved
Run Java PreCommit |
// counters. We do this to reduce contention on monitor which is locked by | ||
// GetWork thread | ||
decrementQueue.add(workBytes); | ||
synchronized (decrementQueueDrainLock) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if this will end up creating more threads in the pool if we have a bunch of threads blocking on this but the count is decremented by another thread and new work is admitted.
maybe we could improve to reduce the # of threads waiting on synchronized block using an atomic? Seems like something like the following would make it so there is at most one thread waiting for the synchronized block
addToQueue
bool coalesce = !atomic_coalescer.getandset(true);
if (!coalesce) return;
{
sync single
atomic_coalescer.set(false);
poll queue, return if empty
monitor {
}
}
The monitor guarding BoundedQueueExecutor's counters was locked by both the work submitting threads and the harness threads (300 of them). The contention can slow down the work submission loop. This change reduces the contention by collapsing counter updates from the harness threads and letting one thread to lock the monitor and update the counters.
Example wallz profile showing the stacks locking the monitor
#33578