You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "ssbr (via GitHub)" <gi...@apache.org> on 2024/10/11 23:04:21 UTC
[PR] `unsafe` improvements [arrow-rs]
ssbr opened a new pull request, #6551:
URL: https://github.com/apache/arrow-rs/pull/6551
# Which issue does this PR close?
N/A
# Rationale for this change
I discovered these while reviewing the unsafe code of arrow-rs -- this mostly just removes some instances of potentially-unsafe code, replacing them with safe equivalents. That makes it easier to review, at (so far as I know) no performance cost.
# What changes are included in this PR?
* Reuse `unsafe` code in the `OffsetBufferBuilder`.
* Use a `u64` instead of an always-initialized `MaybeUninit<u64>`.
* More precisely specify safety for `read_bytes_to_u64`.
# Are there any user-facing changes?
No.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "ssbr (via GitHub)" <gi...@apache.org>.
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 might be difficult to prove that fact by running benchmarks. 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "Manishearth (via GitHub)" <gi...@apache.org>.
Manishearth commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1802146588
##########
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:
@kazuyukitanimura `MaybeUninit(0)` and `0` should have no difference in performance. For this to be an optimization, this ought to have used `MaybeUninit::uninit()`, though that would also be unsound given that the entire integer is not guaranteed to be initialized..
If this is intended to be an optimization, it displays a misunderstanding of what MaybeUninit _does_.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#issuecomment-2408683986
@ssbr can you please check the build results?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "Manishearth (via GitHub)" <gi...@apache.org>.
Manishearth commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1799749413
##########
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
#[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;
let src = data.as_ptr().add(offset);
unsafe {
- std::ptr::copy_nonoverlapping(src, tmp.as_mut_ptr() as *mut u8, count);
- tmp.assume_init()
+ std::ptr::copy_nonoverlapping(src, &mut tmp as *mut _ as *mut u8, count);
Review Comment:
Yes, either the debug assertion should be a real assertion or we should clamp.
##########
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
#[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;
let src = data.as_ptr().add(offset);
unsafe {
- std::ptr::copy_nonoverlapping(src, tmp.as_mut_ptr() as *mut u8, count);
- tmp.assume_init()
+ std::ptr::copy_nonoverlapping(src, &mut tmp as *mut _ as *mut u8, count);
Review Comment:
_or_ the function should be documented with clearer invariants. It is an unsafe function after all, it's allowed to assume these things, but "all arguments are within range" is two separate checks: that count is not more than 8, and that offset/count is in range. The next commit adds the latter, not the former.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "ssbr (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "ssbr (via GitHub)" <gi...@apache.org>.
ssbr commented on PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#issuecomment-2420473685
> Not sure if the CI failure is real or transitional
It's also present without the change, so real, but unrelated.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1799287067
##########
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
#[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;
let src = data.as_ptr().add(offset);
unsafe {
- std::ptr::copy_nonoverlapping(src, tmp.as_mut_ptr() as *mut u8, count);
- tmp.assume_init()
+ std::ptr::copy_nonoverlapping(src, &mut tmp as *mut _ as *mut u8, count);
Review Comment:
Shouldn't we restrict the number of bytes copied to 8 here? The destination is not valid for more writes.
```suggestion
std::ptr::copy_nonoverlapping(src, &mut tmp as *mut _ as *mut u8, count.min(8));
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#issuecomment-2427035165
> > Not sure if the CI failure is real or transitional
>
> It's also present without the change, so real, but unrelated.
@itsjunetime fixed it in https://github.com/apache/arrow-rs/pull/6585
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1802569870
##########
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:
> MaybeUninit(0) and 0 should have no difference in performance.
Understood, but I wanted that to be confirmed experimentally
> For this to be an optimization, this ought to have used MaybeUninit::uninit()
I initially did that but Miri did not allow to leave uninitialized bytes even those are unused. So I quickly switched to `new(0)` and checked the performance implications of the change. Perhaps I should have experimented `0` at the time.
Anyway, I just wanted to make sure the change would not cause any unexpected performance results.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1805318108
##########
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:
Thank you @ssbr
Looks like `boolean_append_packed` was moved to `arrow` instead of `arrow-buffer`
But since you proved the generated code is identical, I will mark this part is resolved
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1801972146
##########
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`.
Review Comment:
@kazuyukitanimura can you please review this change? I believe it was introduced as part of https://github.com/apache/arrow-rs/pull/6288
https://github.com/apache/arrow-rs/blame/b945b15de9085f5961a478d4f35b0c5c3427e248/arrow-buffer/src/util/bit_mask.rs#L132
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1802118010
##########
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:
@ssbr This is a performance sensitive area.
Would you mind benchmarking the followings and compare before and after? (maybe several times as the results may vary due to noise)
```
cd arrow-buffer
cargo bench --bench boolean_append_packed
cargo bench --bench bit_mask
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "Manishearth (via GitHub)" <gi...@apache.org>.
Manishearth commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1803313498
##########
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 initially did that but Miri did not allow to leave uninitialized bytes even those are unused
Yep, like I said, it's UB in this case.
> So I quickly switched to new(0) and checked the performance implications of the change. Perhaps I should have experimented 0 at the time.
yes, the use of MaybeUninit here at best would make performance _worse_ if things don't get inlined (though they ought to). There's nothing special about MaybeUninit that helps it make things better here. MaybeUninit is only special in that it allows you to create uninitialized states without it being instantly UB, but that's not what's happening here.
I would recommend trying to avoid unsafe used in this way, it's not a good sign if a crate uses a lot of completely avoidable unsafe, especially when the unsafe code displays a misunderstanding of the underlying abstractions.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1805318108
##########
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:
Thank you @ssbr
Looks like `boolean_append_packed` was moved to `arrow` instead of `arrow-buffer`
But since you proved the generated code is identical, this looks good to me. We can mark this part as resolved
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#issuecomment-2420380956
Not sure if the CI failure is real or transitional
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1798558441
##########
arrow-buffer/src/builder/offset.rs:
##########
@@ -70,8 +70,11 @@ impl<O: ArrowNativeType> OffsetBufferBuilder<O> {
///
/// Panics if offsets overflow `O`
Review Comment:
is this still applicable?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#issuecomment-2408234083
thanks @ssbr !
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "ssbr (via GitHub)" <gi...@apache.org>.
ssbr commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1799999535
##########
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
#[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;
let src = data.as_ptr().add(offset);
unsafe {
- std::ptr::copy_nonoverlapping(src, tmp.as_mut_ptr() as *mut u8, count);
- tmp.assume_init()
+ std::ptr::copy_nonoverlapping(src, &mut tmp as *mut _ as *mut u8, count);
Review Comment:
Sorry, I don't know how to use github review. I meant to leave a comment and mark conversation as resolved (that's how it works with the code review tool at work), but I hit the button and it ate my comment and immediately closed the thread.
This is a good point. I don't want to change the behavior in this PR, to keep it easy to merge / a completely clear net improvement PR, so I just changed the comment.
In general, I am in favor of adding runtime checks everywhere and removing all `unsafe`, but I worry that it would defeat the point of these files, which are trying so hard to be fast and unsafe and stuff. I wouldn't know what benchmarks to check for regressions in, so I'd prefer to leave that kind of change to the code owners.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "ssbr (via GitHub)" <gi...@apache.org>.
ssbr commented on PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#issuecomment-2412111642
> @ssbr can you please check the build results?
I struggled to find them! Sorry, not used to github. The only way I could find was to click into checks, click into the commit history, find the one with the x, click that, and then click to open every check until I found the failures. Is there an easier way? I found two:
* Tests failing -- those tests are also failing at head, so I ignored this.
* `cargo fmt`. I ran `cargo fmt`. Once again, not used to git or github, so I just made it a commit on top. (At work, this would be `cargo fmt; hg absorb`.)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#discussion_r1799275697
##########
arrow-buffer/src/builder/offset.rs:
##########
@@ -70,8 +70,11 @@ impl<O: ArrowNativeType> OffsetBufferBuilder<O> {
///
/// Panics if offsets overflow `O`
Review Comment:
Yes, via the call to `finish`: https://github.com/apache/arrow-rs/blob/c6201c700f6aa9ff3197e25d78bdeb919a90863c/arrow-buffer/src/builder/offset.rs#L63
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] `unsafe` improvements [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6551:
URL: https://github.com/apache/arrow-rs/pull/6551#issuecomment-2415071686
https://github.com/apache/arrow-rs/actions/runs/11334312589/job/31523941790?pr=6551 does not appear to be related to this PR (it happens in main as well)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org