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 2021/11/18 21:59:24 UTC
[GitHub] [druid] gianm opened a new pull request #11950: Improve on-heap aggregator footprint estimates.
gianm opened a new pull request #11950:
URL: https://github.com/apache/druid/pull/11950
Add a "guessAggregatorHeapFootprint" method to AggregatorFactory that mitigates #6743 by enabling heap footprint estimates based on a specific number of rows. The idea is that at ingestion time, the number of rows that go into an aggregator will be 1 (if rollup is off) or will likely be a small number (if rollup is on).
It's a heuristic, because of course nothing guarantees that the rollup ratio is a small number. But it's a common case, and I expect this logic to go wrong much less often than the current logic. Also, when it does go wrong, users can fix it by lowering maxRowsInMemory or maxBytesInMemory. The current situation is unintuitive: when the estimation goes wrong, users get an OOME, but actually they need to *raise* these limits to fix it.
I don't think this is an ideal solution. In the future, I'd like to see something that involves the IncrementalIndex having much more direct control over how much memory it uses. Probably in concert with implementing off-heap resizeable aggregators, which is a feature that would be very nice to have for other reasons too. But in the meantime, the approach in this patch is simple, non-invasive, and I think it will help people.
--
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] lgtm-com[bot] commented on pull request #11950: Improve on-heap aggregator footprint estimates.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11950:
URL: https://github.com/apache/druid/pull/11950#issuecomment-975227287
This pull request **introduces 1 alert** when merging 2752c2fa7a625ccbed3853b5d491d876c19d782e into b13f07a05744ad1662b84bb9714b7fa51b798d53 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-7849ee875971f8bea615fef90bbdcea153a7cbc9)
**new alerts:**
* 1 for Result of multiplication cast to wider type
--
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] kfaraz commented on a change in pull request #11950: Improve on-heap aggregator footprint estimates.
Posted by GitBox <gi...@apache.org>.
kfaraz commented on a change in pull request #11950:
URL: https://github.com/apache/druid/pull/11950#discussion_r754990694
##########
File path: processing/src/main/java/org/apache/druid/segment/incremental/OnheapIncrementalIndex.java
##########
@@ -114,10 +122,13 @@
private static long getMaxBytesPerRowForAggregators(IncrementalIndexSchema incrementalIndexSchema)
{
long maxAggregatorIntermediateSize = ((long) Integer.BYTES) * incrementalIndexSchema.getMetrics().length;
- maxAggregatorIntermediateSize += Arrays.stream(incrementalIndexSchema.getMetrics())
- .mapToLong(aggregator -> aggregator.getMaxIntermediateSizeWithNulls()
- + Long.BYTES * 2L)
- .sum();
+
+ for (final AggregatorFactory aggregator : incrementalIndexSchema.getMetrics()) {
+ final long rows =
Review comment:
Nit: It seems this line can be outside the for loop.
##########
File path: extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/theta/SketchAggregatorFactory.java
##########
@@ -170,6 +171,25 @@ public int getSize()
return size;
}
+ @Override
+ public int guessAggregatorHeapFootprint(long rows)
+ {
+ final int maxEntries = size * 2;
+ final int expectedEntries;
+
+ if (rows > maxEntries) {
+ expectedEntries = maxEntries;
+ } else {
+ final int minEntries = 1 << Util.MIN_LG_ARR_LONGS;
Review comment:
Should `minEntries = 1 << Util.MIN_LG_ARR_LONGS` and the largest possible preamble, `Family.UNION.getMaxPreLongs() << 3` be taken out into constants?
##########
File path: processing/src/main/java/org/apache/druid/segment/incremental/OnheapIncrementalIndex.java
##########
@@ -114,10 +122,13 @@
private static long getMaxBytesPerRowForAggregators(IncrementalIndexSchema incrementalIndexSchema)
{
long maxAggregatorIntermediateSize = ((long) Integer.BYTES) * incrementalIndexSchema.getMetrics().length;
- maxAggregatorIntermediateSize += Arrays.stream(incrementalIndexSchema.getMetrics())
- .mapToLong(aggregator -> aggregator.getMaxIntermediateSizeWithNulls()
- + Long.BYTES * 2L)
- .sum();
+
+ for (final AggregatorFactory aggregator : incrementalIndexSchema.getMetrics()) {
+ final long rows =
+ incrementalIndexSchema.isRollup() ? ROLLUP_RATIO_FOR_AGGREGATOR_FOOTPRINT_ESTIMATION : 1;
+ maxAggregatorIntermediateSize += (long) aggregator.guessAggregatorHeapFootprint(rows) + 2 * Long.BYTES;
Review comment:
Nit: Is the term `2 * Long.BYTES (= 16)` needed now?
It seems that that the result of the guess would be big enough even when `rows = 1` to make this term relatively much less significant.
--
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 #11950: Improve on-heap aggregator footprint estimates.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #11950:
URL: https://github.com/apache/druid/pull/11950#issuecomment-978019379
@kfaraz thanks for the review. I pushed up some of the changes you suggested.
--
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] lgtm-com[bot] commented on pull request #11950: Improve on-heap aggregator footprint estimates.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11950:
URL: https://github.com/apache/druid/pull/11950#issuecomment-973391048
This pull request **introduces 1 alert** when merging 451f23ee01b5cb31e88bce257fd5d9343bb4a075 into a4353aa1f42a51a1d218547b3a033550e49e5025 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-7b09a890da07830f86aeccd255da325fa90b2ee2)
**new alerts:**
* 1 for Result of multiplication cast to wider type
--
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] abhishekagarwal87 merged pull request #11950: Improve on-heap aggregator footprint estimates.
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged pull request #11950:
URL: https://github.com/apache/druid/pull/11950
--
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] kfaraz commented on a change in pull request #11950: Improve on-heap aggregator footprint estimates.
Posted by GitBox <gi...@apache.org>.
kfaraz commented on a change in pull request #11950:
URL: https://github.com/apache/druid/pull/11950#discussion_r756292940
##########
File path: processing/src/main/java/org/apache/druid/segment/incremental/OnheapIncrementalIndex.java
##########
@@ -114,10 +122,13 @@
private static long getMaxBytesPerRowForAggregators(IncrementalIndexSchema incrementalIndexSchema)
{
long maxAggregatorIntermediateSize = ((long) Integer.BYTES) * incrementalIndexSchema.getMetrics().length;
- maxAggregatorIntermediateSize += Arrays.stream(incrementalIndexSchema.getMetrics())
- .mapToLong(aggregator -> aggregator.getMaxIntermediateSizeWithNulls()
- + Long.BYTES * 2L)
- .sum();
+
+ for (final AggregatorFactory aggregator : incrementalIndexSchema.getMetrics()) {
+ final long rows =
+ incrementalIndexSchema.isRollup() ? ROLLUP_RATIO_FOR_AGGREGATOR_FOOTPRINT_ESTIMATION : 1;
+ maxAggregatorIntermediateSize += (long) aggregator.guessAggregatorHeapFootprint(rows) + 2 * Long.BYTES;
Review comment:
Makes sense 👍
--
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 a change in pull request #11950: Improve on-heap aggregator footprint estimates.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #11950:
URL: https://github.com/apache/druid/pull/11950#discussion_r756228094
##########
File path: processing/src/main/java/org/apache/druid/segment/incremental/OnheapIncrementalIndex.java
##########
@@ -114,10 +122,13 @@
private static long getMaxBytesPerRowForAggregators(IncrementalIndexSchema incrementalIndexSchema)
{
long maxAggregatorIntermediateSize = ((long) Integer.BYTES) * incrementalIndexSchema.getMetrics().length;
- maxAggregatorIntermediateSize += Arrays.stream(incrementalIndexSchema.getMetrics())
- .mapToLong(aggregator -> aggregator.getMaxIntermediateSizeWithNulls()
- + Long.BYTES * 2L)
- .sum();
+
+ for (final AggregatorFactory aggregator : incrementalIndexSchema.getMetrics()) {
+ final long rows =
+ incrementalIndexSchema.isRollup() ? ROLLUP_RATIO_FOR_AGGREGATOR_FOOTPRINT_ESTIMATION : 1;
+ maxAggregatorIntermediateSize += (long) aggregator.guessAggregatorHeapFootprint(rows) + 2 * Long.BYTES;
Review comment:
It's still useful for aggregators that have a small data size, like longSum etc. For those, the overheads are bigger than the intermediate data size.
--
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] kfaraz commented on pull request #11950: Improve on-heap aggregator footprint estimates.
Posted by GitBox <gi...@apache.org>.
kfaraz commented on pull request #11950:
URL: https://github.com/apache/druid/pull/11950#issuecomment-978796502
Thanks for the changes, @gianm .
On running ingestion of sample "wikipedia" data, I got the following persisted stats in the indexing logs.
Before the changes
(total 38 persistence cycles, 652 rows persisted in each cycle):
```
...
2021-11-24T15:55:26,139 INFO [[index_parallel_test_sketch_lolhhmjb_2021-11-24T15:54:54.532Z]-appenderator-persist] org.apache.druid.segment.realtime.appenderator.AppenderatorImpl - Persisted stats: processed rows: [1304], persisted rows[652], sinks: [1], total fireHydrants (across sinks): [1], persisted fireHydrants (across sinks): [1]
2021-11-24T15:55:26,244 INFO [[index_parallel_test_sketch_lolhhmjb_2021-11-24T15:54:54.532Z]-appenderator-persist] org.apache.druid.segment.realtime.appenderator.AppenderatorImpl - Persisted stats: processed rows: [1956], persisted rows[652], sinks: [1], total fireHydrants (across sinks): [2], persisted fireHydrants (across sinks): [2]
2021-11-24T15:55:26,353 INFO [[index_parallel_test_sketch_lolhhmjb_2021-11-24T15:54:54.532Z]-appenderator-persist] org.apache.druid.segment.realtime.appenderator.AppenderatorImpl - Persisted stats: processed rows: [2608], persisted rows[652], sinks: [1], total fireHydrants (across sinks): [3], persisted fireHydrants (across sinks): [2]
2021-11-24T15:55:26,436 INFO [[index_parallel_test_sketch_lolhhmjb_2021-11-24T15:54:54.532Z]-appenderator-persist] org.apache.druid.segment.realtime.appenderator.AppenderatorImpl - Persisted stats: processed rows: [3260], persisted rows[652], sinks: [1], total fireHydrants (across sinks): [4], persisted fireHydrants (across sinks): [2]
2021-11-24T15:55:26,509 INFO [[index_parallel_test_sketch_lolhhmjb_2021-11-24T15:54:54.532Z]-appenderator-persist] org.apache.druid.segment.realtime.appenderator.AppenderatorImpl - Persisted stats: processed rows: [3912], persisted rows[652], sinks: [1], total fireHydrants (across sinks): [5], persisted fireHydrants (across sinks): [2]
2021-11-24T15:55:26,578 INFO [[index_parallel_test_sketch_lolhhmjb_2021-11-24T15:54:54.532Z]-appenderator-persist]
...
```
After the changes
(total 1 persistence cycle, ~24k rows persisted in one cycle):
```
2021-11-25T03:16:30,200 INFO [[index_parallel_test_sketchy_ghobnhcj_2021-11-25T03:15:57.793Z]-appenderator-persist] org.apache.druid.segment.realtime.appenderator.AppenderatorImpl - Persisted stats: processed rows: [24433], persisted rows[24433], sinks: [1], total fireHydrants (across sinks): [1], persisted fireHydrants (across sinks): [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
[GitHub] [druid] lgtm-com[bot] commented on pull request #11950: Improve on-heap aggregator footprint estimates.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11950:
URL: https://github.com/apache/druid/pull/11950#issuecomment-974341874
This pull request **introduces 1 alert** when merging fda450e616b999ee0a432899f111613317fab986 into 36ee0367ffa567c3af5d01fb33dbdf9870a39f8f - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-f5d5f1987457ca544da1ed42157104bd947d04f2)
**new alerts:**
* 1 for Result of multiplication cast to wider type
--
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