Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TEST for allocation strategy in min/max accumulator #12845

Closed
wants to merge 16 commits into from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Oct 10, 2024

This is a draft PR to test a suggestion from @Dandandan in #12792 #12792 (comment)

I wonder if it makes sense to overallocate a bit here to avoid reallocations (e.g. 2x the size, or using a statistic, e.g. (total_data_bytes / min_max.len()) * factor

Change in 576cc32 and I am running performance tests

@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) functions labels Oct 10, 2024
fn set_value(&mut self, group_index: usize, new_val: &[u8]) {
match self.min_max[group_index].as_mut() {
None => {
// No existing value, so allocate a new one (allocate 2x the size of the input)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the change in this PR

@alamb
Copy link
Contributor Author

alamb commented Oct 10, 2024

Here are some benchmark results (tldr I don't think changing the allocation strategy makes a measurable difference)

Maybe the internal allocator already avoids most reallocations (on small existing allocations)

Note that only Q21, Q22 and Q28 actually have MIN on a string column, so changes in other queries I think is noise

Seems like

--------------------
Benchmark clickbench_partitioned.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃  main_base ┃ alamb_min_max_string_test ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0     │     2.65ms │                    2.19ms │ +1.21x faster │
│ QQuery 1     │    38.48ms │                   39.12ms │     no change │
│ QQuery 2     │    94.46ms │                   95.24ms │     no change │
│ QQuery 3     │    99.62ms │                  101.94ms │     no change │
│ QQuery 4     │   933.75ms │                  919.79ms │     no change │
│ QQuery 5     │   967.18ms │                 1006.13ms │     no change │
│ QQuery 6     │    34.64ms │                   34.19ms │     no change │
│ QQuery 7     │    42.35ms │                   42.63ms │     no change │
│ QQuery 8     │  1382.08ms │                 1407.39ms │     no change │
│ QQuery 9     │  1305.09ms │                 1357.32ms │     no change │
│ QQuery 10    │   343.91ms │                  339.84ms │     no change │
│ QQuery 11    │   395.88ms │                  396.92ms │     no change │
│ QQuery 12    │  1047.38ms │                 1088.01ms │     no change │
│ QQuery 13    │  1601.10ms │                 1759.12ms │  1.10x slower │
│ QQuery 14    │  1241.72ms │                 1224.12ms │     no change │
│ QQuery 15    │  1085.60ms │                 1079.29ms │     no change │
│ QQuery 16    │  2534.30ms │                 2492.06ms │     no change │
│ QQuery 17    │  2321.16ms │                 2293.17ms │     no change │
│ QQuery 18    │  4995.62ms │                 5007.32ms │     no change │
│ QQuery 19    │    92.50ms │                   92.95ms │     no change │
│ QQuery 20    │  1732.40ms │                 1729.49ms │     no change │
│ QQuery 21    │  2005.22ms │                 2047.15ms │     no change │
│ QQuery 22    │  5186.07ms │                 5181.12ms │     no change │
│ QQuery 23    │ 10416.13ms │                10446.56ms │     no change │
│ QQuery 24    │   581.18ms │                  587.37ms │     no change │
│ QQuery 25    │   484.96ms │                  492.21ms │     no change │
│ QQuery 26    │   658.09ms │                  664.14ms │     no change │
│ QQuery 27    │  2642.66ms │                 2563.77ms │     no change │
│ QQuery 28    │ 15814.37ms │                14561.36ms │ +1.09x faster │
│ QQuery 29    │   522.09ms │                  541.79ms │     no change │
│ QQuery 30    │  1042.06ms │                 1043.47ms │     no change │
│ QQuery 31    │  1109.43ms │                 1099.06ms │     no change │
│ QQuery 32    │  4340.78ms │                 4307.43ms │     no change │
│ QQuery 33    │  5198.05ms │                 5128.33ms │     no change │
│ QQuery 34    │  5172.24ms │                 5190.19ms │     no change │
│ QQuery 35    │  1873.98ms │                 1948.10ms │     no change │
│ QQuery 36    │   272.06ms │                  273.78ms │     no change │
│ QQuery 37    │   125.57ms │                  121.80ms │     no change │
│ QQuery 38    │   141.25ms │                  145.32ms │     no change │
│ QQuery 39    │   744.75ms │                  736.95ms │     no change │
│ QQuery 40    │    57.16ms │                   58.71ms │     no change │
│ QQuery 41    │    50.42ms │                   45.98ms │ +1.10x faster │
│ QQuery 42    │    63.61ms │                   63.56ms │     no change │
└──────────────┴────────────┴───────────────────────────┴───────────────┘

@alamb alamb closed this Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
functions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant