You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/08/08 13:54:46 UTC
[GitHub] [arrow-rs] Marwes opened a new pull request, #2365: Compiletimes full
Marwes opened a new pull request, #2365:
URL: https://github.com/apache/arrow-rs/pull/2365
# Rationale for this change
The `arrow` crate generates a lot of code due the the dynamic comparison operators which takes a long time to compile and potentially bloats the final binary. By extracting code from generic functions and otherwise reducing how much code gets instantiated we get a faster and slimmer compilation.
Doesn't really fix https://github.com/apache/arrow-rs/issues/1858 as the amount of code is still huge, but it does still help.
(Output of `cargo llvm-line -p arrow`)
### Before
```
Lines Copies Function name
----- ------ -------------
3348856 (100%) 66644 (100%) (TOTAL)
126209 (3.8%) 2323 (3.5%) core::option::Option<T>::map
117648 (3.5%) 912 (1.4%) arrow::compute::kernels::comparison::cmp_dict_bit_equal
82631 (2.5%) 1174 (1.8%) core::iter::traits::iterator::Iterator::fold
76312 (2.3%) 587 (0.9%) <arrow::array::array_boolean::BooleanArray as core::iter::traits::collect::FromIterator<Ptr>>::from_iter
76221 (2.3%) 1332 (2.0%) core::iter::adapters::map::map_fold::{{closure}}
64660 (1.9%) 1332 (2.0%) <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::fold
61814 (1.8%) 314 (0.5%) arrow::buffer::mutable::MutableBuffer::try_from_trusted_len_iter
61759 (1.8%) 301 (0.5%) <arrow::array::array_primitive::PrimitiveArray<T> as core::iter::traits::collect::FromIterator<Ptr>>::from_iter
57896 (1.7%) 62 (0.1%) core::slice::sort::partition_in_blocks
51593 (1.5%) 342 (0.5%) <core::iter::adapters::zip::Zip<A,B> as core::iter::adapters::zip::ZipImpl<A,B>>::next
48259 (1.4%) 320 (0.5%) arrow::buffer::mutable::MutableBuffer::extend_from_iter
46302 (1.4%) 659 (1.0%) <core::iter::adapters::enumerate::Enumerate<I> as core::iter::traits::iterator::Iterator>::fold::enumerate::{{closure}}
45450 (1.4%) 150 (0.2%) arrow::compute::kernels::comparison::compare_op
42851 (1.3%) 587 (0.9%) <arrow::array::array_boolean::BooleanArray as core::iter::traits::collect::FromIterator<Ptr>>::from_iter::{{closure}}
41072 (1.2%) 2606 (3.9%) core::iter::adapters::map::Map<I,F>::new
41048 (1.2%) 296 (0.4%) <core::iter::adapters::zip::Zip<A,B> as core::iter::adapters::zip::ZipImpl<A,B>>::size_hint
41012 (1.2%) 1147 (1.7%) core::iter::traits::iterator::Iterator::for_each
40109 (1.2%) 145 (0.2%) arrow::util::trusted_len::trusted_len_unzip
39509 (1.2%) 312 (0.5%) <arrow::buffer::immutable::Buffer as core::iter::traits::collect::FromIterator<T>>::from_iter
```
### After
```
Lines Copies Function name
----- ------ -------------
3348856 (100%) 66644 (100%) (TOTAL)
126209 (3.8%) 2323 (3.5%) core::option::Option<T>::map
117648 (3.5%) 912 (1.4%) arrow::compute::kernels::comparison::cmp_dict_bit_equal
82631 (2.5%) 1174 (1.8%) core::iter::traits::iterator::Iterator::fold
76312 (2.3%) 587 (0.9%) <arrow::array::array_boolean::BooleanArray as core::iter::traits::collect::FromIterator<Ptr>>::from_iter
76221 (2.3%) 1332 (2.0%) core::iter::adapters::map::map_fold::{{closure}}
64660 (1.9%) 1332 (2.0%) <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::fold
61814 (1.8%) 314 (0.5%) arrow::buffer::mutable::MutableBuffer::try_from_trusted_len_iter
61759 (1.8%) 301 (0.5%) <arrow::array::array_primitive::PrimitiveArray<T> as core::iter::traits::collect::FromIterator<Ptr>>::from_iter
57896 (1.7%) 62 (0.1%) core::slice::sort::partition_in_blocks
51593 (1.5%) 342 (0.5%) <core::iter::adapters::zip::Zip<A,B> as core::iter::adapters::zip::ZipImpl<A,B>>::next
48259 (1.4%) 320 (0.5%) arrow::buffer::mutable::MutableBuffer::extend_from_iter
46302 (1.4%) 659 (1.0%) <core::iter::adapters::enumerate::Enumerate<I> as core::iter::traits::iterator::Iterator>::fold::enumerate::{{closure}}
45450 (1.4%) 150 (0.2%) arrow::compute::kernels::comparison::compare_op
42851 (1.3%) 587 (0.9%) <arrow::array::array_boolean::BooleanArray as core::iter::traits::collect::FromIterator<Ptr>>::from_iter::{{closure}}
41072 (1.2%) 2606 (3.9%) core::iter::adapters::map::Map<I,F>::new
41048 (1.2%) 296 (0.4%) <core::iter::adapters::zip::Zip<A,B> as core::iter::adapters::zip::ZipImpl<A,B>>::size_hint
41012 (1.2%) 1147 (1.7%) core::iter::traits::iterator::Iterator::for_each
40109 (1.2%) 145 (0.2%) arrow::util::trusted_len::trusted_len_unzip
39509 (1.2%) 312 (0.5%) <arrow::buffer::immutable::Buffer as core::iter::traits::collect::FromIterator<T>>::from_iter
32252 (1.0%) 659 (1.0%) <core::iter::adapters::enumerate::Enumerate<I> as core::iter::traits::iterator::Iterator>::fold
31680 (0.9%) 240 (0.4%) arrow::compute::kernels::arithmetic::math_op_dict
30955 (0.9%) 1842 (2.8%) core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &mut F>::call_once
30890 (0.9%) 152 (0.2%) arrow::array::array_primitive::PrimitiveArray<T>::from_trusted_len_iter
30132 (0.9%) 81 (0.1%) arrow::compute::kernels::take::take_primitive
29737 (0.9%) 2606 (3.9%) core::iter::traits::iterator::Iterator::map
29531 (0.9%) 241 (0.4%) <alloc::vec::Vec<T> as alloc::vec::spec_from_iter_nested::SpecFromIterNested<T,I>>::from_iter
28824 (0.9%) 62 (0.1%) core::slice::sort::shift_tail
```
# What changes are included in this PR?
Refactorings to reduce how much code is instantiated. The PR is left in draft as some of the changes to compare native types instead of "arrow types" is a bit hacky.
--
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
[GitHub] [arrow-rs] tustvold closed pull request #2365: refactor: Reduce how much code is instantiated for comparisons
Posted by GitBox <gi...@apache.org>.
tustvold closed pull request #2365: refactor: Reduce how much code is instantiated for comparisons
URL: https://github.com/apache/arrow-rs/pull/2365
--
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
[GitHub] [arrow-rs] tustvold commented on pull request #2365: refactor: Reduce how much code is instantiated for comparisons
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2365:
URL: https://github.com/apache/arrow-rs/pull/2365#issuecomment-1358006466
I'm closing this PR as it has bit-rotted substantially, and there has been significant effort to address this under #2594. Please feel to reopen if/when updated
--
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
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2365: refactor: Reduce how much code is instantiated for comparisons
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2365:
URL: https://github.com/apache/arrow-rs/pull/2365#discussion_r944224937
##########
arrow/src/compute/kernels/comparison.rs:
##########
@@ -2197,6 +2248,76 @@ where
compare_dict_op!(left, right, op, PrimitiveArray<T>)
}
+// Compares two dictionary using bit-equality (helps reduce how much code is instantiated as
+// types with the same underlying native type can share the instantiated code).
+fn cmp_dict_bit_equal<K, T, F>(
+ left: &DictionaryArray<K>,
+ right: &DictionaryArray<K>,
+ op: F,
+) -> Result<BooleanArray>
+where
+ K: ArrowNumericType,
+ T: ArrowNumericType,
+ T::Native: CmpRepr,
+ F: Fn(<T::Native as CmpRepr>::Repr, <T::Native as CmpRepr>::Repr) -> bool,
+{
+ compare_dict_op!(
+ left,
+ right,
+ op,
+ PrimitiveArray<T>,
+ <T::Native as CmpRepr>::Repr
+ )
+}
+
+// Trait to help reduce the number of function instantiations by merging types which compare the
+// same (u8/i8 are compared the same way)
Review Comment:
I'm not sure this is correct, as u8 is an unsigned comparison whereas i8 is signed? You can apply this style optimization for things like TimestampNanosecondArray, i.e. logical types, but I'm not sure you can discard the sign-ness of the type
--
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
[GitHub] [arrow-rs] Marwes commented on a diff in pull request #2365: refactor: Reduce how much code is instantiated for comparisons
Posted by GitBox <gi...@apache.org>.
Marwes commented on code in PR #2365:
URL: https://github.com/apache/arrow-rs/pull/2365#discussion_r944239621
##########
arrow/src/compute/kernels/comparison.rs:
##########
@@ -2197,6 +2248,76 @@ where
compare_dict_op!(left, right, op, PrimitiveArray<T>)
}
+// Compares two dictionary using bit-equality (helps reduce how much code is instantiated as
+// types with the same underlying native type can share the instantiated code).
+fn cmp_dict_bit_equal<K, T, F>(
+ left: &DictionaryArray<K>,
+ right: &DictionaryArray<K>,
+ op: F,
+) -> Result<BooleanArray>
+where
+ K: ArrowNumericType,
+ T: ArrowNumericType,
+ T::Native: CmpRepr,
+ F: Fn(<T::Native as CmpRepr>::Repr, <T::Native as CmpRepr>::Repr) -> bool,
+{
+ compare_dict_op!(
+ left,
+ right,
+ op,
+ PrimitiveArray<T>,
+ <T::Native as CmpRepr>::Repr
+ )
+}
+
+// Trait to help reduce the number of function instantiations by merging types which compare the
+// same (u8/i8 are compared the same way)
Review Comment:
True I were to hasty and only though about `==`/`!=`. It needs to stay for ordered comparisons
--
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
[GitHub] [arrow-rs] tustvold commented on pull request #2365: refactor: Reduce how much code is instantiated for comparisons
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2365:
URL: https://github.com/apache/arrow-rs/pull/2365#issuecomment-1208201180
Do you perhaps have some compile time metrics for this change?
--
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
[GitHub] [arrow-rs] Marwes commented on pull request #2365: refactor: Reduce how much code is instantiated for comparisons
Posted by GitBox <gi...@apache.org>.
Marwes commented on PR #2365:
URL: https://github.com/apache/arrow-rs/pull/2365#issuecomment-1209134206
> Do you perhaps have some compile time metrics for this change?
## Before
```
________________________________________________________
Executed in 17,63 secs fish external
usr time 59,63 secs 381,00 micros 59,63 secs
sys time 3,78 secs 225,00 micros 3,78 secs
```
## After
```
________________________________________________________
Executed in 14,01 secs fish external
usr time 43,51 secs 540,00 micros 43,51 secs
sys time 2,51 secs 270,00 micros 2,51 secs
```
--
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
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2365: refactor: Reduce how much code is instantiated for comparisons
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2365:
URL: https://github.com/apache/arrow-rs/pull/2365#discussion_r940304008
##########
arrow/src/array/array_primitive.rs:
##########
@@ -201,6 +213,45 @@ impl<'a, T: ArrowPrimitiveType> ArrayAccessor for &'a PrimitiveArray<T> {
}
}
+pub(crate) struct NativeArrayView<'a, T> {
Review Comment:
Can we perhaps find some way to eliminate this part, in a trade-off between more unsafe and getting LLVM to do more work, I would always take the latter...
--
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
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2365: refactor: Reduce how much code is instantiated for comparisons
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2365:
URL: https://github.com/apache/arrow-rs/pull/2365#discussion_r944224937
##########
arrow/src/compute/kernels/comparison.rs:
##########
@@ -2197,6 +2248,76 @@ where
compare_dict_op!(left, right, op, PrimitiveArray<T>)
}
+// Compares two dictionary using bit-equality (helps reduce how much code is instantiated as
+// types with the same underlying native type can share the instantiated code).
+fn cmp_dict_bit_equal<K, T, F>(
+ left: &DictionaryArray<K>,
+ right: &DictionaryArray<K>,
+ op: F,
+) -> Result<BooleanArray>
+where
+ K: ArrowNumericType,
+ T: ArrowNumericType,
+ T::Native: CmpRepr,
+ F: Fn(<T::Native as CmpRepr>::Repr, <T::Native as CmpRepr>::Repr) -> bool,
+{
+ compare_dict_op!(
+ left,
+ right,
+ op,
+ PrimitiveArray<T>,
+ <T::Native as CmpRepr>::Repr
+ )
+}
+
+// Trait to help reduce the number of function instantiations by merging types which compare the
+// same (u8/i8 are compared the same way)
Review Comment:
I'm not sure this is correct, as u8 is an unsigned comparison whereas i8 is signed?
--
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