-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: Reduce copy in MemSlice
#17983
Conversation
#[cfg(all(feature = "async", feature = "parquet"))] | ||
ColumnStore::Fetched(fetched) => { | ||
let entry = fetched.get(&start).unwrap_or_else(|| { | ||
panic!( | ||
"mmap_columns: column with start {start} must be prefetched in ColumnStore.\n" | ||
) | ||
}); | ||
MemSlice::from_slice(entry.as_ref()) | ||
MemSlice::from_bytes(entry.clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now support zero-copy from bytes::Bytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
||
/// Construct a `MemSlice` that simply wraps around a `&[u8]`. | ||
#[inline] | ||
pub fn from_slice(slice: &'static [u8]) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes from_slice
- previously it copied into a new Arc<[u8]>
, but now it simply wraps the slice.
For context, this is only used by ReaderBytes<'a>
. I think one day we can refactor/replace ReaderBytes
- the way we currently use that 'a
lifetime is not ideal
// `slice` is not 'static - it is backed by `inner`. This is safe as long as `slice` is not | ||
// directly accessed, and we are in a private module to guarantee that. Access should only | ||
// be done through `Deref<Target = [u8]>`, which automatically gives the correct lifetime. | ||
slice: &'static [u8], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deref without matching on the enum
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17983 +/- ##
==========================================
+ Coverage 80.39% 80.42% +0.03%
==========================================
Files 1494 1494
Lines 196480 196488 +8
Branches 2817 2817
==========================================
+ Hits 157957 158027 +70
+ Misses 38003 37941 -62
Partials 520 520 ☔ View full report in Codecov by Sentry. |
LFTM 😄 |
No description provided.