perf: Faster decimal precision overflow checks #6419

andygrove · 2024-09-18T20:33:39Z

Which issue does this PR close?

N/A

Rationale for this change

Small performance optimization.

null_if_overflow_precision_128
                        time:   [14.371 µs 14.403 µs 14.436 µs]
                        change: [-80.547% -80.475% -80.399%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) low mild
  3 (3.00%) high mild

null_if_overflow_precision_256
                        time:   [26.421 µs 26.531 µs 26.642 µs]
                        change: [-87.653% -87.587% -87.522%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

validate_decimal_precision_128
                        time:   [74.056 ns 74.401 ns 74.752 ns]
                        change: [-11.615% -10.563% -9.4441%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) low mild

validate_decimal_precision_256
                        time:   [215.61 ns 216.62 ns 217.68 ns]
                        change: [-0.8629% -0.3349% +0.1701%] (p = 0.22 > 0.05)
                        No change in performance detected.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high severe

What changes are included in this PR?

I noticed two areas of overhead in the current approach to verifying decimal precision.

In the event of an overflow, we are building a formatted string for an error message, which is then discarded in some cases, so we should avoid that cost
The range check code introduces a memcpy which can be avoided

I tested the following variations of the decimal precision check in Rust playground.

fn validate_decimal_precision1(value: i128, precision: u8) -> bool {
    if precision > DECIMAL128_MAX_PRECISION {
        return false;
    }
    let idx = usize::from(precision) - 1;
    value >= MIN_DECIMAL_FOR_EACH_PRECISION[idx] && value <= MAX_DECIMAL_FOR_EACH_PRECISION[idx]
}

// based on arrow-rs version
fn validate_decimal_precision2(value: i128, precision: u8) -> bool {
    if precision > DECIMAL128_MAX_PRECISION {
        return false;
    }

    let max = MAX_DECIMAL_FOR_EACH_PRECISION[usize::from(precision) - 1];
    let min = MIN_DECIMAL_FOR_EACH_PRECISION[usize::from(precision) - 1];

    if value > max {
        false
    } else if value < min {
        false
    } else {
        true
    }
}

validate_decimal_precision1 avoids a memcpy that appears in validate_decimal_precision2:

playground::validate_decimal_precision1:
	subq	$1304, %rsp
	movb	%dl, %al
	movb	%al, 23(%rsp)
	movq	%rsi, 24(%rsp)
	movq	%rdi, 32(%rsp)
	movq	%rdi, 1264(%rsp)
	movq	%rsi, 1272(%rsp)
	movb	%al, 1287(%rsp)
	cmpb	$38, %al
	ja	.LBB9_2
	movb	23(%rsp), %al
	movb	%al, 1303(%rsp)
	movzbl	%al, %eax
	movq	%rax, %rcx
	subq	$1, %rcx
	movq	%rcx, 8(%rsp)
	cmpq	$1, %rax
	jb	.LBB9_4
	jmp	.LBB9_3

playground::validate_decimal_precision2:
	subq	$1368, %rsp
	movb	%dl, %al
	movb	%al, 55(%rsp)
	movq	%rsi, 56(%rsp)
	movq	%rdi, 64(%rsp)
	movq	%rdi, 1296(%rsp)
	movq	%rsi, 1304(%rsp)
	movb	%al, 1327(%rsp)
	cmpb	$38, %al
	ja	.LBB10_2
	leaq	80(%rsp), %rdi
	leaq	.L__unnamed_5(%rip), %rsi
	movl	$608, %edx
	callq	memcpy@PLT   <----- MEMCPY HERE
	movb	55(%rsp), %al
	movb	%al, 1367(%rsp)
	movzbl	%al, %eax
	movq	%rax, %rcx
	subq	$1, %rcx
	movq	%rcx, 40(%rsp)
	cmpq	$1, %rax
	jb	.LBB10_4
	jmp	.LBB10_3

Are there any user-facing changes?

Technically, this is an API change because I made two consts pub(crate) instead of pub. However, if anyone wants access to the original consts they could just copy them into their code base.

andygrove · 2024-09-18T20:35:57Z

arrow-data/src/decimal.rs

-    let max = MAX_DECIMAL_FOR_EACH_PRECISION[usize::from(precision) - 1];
-    let min = MIN_DECIMAL_FOR_EACH_PRECISION[usize::from(precision) - 1];


I believe that this is where the memcpy was being introduced

Hmm, this is interesting. By assigning a new variable, it did memory copying of the i128 value.

viirya · 2024-09-19T14:51:16Z

arrow-array/src/types.rs

+    fn is_valid_decimal_precision(value: Self::Native, precision: u8) -> bool {
+        is_validate_decimal_precision(value, precision)
+    }


Is this used to avoid creating ArrowError and the error string for the cases that we don't need the error?

arrow-data/src/decimal.rs

comphead

thanks @andygrove
probably a nit: I can see usize::from(precision) - 1 a lot which require new allocation of usize.
My feeling the precision is static? may we can precompute idx and pass it by reference?

Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>

andygrove · 2024-09-19T17:20:53Z

thanks @andygrove probably a nit: I can see usize::from(precision) - 1 a lot which require new allocation of usize. My feeling the precision is static? may we can precompute idx and pass it by reference?

What if we just insert an extra element in the start of each of the lookup arrays and then just use precision as usize directly as the index?

…arrow-rs into faster-decimal-overflow-check

andygrove · 2024-09-19T17:29:04Z

Updated benchmark results after removing the idx allocation:

null_if_overflow_precision_128
                        time:   [12.555 µs 12.591 µs 12.628 µs]
                        change: [-85.358% -85.314% -85.275%] (p = 0.00 < 0.05)
                        Performance has improved.

null_if_overflow_precision_256
                        time:   [25.614 µs 25.755 µs 25.903 µs]
                        change: [-87.983% -87.827% -87.620%] (p = 0.00 < 0.05)
                        Performance has improved.

validate_decimal_precision_128
                        time:   [74.290 ns 74.703 ns 75.109 ns]
                        change: [-7.7409% -7.2664% -6.8248%] (p = 0.00 < 0.05)
                        Performance has improved.

validate_decimal_precision_256
                        time:   [215.27 ns 216.01 ns 216.75 ns]
                        change: [-4.4231% -3.7018% -2.9993%] (p = 0.00 < 0.05)
                        Performance has improved.

comphead

lgtm thanks @andygrove

arrow-data/src/decimal.rs

andygrove · 2024-09-20T14:10:47Z

I reverted the API change

andygrove · 2024-09-20T14:11:18Z

@viirya @comphead There have been changes since this PR was approved. Please take another look when you can.

comphead · 2024-09-20T18:59:07Z

I'm okay with the PR, although the array that changed moved to crate visibility and doesn't seem to affect others, however I'm not sure if anyone else other than arrow-rs actually used it

andygrove · 2024-09-20T20:30:39Z

I'm okay with the PR, although the array that changed moved to crate visibility and doesn't seem to affect others, however I'm not sure if anyone else other than arrow-rs actually used it

I reverted the change to the original array and added a copy with the extra element so as not to break the current API. The new array is pub(crate)

Dandandan

Nice

andygrove added 2 commits September 18, 2024 13:58

add benchmark

1c0a8ee

add optimization

0f6266c

github-actions bot added the arrow Changes to the arrow crate label Sep 18, 2024

andygrove requested a review from Dandandan September 18, 2024 20:34

andygrove commented Sep 18, 2024

View reviewed changes

andygrove marked this pull request as draft September 18, 2024 20:47

andygrove added 4 commits September 18, 2024 14:50

fix

c68432e

fix

5fcd17a

cargo fmt

e4b22ce

clippy

5de6e14

andygrove marked this pull request as ready for review September 19, 2024 14:19

andygrove requested review from comphead and viirya September 19, 2024 14:20

viirya reviewed Sep 19, 2024

View reviewed changes

arrow-data/src/decimal.rs Outdated Show resolved Hide resolved

comphead reviewed Sep 19, 2024

View reviewed changes

viirya approved these changes Sep 19, 2024

View reviewed changes

Update arrow-data/src/decimal.rs

aebad61

Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>

andygrove added 2 commits September 19, 2024 11:23

optimize to avoid allocating an idx variable

34cce0c

Merge branch 'faster-decimal-overflow-check' of github.com:andygrove/…

1a5262d

…arrow-rs into faster-decimal-overflow-check

comphead approved these changes Sep 19, 2024

View reviewed changes

andygrove added the api-change Changes to the arrow API label Sep 19, 2024

andygrove commented Sep 19, 2024

View reviewed changes

arrow-data/src/decimal.rs Outdated Show resolved Hide resolved

tustvold added the next-major-release the PR has API changes and it waiting on the next major version label Sep 19, 2024

andygrove mentioned this pull request Sep 20, 2024

Improve performance of Spark-compatible decimal aggregates apache/datafusion-comet#951

Open

andygrove added 2 commits September 20, 2024 08:06

revert change to public api

c995213

fix error in rustdoc

e8d24ab

andygrove removed the api-change Changes to the arrow API label Sep 20, 2024

andygrove removed the next-major-release the PR has API changes and it waiting on the next major version label Sep 20, 2024

Dandandan approved these changes Sep 21, 2024

View reviewed changes

andygrove merged commit c90713b into apache:master Sep 21, 2024
27 checks passed

andygrove deleted the faster-decimal-overflow-check branch September 21, 2024 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Faster decimal precision overflow checks #6419

perf: Faster decimal precision overflow checks #6419

andygrove commented Sep 18, 2024 •

edited

Loading

andygrove Sep 18, 2024

viirya Sep 19, 2024

viirya Sep 19, 2024 •

edited

Loading

andygrove Sep 19, 2024

comphead left a comment

andygrove commented Sep 19, 2024

andygrove commented Sep 19, 2024 •

edited

Loading

comphead left a comment

andygrove commented Sep 20, 2024

andygrove commented Sep 20, 2024

comphead commented Sep 20, 2024

andygrove commented Sep 20, 2024

Dandandan left a comment

		let max = MAX_DECIMAL_FOR_EACH_PRECISION[usize::from(precision) - 1];
		let min = MIN_DECIMAL_FOR_EACH_PRECISION[usize::from(precision) - 1];

perf: Faster decimal precision overflow checks #6419

perf: Faster decimal precision overflow checks #6419

Conversation

andygrove commented Sep 18, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

andygrove Sep 18, 2024

Choose a reason for hiding this comment

viirya Sep 19, 2024

Choose a reason for hiding this comment

viirya Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

andygrove Sep 19, 2024

Choose a reason for hiding this comment

comphead left a comment

Choose a reason for hiding this comment

andygrove commented Sep 19, 2024

andygrove commented Sep 19, 2024 • edited Loading

comphead left a comment

Choose a reason for hiding this comment

andygrove commented Sep 20, 2024

andygrove commented Sep 20, 2024

comphead commented Sep 20, 2024

andygrove commented Sep 20, 2024

Dandandan left a comment

Choose a reason for hiding this comment

andygrove commented Sep 18, 2024 •

edited

Loading

viirya Sep 19, 2024 •

edited

Loading

andygrove commented Sep 19, 2024 •

edited

Loading