Do not use more then 98K of memory for uniqCombined* #7236
Conversation
azat
force-pushed
the
uniqCombined-memory-usage
branch
2 times, most recently
from
f13c32d
to
d745e7f
Compare
alexey-milovidov
added
can be tested
pr-improvement
| 16, | ||
| 13, | ||
| 12 + (sizeof(Key) == sizeof(UInt32)), |
There was a problem hiding this comment.
Here we have lowered max size degree by one when type is UInt64 and keep as before for UInt32. It's Ok.
There was a problem hiding this comment.
This + (sizeof(Key) == sizeof(UInt32)) looks totally puzzling. Why not just write it out:
template <size_t key_size> constexpr int hll_threshold = 12;
template <> constexpr int hll_threshold<sizeof(UInt32)> = 13;
There was a problem hiding this comment.
Well it doesn't look puzzlingly to me, but I agree with you.
However, this bit is in two places, and writing this in your way will overly complicate the code
Although I don't have any strong feeling about this and can go your way
There was a problem hiding this comment.
If I'm not missing anything, it should reduce duplication, you just write hll_threshold<sizeof(Key)> instead of 12 + sizeof....
There was a problem hiding this comment.
@akuzm Yes, it will be more clear. I'd like if you will do this modification.
azat
force-pushed
the
uniqCombined-memory-usage
branch
from
d745e7f
to
bf5945f
Compare
uniqCombined() uses hashtable for medium cardinality, and since HashTable resize by the power of 2 (well actually HashTableGrower grows double by the power of 2, hence HashTableGrower::increaseSize() should be overwritten to change this), with 1<<13 (default for uniqCombined) and UInt64 HashValueType, the HashTable will takes: getBufferSizeInBytes() == 131072 While it should be not greater then sizeof(HLL) ~= 98K, so reduce the maximum cardinality for hashtable to 1<<12 with UInt64 HashValueType and to 1<13 with UInt32, overwrite HashTableGrower::increaseSize() and cover this using max_memory_usage. Refs: ClickHouse#7221 (comment) v2: cover uniqCombined() with non-default K
azat
force-pushed
the
uniqCombined-memory-usage
branch
from
bf5945f
to
e373862
Compare
|
Apparently some tests were broken in this PR, but we didn't notice because I broke the CI. @azat could you look into this? It's 01017_uniqCombined_memory_usage and 00212_shard_aggregate_function_uniq |
|
Sure, still take a look into this today |
Cannot reproduce the failure of this test, is there any chance on getting logs from the build on CI? |
|
Do you mean the test log or the build log itself? For tests, there are links to logs at the bottom of this test page: In the list of checks, this is Details of 'Functional stateless tests (release)'. The log of clickhouse-test: The build logs and packages can be found here: In the list of checks, this is Details for 'ClickHouse build check'. |
|
The list of checks for each commit in master can be seen if you press a cross or a tick to the right of the commit time at this page: |
|
Thanks, got it! |
Hm, interesting client fails with "Attempt to read after eof" error first time (after server restart) while the server indeed reports "Memory limit (for query) exceeded", and I can reproduce it with packages from this build, but not in my local setup (and I'm not surprised) And this is not related to my changes I guess, since every first error Looking into this more and the problem is in address resolving, look: And indeed: Will submit patch set shortly (that will have details). |
…ined(12)(String) In e373862 the maxium size for HashTable in CombinedCardinalityEstimator had been reduced for power of 2 for String (since otherwise the size of the hashtable becames bigger then the sizeof HLL). Refs: ClickHouse#7236 (comment) Fixes: e373862 ("Do not use more then 98K of memory for uniqCombined*")
|
For changelog. Remove if this is non-significant change.
Category (leave one):
Short description (up to few sentences):
Do not use more then 98K of memory for uniqCombined*
Detailed description (optional):
Refs: #7221 (comment)