Fine grained buffer management for groupby by jihoonson · Pull Request #3863 · apache/druid

jihoonson · 2017-01-19T10:33:06Z

In this PR, I newly added two methods to Sequence for lazy evaluation of initial value of accumulation.

After this patch, the buffer acquisition order for nested group-by query execution is changed from outer -> inner to inner -> outer. In addition, once the intermediate result of inner queries aren't needed anymore, the buffer holding it is released immediately. As a result, nested group-by queries always need at most 2 buffers.

#3693 will cause some conflicts, and I believe that some changes of #3693 should also be applied to this patch. So, please review it first.

This change is

fjy · 2017-01-20T18:05:38Z

@jihoonson lots of merge conflicts due to some other PRs getting merged

jihoonson · 2017-01-24T03:13:13Z

@fjy, thanks. I fixed conflicts.

jon-wei · 2017-01-28T02:14:44Z

I've reviewed the core changes in GroupByRowProcessor so far, the delayed buffer initialization looks okay to me, I'll take a deeper look at the sequence changes in other files

Can you add a description to the PR that this changes the order of nested group by buffer acquisition such that the inner queries acquire buffers first as they are being processed, and also limits the number of merge buffers needed for nested queries to 2, regardless of the nesting depth?

#3806 added a doc comment in docs/content/querying/groupbyquery.md about the nested group by buffer usage, can you also update that?

jihoonson · 2017-01-28T04:17:00Z

@jon-wei, thanks for your review. I updated the PR description.
I'll also update the group-by query document soon.

jon-wei

Sequence changes look okay to me

Re: the deadlock in #3819, couldn't that still happen with this PR if a query timeout is not set? e.g., two nested group by queries running at the same time, with 2 merge buffers available, they both get a buffer for the inner query, but then neither can get the second buffer needed?

jon-wei · 2017-01-31T00:04:01Z

