ARROW-10002: [Rust] Remove trait specialization from arrow crate by jorgecarleitao · Pull Request #8485 · apache/arrow

jorgecarleitao · 2020-10-18T09:53:58Z

This PR removes trait specialization by leveraging the compiler to remove trivial if statements.

I verified that the assembly code was the same in a simple example. I do not know if this generalizes to our use-case, but I suspect so as LLVM is (hopefully) removing trivial branches like if a != a.

The change get_data_type() to DATA_TYPE is not necessary. I did it before realizing this. IMO it makes it more explicit that this is not a function, but a constant, but we can revert it.

jorgecarleitao · 2020-10-18T09:55:25Z

fyi @andygrove @alamb

jorgecarleitao · 2020-10-18T09:58:21Z

    }
 }

+fn as_datetime<T: ArrowPrimitiveType>(v: i64) -> Option<NaiveDateTime> {


This places these functions outside the impl, so that we can use them:

When used with ArrowNumericType, it can convert itself to i64 and thus this is simple.

When we use it with PrimitiveType, we use to_usize to convert to i64.

jorgecarleitao · 2020-10-18T10:01:07Z

-        let buffer = MutableBuffer::new(capacity * mem::size_of::<T::Native>());
+    #[inline]
+    fn new(capacity: usize) -> Self {
+        let buffer = if T::DATA_TYPE == DataType::Boolean {


Note that after templating, this will be either if a == a or if a == b, which I am hoping the compiler to optimize out. I placed an inline to help the compiler a bit, but I am not sure it is really needed.

I think using an if for specialization rather than the compiler is totally legit

Nice! I used a similar trick for the simd sum aggregation that the compiler was also able to optimize away (https://github.com/apache/arrow/pull/8370/files#diff-53a8522c4a482c9566b9032e1bd2c7990bab29640857cb40411beb7158189e75R625)

@jorgecarleitao

I see this in profiling for the BufferBuilderTrait append function:

Which accounts for ~9% of the instruction fetches according to callgrind when running the TPHC 12 benchmark.

So it seems the if T::DATA_TYPE == DataType::Boolean is not being optimized out and goes into the partial eq implementation (or the profiler is wrong), maybe because the implementation there is not easy to optimize / specialize for this case. I will check if another implementation (pattern match?) solves this.

jorgecarleitao · 2020-10-18T10:02:36Z

            return false;
        }

+        if T::DATA_TYPE == DataType::Boolean {


Here we use the same principle: this will be a constant if a == a or if a == b.

jorgecarleitao · 2020-10-18T10:03:28Z

        T: ArrowNumericType,
    {
-        let array_type = T::get_data_type();
+        let array_type = T::DATA_TYPE;


this could actually be removed by making filter_array_impl a generic over T. I left for a future exercise.

jorgecarleitao · 2020-10-18T10:03:58Z

 //!
 //! The parquet implementation is on a [separate crate](https://crates.io/crates/parquet)

-#![feature(specialization)]


#arewestableyet? This is great, I've just compiled Arrow on stable Rust. I think it's just parquet that still uses it after this PR. If DataFusion doesn't use any nightly features, one could make parquet optional, and then Parquet will be the only one left.

github-actions · 2020-10-18T10:07:08Z

https://issues.apache.org/jira/browse/ARROW-10002

alamb

This is a really nice change -- it makes sense to me and I (non bindingly) vote to !

alamb · 2020-10-18T10:59:43Z

+        write!(f, "PrimitiveArray<{:?}>\n[\n", T::DATA_TYPE)?;
+        print_long_array(self, f, |array, index, f| match T::DATA_TYPE {
            DataType::Date32(_) | DataType::Date64(_) => {
-                match array.value_as_date(index) {


so the primary difference here is that all types get converted to i64 and then converted back to the appropriate (potentially smaller) native type by the code (and the compiler is presumably smart enough to make it all happen efficiently)

And by structuring the code in that way, we only need a single type parameter (i64->T rather than all combinations of input and output types?)

The journey to usize via num-traits, then to i64, feels long, but I see the compiler optimises the function nicely in release mode.

pub extern fn convert_u32_to_i64(value: u32) -> i64 { num::ToPrimitive::to_usize(&value).unwrap() as i64 }

playground::convert_u32_to_i64: movl %edi, %eax retq

alamb · 2020-10-18T11:17:11Z

-        let buffer = MutableBuffer::new(capacity * mem::size_of::<T::Native>());
+    #[inline]
+    fn new(capacity: usize) -> Self {
+        let buffer = if T::DATA_TYPE == DataType::Boolean {


I think using an if for specialization rather than the compiler is totally legit

vertexclique · 2020-10-18T11:48:16Z

For the sake of creating less friction at the user side, can you make get_data_type const fn with the same name rather than const with capitals? Still it will generate exactly the same code at the call site.

nevi-me · 2020-10-18T12:21:02Z

For the sake of creating less friction at the user side, can you make get_data_type const fn with the same name rather than const with capitals? Still it will generate exactly the same code at the call site.

I think this won't be possible as functions in traits can't be const

vertexclique · 2020-10-18T16:31:31Z

@nevi-me true :/. We go to datatype from generics. const fn also accepts only Sized any other trait bound won't work.

vertexclique

I need this got merged. I have some ideas on top of this. 🚀 great stuff here.

nevi-me

I've reviewed this and compiled it with nightly, thanks for this @jorgecarleitao. Arrow will be stable in the next release (if one doesn't use simd) 🎆

This PR removes trait specialization by leveraging the compiler to remove trivial `if` statements. I verified that the assembly code was the same in a [simple example](https://rust.godbolt.org/z/qrcW8W). I do not know if this generalizes to our use-case, but I suspect so as LLVM is (hopefully) removing trivial branches like `if a != a`. The change `get_data_type()` to `DATA_TYPE` is not necessary. I did it before realizing this. IMO it makes it more explicit that this is not a function, but a constant, but we can revert it. Closes #8485 from jorgecarleitao/simp_types Authored-by: Jorge C. Leitao <jorgecarleitao@gmail.com> Signed-off-by: Neville Dipale <nevilledips@gmail.com>

jorgecarleitao added 2 commits October 18, 2020 10:40

Made DATA_TYPE a constant.

e777be5

Removed specialization.

12fd9d4

jorgecarleitao requested a review from nevi-me October 18, 2020 09:53

github-actions Bot added the Component: Rust label Oct 18, 2020

jorgecarleitao commented Oct 18, 2020

View reviewed changes

Comment thread rust/arrow/src/array/array.rs

jorgecarleitao commented Oct 18, 2020

View reviewed changes

Comment thread rust/arrow/src/array/array.rs

jorgecarleitao commented Oct 18, 2020

View reviewed changes

Comment thread rust/arrow/src/array/builder.rs

jorgecarleitao commented Oct 18, 2020

View reviewed changes

jorgecarleitao changed the title ~~ARROW-10002: [Rust] Remove trait specialization~~ ARROW-10002: [Rust] Remove trait specialization fro arrow crate Oct 18, 2020

jorgecarleitao changed the title ~~ARROW-10002: [Rust] Remove trait specialization fro arrow crate~~ ARROW-10002: [Rust] Remove trait specialization from arrow crate Oct 18, 2020

alamb approved these changes Oct 18, 2020

View reviewed changes

vertexclique approved these changes Oct 18, 2020

View reviewed changes

nevi-me approved these changes Oct 18, 2020

View reviewed changes

nevi-me closed this in a6c6ef6 Oct 18, 2020

jorgecarleitao deleted the simp_types branch October 28, 2020 04:16

asfimport mentioned this pull request Dec 3, 2020

[Rust] Trait-specialization requires nightly #17346

Closed

Conversation

jorgecarleitao commented Oct 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorgecarleitao commented Oct 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jorgecarleitao Oct 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dandandan Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nevi-me Oct 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 18, 2020

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vertexclique commented Oct 18, 2020

Uh oh!

nevi-me commented Oct 18, 2020

Uh oh!

vertexclique commented Oct 18, 2020

Uh oh!

vertexclique left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nevi-me left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jorgecarleitao commented Oct 18, 2020 •

edited

Loading

jorgecarleitao Oct 18, 2020 •

edited

Loading

Dandandan Dec 3, 2020 •

edited

Loading

nevi-me Oct 18, 2020 •

edited

Loading

vertexclique left a comment •

edited

Loading

nevi-me left a comment •

edited

Loading