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_TimeUnchecked() function by vstinner · Pull Request #115973 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Feb 26, 2024 •
edited by github-actions Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Add functions:

  • PyTime_MonotonicUnchecked()
  • PyTime_PerfCounterUnchecked()
  • PyTime_TimeUnchecked()

Python now fails with a fatal error at startup if reading the system clock, the monotonic clock or the perf counter clock fails.

Add py_perf_counter() function to factorize code.

  • Issue: Please expose _PyTime_t, _PyTime_FromTimeval, and _PyTime_AsSecondsDouble as public APIs #110850

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

Sorry, something went wrong.

Uh oh!

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

All reactions
@vstinner
pythongh-110850: Add PyTime_TimeUnchecked() function
ab0d8f6
Add functions:

* PyTime_MonotonicUnchecked()
* PyTime_PerfCounterUnchecked()
* PyTime_TimeUnchecked()

Python now fails with a fatal error at startup if reading the system
clock, the monotonic clock or the perf counter clock fails.

Add py_perf_counter() function to factorize code.
@vstinner
vstinner requested review from abalkin, ericsnowcurrently and pganssle as code owners February 26, 2024 21:23
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 26, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Feb 26, 2024
Please expose _PyTime_t, _PyTime_FromTimeval, and _PyTime_AsSecondsDouble as public APIs #110850
Closed
@vstinner vstinner mentioned this pull request Feb 26, 2024
gh-110850: Add PyTime C API (PyTime_t type) capi-workgroup/decisions#8
Closed
@vstinner

vstinner commented Feb 26, 2024

Copy link
Copy Markdown
Member Author

See discussion capi-workgroup/decisions#8 about this API.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner added 2 commits February 26, 2024 22:26
@vstinner
Mention GIL in the doc
e45aa70
@vstinner
Fix links in doc
425c6d5
@encukou

encukou commented Feb 28, 2024

Copy link
Copy Markdown
Member

OK, I'll repeat my questions here, too.

  • Do we guarantee that you can call these functions before Python is initialized & after it's finalized?

  • What concrete use cases / user needs does this solve that are not solved by <time.h> functions?

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Feb 28, 2024

Copy link
Copy Markdown
Member Author

Do we guarantee that you can call these functions before Python is initialized & after it's finalized?

No function can be called before init/after init, except of these few functions listed there: https://docs.python.org/dev/c-api/init.html

What concrete use cases / user needs does this solve that are not solved by <time.h> functions?

Convenient int64_t format using Unix epoch 1970 in all platforms. Examples:

  • Windows epoch is in 1600
  • Windows and macOS() return a fraction for perf_counter()
  • Unix return a floating point number as timespec
  • There are 3 to 5 implementations per function, it's hard to rewrite them. Python has already a written implementation with tests and is well tested for 10 years. pytime.c is like 1000 lines of C code, "using time.h" is not so straightforward.
All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented Feb 29, 2024

Copy link
Copy Markdown
Member

No function can be called before init/after init, except of these few functions

Similar for holding the GIL, and in my mind, doesn't need GIL and doesn't need the runtime initialized are the same set of functions. I'm not happy about another bit to track in the upcoming list of all Python API.

Convenient int64_t format using Unix epoch 1970 in all platforms.

I know what it does; I'm asking why. Do we know about a use case that needs this, and can't GIL for exception-based error handling?

I also know Cython wraps and re-exports the private API. I'm asking why it's needed. (If we just need to keep Cython going, we can add back the underscored versions, or add these as PyUnstable.)

There are 3 to 5 implementations per function

And when we add a sixth one, we might need to increase the set of errors the Unchecked API silently ignores. I'd like to avoid that.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Feb 29, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

I know what it does; I'm asking why. Do we know about a use case that needs this, and can't GIL for exception-based error handling?

The issue was created for Cython. I cannot check right now how people use Cython.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Feb 29, 2024

Copy link
Copy Markdown
Member Author

