GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-110850: Add PyTime_t C API by vstinner · Pull Request #112135 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Nov 16, 2023 •
edited by github-actions Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Add PyTime_t API:

  • PyTime_t type.
  • PyTime_MIN and PyTime_MAX constants.
  • PyTime_AsSecondsDouble(), PyTime_GetMonotonicClock(), PyTime_GetPerfCounter() and PyTime_GetSystemClock() functions.
  • Issue: Please expose _PyTime_t, _PyTime_FromTimeval, and _PyTime_AsSecondsDouble as public APIs #110850

📚 Documentation preview 📚: https://cpython-previews--112135.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
vstinner requested review from abalkin, kumaraditya303, pablogsal, pganssle and rhettinger as code owners November 16, 2023 00:07
@bedevere-app bedevere-app Bot added the awaiting core review label Nov 16, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Nov 16, 2023
Please expose _PyTime_t, _PyTime_FromTimeval, and _PyTime_AsSecondsDouble as public APIs #110850
Closed
@vstinner vstinner added the topic-C-API label Nov 16, 2023
@vstinner
vstinner force-pushed the pytime_api branch from 4c0d046 to e9a6c02 Compare November 16, 2023 00:08
@vstinner vstinner mentioned this pull request Nov 16, 2023
Functions which cannot fail capi-workgroup/api-evolution#43
Closed
@rhettinger
rhettinger removed their request for review November 25, 2023 22:24
colesbury
colesbury reviewed Nov 29, 2023
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/c-api/time.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/c-api/time.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/c-api/time.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/c-api/time.rst Outdated
* Monotonic clock
* Performance counter

Internally, operations like ``(t * k / q)`` with integers are implemented in a

@colesbury colesbury Nov 29, 2023

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/c-api/time.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 30, 2023

Copy link
Copy Markdown
Member Author

@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.

encukou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka
serhiy-storchaka self-requested a review November 30, 2023 13:03
scoder
scoder reviewed Dec 14, 2023
View reviewed changes
Comment thread Doc/c-api/time.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread 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.

@scoder scoder Dec 14, 2023

Copy link
Copy Markdown
Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner

vstinner commented Dec 14, 2023

Copy link
Copy Markdown
Member Author

@scoder:

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 PyThread_acquire_lock_timed() API. For example, one implementation uses:

        _PyTime_t deadline = _PyTime_Add(_PyTime_GetMonotonicClock(), timeout);
        _PyTime_AsTimespec_clamp(deadline, &abs_timeout);

It uses _PyTime_Add() and the _clamp function variant which clamp the result into [_PyTime_MIN; _PyTime_MAX] range, rather than raising an exception and report an error.

In existing code which doesn't use the PyTime API, do you know any code which actually check for errors? If yes, how are errors handled?

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.

commit ae6cd7cfdab0599139002c526953d907696d9eef
Author: Victor Stinner <[email protected]>
Date:   Mon Nov 16 16:08:05 2020 +0100

    bpo-37205: time.time() cannot fail with fatal error (GH-23314)
    
    time.time(), time.perf_counter() and time.monotonic() functions can
    no longer fail with a Python fatal error, instead raise a regular
    Python exception on failure.
    
    Remove _PyTime_Init(): don't check system, monotonic and perf counter
    clocks at startup anymore.
    
    On error, _PyTime_GetSystemClock(), _PyTime_GetMonotonicClock() and
    _PyTime_GetPerfCounter() now silently ignore the error and return 0.
    They cannot fail with a Python fatal error anymore.
    
    Add py_mach_timebase_info() and win_perf_counter_frequency()
    sub-functions.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner force-pushed the pytime_api branch from ecaef96 to 1ecfe83 Compare December 14, 2023 16:13
@vstinner
vstinner requested a review from a team as a code owner December 14, 2023 16:13
@vstinner
vstinner force-pushed the pytime_api branch from 1ecfe83 to 6679b9d Compare December 14, 2023 16:19
@vstinner

vstinner commented Dec 14, 2023

Copy link
Copy Markdown
Member Author

