Aligned aggregate state #2754 #2808
Conversation
| total_size_of_aggregate_states += params.aggregates[i].function->sizeOfData(); | ||
|
|
||
| // aggreate states are aligned based on maximum requirement | ||
| align_aggregate_states = std::max(align_aggregate_states, |
There was a problem hiding this comment.
Why do we need to align all states to maximum alignment?
There was a problem hiding this comment.
IF we don't align states based on maximum requirement, we cannot guarantee non-first state to match its requirement.
e.g.
the first state need to be aligned by one byte
the second state to be aligned by four bytes.
the states allocated need to be aligned by four bytes.
There was a problem hiding this comment.
Ok. I forgot all of them are allocated at once.
| @@ -0,0 +1,8 @@ | |||
| drop table if exists test.orin_test; | |||
There was a problem hiding this comment.
Could you please merge with master
(because these changes are already in master)
There was a problem hiding this comment.
Done, yes, I upgrade my private branch.
dbms/src/Common/Arena.h
Outdated
|
|
||
| size_t pos = (size_t)(head->pos); | ||
| // next pos match align requirement | ||
| pos = (pos & ~(align - 1)) + align; |
There was a problem hiding this comment.
It will add extra padding even if pos is already aligned.
There was a problem hiding this comment.
Done, Changing it to pos = ((pos-1) & ~(align - 1)) + align;
dbms/src/Common/Arena.h
Outdated
| // Fast code path for non-alignment requirement | ||
| if (align <= 1) return alloc(size); | ||
|
|
||
| size_t pos = (size_t)(head->pos); |
There was a problem hiding this comment.
According to style, you should use C++ casts.
There was a problem hiding this comment.
uintptr_t is the right type for this purpose.
There was a problem hiding this comment.
There was a problem hiding this comment.
Done, I don't use std::align for now because calculating aligned address is so simple.
dbms/src/Common/Arena.h
Outdated
| char * alloc_align(size_t size, size_t align) | ||
| { | ||
| // Fast code path for non-alignment requirement | ||
| if (align <= 1) return alloc(size); |
dbms/src/Common/Arena.h
Outdated
| @@ -124,6 +124,30 @@ class Arena : private boost::noncopyable | |||
| return res; | |||
| } | |||
|
|
|||
| /// Get peice of memory with alignment | |||
| char * alloc_align(size_t size, size_t align) | |||
There was a problem hiding this comment.
This should be named alignedAlloc.
dbms/src/Common/Arena.h
Outdated
|
|
||
| if (unlikely(pos + size > (size_t)(head->end))) | ||
| { | ||
| addChunk(size); |
There was a problem hiding this comment.
Better to propagate alignment requirement to addChunk.
There was a problem hiding this comment.
hmm, Don't propagate this to addChunk for now.
dbms/src/Interpreters/Aggregator.cpp
Outdated
| @@ -1942,7 +1969,8 @@ void NO_INLINE Aggregator::mergeWithoutKeyStreamsImpl( | |||
| AggregatedDataWithoutKey & res = result.without_key; | |||
| if (!res) | |||
| { | |||
| AggregateDataPtr place = result.aggregates_pool->alloc(total_size_of_aggregate_states); | |||
| AggregateDataPtr place = result.aggregates_pool->alloc_align(total_size_of_aggregate_states, | |||
There was a problem hiding this comment.
Also look at SpecializedAggregator that is used when compile setting is enabled.
There was a problem hiding this comment.
Done, thanks for pointing this out
|
Usages of aggregate states: FunctionsMiscellaneous were forgotten. |
|
AggregateFunctionNull |
| } | ||
| // extend total_size to next alignment requirement | ||
| total_size_of_aggregate_states = | ||
| (total_size_of_aggregate_states & ~(next_align_req - 1)) + next_align_req; |
There was a problem hiding this comment.
Looks like you add a padding of size next_align_req even if the total_size_of_aggregate_states was aligned.
|
AggregateFunctionForEach |
|
ColumnAggregateFunction::getExtremes |
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
The change is to use aligned aggregate state if it is allocated from arena(the problem occur). During my test, the impact is negligible.