fix: optimize some bit functions #718

kazuyukitanimura · 2024-07-24T19:47:14Z

Which issue does this PR close?

Part of #679 and #670

Rationale for this change

The improvement could be negligible in real use cases, but I see some improvements in micro benchmarks

What changes are included in this PR?

Optimizations in some bit functions

How are these changes tested?

Existing tests

andygrove · 2024-07-24T20:54:56Z

It would be nice to have a criterion benchmark as part of the PR

kazuyukitanimura · 2024-07-27T06:36:56Z

@andygrove

bit_util/get_value/i32_num_bits_8
                        time:   [781.53 ps 797.98 ps 818.12 ps]
                        change: [-14.395% -11.959% -9.4057%] (p = 0.00 < 0.05)
                        Performance has improved.
bit_util/get_value/i32_num_bits_32
                        time:   [599.30 ps 605.37 ps 613.10 ps]
                        change: [-21.472% -14.798% -7.3131%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
  13 (13.00%) high mild

bit_util/trailing_bits/num_bits_0
                        time:   [476.87 ps 479.39 ps 482.48 ps]
                        change: [+0.5038% +0.9142% +1.4061%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 12 outliers among 100 measurements (12.00%)
  1 (1.00%) high mild
  11 (11.00%) high severe
bit_util/trailing_bits/num_bits_32
                        time:   [537.73 ps 554.77 ps 576.77 ps]
                        change: [-20.179% -14.236% -7.9038%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 12 outliers among 100 measurements (12.00%)
  1 (1.00%) high mild
  11 (11.00%) high severe
bit_util/trailing_bits/num_bits_64
                        time:   [531.20 ps 537.39 ps 545.31 ps]
                        change: [-12.720% -9.5177% -6.6237%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
  2 (2.00%) high mild
  11 (11.00%) high severe
bit_util/read_u64       time:   [662.74 ps 667.79 ps 675.09 ps]
                        change: [-44.346% -43.866% -43.140%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) high mild
  2 (2.00%) high severe
bit_util/read_u32       time:   [640.71 ps 641.86 ps 643.37 ps]
                        change: [-84.385% -84.321% -84.263%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 15 outliers among 100 measurements (15.00%)
  4 (4.00%) high mild
  11 (11.00%) high severe
bit_util/get_u32_value  time:   [2.0853 ms 2.0860 ms 2.0868 ms]
                        change: [-28.295% -28.107% -27.945%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

The baseline of read_u64(...)/read_u32(...) are read_num_bytes_u64(8, ...)/read_num_bytes_u32(4, ...)

andygrove

Those are some nice speedups. Thanks @kazuyukitanimura. I'm not too familiar with this code so it would be good to get another review before merging (perhaps @viirya or @parthchandra?)

codecov-commenter · 2024-07-29T20:22:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.75%. Comparing base (ded3dd6) to head (30adf06).
Report is 10 commits behind head on main.

Additional details and impacted files

@@              Coverage Diff              @@
##               main     #718       +/-   ##
=============================================
+ Coverage     33.57%   53.75%   +20.17%     
+ Complexity      830      813       -17     
=============================================
  Files           110      107        -3     
  Lines         42608    10273    -32335     
  Branches       9352     1934     -7418     
=============================================
- Hits          14306     5522     -8784     
+ Misses        25347     3774    -21573     
+ Partials       2955      977     -1978

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

parthchandra · 2024-07-29T22:01:52Z

native/core/src/common/bit.rs

@@ -553,8 +564,11 @@ pub struct BitReader {
 /// either byte aligned or not.
 impl BitReader {
    pub fn new(buf: Buffer, len: usize) -> Self {
-        let num_bytes = cmp::min(8, len);
-        let buffered_values = read_num_bytes_u64(num_bytes, buf.as_slice());
+        let buffered_values = if 8 > len {


nit: should we use size_of instead of 8?

parthchandra · 2024-07-29T22:13:57Z

native/core/src/common/bit.rs

+        let buffered_values = if 8 > len {
+            read_num_bytes_u64(len, buf.as_slice())
+        } else {
+            read_u64(buf.as_slice())


If len > 8 then this will not read len bytes will it? Also read_num_bytes_u64 appears to cover this case in an unlikely block (and handles it rather differently).

If len > 8 then, this will read 8 bytes only (u64).
read_num_bytes_u64 has

debug_assert!(size <= src.len()); if unlikely(src.len() < 8) {

So when size=8 we do not need the if unlikely() block

Maybe we can explicitly make the condition if src.len == 0
So

if src.len == 0 { read_u64(buf.as_slice()) } else { read_num_bytes_u64(len, buf.as_slice()) }

When if src.len == 0 is true, we cannot read anything if I understand correctly?

kazuyukitanimura · 2024-07-31T22:08:33Z

@parthchandra do you have any more feedbacks?
cc @comphead @huaxingao @viirya

comphead · 2024-08-01T19:32:29Z

native/core/benches/bit_util.rs

+    group.bench_function("get_u32_value", |b| {
+        b.iter(|| {
+            let mut reader: BitReader = BitReader::new_all(buffer.slice(0));
+            for _ in 0..(buffer.len() * 8 / 31) {


I would add some comments on magic numbers

This is no a magic number, I just chose this randomly, no specific reasons

comphead · 2024-08-01T19:42:13Z

native/core/src/common/bit.rs

+#[inline]
+pub fn read_u64(src: &[u8]) -> u64 {
+    let in_ptr = src.as_ptr() as *const u64;
+    unsafe { in_ptr.read_unaligned() }


I'm curios why to read the raw pointer unaligned? its not guaranteed to be aligned?

hmm not sure if it is always aligned...

I don't think we can ever guarantee that the memory is aligned

comphead · 2024-08-01T19:45:15Z

native/core/src/common/bit.rs

@@ -1019,6 +1033,12 @@ mod tests {
        }
    }

+    #[test]
+    fn test_read_u64() {
+        let buffer: Vec<u8> = vec![0, 1, 2, 3, 4, 5, 6, 7];


why is it u8 if we test u64, should we create a vector of i64?

Because this is a buffer. It reads 8 bytes from here

kazuyukitanimura · 2024-08-02T18:19:24Z

Plan to merge this today

kazuyukitanimura · 2024-08-02T21:36:46Z

Merged, thank you @andygrove @parthchandra @comphead

## Which issue does this PR close? Part of apache#679 and apache#670 ## Rationale for this change The improvement could be negligible in real use cases, but I see some improvements in micro benchmarks ## What changes are included in this PR? Optimizations in some bit functions ## How are these changes tested? Existing tests (cherry picked from commit ffb96c3)

fix: optimize some bit functions

a446871

kazuyukitanimura marked this pull request as ready for review July 24, 2024 19:47

fix: optimize some bit functions

e658e74

kazuyukitanimura marked this pull request as draft July 25, 2024 00:32

address review comments

b952a93

kazuyukitanimura marked this pull request as ready for review July 27, 2024 06:37

kazuyukitanimura added 3 commits July 26, 2024 23:40

address review comments

890b5ac

address review comments

7a170e6

address review comments

0e3f110

andygrove approved these changes Jul 27, 2024

View reviewed changes

kazuyukitanimura added 3 commits July 29, 2024 11:27

address review comments

b6a1e16

address review comments

5bc7c73

address review comments

b3332f2

parthchandra reviewed Jul 29, 2024

View reviewed changes

kazuyukitanimura added 11 commits July 29, 2024 23:52

fix: Optimize decimal Add Sub Mul Mod

8b7cfaf

fix: Remove castting to decimal256

4b12e0e

Merge remote-tracking branch 'upstream/main' into optimize-get-value

2370a7c

fix: Remove castting to decimal256

36126b6

fix: Remove castting to decimal256

fbebd08

fix: Remove castting to decimal256

961720c

fix: Remove castting to decimal256

293471c

fix: Remove castting to decimal256

02e3b57

Merge remote-tracking branch 'upstream/main' into optimize-get-value

432f645

address review comments

66011e2

address review comments

30adf06

Merge remote-tracking branch 'upstream/main' into optimize-get-value

fc2e7df

comphead reviewed Aug 1, 2024

View reviewed changes

kazuyukitanimura merged commit ffb96c3 into main Aug 2, 2024
183 checks passed

kazuyukitanimura deleted the optimize-get-value branch August 9, 2024 20:36

andygrove added the performance label Aug 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: optimize some bit functions #718

fix: optimize some bit functions #718

kazuyukitanimura commented Jul 24, 2024 •

edited

Loading

andygrove commented Jul 24, 2024

kazuyukitanimura commented Jul 27, 2024 •

edited

Loading

andygrove left a comment

codecov-commenter commented Jul 29, 2024 •

edited

Loading

parthchandra Jul 29, 2024

kazuyukitanimura Jul 31, 2024

parthchandra Jul 29, 2024

kazuyukitanimura Jul 29, 2024

parthchandra Jul 30, 2024

kazuyukitanimura Jul 30, 2024

kazuyukitanimura commented Jul 31, 2024

comphead Aug 1, 2024

kazuyukitanimura Aug 1, 2024

comphead Aug 1, 2024

kazuyukitanimura Aug 1, 2024

andygrove Aug 2, 2024

comphead Aug 1, 2024

kazuyukitanimura Aug 1, 2024

kazuyukitanimura commented Aug 2, 2024

kazuyukitanimura commented Aug 2, 2024

fix: optimize some bit functions #718

fix: optimize some bit functions #718

Conversation

kazuyukitanimura commented Jul 24, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

andygrove commented Jul 24, 2024

kazuyukitanimura commented Jul 27, 2024 • edited Loading

andygrove left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazuyukitanimura commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazuyukitanimura commented Aug 2, 2024

kazuyukitanimura commented Aug 2, 2024

kazuyukitanimura commented Jul 24, 2024 •

edited

Loading

kazuyukitanimura commented Jul 27, 2024 •

edited

Loading

codecov-commenter commented Jul 29, 2024 •

edited

Loading