Repository navigation
perf(arrow-buffer): add precomputed divisors for 32-bit and 64-bit integers - #11467
mbutrovich wants to merge 18 commits into
Conversation
|
run benchmark divisor |
|
🤖 Benchmark starting (GKE) | trigger Target: divisor Sharding factor: 1 (1 workers). Comparing 7d3e750 (7d3e750) to 7d3e750 diff Run configurationrun benchmark divisor
baseline:
ref: 7d3e750bacf5ab46e465b2241597a86de054050f
changed:
ref: 7d3e750bacf5ab46e465b2241597a86de054050f
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
|
run benchmark divisor |
|
🤖 Benchmark starting (GKE) | trigger Target: divisor Sharding factor: 1 (1 workers). Comparing 7d3e750 (7d3e750) to 7d3e750 diff Run configurationrun benchmark divisor
baseline:
ref: 7d3e750bacf5ab46e465b2241597a86de054050f
changed:
ref: 7d3e750bacf5ab46e465b2241597a86de054050f
resources:
arch: amd64
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Target: divisor Comparing 7d3e750 (7d3e750) to 7d3e750 diff Run configurationrun benchmark divisor
baseline:
ref: 7d3e750bacf5ab46e465b2241597a86de054050f
changed:
ref: 7d3e750bacf5ab46e465b2241597a86de054050f
resources:
arch: amd64
shards: 1
DetailsPer-runner informationdivisor — shard 1/1Node: gk3-benchmark-cluster-nap-im36g35r-ba30b79c-kdgh Instance: c4-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench divisorCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Target: divisor Comparing 7d3e750 (7d3e750) to 7d3e750 diff Run configurationrun benchmark divisor
baseline:
ref: 7d3e750bacf5ab46e465b2241597a86de054050f
changed:
ref: 7d3e750bacf5ab46e465b2241597a86de054050f
shards: 1
DetailsPer-runner informationdivisor — shard 1/1Node: gk3-benchmark-cluster-nap-1sqhi6ce-a70ada09-95cm Instance: c4a-highcpu-48 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench divisorCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
This comment was marked as outdated.
This comment was marked as outdated.
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
I relied on AI to do this review because I am not an expert in this area. Feel free to push back (as Claude would say) if you disagree with any of this!
Thanks @mbutrovich, this is a careful PR. I checked the math beyond the unit tests, and it holds. I compared div_rem and checked_div_rem against wrapping_* and checked_* for:
- every
u32dividend with 77 divisors, including every2^k ± 1,10^k,MAXand2^31 + 1 - every
i32dividend with 21 divisors, includingMIN,MAXand±1 - every
u32andi32divisor, with boundary dividends such as the largest multiple ofdand the value just below it - about 100M random
u64andi64divisors
There were no mismatches.
My main concern is the premise that LLVM moves the power_of_two branch out of the loop. Since this PR settles the API, it's the right time to raise it.
1. The power_of_two branch is often not hoisted, so performance depends on build settings
LLVM hoists the test only when the loop ends up inlined somewhere it can prove the output stores don't alias the divisor (divisor.rs:80). When it can't, the loop reloads power_of_two, the multiplier and the shift from memory for every element. It branches per element and isn't vectorized. I confirmed this in the disassembly (ldrb [x11, #0x1c]; tbnz inside an out-of-line Vec::from_iter instance).
The benchmark bot forces CARGO_PROFILE_BENCH_LTO=thin (build_env.rs). Building with that setting on an M3 Ultra with rustc 1.99.0, 6 of the 8 divisor loops compile to this scalar, branch-per-element form. Here is the PR's own benchmark, in µs per 8192 values:
| Case | / or % |
div_rem, default cargo bench |
div_rem, thin LTO (bot setting) |
div_rem, fat LTO with 1 codegen unit |
|---|---|---|---|---|
u32 % 13 |
4.19 | 0.96 | 4.56 | 0.93 |
u32 % 16 |
4.18 | 0.43 | 2.45 | 0.58 |
i32 / 13 |
4.90 | 1.48 | 6.08 | 1.43 |
i32 / 16 |
4.40 | 0.83 | 4.41 | 0.80 |
i64 / 13 |
4.41 | 3.12 | 5.72 | 3.09 |
i64 % 16 |
4.33 | 4.02 | 4.09 | 1.44 |
Even the default cargo bench has the problem for i64 %. I got the same result in an aarch64 Linux container. This is probably also what the bot results posted here show, such as u32 % 16 taking 5.5 µs against 0.41 µs for u32 / 16 on Neoverse-V2. A mask isn't 13x slower than a shift unless the loop isn't vectorized. If so, those numbers mostly reflect this codegen behavior rather than the algorithm, so I'd hold off on choosing kernels per type from them.
The same thing happens through PrimitiveArray::unary, which is how the kernels will call this. In a default release build, Int64Array % 16 with unary(|x| d.div_rem(x).1) takes 4.05 µs, against 4.16 µs for wrapping_rem. Testing power_of_two once outside the loop and passing a branch-free closure to unary takes 1.40 µs. With thin LTO, dispatching outside the loop gives:
u32 % 13: 5.00 → 0.96 µsu64 % 16: 3.06 → 0.91 µsi64 % 16: 4.02 → 1.52 µs
It does not fix the signed multiply path: i32 / 13 goes from 6.41 to 5.43 µs, still slower than wrapping_div at 4.28 µs. unary's map(|v| op(*v)) reaches the closure state through a pointer, and once from_iter isn't inlined, LLVM can't hoist those loads. A plain loop writing into a preallocated output, with the divisor in a local, was fast in every case in both the default and thin-LTO builds (for example 1.43 µs for i32 / 13 and 1.27 µs for i64 % 16).
Suggestions:
- Make the shift-or-multiply choice something callers can dispatch on once outside the loop, instead of relying on loop unswitching. For example, add a method that returns an enum of two branch-free divisor types. The epic already plans to match on the divisor's variant once outside the loop for the width variants; the power-of-two choice needs the same treatment. Today that isn't possible, because
power_of_twois private anddiv_remalways branches. - Reword the comment at
divisor.rs:80and the corresponding sentence in the PR description. - Have the benchmark call the divisor through
unary, as the kernels will, and check that the inner loop is vectorized in the bot's thin-LTO build before using its numbers.
2. The signed types could use figure 5.2 of the same paper (optional, API unchanged)
Figure 5.2 divides with a signed multiplier and smulh, with no abs and one sign fix instead of two. I prototyped it and checked it against wrapping_div and wrapping_rem: exhaustively for i32 with 14 divisors, and on 80M random i64 pairs. Timings are for a loop over a preallocated output on the same M3, in µs per 8192 values:
| Case | / or % |
This PR | Figure 5.2 |
|---|---|---|---|
i64 / 13 |
4.21 | 2.92 | 2.22 |
i64 % 13 |
4.04 | 5.57 | 3.38 |
i32 / 13 |
1.40 | 1.02 | |
i32 % 13 |
1.40 | 1.15 |
This turns i64 % 13, the one case where the divisor was slower than %, into a speedup. It would also remove the signed types' dependence on the unsigned ones. Powers of two would still want their own shift path.
Prototype
macro_rules! gm52 {
($name:ident, $t:ty, $u:ty, $wide:ty) => {
#[derive(Clone, Copy)]
pub struct $name { m: $t, d: $t, sh_post: u32, d_sign: $t }
impl $name {
pub fn new(d: $t) -> Self {
let ad = d.unsigned_abs();
let l = (<$u>::BITS - (ad - 1).leading_zeros()).max(1);
// m = 1 + floor(2^(N+l-1) / |d|), m' = m - 2^N (fits in the signed type)
let m = 1 + ((1 as $wide) << (<$t>::BITS + l - 1)) / ad as $wide;
let m_prime = m.wrapping_sub((1 as $wide) << <$t>::BITS) as $t;
Self { m: m_prime, d, sh_post: l - 1, d_sign: d >> (<$t>::BITS - 1) }
}
#[inline]
pub fn wrapping_div_rem(&self, n: $t) -> ($t, $t) {
let mulsh = ((self.m as $wide * n as $wide) >> <$t>::BITS) as $t;
let q0 = n.wrapping_add(mulsh);
let q0 = (q0 >> self.sh_post).wrapping_sub(n >> (<$t>::BITS - 1));
let q = (q0 ^ self.d_sign).wrapping_sub(self.d_sign);
(q, n.wrapping_sub(q.wrapping_mul(self.d)))
}
}
};
}
gm52!(Gm52I32, i32, u32, i64);
gm52!(Gm52I64, i64, u64, i128);3. Naming
i256::wrapping_div_rem and i256::checked_div_rem (#11425, same epic) use the wrapping_ prefix. Here the wrapping version on the signed types is div_rem. The sealed trait will cover both, so it would help to pick one convention now: either wrapping_div_rem here, or rename the i256 methods before a release. A checked_div_rem on the unsigned types that always returns Some would also give the trait the same set of methods on every type.
Smaller things
newcould be aconst fn; in the signednew, amatchwould replace?. That would matchi256's const constructors and allow compile-time tables of10^kdivisors, which the epic mentions.checked_div_remrecomputesminus_oneon every call (divisor.rs:164). It could be a field set innew, which matters when the loop isn't hoisted as described in point 1.- The benchmark groups are named just
u32,i32and so on. Prefixing them withdivisorwould make them easier to identify in bot output. #[doc = concat!("the quotient truncates toward zero …")]concatenates a single literal, so it could be a plain///line.
|
Thanks @andygrove. I made the API change you suggested and adopted figure 5.2, and I updated the PR description to match the new API and numbers. The numbers below are from an Apple M5 Max with rustc 1.99.0, in us per 8192 values, from the PR's Hoisting the power-of-two branch
You were right, and your thin LTO numbers reproduce here. I found two separate problems. The first is how the loop reaches the divisor. The benchmark's closure captured the divisor by reference, which fits the per-element loads you saw in the disassembly. Moving a copy of the divisor into the closure ( The second is that With thin LTO, your cases now look like this:
At
All cases with the new API
Figure 5.2 for the signed types
Thanks for prototyping this. The signed Naming
All four types now use
I'd like to keep The first is consistency with the rest of arrow-rs. The inherent The second is performance in the kernels. When the divisor can't fail, a kernel can use
With nulls, The same reasoning applies to the signed Smaller points
Only the signed
They're
That line went away when I rewrote the signed docs. |
|
run benchmark divisor |
|
🤖 Benchmark starting (GKE) | trigger Target: divisor Sharding factor: 1 (1 workers). Comparing 63ae153 (63ae153) to 63ae153 diff Run configurationrun benchmark divisor
baseline:
ref: 63ae1539bf03bbea3302c06094984320ba89160b
changed:
ref: 63ae1539bf03bbea3302c06094984320ba89160b
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
|
run benchmark divisor |
|
🤖 Benchmark starting (GKE) | trigger Target: divisor Sharding factor: 1 (1 workers). Comparing 63ae153 (63ae153) to 63ae153 diff Run configurationrun benchmark divisor
baseline:
ref: 63ae1539bf03bbea3302c06094984320ba89160b
changed:
ref: 63ae1539bf03bbea3302c06094984320ba89160b
resources:
arch: amd64
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Target: divisor Comparing 63ae153 (63ae153) to 63ae153 diff Run configurationrun benchmark divisor
baseline:
ref: 63ae1539bf03bbea3302c06094984320ba89160b
changed:
ref: 63ae1539bf03bbea3302c06094984320ba89160b
resources:
arch: amd64
shards: 1
DetailsPer-runner informationdivisor — shard 1/1Node: gk3-benchmark-cluster-nap-3lld71t1-69c7ab42-nfkx Instance: c4-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench divisorCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Target: divisor Comparing 63ae153 (63ae153) to 63ae153 diff Run configurationrun benchmark divisor
baseline:
ref: 63ae1539bf03bbea3302c06094984320ba89160b
changed:
ref: 63ae1539bf03bbea3302c06094984320ba89160b
shards: 1
DetailsPer-runner informationdivisor — shard 1/1Node: gk3-benchmark-cluster-nap-1konwh4m-230e413b-55zz Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench divisorCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
The benchmark bot reran the 
@andygrove, in your review you asked to
I built the On Neoverse-V2, Numbers behind the charts (us per 8192 values)
|
github-actions
Bot
added
the
arrow-core
…ray::unary` (apache#11525) # Which issue does this PR close? - Part of apache#11421. # Rationale for this change [`PrimitiveArray::unary`](https://github.com/apache/arrow-rs/blob/01b1275aab90c033da3f64f5edbfb4f061606cae/arrow-array/src/array/primitive_array.rs#L933) maps the values with `|v| op(*v)`, which captures `op` by reference. When `Vec::from_iter` isn't inlined into `unary`, the loop reads the captures of `op` through that reference and reloads them for every value. I found this in apache#11467 ([comment](apache#11467 (comment))), where it kept LLVM from moving a `match` on the divisor out of the loop. The decimal downscale cast on `main` has the same problem. With 16 codegen units, the default, `from_iter` for the infallible closure from [`make_downscaler`](https://github.com/apache/arrow-rs/blob/01b1275aab90c033da3f64f5edbfb4f061606cae/arrow-cast/src/cast/decimal.rs#L238-L263) isn't inlined. In the thin LTO disassembly for aarch64 and x86-64, the `Decimal64` loop reloads `div`, `half` and `half_neg` for each value, checks `div` for zero and -1, and branches on the sign of the value. With `op` moved into the closure, it loads the captures and checks `div` once, before the loop, and the rounding uses `csel` on aarch64 and `cmov` on x86-64 instead of a branch. # What changes are included in this PR? `unary` maps the values with `move |v| op(*v)`, so the iterator holds `op` instead of a reference to it. # Are these changes tested? The existing tests cover `unary`, and its results don't change. The benchmark bot ran the `decimal_downscale` cases of the `cast_decimal` benchmark from apache#11426 on [x86_64](apache#11525 (comment)) (Xeon Platinum 8581C) and [arm64](apache#11525 (comment)) (Neoverse-V2). <img width="1600" height="1124" alt="unary-move-bot" src="https://github.com/user-attachments/assets/c1d7d1f6-f14b-4ff6-ab1e-c7b0db5ebc9f" /> <details><summary>Numbers behind the chart</summary> In us per 8192 values with no nulls: | Case | x86_64: `main` | This PR | arm64: `main` | This PR | |---|---|---|---|---| | `Decimal32` (9, 4) to (9, 2) | 29.1 | 15.0 | 30.4 | 30.4 | | `Decimal64` (18, 6) to (18, 2) | 25.3 | 27.6 | 46.7 | 46.7 | | `Decimal128` (18, 6) to (18, 2) | 74.0 | 67.9 | 51.7 | 48.2 | | `Decimal128` (38, 10) to (38, 2), 15 digits | 74.1 | 67.8 | 49.4 | 42.9 | | `Decimal128` (38, 10) to (38, 2), 38 digits | 83.6 | 77.8 | 114.7 | 115.5 | | `Decimal128` (38, 20) to (38, 2), 38 digits | 83.7 | 68.6 | 170.2 | 170.7 | </details> The `Decimal256` and fallible cases changed by 6% or less on both runners. On x86_64, `Decimal64` was 9% slower without nulls and 2% slower with nulls. `unary` computes every slot either way, so these two cases should cost the same, and I don't know yet why they differ. The single-value case of `decimal_downscale_small` was 8% slower on x86_64 (138 vs 150 ns). The gain depends on the number of codegen units. These numbers are from an Apple M5 Max with rustc 1.99.0, in us per 8192 values with no nulls. <details><summary>Apple M5 Max by build setting</summary> | Case | 16 codegen units, no LTO: `main` | This PR | 1 codegen unit, fat LTO: `main` | This PR | |---|---|---|---|---| | `Decimal32` (9, 4) to (9, 2) | 7.25 | 4.56 | 4.50 | 4.49 | | `Decimal64` (18, 6) to (18, 2) | 7.21 | 4.48 | 4.30 | 4.22 | | `Decimal128` (18, 6) to (18, 2) | 21.36 | 19.52 | 20.27 | 20.10 | </details> Thin LTO with 16 codegen units gave a similar gain (6.06 to 4.49 us for `Decimal32`, 6.86 to 4.27 us for `Decimal64`), and thin LTO with one codegen unit measured the same as fat LTO. With one codegen unit, `main` is already as fast as this PR. # Are there any significant user-facing changes? No. ## AI Disclosure I wrote this PR with help from Claude but understand and support the core approach, implementation, and test coverage.
|
run benchmark divisor env:
BENCH_FILTER: "round_down_|magnitude_" |
|
🤖 Benchmark starting (GKE) | trigger Target: divisor Sharding factor: 1 (1 workers). Comparing divisor-types (7ef5ad9) to b391e5a (merge-base) diff Run configurationrun benchmark divisor
env:
BENCH_FILTER: round_down_|magnitude_
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
…benchmark bot Temporary. Adds experiment types and benchmark groups so the bot can compare figure 4.1 and 5.1 with Up, Down and devanbenz's remainder in one run. The next commit removes them.
… fix divisor docs
|
🤖 Benchmark completed (GKE) | trigger Target: divisor Comparing divisor-types (7ef5ad9) to b391e5a (merge-base) diff Run configurationrun benchmark divisor
env:
BENCH_FILTER: round_down_|magnitude_
shards: 1
Baseline build unavailable for shard(s) 1; changed-only measurements for those workers. DetailsPer-runner informationdivisor — shard 1/1Node: gk3-benchmark-cluster-nap-1jpat00e-95e6cfb4-fdj7 Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench divisorCPU Details (lscpu)Resource Usagebranch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Target: divisor Comparing divisor-types (7ef5ad9) to b391e5a (merge-base) diff Run configurationrun benchmark divisor
env:
BENCH_FILTER: round_down_|magnitude_
RUSTFLAGS: -C target-cpu=x86-64-v3
resources:
arch: amd64
shards: 1
Baseline build unavailable for shard(s) 1; changed-only measurements for those workers. DetailsPer-runner informationdivisor — shard 1/1Node: gk3-benchmark-cluster-nap-1j6n3dna-4c7ffb6a-dskm Instance: c4-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench divisorCPU Details (lscpu)Resource Usagebranch
File an issue against this benchmark runner |
|
Thanks @mightsleep and @devanbenz. I adopted the The benchmark bot compared the new forms with the PR's previous ones in the same run, on x86_64 (Xeon Platinum 8581C, 
On an Apple M5 Max the same comparison gave 1.47x and 1.38x for Two cases lose a little on the Xeon. A few things I tried and left out:
|
|
run benchmark divisor baseline:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
changed:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
resources:
arch: amd64 |
|
🤖 Benchmark starting (GKE) | trigger Target: divisor Sharding factor: 1 (1 workers). Comparing b78c2b4 (b78c2b4) to b78c2b4 diff Run configurationrun benchmark divisor
baseline:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
changed:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
resources:
arch: amd64
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
|
run benchmark divisor baseline:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
changed:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4 |
|
🤖 Benchmark starting (GKE) | trigger Target: divisor Sharding factor: 1 (1 workers). Comparing b78c2b4 (b78c2b4) to b78c2b4 diff Run configurationrun benchmark divisor
baseline:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
changed:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
|
run benchmark divisor baseline:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
changed:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
env:
RUSTFLAGS: "-C target-cpu=x86-64-v3"
resources:
arch: amd64 |
|
🤖 Benchmark starting (GKE) | trigger Target: divisor Sharding factor: 1 (1 workers). Comparing b78c2b4 (b78c2b4) to b78c2b4 diff Run configurationrun benchmark divisor
baseline:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
changed:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
env:
RUSTFLAGS: -C target-cpu=x86-64-v3
resources:
arch: amd64
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Target: divisor Comparing b78c2b4 (b78c2b4) to b78c2b4 diff Run configurationrun benchmark divisor
baseline:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
changed:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
shards: 1
DetailsPer-runner informationdivisor — shard 1/1Node: gk3-benchmark-cluster-nap-i674pijq-344a8a76-hlnd Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench divisorCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Target: divisor Comparing b78c2b4 (b78c2b4) to b78c2b4 diff Run configurationrun benchmark divisor
baseline:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
changed:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
resources:
arch: amd64
shards: 1
DetailsPer-runner informationdivisor — shard 1/1Node: gk3-benchmark-cluster-nap-1rw85303-e515ed6b-fnbk Instance: c4-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench divisorCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Target: divisor Comparing b78c2b4 (b78c2b4) to b78c2b4 diff Run configurationrun benchmark divisor
baseline:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
changed:
ref: b78c2b43ab3feb8aed7c7c0dec922f5d45bf7ae4
env:
RUSTFLAGS: -C target-cpu=x86-64-v3
resources:
arch: amd64
shards: 1
DetailsPer-runner informationdivisor — shard 1/1Node: gk3-benchmark-cluster-nap-1rw85303-5be73ff4-5bnj Instance: c4-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench divisorCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
mightsleep
left a comment
mightsleep
left a comment
There was a problem hiding this comment.
LGTM! Ran the divisor tests on x86-64 (AddIndicator path) and x86-64-v2 (Down path), and checked the new figure 4.1 multiplier against the direct floor(2^(N+l) / d) - 2^N + 1 for every u32 divisor, everything ok.
On the signed side I have a few more tricks up my sleeve, not sure what you'd want to do with them though, so just dropping them here, maybe they could go into follow-ups, if you're interested. They should be faster than the classic forms, mostly on x86:
|n| has one bit less than the word, so the rounded-up multiplier is exact for every divisor and the quotient is an unsigned mulhi, no signed one needed:
let s = n >> 31;
let a = (n ^ s).wrapping_sub(s) as u32;
let q = (mulhi(a, m) >> p) as i32;
let qs = s ^ d_sign;
(q ^ qs).wrapping_sub(qs)For i32, every value fits in f64 exactly, so the quotient can be a float multiply. With 1/|d| rounded up by one ulp, n * rcp never lands below the true quotient, and its error stays smaller than the gap to the next integer, so truncating gives exactly n / d for every pair. as saturates and checks, so it needs to_int_unchecked, which is a plain cvttpd2dq; the only out-of-range case is MIN / -1, and ±1 are in the power-of-two variant:
let up = f64::from_bits((1.0 / d.unsigned_abs() as f64).to_bits() + 1);
let rcp = if d < 0 { -up } else { up };
// SAFETY: |n * rcp| < 2^31 for |d| >= 2
let q = unsafe { (n as f64 * rcp).to_int_unchecked::<i32>() };div_euclid by a positive d through the one's complement, which has 31 bits, so it's one unsigned mulhi and three ops around it:
let s = n >> 31;
((mulhi((n ^ s) as u32, m) >> p) as i32) ^ s
|
To check that the API works in a kernel, I prototyped integer
On an Apple M5 Max with the Since they perform the same, I'd use the macro and keep this PR's API as it is, without the traits. The cost is that each kernel lists each type's variants, and because the enums are |
devanbenz
left a comment
devanbenz
left a comment
There was a problem hiding this comment.
Nice, the changes made from my request look good. Thanks!
…ivisors of 2^N - 1
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
LGTM. Thanks for addressing my earlier comments!
|
@mbutrovich I left my agent off the leash and it produced some (more or less) improvements on top of your perf results. I'll file a gist to show the diff, so you can try it out. Or file a draft PR and ask the bot to run the bench between it and this PR 🤷 The bench results will be here -- #11979 🤖 💬 : All nine bot comparisons have completed, including the additional SVE, SVE2, and Neoverse-V2 runs. The rewrite has substantial x86 wins, but the ARM results remain mixed.
|
|
Thanks for running all nine comparisons, @sdf-jkl.
I'd like to keep figure 5.1 as it is in this PR. The biggest wins are on x86-64 before the v3 target (i.e., without AVX2, which are pretty rare anymore): 1.7-2.1x for The generated code explains the split. On baseline x86-64 and at Taking only the wins would need a variant per target and per type. The kernel PR will measure each type through |
Which issue does this PR close?
Rationale for this change
This is the first divisor PR in #11421, so it settles the API that the
i128andi256divisors and the kernel changes build on.What changes are included in this PR?
DivisorU32,DivisorI32,DivisorU64andDivisorI64inarrow-buffer. No kernel uses them yet.#[non_exhaustive]enum, and each variant is its own type whose methods don't branch on the divisor. A kernel matches on the variant once, outside the loop, and can fall back towrapping_div_remfor variants it doesn't specialize. Matching on each value instead was 7-20% slower in Comet's bloom filter probe on four CPUs, as @mightsleep measured.newpicks the variant from the divisor and the target:PowerOfTwoshifts and masks.Up(u32,u64) uses the rounded-up multiplier when it is exact in N bits, so it needs no add and halve.Down(u32on aarch64 and on x86 with SSE4.1) uses the rounded-down multiplier with a saturating increment of the dividend, from Labor of Division (Episode III). Baseline x86-64 has no unsigned 32-bit min (pminudneeds SSE4.1), so the saturating increment takes several instructions, and there its loop has more instructions thanAddIndicator's.AddIndicatoruses figure 4.1 or 5.1 of Granlund and Montgomery for any other divisor.|divisor|, because the remainder doesn't depend on the sign of the divisor (section 3.1 of Lemire, Kaser and Kurz).Are these changes tested?
arrow-buffer/src/divisor.rscompare every type with the standard integer operators. They include both ends of the last quotient steps, every divisor of2^N - 1, and the variantnewpicks for each divisor.divisorbenchmark inarrow-buffer/benches/divisor.rsdivides 8192 random values by 7, 13 and 16 with/and%and with the divisor, which reaches every variant.x86-64-v3, and on arm64 (Neoverse-V2). The charts compare/and%withwrapping_div_remin the changed column of each run. The divisor is faster in all 72 cases.u64andi64, whose high multiplies run in scalar registers at both targets, and 2.0-2.6x fori32at the default target, because SSE2 has no signed 32-bit widening multiply (pmuldqneeds SSE4.1).x86-64-v3, no inner loop branches on the divisor or reloads its fields.unarypath with theinteger_divisionbenchmark from bench(arrow-arith): add integer and decimal division by scalar benchmarks #11427 and decide per type which kernels use the divisor.Are there any significant user-facing changes?
No existing API changes. The PR adds
DivisorU32,DivisorU64,DivisorI32andDivisorI64toarrow-buffer. To divide a batch of values by one divisor, match on the variant once and call itswrapping_div_remin the loop:The enums are
#[non_exhaustive], so amatchoutsidearrow-bufferneeds the last arm. The rustdoc on each enum describes the variants.AI Disclosure
I wrote this PR with help from Claude but understand and support the core approach, implementation, and test coverage.