ssbr commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1803774483
##########
arrow-buffer/src/util/bit_mask.rs:
##########
@@ -127,16 +127,16 @@ unsafe fn set_upto_64bits(
}
/// # Safety
-/// The caller must ensure all arguments are within the valid range.
+/// The caller must ensure `data` has `offset..(offset + 8)` range, and `count
<= 8`.
#[inline]
unsafe fn read_bytes_to_u64(data: &[u8], offset: usize, count: usize) -> u64 {
debug_assert!(count <= 8);
- let mut tmp = std::mem::MaybeUninit::<u64>::new(0);
+ let mut tmp: u64 = 0;
Review Comment:
I agree with @Manishearth.
A `MaybeUninit<T>` is just a union of `T` and `()`. In this case, where we
have a a fully initialized `MaybeUninit`, and we only use that `T` field, it
cannot ever be faster, only slower from failed inlining. We can doubt our
reasoning facilities here, but that is paralysis, as we can doubt anything. In
fact, in this case, we are probably more likely to misunderstand the benchmark
results than we are to misunderstand the code change.
---
Since you asked:
First benchmark doesn't exist:
>```
> error: no bench target named `boolean_append_packed`.
> Available bench targets:
> bit_mask
> i256
> offset
>```
Second:
**Before**:
```
time: [4.8703 ns 4.8753 ns 4.8806 ns]
time: [4.8072 ns 4.8222 ns 4.8419 ns]
time: [12.428 ns 12.615 ns 12.842 ns]
time: [13.132 ns 13.187 ns 13.251 ns]
time: [6.5690 ns 6.5904 ns 6.6125 ns]
time: [6.6956 ns 6.7446 ns 6.8032 ns]
time: [4.8632 ns 4.8725 ns 4.8836 ns]
time: [4.8567 ns 4.8624 ns 4.8689 ns]
time: [13.124 ns 13.203 ns 13.298 ns]
time: [13.107 ns 13.141 ns 13.176 ns]
time: [14.895 ns 14.983 ns 15.082 ns]
time: [15.131 ns 15.205 ns 15.287 ns]
time: [5.1409 ns 5.2336 ns 5.3743 ns]
time: [4.9269 ns 4.9634 ns 4.9994 ns]
time: [12.820 ns 13.115 ns 13.385 ns]
time: [13.602 ns 13.632 ns 13.664 ns]
time: [14.285 ns 14.451 ns 14.606 ns]
time: [14.842 ns 14.925 ns 15.036 ns]
time: [5.0809 ns 5.2111 ns 5.3891 ns]
time: [4.8193 ns 4.8283 ns 4.8374 ns]
time: [13.008 ns 13.050 ns 13.100 ns]
time: [13.298 ns 13.421 ns 13.575 ns]
time: [14.189 ns 14.225 ns 14.260 ns]
time: [14.322 ns 14.448 ns 14.612 ns]
```
**After:**
```
time: [5.1458 ns 5.1959 ns 5.2496 ns]
time: [4.9022 ns 4.9157 ns 4.9310 ns]
time: [13.006 ns 13.179 ns 13.339 ns]
time: [13.808 ns 13.887 ns 13.983 ns]
time: [6.5402 ns 6.5575 ns 6.5770 ns]
time: [6.5290 ns 6.5851 ns 6.6533 ns]
time: [4.8950 ns 4.9234 ns 4.9575 ns]
time: [4.9041 ns 4.9344 ns 4.9745 ns]
time: [13.676 ns 13.818 ns 13.984 ns]
time: [13.711 ns 13.782 ns 13.856 ns]
time: [14.848 ns 14.884 ns 14.920 ns]
time: [14.867 ns 14.899 ns 14.928 ns]
time: [4.9025 ns 4.9368 ns 4.9734 ns]
time: [5.0640 ns 5.1541 ns 5.2420 ns]
time: [13.233 ns 13.342 ns 13.443 ns]
time: [13.843 ns 13.911 ns 13.976 ns]
time: [13.934 ns 14.158 ns 14.367 ns]
time: [14.874 ns 14.915 ns 14.960 ns]
time: [4.9245 ns 4.9641 ns 5.0034 ns]
time: [4.8752 ns 4.9060 ns 4.9499 ns]
time: [13.529 ns 13.542 ns 13.555 ns]
time: [13.555 ns 13.594 ns 13.642 ns]
time: [14.279 ns 14.327 ns 14.380 ns]
time: [14.265 ns 14.290 ns 14.316 ns]
```
The differences are noise, but it would be very difficult to prove that fact
by running benchmarks, unless I spent quite a bit of time gathering raw data
and drawing error bars. Instead, I will note that the generated machine code is
[identical](https://godbolt.org/z/1TWG34vfd), so it must be noise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
