PERF: (partial) fix for np_datetime.c performance regression #57988
Conversation
|
cc @WillAyd |
…n_induced_by_re' into 240324_57951_potential_regression_induced_by_re
| @@ -752,66 +752,56 @@ void pandas_timedelta_to_timedeltastruct(npy_timedelta td, | |||
| } | |||
|
|
|||
| const npy_int64 per_day = sec_per_day * per_sec; | |||
| npy_int64 frac; | |||
| const int sign = td < 0 ? -1 : 1; | |||
| const int is_negative = td < 0 ? 1 : 0; | |||
There was a problem hiding this comment.
You can use bool here - our minimum supported standard is C11
|
|
||
| const int sign = frac < 0 ? -1 : 1; | ||
| if (frac < 0) { | ||
| npy_int64 sfrac = td / per_sec - is_negative * uneven_in_seconds; |
There was a problem hiding this comment.
Am I reading this right that it just can add/subtract 1 when uneven_in_seconds is true? I might be missing the point but that feels a bit off - not sure we have full test coverage in all precisions
There was a problem hiding this comment.
Thanks for scratching your head at this. It's a leftover from a different optimisation route that was not successful in the end. I moved the subtraction into the if body again (same as in the main version) so it should be clearer now. Also now the speedup is at where I had it with the previous version that did not pass the tests.
| } else { | ||
| frac = -frac; | ||
| if (sfrac <= sec_per_day) { |
There was a problem hiding this comment.
This seems to add more branching than what we had before, so I'm a little hesitant to say this is faster overall even though it may show up in some of our benchmarks. What kind of difference are you seeing in the current state?
| frac += sec_per_day * out->days; | ||
| if ((-sfrac % sec_per_day) != 0) { | ||
| out->days = sfrac / sec_per_day - 1; | ||
| sfrac -= sec_per_day * out->days; |
There was a problem hiding this comment.
Is this exactly the same as before? Previously it looks like out->days would be positive but this is now negative?
Sorry if misreading - again just want to be careful as I'm not sure how well our test cases are hitting all of these branches
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
|
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen. |
Fixes (part) of the performance regression seen mentioned in #57951. The main performance improvements are the ~~
refactoring of the "days" parsing logicand the simpler calculation of ifrac.The days parsing refactoring needed to be reverted. I overlooked an issue that happens for td close to int64 bounds there.