Automatic sizing for GroupBy dictionaries. by gianm · Pull Request #12763 · apache/druid

gianm · 2022-07-09T03:39:26Z

Merging and selector dictionary sizes currently both default to 100MB.
This is not optimal, because it can lead to OOM on small servers and
insufficient resource utilization on larger servers. It also invites
end users to try to tune it when queries run out of dictionary space,
which can make things worse if the end user sets it to too high.

So, this patch:

Adds automatic tuning for selector and merge dictionaries. Selectors
use up to 10% of the heap and merge buffers use up to 30% of the heap
(aggregate across all queries).
Updates out-of-memory error messages to emphasize enabling disk
spilling vs. increasing memory parameters. With the memory parameters
automatically sized, it is more likely that an end user will get
benefit from enabling disk spilling.
Removes the query context parameters that allow lowering of configured
dictionary sizes. These complicate the calculation, and I don't see a
reasonable use case for them.

Merging and selector dictionary sizes currently both default to 100MB. This is not optimal, because it can lead to OOM on small servers and insufficient resource utilization on larger servers. It also invites end users to try to tune it when queries run out of dictionary space, which can make things worse if the end user sets it to too high. So, this patch: - Adds automatic tuning for selector and merge dictionaries. Selectors use up to 15% of the heap and merge buffers use up to 30% of the heap (aggregate across all queries). - Updates out-of-memory error messages to emphasize enabling disk spilling vs. increasing memory parameters. With the memory parameters automatically sized, it is more likely that an end user will get benefit from enabling disk spilling. - Removes the query context parameters that allow lowering of configured dictionary sizes. These complicate the calculation, and I don't see a reasonable use case for them.

vogievetsky

I pair programmed with Gian on this (read: sat there and watched while making snarky comments about Java)

FrankChen021 · 2022-07-09T05:16:12Z

+  private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+  private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
+  private static final long MIN_AUTOMATIC_DICTIONARY_SIZE = 1_000_000;
+  private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;


These two MIN and MAX ensure the range of automatic computed value, I think it's better to document them.

BTW, these two are in decimal byte format, should we use binary byte which is 1024 * 1024 and 1024 * 1024 * 1024 ?

Yeah, I agree. I changed MIN to 1 byte and didn't document it (it seems unnecessary to mention). I documented the 1GB max.

How about defining the MAX as 1GiB instead of 1GB? I think 1GiB would be less confusing.

Ah, I missed the comment about binary bytes. I dunno if there is a reason to use binary bytes. I kept it decimal since they both seem fine to me.

Hmm. Why is it less confusing to use 1GiB instead of 1GB? As one data point, the old default (100MB) was in decimal.

When representing a size of memory, binary format is commonly used while decimal format is usually for disk size.
It's OK to keep it in decimal format.

Ah, I see. I wrote it as decimal since I am not familiar with that convention. I'll leave it as is for this patch.

FrankChen021 · 2022-07-09T05:17:17Z

  private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
  private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+  private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+  private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;


Why it's 0.3 and 0.1 for these two variables respectively? Could you add some explanation here?

I added this explanation:

+ // Constants for sizing merging and selector dictionaries. Rationale for these constants: + // 1) In no case do we want total aggregate dictionary size to exceed 40% of max memory. + // 2) In no case do we want any dictionary to exceed 1GB of memory: if heaps are giant, better to spill at + // "reasonable" sizes rather than get giant dictionaries. (There is probably some other reason the user + // wanted a giant heap, so we shouldn't monopolize it with dictionaries.) + // 3) Use somewhat more memory for merging dictionary vs. selector dictionaries, because if a merging + // dictionary is full we must spill to disk, whereas if a selector dictionary is full we simply emit + // early to the merge buffer. So, a merging dictionary filling up has a more severe impact on + // query performance.

FrankChen021 · 2022-07-09T05:20:08Z

  @JsonProperty
  // Size of on-heap string dictionary for merging, per-query; when exceeded, partial results will be spilled to disk
-  private long maxMergingDictionarySize = 100_000_000L;
+  private long maxMergingDictionarySize = AUTOMATIC;


I think we can change the type of maxSelectorDictionarySize and maxMergingDictionarySize from long to HumanReadableBytes to support human-readable format configuration.

Good idea. I changed it.

FrankChen021 · 2022-07-09T05:21:05Z

-            getMaxMergingDictionarySize()
-        )).longValue(),
-        getMaxMergingDictionarySize()
-    );


Since these two are removed from QueryContext, should we document it in the release notes?

Yes, we should. I added a release notes label.

gianm · 2022-07-11T15:20:47Z

Thanks for reviewing @FrankChen021 @vogievetsky

* Automatic sizing for GroupBy dictionary sizes. Merging and selector dictionary sizes currently both default to 100MB. This is not optimal, because it can lead to OOM on small servers and insufficient resource utilization on larger servers. It also invites end users to try to tune it when queries run out of dictionary space, which can make things worse if the end user sets it to too high. So, this patch: - Adds automatic tuning for selector and merge dictionaries. Selectors use up to 15% of the heap and merge buffers use up to 30% of the heap (aggregate across all queries). - Updates out-of-memory error messages to emphasize enabling disk spilling vs. increasing memory parameters. With the memory parameters automatically sized, it is more likely that an end user will get benefit from enabling disk spilling. - Removes the query context parameters that allow lowering of configured dictionary sizes. These complicate the calculation, and I don't see a reasonable use case for them. * Adjust tests. * Review adjustments. * Additional comment. * Remove unused import.

gianm added Ease of Use Area - Querying labels Jul 9, 2022

vogievetsky approved these changes Jul 9, 2022

View reviewed changes

Adjust tests.

c3ec6aa

FrankChen021 reviewed Jul 9, 2022

View reviewed changes

gianm added the Release Notes label Jul 10, 2022

gianm added 3 commits July 10, 2022 14:30

Review adjustments.

efe498d

Additional comment.

2338405

Remove unused import.

90238cd

FrankChen021 approved these changes Jul 11, 2022

View reviewed changes

gianm merged commit 97207cd into apache:master Jul 11, 2022

gianm deleted the merging-dictionary-size branch July 11, 2022 15:20

abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022

techdocsmith mentioned this pull request Aug 26, 2022

[Draft] 24.0 Release notes #12825

Closed

abhishekagarwal87 mentioned this pull request Sep 8, 2022

Test issue [Please ignore] #13055

Closed

LakshSingla mentioned this pull request Nov 24, 2023

Fix dictionarySize overrides in tests #15354

Merged

4 tasks

Uh oh!

Conversation

gianm commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vogievetsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankChen021 Jul 9, 2022 • 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!

gianm Jul 11, 2022 • 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!

gianm commented Jul 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gianm commented Jul 9, 2022 •

edited

Loading

FrankChen021 Jul 9, 2022 •

edited

Loading

gianm Jul 11, 2022 •

edited

Loading