@colesbury @scoder @serhiy-storchaka: Please review the updated PR.

  • I cleaned up the C API doc.
  • I renamed the functions to use shorter names.
  • I added dedicated tests to test_capi.test_time.
  • I kept aliases to old names for this PR to make the PR shorter and easier to review. I will remove the private aliases in a follow-up PR once this once is merged.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner force-pushed the pytime_api branch from 6679b9d to d73482e Compare December 14, 2023 16:23
colesbury
colesbury reviewed Dec 14, 2023
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/c-api/time.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/c-api/time.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/c-api/time.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Dec 14, 2023

Copy link
Copy Markdown
Member Author

A few suggestions about the documentation below.

@colesbury: Thanks, I applied your suggestions. Does the updated doc look better?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka
serhiy-storchaka approved these changes Dec 15, 2023
View reviewed changes
Comment thread Doc/c-api/time.rst
Functions
---------

.. c:function:: double PyTime_AsSecondsDouble(PyTime_t t)

@serhiy-storchaka serhiy-storchaka Dec 15, 2023

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vstinner vstinner Dec 15, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Dec 15, 2023
@vstinner

vstinner commented Dec 20, 2023

Copy link
Copy Markdown
Member Author

I have too many things on my plate. I prefer to close the PR for now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner closed this Dec 20, 2023
@vstinner
vstinner deleted the pytime_api branch December 20, 2023 12:41
@vstinner
vstinner restored the pytime_api branch January 18, 2024 16:17
@vstinner vstinner reopened this Jan 18, 2024
vstinner added 3 commits January 18, 2024 17:17
@vstinner
pythongh-110850: Add PyTime_t C API
6c40241
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.
@vstinner
Address Sam's review: update doc
d8cf97d
@vstinner
Avoid reference to epoch_
c6f5be9
@vstinner
vstinner force-pushed the pytime_api branch from 0368bb6 to c6f5be9 Compare January 18, 2024 16:19
@vstinner

vstinner commented Jan 18, 2024

Copy link
Copy Markdown
Member Author

I reopened my PR since #110850 issue is now marked as a release blocker issue. I rebased my PR on main.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Jan 18, 2024

Copy link
Copy Markdown
Member Author

I created a C API Working Group issue: capi-workgroup/decisions#8

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner mentioned this pull request Jan 30, 2024
gh-110850: Add PyTime C API (PyTime_t type) capi-workgroup/decisions#8
Closed
erlend-aasland
erlend-aasland reviewed Feb 8, 2024
View reviewed changes
Comment thread Modules/_testcapi/time.c
Comment on lines +87 to +92
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;
}

@erlend-aasland erlend-aasland Feb 8, 2024

Copy link
Copy Markdown
Contributor

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.

Suggested change
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;
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
encukou
encukou reviewed Feb 8, 2024
View reviewed changes
Comment thread Doc/c-api/time.rst
Comment on lines +20 to +21
A timestamp or duration in nanoseconds represented as a 64-bit signed
integer.

@encukou encukou Feb 8, 2024

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vstinner vstinner Feb 8, 2024

Copy link
Copy Markdown
Member Author

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

colesbury and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@encukou encukou mentioned this pull request Feb 9, 2024
gh-110850: Add PyTime_t C API #115215
Merged
@vstinner

vstinner commented Feb 12, 2024

Copy link
Copy Markdown
Member Author

I close this PR: the C API Working Group is against an API which cannot fail.

Instead, @encukou modified to PR as PR gh-115215 which can report errors.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner closed this Feb 12, 2024
@vstinner
vstinner deleted the pytime_api branch February 12, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou left review comments
@scoder scoder scoder left review comments
@colesbury colesbury colesbury left review comments
@erlend-aasland erlend-aasland erlend-aasland left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@pablogsal pablogsal Awaiting requested review from pablogsal
@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner
@abalkin abalkin Awaiting requested review from abalkin
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting merge topic-C-API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@vstinner @encukou @scoder @colesbury @serhiy-storchaka @erlend-aasland
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.