colexecagg: fix memory accounting for decimals with avg and sum#57318
colexecagg: fix memory accounting for decimals with avg and sum#57318craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
Thoughts on calling |
|
Benchmarks are here. |
|
Ran the benchmarks if we call |
asubiotto
left a comment
There was a problem hiding this comment.
I think we shouldn't compromise on stability for performance.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @asubiotto)
Previously, we were not calling `PerformOperation` to account for the memory used by the decimals (which are of variable size), and this is now fixed. Additionally, this commit refactors several other templates to be more similar to each other. Note that we decided to call `PerformOperation` on all of the aggregate functions even if we know that all types that the functions operate on are of fixed size and that call is not strictly necessary. However, this has negligible impact on performance in most cases and makes the layout of `Compute` methods of all functions the same ensuring that we don't forget these calls in the future. Additionally, this commit adds capturing of the column to force bounds check to work for min/max and sum aggregates. Release note: None
d2e4698 to
fce076e
Compare
|
I agree, updated the commit. The newest benchmarks are here. TFTR! bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
Previously, we were not calling
PerformOperationto account for thememory used by the decimals (which are of variable size), and this is
now fixed. Additionally, this commit refactors several other templates to
be more similar to each other.
Note that we decided to call
PerformOperationon all of the aggregatefunctions even if we know that all types that the functions operate on
are of fixed size and that call is not strictly necessary. However, this
has negligible impact on performance in most cases and makes the layout
of
Computemethods of all functions the same ensuring that we don'tforget these calls in the future.
Additionally, this commit adds capturing of the column to force bounds
check to work for min/max and sum aggregates.
Release note: None