-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
perf: Speed up dt.offset_by
2x for constant durations
#16728
Conversation
// fastpath! | ||
match datetime.time_unit() { | ||
TimeUnit::Milliseconds => { | ||
Ok(datetime.0.apply_values(|v| v + offset.duration_ms())) |
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 think this makes the same duration_*s()
call for every element, right? You can pull the offset out front and then apply the value, as in:
let offset = match datetime.time_unit() {
TimeUnit::Milliseconds => offset.duration_ms(),
TimeUnit::Microseconds => offset.duration_us(),
TimeUnit::Nanoseconds => offset.duration_ns(),
};
Ok(datetime.0.apply_values(|v| v + offset)
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.
thanks @mcrumiller!
f2f1893
to
559c3ac
Compare
CodSpeed Performance ReportMerging #16728 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16728 +/- ##
=======================================
Coverage 81.45% 81.45%
=======================================
Files 1413 1413
Lines 186096 186057 -39
Branches 2776 2756 -20
=======================================
- Hits 151585 151557 -28
- Misses 33991 33997 +6
+ Partials 520 503 -17 ☔ View full report in Codecov by Sentry. |
@@ -189,33 +189,53 @@ pub(super) fn datetime( | |||
fn apply_offsets_to_datetime( | |||
datetime: &Logical<DatetimeType, Int64Type>, | |||
offsets: &StringChunked, | |||
offset_fn: fn(&Duration, i64, Option<&Tz>) -> PolarsResult<i64>, | |||
time_zone: Option<&Tz>, | |||
) -> PolarsResult<Int64Chunked> { |
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.
Just notice this now, but ideally the implementation should be in polars-ops
or polars-time
and then we only dispatch to those functions here. I'd like polars-plan
to only be about the query planning/resolving.
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.
yup - ok if I do that separately?
TimeUnit::Microseconds => offset.duration_us(), | ||
TimeUnit::Nanoseconds => offset.duration_ns(), | ||
}; | ||
if offset.negative() { |
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.
To reduce binary bloat, I think we can make one code branch of this that does the apply_values
.
if offset.negative() {
duration = - duration;
}
Ok(datetime.0.apply_values(|v| v + duration))
I think we can also use our already compiled kernels now. Which might also have more SIMD acceleration. If not, it at least saves compiler bloat.
// apply to inner and then you have to reconstruct the datetime.
datetime.0.clone().wrapping_add_scalar(duration);
Great hammering on those temporal performance! 💪 Left some comments. |
dt.offset_by
2x for constant durations
closes #16722 (timing becomes in line with the alternative method presented - I couldn't reproduce the 10x difference reported)
timing results available here https://www.kaggle.com/code/marcogorelli/polars-timing?scriptVersionId=181508652
I think this is fine but I didn't sleep too well so I'll check over it again tomorrow before marking as 'ready for review'The gist of the change is: in the simple case, keep things simple and use
apply_values