Skip to content

ARROW-11354: [Rust] Speed-up cast of dates and times (2-4x) - #9297

Closed
jorgecarleitao wants to merge 2 commits into
apache:masterfrom
jorgecarleitao:fast_unary
Closed

ARROW-11354: [Rust] Speed-up cast of dates and times (2-4x)#9297
jorgecarleitao wants to merge 2 commits into
apache:masterfrom
jorgecarleitao:fast_unary

Conversation

@jorgecarleitao

@jorgecarleitaojorgecarleitao commented Jan 23, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown

@codecov-io

codecov-io commented Jan 23, 2021

Copy link
Copy Markdown

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 FilesCoverage Δ
rust/arrow/src/compute/kernels/arity.rs91.66% <91.66%> (ø)
rust/arrow/src/compute/kernels/cast.rs97.08% <100.00%> (+0.09%)⬆️
rust/parquet/src/encodings/encoding.rs95.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
Copy Markdown
MemberAuthor

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

@nevi-menevi-me left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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.

@nevi-menevi-meJan 24, 2021

Copy link
Copy Markdown
Contributor

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 arityhttps://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

@jorgecarleitaojorgecarleitaoJan 25, 2021

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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 freeto 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

@jorgecarleitao@codecov-io@nevi-me