You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/05/12 18:46:20 UTC
[GitHub] [druid] gianm opened a new pull request, #12513: ConcurrentGrouper: Add option to always slice up merge buffers thread-locally.
gianm opened a new pull request, #12513:
URL: https://github.com/apache/druid/pull/12513
Normally, the ConcurrentGrouper shares merge buffers across processing
threads until spilling starts, and then switches to a thread-local model.
This minimizes memory use and reduces likelihood of spilling, which is
good, but it creates thread contention. The new mergeThreadLocal option
causes a query to start in thread-local mode immediately, and allows us
to experiment with the relative performance of the two modes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #12513: ConcurrentGrouper: Add mergeThreadLocal option, fix bug around the switch to spilling.
Posted by GitBox <gi...@apache.org>.
gianm merged PR #12513:
URL: https://github.com/apache/druid/pull/12513
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12513: ConcurrentGrouper: Add option to always slice up merge buffers thread-locally.
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12513:
URL: https://github.com/apache/druid/pull/12513#issuecomment-1126802203
Tagging this "bug" too since it also fixes a bug with ConcurrentGrouper. There was a race condition as part of the switch from hash-based to thread-local buffer selection: it's possible that thread A is still aggregating into grouper B (selected via hash) while thread B is simultaneously trying to aggregate into it (selected via thread-locality). The fix is to always synchronize on the grouper, regardless of how it was selected. I expect that this fix will be fine performance-wise, since contention will be nil once things switch over to thread-local selection.
I noticed this when running ConcurrentGrouperTest in an infinite loop. The tests that involved spilling with `mergeThreadLocal: false` failed on one out of every few thousand iterations. The bug isn't triggered when `mergeThreadLocal: true`, because the switch doesn't happen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] cryptoe commented on a diff in pull request #12513: ConcurrentGrouper: Add option to always slice up merge buffers thread-locally.
Posted by GitBox <gi...@apache.org>.
cryptoe commented on code in PR #12513:
URL: https://github.com/apache/druid/pull/12513#discussion_r871732259
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java:
##########
@@ -279,13 +286,15 @@ public AggregateResult aggregate(KeyType key, int keyHash)
}
}
- // At this point we know spilling = true
- final SpillingGrouper<KeyType> tlGrouper = threadLocalGrouper.get();
+ assert spilling;
+ return aggregateThreadLocal(key, keyHash);
+ }
- synchronized (tlGrouper) {
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org