Skip to content
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

Support for Python3's time.monotonic() #122

Merged
merged 18 commits into from
Sep 28, 2019

Conversation

arizvisa
Copy link
Contributor

This adds support for the monotonic timers in Python3's time module. The basic functions, monotonic and monotonic_ns were the original intention, but it was simple to add the others process_time, process_time_ns, perf_counter, and perf_counter_ns.

I skipped over get_clock_info because it might require a little bit of re-working. So if something in the future depends on this, lmk and I can include it by possibly substituting a named tuple for the namespace that it's supposed to return.

I've only tested this on Windows and Linux as those're the only two platforms that I have shells to. I also needed to modify the configuration a bit to add the CLOCK_* definitions that Python3 also checks for.

I also use the "HAVE_STDINT_H" definition to check for C99 support. I'm also not sure whether these new functions compile with a non-C99 compliant compiler (because who has one anyways).

@stefantalpalaru
Copy link
Collaborator

What do we need in AppVeyor to enable C99 support? PCbuild\build.bat -e fails with Cannot open include file: 'stdint.h': No such file or directory [C:\projects\tauthon\PCbuild\pythoncore.vcxproj].

@arizvisa
Copy link
Contributor Author

Hm. I think we need to use a newer visual studio base image. But I can add a workaround for earlier versions of VS that don't include support for C99. I only have VS2012 and VS2015, so I haven't tested on prior.

One second and I'll add a new commit that should get it to build.

@arizvisa
Copy link
Contributor Author

@stefantalpalaru, I added the fix that should get things working.

But, we actually we might have a small issue. According to the following MSDN docs, we'll need to pump up WIN32_WINNT to Windows Vista in order to use the GetTickCount64(). (WIN32_WINNT in PC/pyconfig.h will need to be set from 0x0500 to 0x0600).

https://docs.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-gettickcount64

This is a requirement from Python3, but it kills our compatibility if we want to support platforms prior to Windows Vista. I might be able to come up with something else to give us nanosecond precision, but the other API I was looking at isn't guaranteed to be monotonically increasing.

Should I drop support for things prior to Vista, or should I try and implement a workaround that could change the semantics?

@cemeyer
Copy link
Collaborator

cemeyer commented Sep 24, 2019

I'd certainly encourage dropping support for Windows XP and earlier.

@stefantalpalaru
Copy link
Collaborator

I think we need to use a newer visual studio base image.

I already switched to the VS 2019 image, with no effect.

Should I drop support for things prior to Vista, or should I try and implement a workaround that could change the semantics?

Up to you. I wouldn't waste time on supporting very old systems, until there's a real demand for it.

…ows Vista in order to support GetTickCount64() used by Modules/pytime.c.
@stefantalpalaru
Copy link
Collaborator

Are there any associated tests upstream?

@arizvisa
Copy link
Contributor Author

Sure. Lemme check about importing them too.

@arizvisa
Copy link
Contributor Author

Okay. Added the monotonic tests. I excluded the clock_get_info tests as a result of the first comment in this PR.

Something which is weird is that on Windows, the nanoseconds seem to be missing. Also time.thread_time_ns and time.process_time_ns seem to also return constants instead of actual timing info. Checking these functions against Python3, however, yields the same results.. So I guess Python3 is could be inadequate here, but nobody has complained about it.

…e deprecateed time.clock() function is enabled on Windows.
@arizvisa
Copy link
Contributor Author

Ok. Running some final tests to see how things look. Please let me know if you catch anything as well.

I'm just kind of going through adding some of these minor things with the hope that tauthon gets more visibility (and hopefully users) so that we can really refine some of these features as we near Python2's eol.

… they test for the Python2 side-effect instead of the Python3 effect.
#define Py_PYMACRO_H

/* Minimum value between x and y */
#define Py_MIN(x, y) (((x) > (y)) ? (y) : (x))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These defines are supposed to eventually replace all the similar ones sprinkled around the code, right? (not requesting a PR change, just making a note that upstream seems to have done that and it's a good idea for a future PR)

$ rg 'define MIN\('
Objects/frameobject.c
12:#define MIN(a, b) ((a) < (b) ? (a) : (b))

Objects/floatobject.c
15:#define MIN(x, y) ((x) < (y) ? (x) : (y))

Objects/longobject.c
32:#define MIN(x, y) ((x) > (y) ? (y) : (x))

Objects/stringlib/localeutil.h
9:#define MIN(x, y) ((x) < (y) ? (x) : (y))

Modules/bz2module.c
76:#define MIN(X, Y) (((X) < (Y)) ? (X) : (Y))

Modules/_cursesmodule.c
167:#define MIN(x,y) ((x) < (y) ? (x) : (y))

Modules/_multiprocessing/multiprocessing.h
185:#  define MIN(x, y) ((x) < (y) ? x : y)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah. I believe it to be so.

@stefantalpalaru stefantalpalaru merged commit 8338f9f into naftaliharris:master Sep 28, 2019
@arizvisa
Copy link
Contributor Author

Thanks!

I'll try and dig up some more windowsy stuff to also backport. Any recommendations that you're interested in?

@stefantalpalaru
Copy link
Collaborator

Anything that's backwards-compatible is fair game. My personal interest is in the performance enhancing patches: https://hackernoon.com/5-speed-improvements-in-python-3-7-1b39d1581d86 and whatever else influences these tests.

@arizvisa
Copy link
Contributor Author

arizvisa commented Oct 1, 2019

Nothing windows-specific then?

@stefantalpalaru
Copy link
Collaborator

Not that I can think of, but I'm not a Windows user. I only fixed the Windows build because I needed AppVeyor up and running. Feel free to take the lead on Windows-specific fixes and additions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants