Skip to content

Conversation

@jorgecarleitao
Copy link
Member

@jorgecarleitao jorgecarleitao commented Jan 23, 2021

This PR improves the performance of certain time / date casts by using the brand new API proposed in #9235 .

That API allows for a very fast execution of unary and infalible operations on primitive arrays, and this PR leverages that for cast operations that require a simple mathematical operation.

Switched to branch 'fast_unary'
   Compiling arrow v3.0.0-SNAPSHOT (/Users/jorgecarleitao/projects/arrow/rust/arrow)
    Finished bench [optimized] target(s) in 1m 06s
     Running /Users/jorgecarleitao/projects/arrow/rust/target/release/deps/cast_kernels-25ee76597a8b997b
Gnuplot not found, using plotters backend

cast date64 to date32 512                                                                             
                        time:   [1.1668 us 1.1706 us 1.1749 us]
                        change: [-83.347% -83.248% -83.144%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe

cast date32 to date64 512                                                                             
                        time:   [899.73 ns 930.58 ns 971.56 ns]
                        change: [-86.799% -86.520% -86.190%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
  3 (3.00%) high mild
  7 (7.00%) high severe

cast time32s to time32ms 512                                                                             
                        time:   [728.73 ns 732.33 ns 735.90 ns]
                        change: [-54.503% -54.201% -53.917%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

cast time64ns to time32s 512                                                                             
                        time:   [4.8374 us 4.8447 us 4.8526 us]
                        change: [-57.022% -56.791% -56.587%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  6 (6.00%) high mild
  2 (2.00%) high severe

@github-actions
Copy link

@codecov-io
Copy link

codecov-io commented Jan 23, 2021

Codecov Report

Merging #9297 (f88847c) into master (437c8c9) will increase coverage by 0.00%.
The diff coverage is 97.36%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9297   +/-   ##
=======================================
  Coverage   81.88%   81.89%           
=======================================
  Files         215      216    +1     
  Lines       52988    52983    -5     
=======================================
- Hits        43391    43388    -3     
+ Misses       9597     9595    -2     
Impacted Files Coverage Δ
rust/arrow/src/compute/kernels/arity.rs 91.66% <91.66%> (ø)
rust/arrow/src/compute/kernels/cast.rs 97.08% <100.00%> (+0.09%) ⬆️
rust/parquet/src/encodings/encoding.rs 95.43% <0.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 437c8c9...f88847c. Read the comment docs.

@jorgecarleitao
Copy link
Member Author

NOTE: this PR is 40 LOC change. The rest comes from the other PR. Please wait for the merge, this is only a draft to indicate what can be done with this.

@jorgecarleitao jorgecarleitao marked this pull request as ready for review January 24, 2021 10:03
@nevi-me nevi-me self-requested a review January 24, 2021 12:47
Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for generalising this, and cleaning up the cast kernels. There were plenty of fruits hanging low there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could save a further few cycles here by specifying the null count, as it's known and trusted to be correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great and simple example, and I wonder if the compiler is smart enough to optimise 2 commutative unary functions to perform as well as a single combined one.

Worth trying out some time.

Copy link
Contributor

@nevi-me nevi-me Jan 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call this something more general, like arity https://en.wikipedia.org/wiki/Arity

We'd probably want a binary function that takes 2 arguments, or maybe ternary if we're feeling cute enough to push it 😉.

EDIT: math_op is binary

Copy link
Member Author

@jorgecarleitao jorgecarleitao Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this was only unary, I gave it that name. Note that we can lift all binary arithmetics into a generic like this one and place it together.

I do not have strong feelings here, but it is the first time I hear about arity xD

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, whoever adds a ternary function can change the name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be backward incompatible, so it would be good to change it soon. I will change to arity. It is a fun name. Another option would be generics, but that is too generic. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants