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

feat(speedups): use ciso8601 for deserializing datetimes from the gateway #658

Closed
wants to merge 11 commits into from
Closed

Conversation

elenakrittik
Copy link
Contributor

@elenakrittik elenakrittik commented Jul 26, 2022

feat(speedups): use ciso8601 for deserializing datetimes from the gateway

Summary

Resolves #656

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running task lint
    • I have type-checked the code by running task pyright
  • This PR fixes an issue
  • This PR adds something new (e.g. new method or parameters)
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@shiftinv shiftinv added t: enhancement New feature s: needs review Issue/PR is awaiting reviews labels Jul 27, 2022
@shiftinv shiftinv added this to the disnake v2.6 milestone Jul 27, 2022
@onerandomusername
Copy link
Member

@shiftinv did you have an update about the speed on this?

@shiftinv
Copy link
Member

@shiftinv did you have an update about the speed on this?

Yup! Turns out, at least for our purposes (parsing an ISO 8601 timestamp with a +00:00 tz), the builtin datetime.fromisoformat is faster:

$ pyperf timeit --rigorous -s 'dt = "2022-08-13T13:03:23.237000+00:00"; from datetime import datetime; fromisoformat = datetime.fromisoformat' 'fromisoformat(dt)'
.........................................
Mean +- std dev: 86.9 ns +- 4.9 ns
$ pyperf timeit --rigorous -s 'dt = "2022-08-13T13:03:23.237000+00:00"; from ciso8601 import parse_datetime' 'parse_datetime(dt)'
.........................................
Mean +- std dev: 110 ns +- 6 ns
$ pyperf timeit --rigorous -s 'dt = "2022-08-13T13:03:23.237000+00:00"; from ciso8601 import parse_rfc3339' 'parse_rfc3339(dt)'
.........................................
Mean +- std dev: 109 ns +- 7 ns

While there is no guarantee on the API side about the specific timezone present in the timestamp, it should be pretty safe to assume that it'll always be UTC.

It's still noteworthy though that ciso8601 is slightly faster than datetime in cases where the timezone is something other than UTC, but slower when it's missing or UTC:

$ python -c 'import sys; print(sys.version)'
3.10.4 (main, Jun 29 2022, 12:14:53) [GCC 11.2.0]
$ python -c 'import platform; print(platform.platform())'
Linux-5.17.15-76051715-generic-x86_64-with-glibc2.35

# `+00:00`, same as above
$ pyperf timeit --rigorous -s 'dt = "2022-08-13T13:03:23.237000+00:00"; from datetime import datetime; fromisoformat = datetime.fromisoformat' 'fromisoformat(dt)'
.........................................
Mean +- std dev: 86.9 ns +- 4.9 ns
$ pyperf timeit --rigorous -s 'dt = "2022-08-13T13:03:23.237000+00:00"; from ciso8601 import parse_datetime' 'parse_datetime(dt)'
.........................................
Mean +- std dev: 110 ns +- 6 ns

# `-06:00`
$ pyperf timeit --rigorous -s 'dt = "2022-08-13T13:03:23.237000-06:00"; from datetime import datetime; fromisoformat = datetime.fromisoformat' 'fromisoformat(dt)'
.........................................
Mean +- std dev: 123 ns +- 6 ns
$ pyperf timeit --rigorous -s 'dt = "2022-08-13T13:03:23.237000-06:00"; from ciso8601 import parse_datetime' 'parse_datetime(dt)'
.........................................
Mean +- std dev: 110 ns +- 6 ns

# no timezone
$ pyperf timeit --rigorous -s 'dt = "2022-08-13T13:03:23.237000"; from datetime import datetime; fromisoformat = datetime.fromisoformat' 'fromisoformat(dt)'
.........................................
Mean +- std dev: 75.8 ns +- 2.4 ns
$ pyperf timeit --rigorous -s 'dt = "2022-08-13T13:03:23.237000"; from ciso8601 import parse_datetime' 'parse_datetime(dt)'
.........................................
Mean +- std dev: 101 ns +- 5 ns

In summary, while I appreciate the idea (#656) of improving performance through new packages, it appears that this one won't result in performance improvements in most cases (and likely lower performance slightly).
This issue also outlines the differences: closeio/ciso8601#64

@onerandomusername
Copy link
Member

That's a disappointing discovery shiftinv, but thank you @ItsAleph for putting in the work and implementing this feature, even if it doesn't end up providing speed enhancements (which we should've tested before making the issue asking to implement this 😅)

@onerandomusername onerandomusername removed the s: needs review Issue/PR is awaiting reviews label Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: enhancement New feature
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

feat(speedups): use ciso8601 for deserializing datetimes from the gateway
3 participants