Similar for holding the GIL, and in my mind, doesn't need GIL and doesn't need the runtime initialized are the same set of functions. I'm not happy about another bit to track in the upcoming list of all Python API.

The Python C API already has many functions which don't need to hold the GIL. Check fileutils.h functions.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Feb 29, 2024

Copy link
Copy Markdown
Member Author

@scoder: You may reply to the Cython question.

All reactions

Sorry, something went wrong.

Uh oh!

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

@scoder

scoder commented Feb 29, 2024

Copy link
Copy Markdown
Contributor

I also know Cython wraps and re-exports the private API. I'm asking why it's needed.

It's convenient. It gives the same results as Python's time module, in a platform independent way. Exposing the C functions without requiring the GIL makes them available for e.g. timing sections in thread-parallel code.

If you hold the GIL, you can probably (often) afford to keep a reference to time.time et at. around and call that, which is fairly fast except for the object creation. You'd have less of an urge to require a C-API for that. Removing the GIL requirement opens up a much larger plane of applicability.

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

@encukou

encukou commented Mar 1, 2024

Copy link
Copy Markdown
Member

The Python C API already has many functions which don't need to hold the GIL. Check fileutils.h functions.

OK! There are three functions in two fileutils.h files:

  • Py_DecodeLocale
  • Py_EncodeLocale
  • _Py_fopen_obj

None of these are documented to not require the GIL. If they work without the GIL in practice, I consider that an accident of the implementation.

There is also pycore_fileutils.h, but private API is not relevant here.


Thanks for the use cases.
I think they're conflating use cases for perf timers with ones for wall-clock time (where the epoch matters, but GIL overhead seems less of an issue).
As far as I can see, Cython only exposes wall-clock time.

I get that Python exposes the “best” implementation, across platforms, but it still seems to me that Python-style error handling is an appropriate price for that, and <time.h> is a reasonable fallback.

But now I can see myself being convinced.
I still think these changes would be good:

  • The docs should say that a 0 return value always indicates an error. (Where 0 is a correct return value, we should e.g. fudge it and return 1, losing a nanosecond of accuracy.)
  • The docs should say that the value is clamped on overflow
  • Add a comment to the startup check to explicitly mark it as an implementation detail. If and when we port to a platform that doesn't provide the necessary clocks, we should be free to change CPython so that only some parts of it don't work, and remove the check. If third-party code wants the check, it should do it itself.
  • Allow the functions to be called without an initialized runtime. If the implementation needs the runtime, it can check and fail (return 0). (I you don't agree I can argue more about the benefits of merging the ”needs GIL” and “needs runtime” prerequisites. But maybe you'd be willing to humour me here.)
All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Mar 2, 2024

Copy link
Copy Markdown
Member Author

There are three functions in two fileutils.h files

Oh I forgot that most fileutils.h functions are private. It's a good thing 😁

<time.h> is a reasonable fallback

You need more includes to access all functions.

#include "Python.h"
#include <time.h>                 // gmtime_r()
#ifdef HAVE_SYS_TIME_H
#  include <sys/time.h>           // gettimeofday()
#endif
#ifdef MS_WINDOWS
#  include <winsock2.h>           // struct timeval
#endif
#if defined(__APPLE__)
#  include <mach/mach_time.h>     // mach_absolute_time(), mach_timebase_info()
#endif

The basic gettimeofday() is not provided by time.h.

time() is fine if you don't need better resolution than 1 second. Here we are talking about nanoseconds. See PEP 564 and PEP 418 for the rationale.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner mentioned this pull request Mar 5, 2024
gh-110850: Change PyTime C API error handling capi-workgroup/decisions#15
Closed
@vstinner vstinner closed this Mar 19, 2024
@vstinner
vstinner deleted the pytime_unchecked branch March 19, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner
@abalkin abalkin Awaiting requested review from abalkin
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review

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.

3 participants

@vstinner @encukou @scoder
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 01402520 — 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.