Rewrite pytime.h to work on nanoseconds #66315
Description
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:
assignee = None
closed_at = <Date 2015-03-30.09:52:22.799>
created_at = <Date 2014-07-31.23:08:13.499>
labels = ['type-feature']
title = 'Rewrite pytime.h to work on nanoseconds'
updated_at = <Date 2019-07-04.11:05:28.158>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2019-07-04.11:05:28.158>
actor = 'jdemeyer'
assignee = 'none'
closed = True
closed_date = <Date 2015-03-30.09:52:22.799>
closer = 'vstinner'
components = []
creation = <Date 2014-07-31.23:08:13.499>
creator = 'vstinner'
dependencies = []
files = ['36186', '36197', '36200', '36538']
hgrepos = []
issue_num = 22117
keywords = ['patch']
message_count = 49.0
messages = ['224452', '224460', '224461', '224527', '224530', '224560', '224570', '224571', '225861', '225906', '225918', '225919', '225920', '226384', '239395', '239402', '239405', '239415', '239440', '239443', '239448', '239451', '239452', '239453', '239528', '239530', '239531', '239542', '239552', '239558', '239559', '239566', '239568', '239579', '239580', '239590', '239718', '239823', '239969', '240181', '331983', '331990', '343914', '343915', '344711', '344712', '344717', '344718', '344737']
nosy_count = 10.0
nosy_names = ['tim.peters', 'loewis', 'belopolsky', 'pitrou', 'vstinner', 'python-dev', 'serhiy.storchaka', 'jdemeyer', 'koobs', 'miss-islington']
pr_nums = ['1106', '11190', '11191', '11636']
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue22117'
versions = ['Python 3.5']
Activity
vstinner commentedon Jul 31, 2014
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:
vstinner commentedon Aug 1, 2014
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:
http://legacy.python.org/dev/peps/pep-0418/#system-time
vstinner commentedon Aug 1, 2014
The effective* resolution
vstinner commentedon Aug 1, 2014
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.
vstinner commentedon Aug 2, 2014
timespec-3.patch is quite large:
Include/pyport.h | 14 +
Include/pytime.h | 159 +++++++----
Lib/test/test_time.py | 340 +++++++++++++++++++------
Modules/_datetimemodule.c | 40 ++
Modules/_ssl.c | 31 +-
Modules/_testcapimodule.c | 238 ++++++++++++++++-
Modules/_threadmodule.c | 196 +++++++-------
Modules/gcmodule.c | 15 -
Modules/posixmodule.c | 24 -
Modules/selectmodule.c | 36 --
Modules/signalmodule.c | 12
Modules/socketmodule.c | 206 +++++++++------
Modules/socketmodule.h | 6
Modules/timemodule.c | 22 -
Python/pythonrun.c | 3
Python/pytime.c | 616 +++++++++++++++++++++++++++++++++-------------
16 files changed, 1371 insertions(+), 587 deletions(-)
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.
loewis commentedon Aug 2, 2014
What problem does this solve?
vstinner commentedon Aug 2, 2014
Oh, i should read what i wrote before pushing the submit button. The last
part is"remove the old functions"...
vstinner commentedon Aug 2, 2014
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
that the behaviour on overflow is undefined. I should test each function.
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
C code which has fewer constraints (we don't need a "full" API). I expect
that C code is more concerned by the resolution because C code is faster.
You need to recompute timeout on EINTR (see the PEP-475 which is still a
draft). I don't want to loose precision and I want to round correctly.
My main usecase is to compute a timeout from two timestamps of a monotonic
clock.
IMO it's better to use PyTimeSpec structure everywhere to reuse the code
which is now well tested (by unit tests) and make the code more consistent.
For example, the datetime module rounds currently using the "down" method,
whereas it needs to round "floor" in fact. I saw this issue when I changed
the code to use PyTimeSpec in all functions.
I agree that my patch is large, especially the new code in pytime.c. I
would like to hide the complexity in functions, the API should be
simple. Do you think that changes in modules like time, socket or select
make the code more complex? I don't know if it's worth it.
vstinner commentedon Aug 25, 2014
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:
1677-09-21 00:12:43.145224
2262-04-11 23:47:16.854776
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).
vstinner commentedon Aug 25, 2014
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."
pitrou commentedon Aug 26, 2014
Do we have 64-bit integers on all architectures?
loewis commentedon Aug 26, 2014
Am 26.08.14 15:32, schrieb Antoine Pitrou:
On all "supported" architectures, yes. gcc supports long long
everywhere, using a library if hardware support is not feasible.
Visual C++ supports __int64 on all targets. Other compilers typically
strive for compatibility with either of these two.
vstinner commentedon Aug 26, 2014
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? :-)
vstinner commentedon Sep 4, 2014
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 ++++++-----
Modules/_datetimemodule.c | 43 ++--
Modules/_testcapimodule.c | 25 +-
Modules/_threadmodule.c | 156 +++++++-------
Modules/gcmodule.c | 12 -
Modules/posixmodule.c | 29 +-
Modules/selectmodule.c | 33 ---
Modules/signalmodule.c | 20 -
Modules/socketmodule.c | 151 ++++++++------
Modules/socketmodule.h | 2
Modules/timemodule.c | 40 ++-
Python/pytime.c | 480 +++++++++++++++++++++++++++++-----------------
12 files changed, 645 insertions(+), 471 deletions(-)
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.
24 remaining items
python-dev commentedon Mar 31, 2015
New changeset dbc92a254173 by Victor Stinner in branch 'default':
Issue bpo-22117: Fix integer overflow check in socket_parse_timeout() on Windows
https://hg.python.org/cpython/rev/dbc92a254173
python-dev commentedon Apr 1, 2015
New changeset 65ac8e587bb0 by Victor Stinner in branch 'default':
Issue bpo-22117, issue bpo-23485: Fix _PyTime_AsMilliseconds() and
https://hg.python.org/cpython/rev/65ac8e587bb0
python-dev commentedon Apr 3, 2015
New changeset d976683671ba by Victor Stinner in branch 'default':
Issue bpo-22117: Add a new _PyTime_FromSeconds() function
https://hg.python.org/cpython/rev/d976683671ba
python-dev commentedon Apr 6, 2015
New changeset af664f48b9b8 by Victor Stinner in branch 'default':
Issue bpo-22117: Fix sock_call_ex() for non-blocking socket
https://hg.python.org/cpython/rev/af664f48b9b8
vstinner commentedon Dec 17, 2018
New changeset 3ab064e by Victor Stinner in branch 'master':
bpo-23451: Update time.monotonic() documentation (GH-11190)
3ab064e
miss-islington commentedon Dec 17, 2018
New changeset c367d52 by Miss Islington (bot) in branch '3.7':
bpo-23451: Update time.monotonic() documentation (GH-11190)
c367d52
jdemeyer commentedon May 29, 2019
Can somebody here explain the meaning of the comment in test_gdb.py
# Tested function must not be defined with METH_NOARGS or METH_O,
# otherwise call_function() doesn't call PyCFunction_Call()
This test is breaking with PEP-590, see #13185
jdemeyer commentedon May 29, 2019
Petr claims to have a fix, #13665
vstinner commentedon Jun 5, 2019
@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 ;-)
jdemeyer commentedon Jun 5, 2019
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.
serhiy-storchaka commentedon Jun 5, 2019
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.
jdemeyer commentedon Jun 5, 2019
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.
vstinner commentedon Jun 5, 2019
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.