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
Rewrite pytime.h to work on nanoseconds #66315
Comments
To prepare Python to add support of monotonic clock in pytime.h, I propose to rewrite pytime.h to use a new _PyTimeSpec structure which has a resolution of 1 nanosecond on only work on integers. Currently, pytime.h uses a _PyTime_timeval structure which has a solution of 1 microsecond and works internally on floating point numbers. Computing the difference between two timestamps may loose precision. The tv_nsec field of _PyTimeSpec must be in the range [0; 999999999], even for negative numbers. For example, -1 ns is stored as (-1, 999999999). This property makes the code more complex, especially to round correctly. The new API is based on the idea that all timestamps must be stored as _PyTimeSpec. Convert a value into a _PyTimeSpec:
Convert a _PyTimeSpec timestamp into a value:
Operations on _PyTimeSpec:
I removed high-level functions like _PyTime_ObjectToTimeval(): you should now combine _PyTimeSpec_from_object() with _PyTimeSpec_to_timeval(). I did this to not multiply the number of functions, because I want to test all functions in unit tests and have a short API. I tried to enhance detecton of overflow. I didn't review carefuly my patch yet. I'm not sure that all calls check for error and handle exceptions correctly. Only the following functions raise an exception on error:
Other functions sets the minimum/maximum value in case of underflow/overflow. So even if you don't check for errors, the behaviour on overflow should be acceptable. The new _PyTimeSpec_Init() function checks that the system clock works, so _PyTimeSpec_get_time() and _PyTimeSpec_get_time_info() don't need to check for error. In fact, these functions call Py_FatalError() on error, but it should never happen. This idea was proposed in the issue bpo-22043 to check if the monotonic clock is working. Maybe we can avoid checking the system clock in _PyTimeSpec_Init() and only check the monotonic clock. I hope that all platforms have a working system clock! The oppoosite would be a huge issue. The patch removes function which are exposed in the stable ABI. Since the functions are private, name starting with ("_Py"), I consider that it's ok to remove them. The functions are replaced with new functions which a have a new name. I excluded pytime.h from the stable ABI. I don't know why private functions were part of the stable ABI :-/ The patch comes with unit tests for each function. I didn't implement handling of overflow and underflow in _PyTimeSpec_add() and _PyTimeSpec_sub() yet. But I implemented detection for overflow for the simple case. See also: |
Oh, I forgot to mention that the patch of the issue bpo-22043 also changes _PyTimeSpec_get_time() to use clock_gettime(CLOCK_REALTIME) which has a resolution of 1 nanosecond. _PyTimeSpec_get_time() already uses GetSystemTimeAsFileTime() which has a resolution of 100 nanosecond. See also the issue bpo-19007 "precise time.time() under Windows 8: use GetSystemTimePreciseAsFileTime". I'm talking about the resolution of the C structure. The effive resolution can be much worse than that. For example, the resolution measured in Python of clock_gettime(CLOCK_REALTIME) is closer to 160 nanoseconds (on my laptop) than 1 nanoescond. See the "Python Resolution" column the second table of: |
The effective* resolution |
Here is a more complete patch (version 3). It uses _PyTimeSpec in more functions. I tested the patch on Linux, Windows, FreeBSD, OpenBSD. I was surprised to find bugs. For example, Windows has a time_t larger than long, whereas OpenBSD 5.4 (on 64 bit) has a time_t smaller than long. |
timespec-3.patch is quite large: Include/pyport.h | 14 + Tell me if you prefer to review shorter patches. I can try to only add new functions, then use new functions, and finally remove new functions. I wanted to keep changes in a single patch at least one to show how the new API is used. |
What problem does this solve? |
Oh, i should read what i wrote before pushing the submit button. The last |
Le samedi 2 août 2014, Martin v. Löwis <report@bugs.python.org> a écrit :
My patch detects overflows which are not detected yet. Currently i guess I also want a nanosecond resolution. It's the same motivation than the PEP-410, except that the patch doesn't touch the Python API, I only care of the My main usecase is to compute a timeout from two timestamps of a monotonic IMO it's better to use PyTimeSpec structure everywhere to reuse the code I agree that my patch is large, especially the new code in pytime.c. I |
Instead of a complex structure, we can use a 64-bit signed integer to store a number of nanoseconds. For a UNIX epoch, nanoseconds since January 1st 1970, the min/max are: The Linux kernel is going to use 64-bit integer even on 32-bit CPU to store timestamps, to simplify the code (to avoid the structure). |
Read this article: http://lwn.net/Articles/607741/ "One of the first changes merged for 3.17 is to simply get rid of the non-scalar form of ktime_t and force all architectures to use the 64-bit nanosecond count representation. This change may slow things down on 32-bit systems; in particular, conversions from other time formats may be significantly slower. But, as noted in the changelog, the ARM and x86 architectures were already using the scalar format anyway, so they will not get any slower." |
Do we have 64-bit integers on all architectures? |
Am 26.08.14 15:32, schrieb Antoine Pitrou:
On all "supported" architectures, yes. gcc supports long long |
That's a good question! Visual Studio provides __int64. GCC provides "long long" (64 bit on 32 bit platform). I guess that ICC also supports int64_t. It would be a shame to not support 64-bit integers in 2014, no? :-) |
And now, something completly different. nanosec-wip.patch is a work-in-progress patch to use a unified (64 bits) integer type to store a timestamp: _PyTime_t. The type is written to be opaque, the unit is undefined, you must use functions to convert from and to this type. Well, in fact it's just a number of nanoseconds. The patch is large: Include/pytime.h | 125 ++++++----- Conversion functions require a rounding mode. I didn't update all tests yet. For example, test_datetime crash, test_time and test_threading fail. There are also 3 remaining FIXME for myself in pytime.c. |
New changeset 2309597e7a00 by Victor Stinner in branch 'default': |
New changeset f64d0b99d405 by Victor Stinner in branch 'default': |
New changeset b0b4c4d365b1 by Victor Stinner in branch 'default': |
New changeset 0378c10ba164 by Victor Stinner in branch 'default': |
New changeset 45d9093d259d by Victor Stinner in branch 'default': New changeset a88735cbeb50 by Victor Stinner in branch 'default': New changeset abf38a17d3a8 by Victor Stinner in branch 'default': New changeset a3c5e05d2cef by Victor Stinner in branch 'default': |
New changeset f841d3bc30ee by Victor Stinner in branch 'default': New changeset ae551abe398d by Victor Stinner in branch 'default': New changeset c0c7d258c1ed by Victor Stinner in branch 'default': |
New changeset 7605d9d262ca by Victor Stinner in branch 'default': New changeset d1ef5ff79125 by Victor Stinner in branch 'default': |
New changeset adbc9e6162fe by Victor Stinner in branch 'default': |
New changeset 930be74bbae5 by Victor Stinner in branch 'default': New changeset 5aa39b88bd55 by Victor Stinner in branch 'default': New changeset 47123ac83733 by Victor Stinner in branch 'default': |
New changeset 0850452048ec by Victor Stinner in branch 'default': |
New changeset e93eeadef0c3 by Victor Stinner in branch 'default': New changeset b16fc95b66e4 by Victor Stinner in branch 'default': |
New changeset 626575d756da by Victor Stinner in branch 'default': New changeset c26b4b85dfc4 by Victor Stinner in branch 'default': |
New changeset 6bae19f4e7b2 by Victor Stinner in branch 'default': |
New changeset d938533b43f7 by Victor Stinner in branch 'default': New changeset 49d3ff81f31f by Victor Stinner in branch 'default': |
I rewrite the implementation of this issue 2 or 3 times. At the end, I decided to rewrite it again with a more incremental approach: add the new API, use the new API, and then drop slowly the old API. I was used to rewrite the implementation multiple times to fix issues in the API and enhance the implementation. The new API now handles better rounding with more unit tests. The datetime module only uses the new API for the datetime.datetime() constructor. It's not used for datetime.datetime.fromtimestamp(), because _PyTime_t is limited to a range of [-292 years; 292 years]. The datetime module supports a much large range: year in range [1; 9999]. So some parts of the old API will survive for the datetime module. |
New changeset f6b41566ca28 by Victor Stinner in branch 'default': New changeset 0a8015a4ff97 by Victor Stinner in branch 'default': New changeset 350eb1ca561a by Victor Stinner in branch 'default': |
Ok, I'm quite API with the new API and I have what I need to rework the select module for the PEP-475, so I close this issue. |
May be rename _PyTime_AsTimeval_impl() to _PyTime_AsTimeval_noraise() and check a result to raise an exception in _PyTime_AsTimeval()? |
Ah yes correct, can you modify directly the code please? |
Hum, conversion from Python float to _PyTime_t is not rounded as expected on x86 Ubuntu Shared 3.x. Example: ====================================================================== Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_time.py", line 779, in test_FromSecondsObject
self.assertEqual(PyTime_FromSecondsObject(obj, rnd), ts)
AssertionError: 999 != 1000 ====================================================================== Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_time.py", line 779, in test_FromSecondsObject
self.assertEqual(PyTime_FromSecondsObject(obj, rnd), ts)
AssertionError: 4194304000000000 != 4194304000000001 |
New changeset e00b581a65ec by Victor Stinner in branch 'default': |
Oh, test_time now pass on "x86 Ubuntu Shared 3.x". It looks like the volatile keyword was enough to fix rounding issues, cool :-) |
New changeset dbc92a254173 by Victor Stinner in branch 'default': |
New changeset 65ac8e587bb0 by Victor Stinner in branch 'default': |
New changeset d976683671ba by Victor Stinner in branch 'default': |
New changeset af664f48b9b8 by Victor Stinner in branch 'default': |
Petr claims to have a fix, #13665 |
@jeroen Demeyer: Both mentionned PRs are merged, so I understand that you found your answer. Commenting closed issues is not the most efficient way to get an answer ;-) |
Serious question: why is that? I got an email from bugs.python.org with your comment, so why should commenting on closed issues be any worse than commenting on open issues? Especially if the people on the issue are still active core developers. |
Because this reduces the number of people which can notice your comment. Active core developers receive too much messages. Currently I have 473 unread messages from b.p.o (out of 22425) and 299 unread messages from GitHub (out of 20476). And this is only for open and recently closed issues (messages for closed issues are manually moved to other folders). There are also separate folders for other projects, mailing lists, etc. It is not surprising if I miss some messages. |
Right, but my question was very specifically about a test added for this issue. So asking it here first made sense to me. If nobody would reply, I could still ask somewhere else. In the end, Petr solved the problem anyway, so the question is irrelevant now. |
This issue is about nanoseconds. I looked again and I found the commit https://hg.python.org/cpython/rev/b0b4c4d365b1 related to this issue. So to reply to your question, honestly, I don't recall why I had to replace sleep() with gmtime(): all I know is written in the comment. But Python internals changed *a lot* since 2015... My comment is more general: closed issues are hidden from bugs.python.org home page, so only people in the nosy list get a notification. The discussion is "hidden". Since issue is 4 years old, some people in the nosy list no longer contribute to Python. Moreover, the bug tracker is usually not a good place to ask a question. I prefer to only use it to discuss bugs. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: