-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
gh-110850: Add PyTime_t C API #112135
gh-110850: Add PyTime_t C API #112135
Conversation
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.
A few drive-by comments
Doc/c-api/time.rst
Outdated
* Monotonic clock | ||
* Performance counter | ||
|
||
Internally, operations like ``(t * k / q)`` with integers are implemented in a |
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.
This paragraph made more sense for someone reading the code than someone reading the docs and looking to use this API.
@colesbury: I copied the long comment from the private header file as the documentation. I already had doubts about mentioning details of the internal C API which are not exposed in the public C API yet. I will reorganize the "public" documentation to only leave documentation relevant to the few functions made public. |
Doc/c-api/time.rst
Outdated
the returned value is undefined, so that only the difference between the | ||
results of consecutive calls is valid. | ||
|
||
If reading the clock fails, silently ignore the error and return 0. |
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.
We're adding new functions here, so shouldn't we let them report errors? I don't think silent failures are what you'd want for a new API. Why not return -1
(without exception) in the case of failures for all three get-time functions? Then users can decide themselves whether they want to handle or ignore the error. Or do you consider 0
a safe error indicator?
Not being able to report errors is not safe. The risk of error while reading time is low, whereas the cost of having to report errors to the caller while reading time is high. It makes code way more complicated. There are some specific C extensions where we cannot even report errors, since the API doesn't let to report errors to the caller. One example if the _PyTime_t deadline = _PyTime_Add(_PyTime_GetMonotonicClock(), timeout);
_PyTime_AsTimespec_clamp(deadline, &abs_timeout); It uses In existing code which doesn't use the Since I designed and implemented PEP 418 in 2012, I paid a lot of attention to bug reports about "reading time". I only recall on bug report in Red Hat bug tracker of an application running in a sandbox and the sandbox blocked syscalls to read time. Well, it was more a sandbox configuration issue, than a bug in Python. When I implemented PEP 418, I was scared of regressions, so I added a read the monotonic clock at Python startup: so Python was able to check for "read time" error at one place, and only one place. That's how I discovered the sandbox issue. Later, I removed these checks at startup. I decided that it's not worth it.
|
@colesbury @scoder @serhiy-storchaka: Please review the updated PR.
|
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.
A few suggestions about the documentation below.
@colesbury: Thanks, I applied your suggestions. Does the updated doc look better? |
Functions | ||
--------- | ||
|
||
.. c:function:: double PyTime_AsSecondsDouble(PyTime_t t) |
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.
What is the rounding mode? Do we need an argument for specifying it?
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 don't know how to implement a rounding method on this function. I don't know how numbers are rounded. The function starts to lose precision at 2**53
nanoseconds:
>>> x=2**53+1; y=int(_testcapi.PyTime_AsSecondsDouble(x)*1e9); f"ulp(x)={math.ulp(x)} and {y-x=}"
'ulp(x)=2.0 and y-x=-1'
In short, the function is implemented as:
def PyTime_AsSecondsDouble(t): return float(t) / 1e9
I have too many things on my plate. I prefer to close the PR for now. |
Add PyTime_t API: * PyTime_t type. * PyTime_MIN and PyTime_MAX constants. * PyTime_AsSecondsDouble(), PyTime_Monotonic(), PyTime_PerfCounter() and PyTime_GetSystemClock() functions. Changes: * Add Include/cpython/pytime.h header. * Add Modules/_testcapi/time.c and Lib/test/test_capi/test_time.py tests on these new functions. * Rename _PyTime_GetMonotonicClock() to PyTime_Monotonic(). * Rename _PyTime_GetPerfCounter() to PyTime_PerfCounter(). * Rename _PyTime_GetSystemClock() to PyTime_Time(). * Rename _PyTime_AsSecondsDouble() to PyTime_AsSecondsDouble(). * Rename _PyTime_MIN to PyTime_MIN. * Rename _PyTime_MAX to PyTime_MAX.
I reopened my PR since #110850 issue is now marked as a release blocker issue. I rebased my PR on main. |
I created a C API Working Group issue: capi-workgroup/decisions#8 |
if (PyModule_AddObject(m, "PyTime_MIN", PyLong_FromLongLong(PyTime_MIN)) < 0) { | ||
return 1; | ||
} | ||
if (PyModule_AddObject(m, "PyTime_MAX", PyLong_FromLongLong(PyTime_MAX)) < 0) { | ||
return 1; | ||
} |
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.
if (PyModule_AddObject(m, "PyTime_MIN", PyLong_FromLongLong(PyTime_MIN)) < 0) { | |
return 1; | |
} | |
if (PyModule_AddObject(m, "PyTime_MAX", PyLong_FromLongLong(PyTime_MAX)) < 0) { | |
return 1; | |
} | |
if (PyModule_Add(m, "PyTime_MIN", PyLong_FromLongLong(PyTime_MIN)) < 0) { | |
return -1; | |
} | |
if (PyModule_Add(m, "PyTime_MAX", PyLong_FromLongLong(PyTime_MAX)) < 0) { | |
return -1; | |
} |
A timestamp or duration in nanoseconds represented as a 64-bit signed | ||
integer. |
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.
At the C-API WG issue, you say that the units are supposed to be an implementation detail.
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.
Oh, now I'm confused. I don't recall the exact history of this PR that I wrote 3 months ago.
When I wrote that API 10 years ago, I tried hard to not expose the "unit" (1 nanosecond). In practice, it was always 1 nanosecond, even if on Windows, clocks only have a resolution of 100 nanoseconds in the best case.
Maybe it's time to stick to nanoseconds. On Linux, timespec
, clock_gettime()
, nanosleep()
have a resolution of 1 nanosecond. I don't know any operating system API using a better resolution.
So well, we should stick to 1 nanosecond to avoid any confusion. So creating a PyTime_t
of 1 nanosecond is just PyTime_t one_ns = 1;
. No "from nanoseconds" or "to nanoseconds" are needed. It makes the API simpler to use (so less error prone).
Add PyTime_t API:
📚 Documentation preview 📚: https://cpython-previews--112135.org.readthedocs.build/