+
+  @Override
+  public <OutType> Yielder<OutType> toYielder(
+      Supplier<OutType> initValue, YieldingAccumulator<OutType, T> accumulator


suggest renaming initValue in the methods with supplier parameters to initValueSupplier to make it easier to distinguish the methods

jon-wei · 2017-01-31T00:33:06Z

Also, can you point me to the change related to the setting a blocking pool timeout? I wasn't able to find that while reviewing

jihoonson · 2017-01-31T02:06:28Z

@jon-wei and I had a talk via a messenger, and we concluded this patch isn't enough to #3819. I'll make another PR for it.

Regarding the change for setting a blocking pool timeout, I was little confused. Sorry for your confusion. I'll address your comments soon.

jihoonson · 2017-01-31T08:46:17Z

@jon-wei, thanks. I addressed your comments.

jon-wei · 2017-01-31T19:43:04Z

thanks, 👍

gianm

@jihoonson, re: the comment on Sequence.accumulate, do you think it makes sense to do this without modifying the Sequence interface?

Please also add a groupBy query test verifying that no more than 2 buffers are needed for a deeply nested groupBy. I recognize that there's a SQL test that hits this, but the SQL tests shouldn't be relied on for testing query engine specific behaviors.

gianm · 2017-02-07T00:12:16Z

+   *
+   * @return accumulated value.
+   */
+  <OutType> OutType accumulate(Supplier<OutType> initValSupplier, Accumulator<OutType, T> accumulator);


I don't understand the point of having a lazy initValue option to accumulate. Accumulate is supposed to do all of its work immediately, so why would the initValue need to be lazy?

Oh, I see, it's so things like BaseSequence can defer creation of initValue until accumulation actually starts (after the iterator is made).

In that case, instead of modifying the Sequence interface, what do you think about moving buffer-taking from out of iterator-making and into the Accumulator in GroupByRowProcessor?

@gianm yes, I also considered that way. To do so, I think it is inevitable to check the grouper's initialization state in the accumulate() method. I would like to avoid checking it for every accumulate() call. Another way may be to add an initialization method to Accumulator, but I don't think this is a good way.

If there are some reasonable reasons, I can consider moving buffer-taking to Accumulator again. Would you share if you have?

The main reasons I'm asking these questions are:

the contracts of the new methods confused me: it took me a bit to understand why there was both a lazy-init and non-lazy-init version of accumulate, given that both are expected to init the value and then fully exhaust the sequence before returning.

increasing size of the Sequence interface makes usage and implementation more complex, so I want to make sure to consider other options first.

I agree about trying to avoid an init for Accumulator.

I bet the overhead of checking for initialization in each call to accumulate() is going to be unmeasurably low. There's a lot of stuff going on per row (reading values, hashing, table lookup, aggregation).

What do you think is best?

I understand what you're concerned with. I agree on it may confuse users. How about adding afterMake() method to BaseSequence.IteratorMaker?

That sounds like a good approach, along with some javadoc that the purpose of separating afterMake() from make() is that it allows resource allocation to be deferred until iteration actually begins, which matters if Sequences are nested inside each other.

Hmm, after thinking about it more maybe I changed my mind… I think afterMake() doesn't make much sense for this since the resource allocation is tied to accumulation, not iteration. So it should really be associated with the Accumulator and not the IteratorMaker.

I'd be fine with either of these:

Stick with your original idea of accumulate taking a Supplier of the initValue, and update the javadocs to clarify that the purpose is to defer initialization of initValue until accumulation actually begins. Actually this seems like behavior we'd always want (why not?) and so I'd also consider making this the only API rather than having both a supplier and nonsupplier one. Callers that don't care about deferring initialization can wrap the thing in Suppliers.ofInstance.

Just keep things simple and have the GroupBy Accumulator check for inited-ness on each call to accumulate(), I think the perf overhead here won't be bad.

Sorry for the back and forth, just wrapping my head around what is the best and clearest way of doing this.

Right. afterMake() is not a good option.

I think making an iterator in BaseSequence.accumulate() causes this problem. Sequence.accumulate() method is supposed to start to do only its accumulation work immediately, but it actually accompanies a side-effect, initialization and cleanup of an iterator. It looks for convenience of initializing and cleaning up the iterator, but I'm not sure this is a good design choice. Maybe WrappingSequence added recently (in #3693) looks more proper for this purpose, but I don't have any idea how to move iterator initialization and cleanup out of BaseSequence without a big change.

I feel that my idea is little tricky to work around the problem of tightly coupled iterator initialization and accumulation. Let's do it simply for now.

gianm · 2017-02-07T01:44:42Z

+
+  /**
+   * Return an Yielder for accumulated sequence.
+   * The {@code initValSupplier} provides an way for lazy evaluation of the initial value.


If this method guarantees lazy evaluation of the initial value, then this javadoc should be stronger and say that.

gianm · 2017-02-07T01:46:02Z

+   *
+   * @see Yielder
+   */
+  <OutType> Yielder<OutType> toYielder(Supplier<OutType> initValSupplier, YieldingAccumulator<OutType, T> accumulator);


The behavior of this method is different enough from toYielder that maybe a new name is warranted, like toYielderLazy.

gianm · 2017-02-07T01:46:19Z

    {
      @Override
-      public <OutType> Yielder<OutType> toYielder(OutType initValue, YieldingAccumulator<OutType, T> accumulator)
+      public <OutType> Yielder<OutType> toYielder(OutType initValSupplier, YieldingAccumulator<OutType, T> accumulator)


Rename gone wild? This isn't a Supplier.

gianm · 2017-02-07T01:49:50Z

+This merge buffer is immediately released once they are not used anymore during the query processing,
+but two or more concurrent nested groupBys can potentially lead to deadlocks since the merge buffers are limited in number
+and are acquired one-by-one instead of a complete set. At this time we recommend that you avoid too many concurrent
+execution of groupBys with the v2 strategy.


At this time we recommend that you avoid too many concurrent execution of groupBys with the v2 strategy.

This is stronger than what was there before… probably too strong. It's fine to execute as many concurrent non-deeply-nested groupBys as you want. Even double-nested groupBys are fine (groupBy -> groupBy -> table). Could you please adjust the wording? I don't want to scare people too much, just an appropriate amount.

Thanks. I'll improve the doc.

gianm · 2017-02-07T01:50:14Z

-that you avoid deeply-nested groupBys with the v2 strategy. Doubly-nested groupBys (groupBy -> groupBy -> table) are
-safe and do not suffer from this issue. If you like, you can forbid deeper nesting by setting
-`druid.sql.planner.maxQueryCount = 2`.
+For executing nested groupBys with the v2 groupBy strategy, you need to set `druid.processing.numMergeBuffers` to at least 2.


Similar comment to groupbyquery.md

jihoonson · 2017-02-08T06:41:36Z

@gianm thanks for your review. I'll add tests to check the number of merge buffers used for group-by queries.

- Revert Sequence - Add isInitialized() to Grouper - Initialize the grouper in RowBasedGrouperHelper.Accumulator - Simple refactoring RowBasedGrouperHelper.Accumulator - Add tests for checking the number of used merge buffers - Improve docs

…grained-buffer-management-for-groupby

jihoonson · 2017-02-10T12:46:34Z

@gianm thanks. I addressed your comments. Additionally, I did a simple refactoring for RowBasedGrouperHelper.

gianm

@jihoonson, code looks good.

Were the RowBasedGrouperHelper refactorings meant to improve performance or readability? Did you do benchmarks of groupBy before and after the refactorings to confirm performance is equal or better?

gianm · 2017-02-13T18:00:02Z

 Additionally, the "v2" strategy uses merging buffers for merging. It is currently the only query implementation that
 does so. By default, Druid is configured without any merging buffer pool, so to use the "v2" strategy you must also
-set `druid.processing.numMergeBuffers` to some non-zero number.
+set `druid.processing.numMergeBuffers` to some non-zero number. Furthermore, if you want to execute deeply nested gropuBys,


groupBys (spelling)

Thanks. Fixed.

jihoonson · 2017-02-13T23:41:30Z

@gianm just for readability. I simply run GroupByBenchmark and got the same result.

gianm

thx @jihoonson. 👍 after travis

jihoonson added 3 commits January 19, 2017 18:33

Fine-grained buffer management for group by queries

4652250

Remove maxQueryCount from GroupByRules

1ed6ce2

Fix code style

6d3f533

gianm closed this Jan 19, 2017

gianm reopened this Jan 19, 2017

fjy added this to the 0.10.0 milestone Jan 20, 2017

jihoonson added 3 commits January 24, 2017 10:07

Merge master

7af3b23

Merge branch 'master' into fine-grained-buffer-management-for-groupby

3a13107

Fix compilation failure

e731c50

gianm assigned gianm, fjy and jon-wei and unassigned gianm and fjy Jan 24, 2017

jon-wei reviewed Jan 31, 2017

View reviewed changes

Address comments

4f11414

gianm reviewed Feb 7, 2017

View reviewed changes

gianm assigned leventov and unassigned jon-wei Feb 9, 2017

Address comments

8a79700

- Revert Sequence - Add isInitialized() to Grouper - Initialize the grouper in RowBasedGrouperHelper.Accumulator - Simple refactoring RowBasedGrouperHelper.Accumulator - Add tests for checking the number of used merge buffers - Improve docs

jihoonson added 2 commits February 10, 2017 19:48

Revert unnecessary changes

1ab663b

Merge branch 'master' of https://github.com/druid-io/druid into fine-…

44c09dd

…grained-buffer-management-for-groupby

change to visible to testing

9c1ca81

gianm reviewed Feb 13, 2017

View reviewed changes

fix misspelling

54eefd3

gianm approved these changes Feb 13, 2017

View reviewed changes

gianm closed this Feb 14, 2017

gianm reopened this Feb 14, 2017

gianm merged commit a459db6 into apache:master Feb 14, 2017

clambertus unassigned leventov and gianm Jul 6, 2018

Conversation

jihoonson commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjy commented Jan 20, 2017

Uh oh!

jihoonson commented Jan 24, 2017

Uh oh!

jon-wei commented Jan 28, 2017

Uh oh!

jihoonson commented Jan 28, 2017

Uh oh!

jon-wei left a comment • 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!

jon-wei commented Jan 31, 2017

Uh oh!

jihoonson commented Jan 31, 2017

Uh oh!

jihoonson commented Jan 31, 2017

Uh oh!

jon-wei commented Jan 31, 2017

Uh oh!

gianm left a comment

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!

gianm Feb 9, 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!

jihoonson Feb 10, 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!

jihoonson commented Feb 8, 2017

Uh oh!

jihoonson commented Feb 10, 2017

Uh oh!

gianm left a comment

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!

jihoonson commented Feb 13, 2017

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

jihoonson commented Jan 19, 2017 •

edited

Loading

jon-wei left a comment •

edited

Loading

gianm Feb 9, 2017 •

edited

Loading

jihoonson Feb 10, 2017 •

edited

Loading