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

Simplify OrderPreservingInterner allocation strategy ~97% faster (#2677) #2827

Merged
merged 1 commit into from Oct 13, 2022

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Oct 5, 2022

Which issue does this PR close?

Closes #2677

Rationale for this change

Not only was the previous allocation strategy quite hard to follow, when interning sorted data it would end up comparing the value against at least 1/4 of the already interned values. This was sub-optimal.

Taking a step back the original approach of leaving spaces was to mitigate the impact of unsorted data, as the design evolved OrderPreservingInterner now always sorts the dictionary prior to interning. This dramatically reduces the likelihood of out-of-order inserts, and consequently diminishes the value of this mitigation.

A simpler, in-order allocation strategy is both faster and easier to understand

Benchmarking convert_columns 4096 string_dictionary(10, 0): Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.6s, enable flat sampling, or reduce sample count to 60.
convert_columns 4096 string_dictionary(10, 0)
                        time:   [1.1147 ms 1.1156 ms 1.1169 ms]
                        change: [-96.615% -96.612% -96.607%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) high mild
  2 (2.00%) high severe

Benchmarking convert_columns 4096 string_dictionary(30, 0): Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.8s, enable flat sampling, or reduce sample count to 60.
convert_columns 4096 string_dictionary(30, 0)
                        time:   [1.1510 ms 1.1514 ms 1.1519 ms]
                        change: [-96.623% -96.621% -96.619%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

Benchmarking convert_columns 4096 string_dictionary(100, 0): Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 6.1s, enable flat sampling, or reduce sample count to 60.
convert_columns 4096 string_dictionary(100, 0)
                        time:   [690.29 µs 692.36 µs 695.23 µs]
                        change: [-97.493% -97.335% -97.211%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 20 outliers among 100 measurements (20.00%)
  1 (1.00%) low mild
  19 (19.00%) high severe

convert_columns 4096 string_dictionary(100, 0.5)
                        time:   [316.77 µs 316.89 µs 317.00 µs]
                        change: [-95.710% -95.708% -95.705%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) high mild
  2 (2.00%) high severe

What changes are included in this PR?

Changes the allocation strategy for OrderPreservingInterner to be simpler to understand and faster.

Are there any user-facing changes?

No

@github-actions github-actions bot added the arrow Changes to the arrow crate label Oct 5, 2022
@tustvold tustvold force-pushed the simplify-allocation-strategy branch from 4c4b993 to 210a35d Compare October 6, 2022 13:16
@tustvold tustvold marked this pull request as ready for review October 6, 2022 13:16
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

I read the code carefully and it makes sense to me. I had one question about test coverage but otherwise 👌

I look forward to seeing how this works

if &values_buf[slot.value] < data {
if slots_len == 254 {
out.push(255);
self.next
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there coverage of this branch? I think it happens when data is inserted in backwards sorted order, right? Maybe we can add at test to ensure it is covered?

Copy link
Contributor Author

@tustvold tustvold Oct 13, 2022

Choose a reason for hiding this comment

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

This branch is hit when the current bucket is "full" and it is trying to insert a value that is greater than the last value in the bucket. This therefore occurs when interning more than 255 values in order, and is therefore hit by test_interner() in particular

let mut values: Vec<_> = (0_u64..2000).collect();
test_intern_values(&values);

I have confirmed this with a debugger

@tustvold tustvold merged commit 65d5576 into apache:master Oct 13, 2022
@ursabot
Copy link

ursabot commented Oct 13, 2022

Benchmark runs are scheduled for baseline = f4ee8b9 and contender = 65d5576. 65d5576 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arrow Row Format
3 participants