-
Notifications
You must be signed in to change notification settings - Fork 338
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
Speed up fixed-sized array iteration #1050
Conversation
b93e8ff
to
e5520fe
Compare
} | ||
|
||
let out: [T; SIZE] = | ||
array_init::array_init(|i: usize| self.values.value(self.offset * SIZE + i)); |
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.
Did we already have this crate in the deps I guess? We should add a note and track the stabilization of the stdlib feature we actually would use instead:
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 did -- see the original implementation of FixedSizeArrayField::arrow_deserialize
below.
e5520fe
to
58a03f1
Compare
58a03f1
to
c5bb896
Compare
// Iteration should only happen via iter_from_array_ref. | ||
// This is a quirk of the way the traits work in arrow2_convert. | ||
unsafe { | ||
do_not_call_into_iter(); |
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.
I love this trick :)
This optimizes iteration of fixed-sized arrays, but restricts them to Primitives, which seems like a reasonable trade-off for now. We can revisit if we find a need for non-primitive fixed-sized arrays at some point in the future.
I introduced a new benchmark in
re_query
for Vecs as separate from points. Specifically I use 100k vec arrays so make the issue more readily apparent. For arrays of 1k the other query overhead dominates.New vec bench
Before:
After:
Checklist
CHANGELOG.md
(if this is a big enough change to warrant it)