Skip to content
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

Adds conversion between SystemTime and datetime #3736

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

Tpt
Copy link
Contributor

@Tpt Tpt commented Jan 11, 2024

No description provided.

Copy link

codspeed-hq bot commented Jan 11, 2024

CodSpeed Performance Report

Merging #3736 will degrade performances by 11.37%

Comparing Tpt:tpt/systemtime (f835449) with main (5f320d7)

Summary

❌ 3 regressions
✅ 75 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main Tpt:tpt/systemtime Change
extract_float_extract_success 436.7 ns 492.2 ns -11.29%
extract_float_downcast_success 446.7 ns 502.2 ns -11.06%
clean_gilpool_new 649.4 ns 732.8 ns -11.37%

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review on my side here. Overall this makes sense and is well implemented, thanks!

My main thing I wonder is about the timestamp accuracy. If there's a range where the floating-point APIs are accurate enough, we might want to consider using that, as it's probably faster than going through an intermediate timedelta.

src/conversions/std/time.rs Outdated Show resolved Hide resolved
src/conversions/std/time.rs Show resolved Hide resolved
src/conversions/std/time.rs Outdated Show resolved Hide resolved
@Tpt
Copy link
Contributor Author

Tpt commented Jan 27, 2024

Thank you for your review!

My main thing I wonder is about the timestamp accuracy. If there's a range where the floating-point APIs are accurate enough, we might want to consider using that, as it's probably faster than going through an intermediate timedelta.

That's a great point. Indeed for Python -> Rust getting a flowing point timestamp (with a Python function call there is no function in the C API for that) might work better. For the Rust -> Python code the fromtimestamp implementation in C Python relies on the C localtime and gmtime functions so, they might get restricted to up to 2038 on some platforms. I did not felt like trying to know what the bound is and preferred to use a safer fallback. Would you be ok if I leave the implementation as it is with a TODO to investigate using timestamps directly for a possible speedup?

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks great to me, sorry for the slow review cycles on this one.

Would you be ok if I leave the implementation as it is with a TODO to investigate using timestamps directly for a possible speedup?

Yes, I think that makes sense; given the edge cases around the use of timestamps I think it makes sense that we get this base known-good implementation in, and can review the performance enhancement in its own diff.

@davidhewitt davidhewitt added this pull request to the merge queue Feb 1, 2024
Merged via the queue into PyO3:main with commit 5773554 Feb 1, 2024
37 of 38 checks passed
@Tpt Tpt deleted the tpt/systemtime branch February 1, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants