diff --git a/processing/src/main/java/io/druid/query/topn/Generic1AggPooledTopNScannerPrototype.java b/processing/src/main/java/io/druid/query/topn/Generic1AggPooledTopNScannerPrototype.java index d98d1fbd2ee..9163eb5ceac 100644 --- a/processing/src/main/java/io/druid/query/topn/Generic1AggPooledTopNScannerPrototype.java +++ b/processing/src/main/java/io/druid/query/topn/Generic1AggPooledTopNScannerPrototype.java @@ -28,6 +28,18 @@ import java.nio.ByteBuffer; public final class Generic1AggPooledTopNScannerPrototype implements Generic1AggPooledTopNScanner { + /** + * Any changes to this method should be coordinated with {@link TopNUtils}, {@link + * PooledTopNAlgorithm#computeSpecializedScanAndAggregateImplementations} and downstream methods. + * + * It should be checked with a tool like https://github.com/AdoptOpenJDK/jitwatch that C2 compiler output for this + * method doesn't have any method calls in the while loop, i. e. all method calls are inlined. To be able to see + * assembly of this method in JITWatch and other similar tools, {@link + * PooledTopNAlgorithm#specializeGeneric1AggPooledTopN} should be turned off. Note that in this case the benchmark + * should be "naturally monomorphic", i. e. execute this method always with the same runtime shape. + * + * If the while loop contains not inlined method calls, it should be considered as a performance bug. + */ @Override public long scanAndAggregate( DimensionSelector dimensionSelector, diff --git a/processing/src/main/java/io/druid/query/topn/Generic2AggPooledTopNScannerPrototype.java b/processing/src/main/java/io/druid/query/topn/Generic2AggPooledTopNScannerPrototype.java index 491cb71f766..402535e6dd3 100644 --- a/processing/src/main/java/io/druid/query/topn/Generic2AggPooledTopNScannerPrototype.java +++ b/processing/src/main/java/io/druid/query/topn/Generic2AggPooledTopNScannerPrototype.java @@ -28,6 +28,18 @@ import java.nio.ByteBuffer; public final class Generic2AggPooledTopNScannerPrototype implements Generic2AggPooledTopNScanner { + /** + * Any changes to this method should be coordinated with {@link TopNUtils}, {@link + * PooledTopNAlgorithm#computeSpecializedScanAndAggregateImplementations} and downstream methods. + * + * It should be checked with a tool like https://github.com/AdoptOpenJDK/jitwatch that C2 compiler output for this + * method doesn't have any method calls in the while loop, i. e. all method calls are inlined. To be able to see + * assembly of this method in JITWatch and other similar tools, {@link + * PooledTopNAlgorithm#specializeGeneric2AggPooledTopN} should be turned off. Note that in this case the benchmark + * should be "naturally monomorphic", i. e. execute this method always with the same runtime shape. + * + * If the while loop contains not inlined method calls, it should be considered as a performance bug. + */ @Override public long scanAndAggregate( DimensionSelector dimensionSelector, diff --git a/processing/src/main/java/io/druid/query/topn/Historical1SimpleDoubleAggPooledTopNScannerPrototype.java b/processing/src/main/java/io/druid/query/topn/Historical1SimpleDoubleAggPooledTopNScannerPrototype.java index 173cb9914e5..a3106f7e2a2 100644 --- a/processing/src/main/java/io/druid/query/topn/Historical1SimpleDoubleAggPooledTopNScannerPrototype.java +++ b/processing/src/main/java/io/druid/query/topn/Historical1SimpleDoubleAggPooledTopNScannerPrototype.java @@ -35,6 +35,18 @@ public class Historical1SimpleDoubleAggPooledTopNScannerPrototype SimpleDoubleBufferAggregator > { + /** + * Any changes to this method should be coordinated with {@link TopNUtils}, {@link + * PooledTopNAlgorithm#computeSpecializedScanAndAggregateImplementations} and downstream methods. + * + * It should be checked with a tool like https://github.com/AdoptOpenJDK/jitwatch that C2 compiler output for this + * method doesn't have any method calls in the while loop, i. e. all method calls are inlined. To be able to see + * assembly of this method in JITWatch and other similar tools, {@link + * PooledTopNAlgorithm#specializeHistorical1SimpleDoubleAggPooledTopN} should be turned off. Note that in this case + * the benchmark should be "naturally monomorphic", i. e. execute this method always with the same runtime shape. + * + * If the while loop contains not inlined method calls, it should be considered as a performance bug. + */ @Override public long scanAndAggregate( HistoricalDimensionSelector dimensionSelector, diff --git a/processing/src/main/java/io/druid/query/topn/HistoricalSingleValueDimSelector1SimpleDoubleAggPooledTopNScannerPrototype.java b/processing/src/main/java/io/druid/query/topn/HistoricalSingleValueDimSelector1SimpleDoubleAggPooledTopNScannerPrototype.java index 10ca29f51be..916e3c4a0ee 100644 --- a/processing/src/main/java/io/druid/query/topn/HistoricalSingleValueDimSelector1SimpleDoubleAggPooledTopNScannerPrototype.java +++ b/processing/src/main/java/io/druid/query/topn/HistoricalSingleValueDimSelector1SimpleDoubleAggPooledTopNScannerPrototype.java @@ -34,6 +34,19 @@ public class HistoricalSingleValueDimSelector1SimpleDoubleAggPooledTopNScannerPr SimpleDoubleBufferAggregator > { + /** + * Any changes to this method should be coordinated with {@link TopNUtils}, {@link + * PooledTopNAlgorithm#computeSpecializedScanAndAggregateImplementations} and downstream methods. + * + * It should be checked with a tool like https://github.com/AdoptOpenJDK/jitwatch that C2 compiler output for this + * method doesn't have any method calls in the while loop, i. e. all method calls are inlined. To be able to see + * assembly of this method in JITWatch and other similar tools, {@link + * PooledTopNAlgorithm#specializeHistoricalSingleValueDimSelector1SimpleDoubleAggPooledTopN} should be turned off. + * Note that in this case the benchmark should be "naturally monomorphic", i. e. execute this method always with the + * same runtime shape. + * + * If the while loop contains not inlined method calls, it should be considered as a performance bug. + */ @Override public long scanAndAggregate( SingleValueHistoricalDimensionSelector dimensionSelector, diff --git a/processing/src/main/java/io/druid/query/topn/PooledTopNAlgorithm.java b/processing/src/main/java/io/druid/query/topn/PooledTopNAlgorithm.java index ab40484f8dd..d1bbf954d95 100644 --- a/processing/src/main/java/io/druid/query/topn/PooledTopNAlgorithm.java +++ b/processing/src/main/java/io/druid/query/topn/PooledTopNAlgorithm.java @@ -36,6 +36,7 @@ import io.druid.query.monomorphicprocessing.StringRuntimeShape; import io.druid.segment.Capabilities; import io.druid.segment.Cursor; import io.druid.segment.DimensionSelector; +import io.druid.segment.FilteredOffset; import io.druid.segment.column.ValueType; import io.druid.segment.data.IndexedInts; import io.druid.segment.data.Offset; @@ -128,17 +129,21 @@ public class PooledTopNAlgorithm if (theAggregators.length == 1) { BufferAggregator aggregator = theAggregators[0]; final Cursor cursor = params.getCursor(); - if (cursor instanceof HistoricalCursor && aggregator instanceof SimpleDoubleBufferAggregator) { - if (params.getDimSelector() instanceof SingleValueHistoricalDimensionSelector && - ((SimpleDoubleBufferAggregator) aggregator).getSelector() instanceof HistoricalColumnSelector) { - return scanAndAggregateHistorical1SimpleDoubleAgg( - params, - positions, - (SimpleDoubleBufferAggregator) aggregator, - (HistoricalCursor) cursor, - defaultHistoricalSingleValueDimSelector1SimpleDoubleAggScanner - ); - } + if (cursor instanceof HistoricalCursor && + // FilteredOffset.clone() is not supported. This condition should be removed if + // HistoricalSingleValueDimSelector1SimpleDoubleAggPooledTopNScannerPrototype + // doesn't clone offset anymore. + !(((HistoricalCursor) cursor).getOffset() instanceof FilteredOffset) && + aggregator instanceof SimpleDoubleBufferAggregator && + params.getDimSelector() instanceof SingleValueHistoricalDimensionSelector && + ((SimpleDoubleBufferAggregator) aggregator).getSelector() instanceof HistoricalColumnSelector) { + return scanAndAggregateHistorical1SimpleDoubleAgg( + params, + positions, + (SimpleDoubleBufferAggregator) aggregator, + (HistoricalCursor) cursor, + defaultHistoricalSingleValueDimSelector1SimpleDoubleAggScanner + ); } } return -1; @@ -149,17 +154,21 @@ public class PooledTopNAlgorithm if (theAggregators.length == 1) { BufferAggregator aggregator = theAggregators[0]; final Cursor cursor = params.getCursor(); - if (cursor instanceof HistoricalCursor && aggregator instanceof SimpleDoubleBufferAggregator) { - if (params.getDimSelector() instanceof HistoricalDimensionSelector && - ((SimpleDoubleBufferAggregator) aggregator).getSelector() instanceof HistoricalColumnSelector) { - return scanAndAggregateHistorical1SimpleDoubleAgg( - params, - positions, - (SimpleDoubleBufferAggregator) aggregator, - (HistoricalCursor) cursor, - defaultHistorical1SimpleDoubleAggScanner - ); - } + if (cursor instanceof HistoricalCursor && + // FilteredOffset.clone() is not supported. This condition should be removed if + // Historical1SimpleDoubleAggPooledTopNScannerPrototype + // doesn't clone offset anymore. + !(((HistoricalCursor) cursor).getOffset() instanceof FilteredOffset) && + aggregator instanceof SimpleDoubleBufferAggregator && + params.getDimSelector() instanceof HistoricalDimensionSelector && + ((SimpleDoubleBufferAggregator) aggregator).getSelector() instanceof HistoricalColumnSelector) { + return scanAndAggregateHistorical1SimpleDoubleAgg( + params, + positions, + (SimpleDoubleBufferAggregator) aggregator, + (HistoricalCursor) cursor, + defaultHistorical1SimpleDoubleAggScanner + ); } } return -1; diff --git a/processing/src/main/java/io/druid/query/topn/TopNUtils.java b/processing/src/main/java/io/druid/query/topn/TopNUtils.java index 08afbcb3afd..c1fe093898c 100644 --- a/processing/src/main/java/io/druid/query/topn/TopNUtils.java +++ b/processing/src/main/java/io/druid/query/topn/TopNUtils.java @@ -35,6 +35,10 @@ final class TopNUtils * Casting to the specific Offset subtype helps Hotspot JIT (OpenJDK 8) to generate better assembly. It shouldn't be * so, because the Offset subtype is still always the same (otherwise cast wouldn't be possible), so JIT should * generate equivalent code. In OpenJDK 9 Hotspot could be improved and this "casting hack" is not needed anymore. + * + * TODO check if offset.clone() is also necessary for generating better assembly, or it could be removed. + * + * See also javadoc comments to methods, where this method is used. */ static Object copyOffset(HistoricalCursor cursor) { diff --git a/processing/src/main/java/io/druid/segment/FilteredOffset.java b/processing/src/main/java/io/druid/segment/FilteredOffset.java index 685c0cf6ef8..3becd42533a 100644 --- a/processing/src/main/java/io/druid/segment/FilteredOffset.java +++ b/processing/src/main/java/io/druid/segment/FilteredOffset.java @@ -110,10 +110,20 @@ public final class FilteredOffset extends Offset return baseOffset.getBaseReadableOffset(); } + /** + * clone() is not supported by FilteredOffset because it's not possible to clone {@link #filterMatcher}, and + * while re-creating filterMatcher could be not very cheap for some implementations of {@link Filter}. Although this + * approach could be investigated. + * + * If clone is made possible for FilteredOffset, some improvements could become possible in {@link + * io.druid.query.topn.PooledTopNAlgorithm#computeSpecializedScanAndAggregateImplementations}. + * + * See also https://github.com/druid-io/druid/issues/5132. + */ @Override public Offset clone() { - throw new UnsupportedOperationException("FilteredOffset should not be cloned"); + throw new UnsupportedOperationException("FilteredOffset could not be cloned"); } @Override diff --git a/processing/src/test/java/io/druid/query/topn/TopNQueryRunnerTest.java b/processing/src/test/java/io/druid/query/topn/TopNQueryRunnerTest.java index afa69af6506..4896910e78f 100644 --- a/processing/src/test/java/io/druid/query/topn/TopNQueryRunnerTest.java +++ b/processing/src/test/java/io/druid/query/topn/TopNQueryRunnerTest.java @@ -5605,4 +5605,36 @@ public class TopNQueryRunnerTest ); assertExpectedResults(expectedResults, query); } + + /** + * Regression test for https://github.com/druid-io/druid/issues/5132 + */ + @Test + public void testTopNWithNonBitmapFilter() + { + TopNQuery query = new TopNQueryBuilder() + .dataSource(QueryRunnerTestHelper.dataSource) + .granularity(QueryRunnerTestHelper.allGran) + .filters(new BoundDimFilter( + Column.TIME_COLUMN_NAME, + "0", + String.valueOf(Long.MAX_VALUE), + true, + true, + false, + null, + StringComparators.NUMERIC + )) + .dimension(QueryRunnerTestHelper.marketDimension) + .metric("count") + .threshold(4) + .intervals(QueryRunnerTestHelper.firstToThird) + .aggregators( + Collections.singletonList(new DoubleSumAggregatorFactory("count", "qualityDouble")) + ) + .build(); + + // Don't check results, just the fact that the query could complete + Assert.assertNotNull(Sequences.toList(runWithMerge(query), new ArrayList<>())); + } }