-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[mypyc] Add experimental librt.time module with time() #20723
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
Conversation
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
| extra_compile_args=cflags, | ||
| ), | ||
| Extension( | ||
| "librt.time", ["time/librt_time.c"], include_dirs=["."], extra_compile_args=cflags |
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.
add "time" to include_dirs?
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.
It seems fine to requite time/ prefix in includes.
| ["vecs/librt_vecs.h", "vecs/vec_template.c"], | ||
| ["vecs"], | ||
| ), | ||
| ModDesc("librt.time", ["time/librt_time.c"], ["time/librt_time.h"], []), |
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.
also would make sense to add the include dir here.
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.
Currently everything works without the include dir, so I'm leaning towards keeping it as is.
I've seen many performance-critical functions that have calls to
time.time(), which is perhaps not that surprising. I don't want to just add a primitive fortime.time(), since it's often monkey patched, and primitive functions generally can't be monkey patched. Instead, I add thelibrt.timemodule here, which has an efficienttime()function that can be used in performance-critical code (but it can't be monkey patched).In a microbenchmark this was up to 70% faster than using
time.time().I used a lot of coding agent assist. I'm relying on CI to test the Windows implementation.