Monomorphic processing of TopN queries with simple double aggregators over historical segments (part of #3798) by leventov · Pull Request #4079 · apache/druid

leventov · 2017-03-18T00:28:46Z

Add a Cursor specialization: HistoricalCursor
Add DimensionSelector specializations: SingleValueDimensionSelector, HistoricalDimensionSelector, SingleValueHistoricalDimensionSelector
Add a FloatColumnSelector specialization: HistoricalFloatColumnSelector
Monomorphic processing of TopN queries with simple double aggregators over historical segments
Make Indexed and ValueMatcher interfaces to extend HotLoopCallee
Improvements to SpecializationService

Follow-up of #3889, part of #3798

… and historical segments

…bleBufferAggregator

egor-ryashin · 2017-03-22T21:17:12Z

When we reach the global limit here, we begin to spam to logs on each call

     @Override
     public void run()
     {
       try {
         T specialized;
         if (specializedClassCounter.get() > maxSpecializations) {
           // Don't specialize, just instantiate the prototype class and emit a warning.
           // The "better" approach is probably to implement some kind of cache eviction from
           // PerPrototypeClassState.specializationStates. But it might be that nobody ever hits even the current
           // maxSpecializations limit, so implementing cache eviction is an unnecessary complexity.
           specialized = perPrototypeClassState.prototypeClass.newInstance();
           LOG.warn(
               "SpecializationService couldn't make more than [%d] specializations. "
               + "Not doing specialization for runtime shape[%s] and class remapping[%s], using the prototype class[%s]",
               maxSpecializations,
               specializationId.runtimeShape,
               specializationId.classRemapping,
               perPrototypeClassState.prototypeClass

leventov · 2017-03-23T01:58:59Z

@egor-ryashin thanks, fixed to emit only once.

egor-ryashin · 2017-03-23T15:54:39Z

+    @Override
+    public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+    {
+      inspector.visit("theBuffer", theBuffer);


Is it ok that theBuffer is not final and changed afterwards?

Changed theBuffer to be final

egor-ryashin · 2017-03-23T15:55:51Z

+  @Override
+  public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+  {
+    inspector.visit("baseList", baseList);


What about the clazz field?

It's not used in get() (the only @CalledFromHotLoop method in Indexed), so it shouldn't be visited in inspectRuntimeShape()

I would create an annotation like:

@Inspect private final List<T> baseList;

make an abstract super class with a method

inspectRuntimeShape(RuntimeShapeInspector inspector) { for(Field field : this.getClass().getDeclaredFields()){ field.isAnnotationPresent(Inspect.class); field.setAccessible(true); inspector.visit(field.getName(), field.get(this)) } }

and forget about overriding methods.

It's up to you though.

It would make it too easy to forget to annotate some fields. For the same reason inspectRuntimeShape() method doesn't have default Java 8 implementation with empty body, although it is always correct and a lot of implementations just do that: to force developers to implement and think about the runtime shape.

It's just my opinion, though. Maybe it makes sense to add annotation-based runtime shape inspection in some form. But anyway, not as part of this PR.

dgolitsyn · 2017-03-22T15:32:30Z

+  public abstract void aggregate(ByteBuffer buf, int position, double value);
+
+  @Override
+  public final void aggregate(ByteBuffer buf, int position)


Should this method be annotated with @CalledFromHotLoop?

No, because this method is annotated in BufferAggregator

dgolitsyn · 2017-03-22T15:43:17Z

+  Historical1AggPooledTopNScanner defaultHistoricalSingleValueDimSelector1SimpleDoubleAggScanner =
+      new HistoricalSingleValueDimSelector1SimpleDoubleAggPooledTopNScannerPrototype();

  private final Capabilities capabilities;


Why do we need additional ref to Capabilities here?

It is used to create BaseArrayProvider in makeInitParams().

Capabilities is protected field in BaseTopNAlgorithm

Thanks, removed

dgolitsyn · 2017-03-22T15:45:13Z

+        SpecializationService.getSpecializationState(
+            prototypeScanner.getClass(),
+            runtimeShape,
+            ImmutableMap.<Class<?>, Class<?>>of(Offset.class, historicalCursor.getOffset().getClass())


<Class<?>, Class<?>> could be removed

dgolitsyn · 2017-03-27T15:23:45Z

  )
  {
-    return getSpecializationState(prototypeClass, runtimeShape, ImmutableMap.<Class<?>, Class<?>>of());
+    return getSpecializationState(prototypeClass, runtimeShape, ImmutableMap.of());


is synchronization required on call to perPrototypeClassState.get() from multiple threads? or call to computeValue()?
Could you please clarify, why WindowedLoopIterationCounter#getSpecialized() always returns null?

is synchronization required on call to perPrototypeClassState.get() from multiple threads? or call to computeValue()?

According to ClassValue documentation, it should be safe: "The actual installation of the value on the class is performed atomically. At that point, if several racing threads have computed values, one is chosen, and returned to all the racing threads."

Could you please clarify, why WindowedLoopIterationCounter#getSpecialized() always returns null?

Added comment

…storical-topn-monomorphic-processing

dgolitsyn · 2017-03-28T18:24:18Z

+  @Override
+  public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+  {
+    inspector.visit("delegate", delegate);


should we add lookupCacheSize to runtimeShape?

Or lookupCacheSize != 0

Added, thanks

leventov · 2017-03-28T18:41:46Z

@fjy we run this in production since December: charts in #3798 show that.

egor-ryashin · 2017-03-29T12:09:00Z

  }
+
+  @Override
+  public void inspectRuntimeShape(RuntimeShapeInspector inspector)


baseArray is not inspected, but is used in get()

It's used just for indexing, there should be no difference for Hotspot for any Object array type. So no need to distinguish between String[] and Object[], for example.

egor-ryashin · 2017-03-29T12:23:06Z

+  @Override
+  public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+  {
+    inspector.visit("cachedValues", cachedValues != null);


I wonder why we does not care about cachedValues element type?

Because CachingIndexed.get() doesn't call methods on instances of this type directly. So for Hotspot, it's just an object, doesn't matter of what type. If it does matter in delegate.get(), delegate's runtime shape should reflect this, delegate is inspected in CachingIndexed.inspectRuntimeShape().

egor-ryashin · 2017-03-29T12:40:35Z

+    @Override
+    public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+    {
+      // ideally should inspect buffer, but at the moment of inspectRuntimeShape() call buffer might be null although


So if we get those fields with subclasses different from the original object then we still get the first mapping and ignore the second implementation and eventually get unexpected/different results on dash, right?

Didn't understand this question. However any implementation of inspectRuntimeShape() anywhere doesn't affect correctness.

The purpose of inspectRuntimeShape() is to ensure that some copy of the code is always called with the same runtime shape (monomorphic), and (ideally) there is only one copy of the code for each runtime shape, not to make JIT perform the same compilation work twice and waste code cache (on Hotspot level) and instruction cache (on hardware level).

Not reflecting all fields which actually belong to runtime shape in inspectRuntimeShape() could lead to the situation when some copy of the code is called with non-monomorphic runtime shape, that will make Hotspot JIT to generate slower (but still correct) code.

Reflecting some fields which actually don't belong to runtime shape in inspectRuntimeShape() could lead to the situation when there are several copies of the code which are called with the same runtime shape, that will pollute code cache and will make JIT to perform the same work several times, but the code is still correct.

egor-ryashin · 2017-03-29T12:46:41Z

+    @Override
+    public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+    {
+      inspector.visit("buffer", buffer);


As I see this is a mutable field. Could have "null", then something else, but description will be 'null' for all the instances, right?
Also 'bigEndian' is changed during processing when loadBuffer() is called.

Yes, that's bad that buffer couldn't be properly inspected here. Probably this situation could be improved later.

Also 'bigEndian' is changed during processing when loadBuffer() is called.

Thanks, removed inspection of bigEndian too.

…IndexedInts

egor-ryashin · 2017-03-29T13:45:07Z

      }

-      return new DimensionSelector()
+      return new SingleValueDimensionSelector()


There is an inspection of extractionFn below, but I don't see it plays as a 'boolean flag'.
io/druid/query/groupby/RowBasedColumnSelectorFactory.java:180

It is used in lookupName() which is @CalledFromHotLoop

…umnSelector with @CalledFromHotLoop

drcrallen · 2017-05-10T17:31:33Z

+    Offset offset = (Offset) TopNUtils.copyOffset(cursor);
+    long scannedRows = 0;
+    int positionToAllocate = 0;
+    while (offset.withinBounds() && !Thread.currentThread().isInterrupted()) {


if this is interrupted, where is it intended to be caught?

In PooledTopNAlgorithm.scanAndAggregate(), BaseQuery.checkInterrupted() is called in the end of each branch.

drcrallen · 2017-05-10T17:45:08Z

+    Offset offset = (Offset) TopNUtils.copyOffset(cursor);
+    long scannedRows = 0;
+    int positionToAllocate = 0;
+    while (offset.withinBounds() && !Thread.currentThread().isInterrupted()) {


It feels like we should be able to use an Iterator or IntStream here, but I'm not finding anything reasonable in the standard java libraries at a cursory glance.

There is IntIterator from fastutil, and even https://docs.oracle.com/javase/8/docs/api/java/util/PrimitiveIterator.OfInt.html.

I don't think such change should be part of this PR.

drcrallen · 2017-05-10T17:56:10Z

-    if (specializeGeneric1AggPooledTopN && theAggregators.length == 1) {
-      scanAndAggregateGeneric1Agg(params, positions, theAggregators[0], cursor);
-    } else if (specializeGeneric2AggPooledTopN && theAggregators.length == 2) {
+    if (theAggregators.length == 1) {


Can we abstract the predicate / function checking into a List of Pairs of predicates and runnables or similar? and loop through the list until it finds a matching predicate to run? Then to deny specializeHistoricalSingleValueDimSelector1SimpleDoubleAggPooledTopN is simply to not add it in the list.

Those BaseQuery.checkInterrupted(); + return; are pretty buried and would be easy to miss in future additions. So having a loop and finishing on those two items feels more sustainable.

Nice idea, implemented

drcrallen · 2017-05-10T19:05:09Z

+  )
+  {
+    String runtimeShape = StringRuntimeShape.of(aggregator);
+    HistoricalCursor historicalCursor = (HistoricalCursor) cursor;


suggest only taking in HistoricalCursor and doing the cast in the caller

drcrallen · 2017-05-10T23:22:47Z

+
+      @Override
+      public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+      {


If really nothing suggest also adding comment as per the other examples below

drcrallen · 2017-05-10T23:32:35Z

+      public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+      {
+        inspector.visit("firstBaseMatcher", baseMatchers[0]);
+        inspector.visit("secondBaseMatcher", baseMatchers[1]);


What guarantees baseMatchers.length() > 1?

Lines 109-113

ah, I see, >0 and !=1 ok

drcrallen · 2017-05-10T23:33:39Z

        public void inspectRuntimeShape(RuntimeShapeInspector inspector)
        {
          inspector.visit("baseSelector", baseSelector);
+          inspector.visit("extractionFn", extractionFn);


was this a bug?

drcrallen

A lot of neat added functionality that seems to be ok, but there is a severe lack of unit tests proving to future developers that they don't mess it up somehow. Please add unit tests for the new features.

Also, there seems to be a lot of discussion or changes around what needs to be included in inspectRuntimeShape. Can checks for that be automated or at least tested in unit tests somehow?

…meShape() implementations

…type and HistoricalSingleValueDimSelector1SimpleDoubleAggPooledTopNScannerPrototype, cover them with tests

leventov · 2017-05-11T19:35:49Z

@drcrallen I've changed existing tests so that they cover all added "prototypes" (and it helped to find bugs).

Regarding testing inspectRuntimeShape(), I don't think it's needed. Mistakes in inspectRuntimeShape would cause performance regression at worst, and to the level not worse than pre-#3798.

Manual testing of inspectRuntimeShape() is

a giant amount of work
often not possible, with anonymous or private classes
still won't help much, because most likely type of mistake is adding a new field to HotLoopCallee-extending class, that should be inspected, but forgetting to add it to inspectRuntimeShape(). But in this case manual test would still pass, because it is also not updated.

Ways to test inspectRuntimeShape() automatically could exist, some approaches possibly similar to mocking, when entering @CalledFromHotLoop-annotated methods is recorded, entering downstream methods is recorded, and checking that for the same runtime shape, the same methods are called at all downstream call sites. However it's a huge and complex task to develop such automated inspectRuntimeShape() testing framework, and definitely not as part of this PR.

drcrallen · 2017-05-12T19:58:36Z

+  @Override
+  public Offset clone()
+  {
+    FilteredOffset offset = (FilteredOffset) super.clone();


What guarantees an Offset clone is castable like this?

It delegates to Object.clone(), which returns instance of the same class as this.

drcrallen · 2017-05-12T19:59:51Z

+import io.druid.segment.historical.OffsetHolder;
+import org.roaringbitmap.IntIterator;
+
+final class FilteredOffset extends Offset


This class seems to have no unit tests

This class if factored out of QueryableIndexStorageAdapter. It didn't exist before this PR, because QueryableIndexStorageAdapter used to handle post filters using a special cursor, and now there is the same cursor accepting any provided Offset, post-filtered like FilteredOffset, or not. So this class is tested by all the same tests which test QueryableIndexStorageAdapter directly or indirectly, mostly tests of specific query types.

drcrallen · 2017-05-12T20:01:28Z

-                          advance();
-                          count++;
+                        if (!Thread.currentThread().isInterrupted()) {
+                          cursorOffset.increment();


incrementInterruptibly?

drcrallen · 2017-05-12T20:02:55Z

  public int lookupId(ActualType name);
  public int getCardinality();
+
+  HistoricalDimensionSelector makeDimensionSelector(OffsetHolder offsetHolder, ExtractionFn extractionFn);


I'm curious if this will screw up @cheddar / @himanshug 's lucine index stuff

Is there a way to architect this so there isn't a historical-specific method in DictionaryEncodedColumn?

Changed to return just DimensionSelector

drcrallen · 2017-05-12T20:04:59Z

    return cachedLookups.size();
  }

+  @Override


This seems to have no unit tests

The same as for FilteredOffset, this method is factored out of QueryableIndexStorageAdapter and tested through it

drcrallen · 2017-05-12T20:18:09Z

-        return true;
-      }
-    };
+    return makeMatcher(matchers.toArray(new ValueMatcher[0]));


new ValueMatcher[0] can be a static constant

drcrallen · 2017-05-12T20:19:26Z


  private ValueMatcher makeMatcher(final ValueMatcher[] baseMatchers)
  {
+    Preconditions.checkState(baseMatchers.length > 0);


Can this support a 0 length array and just return BooleanValueMatcher.of(false);?

We prohibit 0 length in constructor, so here it shouldn't appear too

drcrallen · 2017-05-12T20:23:05Z

  public ValueMatcher makeMatcher(ColumnSelectorFactory factory)
  {
-    if (filters.size() == 0) {
-      return BooleanValueMatcher.of(false);


This is odd because on one hand you end up with a value matcher that returned false here, but makeMatcher has a fall-through case of true.

makeMatcher(ValueMatcher[]) has Preconditions.checkState(baseMatchers.length > 0);, so it should end up throwing an exception anyway

drcrallen · 2017-05-12T20:25:53Z

+      public void inspectRuntimeShape(RuntimeShapeInspector inspector)
+      {
+        inspector.visit("firstBaseMatcher", baseMatchers[0]);
+        inspector.visit("secondBaseMatcher", baseMatchers[1]);


ah, I see, >0 and !=1 ok

drcrallen · 2017-05-12T20:27:17Z

+/**
+ * Public bridge is needed, because MagicAccessorImpl is a package-private class.
+ */
+public class MagicAccessorBridge extends MagicAccessorImpl


Doesn't extending sun.* packages violate the java license agreement?

Seems so (http://www.oracle.com/technetwork/java/javase/terms/license/index.html). Among high-profile projects, I found Groovy extends MagicAccessorImpl:

https://github.com/apache/groovy/blob/183b8fbf0248a2dceffbba684e50c3c2c060e46c/src/main/org/codehaus/groovy/reflection/SunClassLoader.java#L70

but org.codehaus.groovy.reflection is not in sun.reflect

It creates "sun/reflect/GroovyMagic" class. Actually an interesting question, asked here: https://twitter.com/leventov/status/863155057113669632

leventov · 2017-05-12T22:03:50Z

On #4079 (comment): no, because it's in the method called "advanceUninterruptibly"

drcrallen

Thanks @leventov !

…storical-topn-monomorphic-processing

leventov added 2 commits March 17, 2017 18:17

Monomorphic processing of topN queries with simple double aggregators…

42c080b

… and historical segments

Add CalledFromHotLoop annocations to specialized methods in SimpleDou…

d2d53c5

…bleBufferAggregator

leventov assigned gianm Mar 18, 2017

leventov added this to the 0.10.1 milestone Mar 18, 2017

leventov changed the title ~~Monomorphic processing of TopN queries with simple double aggregators over and historical segments (part of #3798)~~ Monomorphic processing of TopN queries with simple double aggregators over historical segments (part of #3798) Mar 18, 2017

leventov added 2 commits March 17, 2017 20:26

Fix a bug in Historical1SimpleDoubleAggPooledTopNScannerPrototype

719e979

Fix a bug in SpecializationService

37b04c3

In SpecializationService, emit maxSpecializations warning only once

c63901b

egor-ryashin reviewed Mar 23, 2017

View reviewed changes

Make GenericIndexed.theBuffer final

439c906

dgolitsyn reviewed Mar 27, 2017

View reviewed changes

leventov added 5 commits March 27, 2017 17:11

Address comments

19b4fa5

Merge remote-tracking branch 'upstream/master' into double-agg-and-hi…

005fa81

…storical-topn-monomorphic-processing

Newline

6b233f2

Reapply 439c906 (Make GenericIndexed.theBuffer final)

18e9ad0

Remove extra PooledTopNAlgorithm.capabilities field

996874d

dgolitsyn reviewed Mar 28, 2017

View reviewed changes

Improve CachingIndexed.inspectRuntimeShape()

10c6735

egor-ryashin reviewed Mar 29, 2017

View reviewed changes

leventov added 2 commits March 29, 2017 14:31

Fix CompressedVSizeIntsIndexedSupplier.inspectRuntimeShape()

8ded30c

Don't override inspectRuntimeShape() in subclasses of CompressedVSize…

c9339c9

…IndexedInts

egor-ryashin reviewed Mar 30, 2017

View reviewed changes

leventov added 4 commits March 30, 2017 16:22

Annotate methods in specializations of DimensionSelector and FloatCol…

de77b76

…umnSelector with @CalledFromHotLoop

Make ValueMatcher to implement HotLoopCallee

378f622

Doc fix

7b89337

Fix inspectRuntimeShape() impl in ExpressionSelectors

5cc302b

leventov assigned drcrallen May 10, 2017

drcrallen reviewed May 10, 2017

View reviewed changes

Refactor PooledTopNAlgorithm.scanAndAggregate()

9771c3a

drcrallen reviewed May 10, 2017

View reviewed changes

Small refactoring

d405b97

drcrallen reviewed May 10, 2017

View reviewed changes

drcrallen requested changes May 10, 2017

View reviewed changes

leventov added 2 commits May 10, 2017 19:26

Add 'nothing to inspect' messages in empty HotLoopCallee.inspectRunti…

58f0821

…meShape() implementations

Don't care about runtime shape in tests

f6efc31

leventov added the WIP label May 11, 2017

Fix accessor bugs in Historical1SimpleDoubleAggPooledTopNScannerProto…

d63b26e

…type and HistoricalSingleValueDimSelector1SimpleDoubleAggPooledTopNScannerPrototype, cover them with tests

leventov added Performance and removed WIP labels May 11, 2017

Doc wording

35341a5

drcrallen requested changes May 12, 2017

View reviewed changes

Address comments

0a48d89

leventov added 2 commits May 12, 2017 17:57

Remove MagicAccessorBridge and ensure Offset subclasses are public

331a32a

Attach error message to element

69988b5

drcrallen approved these changes May 15, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into double-agg-and-hi…

53f77fd

…storical-topn-monomorphic-processing

drcrallen merged commit d400f23 into apache:master May 16, 2017

drcrallen deleted the double-agg-and-historical-topn-monomorphic-processing branch May 16, 2017 23:19

clambertus unassigned drcrallen Jul 6, 2018

leventov mentioned this pull request Jul 22, 2018

Druid 'Shapeshifting' Columns #6016

Open

7 tasks

Conversation

leventov commented Mar 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

egor-ryashin commented Mar 22, 2017

Uh oh!

leventov commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egor-ryashin Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgolitsyn Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov commented Mar 18, 2017 •

edited

Loading

egor-ryashin Mar 23, 2017 •

edited

Loading

dgolitsyn Mar 27, 2017 •

edited

Loading

leventov Mar 29, 2017 •

edited

Loading

leventov Mar 29, 2017 •

edited

Loading