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
Make gettimeofday available in time module #53325
Comments
Attached patch moves cross-platform logic of obtaining current time to _time.c which is shared between time and datetime modules. This simplifies both modules and reduces the datetime module dependency on the time module. |
The new patch, bpo-9079.diff exposes gettimeofday as time.gettimeofday() returning (sec, usec) pair. |
Mark, I am reassigning this to you for commit review. I am changing the title to reflect the visible part of the change. The datetime module gains direct access to system gettimeofday at the C level while time module grows time.gettimeofday() Python method. I am not sure I made the best choice defining struct timeval on systems without gettimeofday. Maybe it is better to do typedef struct timeval _PyTime_timeval; on systems with gettimeofday and equivalent explicit definition on systems without. The cast selection logic for time_t is a bit of a hack as well, but this is the best I could come up with without resorting to configure tests. Unless this breaks on a known platform, I would rather commit this as is and have a separate project to clean up cross-platform time handling later. This patch builds on refactoring started in bpo-9012. |
It would be great if Tim Peters could be consulted about some of these ideas. He put a great deal of thought into the API, what should be included and what should be excluded. |
I would very much appreciate Tim's input on datetime issues. This particular issue is fairly minor, but Tim's expertise will be invaluable for anything timezone related. I do appreciate the incredible amount of brainpower that went into the design of datetime module and I think the result is very good. There is only one thing that I would like to improve - eliminate the need to reach out to the time module when working with the datetime instances. This patch is a small step to removing the dependency at the implementation level. |
To keep things clean, would it be possible to separate this into two patches, one containing the refactoring and one for the newly exposed gettimeofday? |
The original patch, gettimeofday.diff was just refactoring. I unlinked it to keep the file list clean, but it is still available: http://bugs.python.org/file17766/gettimeofday.diff I decided to expose time.gettimeofday() in the same patch mostly in order to be able to better explain the consequences of the change such as date.today() now being equivalent to date.fromtimestamp(time.gettimeofday()[0]) rather than date.fromtimestamp(time.time()). |
Instead of being defined in _time.h, perhaps the new API function should be provided by core Python? It would probably help in certain threading primitives. |
On Mon, Jul 12, 2010 at 12:07 PM, Antoine Pitrou <report@bugs.python.org> wrote:
This is an interesting idea, but proposed Py_gettimeofday inherits |
I would say this is fine as long as it's "documented". The whole point
Indeed, the GIL code would probably still use its own code paths. |
On Mon, Jul 12, 2010 at 1:06 PM, Antoine Pitrou <report@bugs.python.org> wrote:
That may be true, but I would rather proceed in small steps. First |
Assigning back to Alexander; sorry I haven't had time to look at this. |
The patch looks good to me FWIW. I would be interested in using this perhaps in bpo-8844, which involves lock timeouts. It may be true that the POSIX API uses nanoseconds, but pythreads only exposes microsecond precision. In order to use it from the thread module, it need to get moved into Python/. My best guess at the best place to put it would be sysmodule.c, since that already wraps other system calls. Or, you know, we could live a little and make a new file for it. :) |
OK, can someone show me an example of how functions defined in core Python can be made available to extension modules? I thought I could model pytime.h/.c after pymath.h/.c, but the later is not used in extension modules. I must be missing something trivial here. |
You should just need to #include "pytime.h" from Include/Python.h. |
No it must be something else. Attached bpo-9079-fail.diff fails with Symbol not found: __PyTime_gettimeofday even though it is in pytime.o $ nm ./Python/pytime.o
00000000000009f0 s EH_frame1
0000000000000000 T __PyTime_gettimeofday
0000000000000a08 S __PyTime_gettimeofday.eh
U _gettimeofday
U _time |
Le vendredi 16 juillet 2010 à 19:51 +0000, Alexander Belopolsky a
Here it fails earlier: gcc -pthread -c -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -I. -IInclude -I./Include -DPy_BUILD_CORE -o Python/structmember.o Python/structmember.c |
Antoine, you must be building on Windows. I'll try to guess where ftime is defined and repost the patch. |
I'd like to help diagnose but I don't know what "Erreur 1" means. |
I fixed the ftime issue (I hope), but the build still fails. I did not test on Linux, but I tested on OSX with HAVE_FTIME. Replacing the failing patch. |
Removing spurious configure change from the "fail" patch. |
It looks like my reluctance to add gettimeofday to core without using it there was well founded. Simply adding a dummy call to _PyTime_gettimeofday in main() fixed the build problem. This is a hack, of course, so I am still looking for suggestions on how to make this work. =================================================================== --- Modules/python.c (revision 82921)
+++ Modules/python.c (working copy)
@@ -34,6 +34,8 @@
m = fpgetmask();
fpsetmask(m & ~FP_X_OFL);
#endif
+ struct timeval tv;
+ _PyTime_gettimeofday(&tv);
if (!argv_copy || !argv_copy2) {
fprintf(stderr, "out of memory\n");
return 1; |
It's rather strange. I'm sure we have external API functions which never $ grep -r _PyImport_IsScript Modules/ Objects/ Python/ Include/
Python/import.c:1804:PyAPI_FUNC(int) _PyImport_IsScript(struct filedescr * fd)
Include/import.h:43:PyAPI_FUNC(int) _PyImport_IsScript(struct filedescr *); |
Hmm, after thinking about it, what happens is that the C object file is |
That sounds right. I recall similar problems with pymath.c at one stage: none of the functions it defined was being used in the core, so it didn't get compiled into the python executable and the build failed similarly. |
Right, it's one of the peculiarities of archive files (I think). When none of an object file's symbols are used from the main program, the object file is dropped on the floor, ie not included. This has bizarre consequences in C++ with static initializers, which get dropped. On Windows, the PyAPI_FUNC macros should prevent the linker from stripping the datetime stuff. Jeff Yasskin says you should create a noop function in your object file and call it from PyMain for force linkage of the object file you're building. |
Indeed, something like _PyTime_Init(). |
Reid, I am leaning towards reverting to Plan A (bpo-9079.diff). Would your use case be served well by a _time module exposing C API via a capsule? This what datetime does currently. |
I'd really rather not try to rely module loading from a threading primitive. :) I think if you follow Antoine's suggestion of adding _PyTime_Init (which does nothing in the body other than a comment) it should work fine. |
It turns out I misunderstood how date.today() worked [1] and bpo-9079.diff introduced significant change in behavior. Bringing timeofday into core rather than _time.c also introduced several complications. As a result it makes sense to start with just internal restructuring: bring gettimeofday into core and reuse it in time.time and datetime.datetime.now. The new patch, issue9079a.diff contains no user visible changes. [1] "Curious datetime method" http://mail.python.org/pipermail/python-dev/2010-July/102003.html |
I think you forgot to svn add pytime.c before making the diff. |
Indeed. Replacing issue9079a.diff now. |
pytime.h looks like it got pasted into the file twice. Other than that, it looks good to me and the tests pass on OS X here. |
issue9079a.diff doesn't compile on Windows - timeval isn't defined. You'd have to include Winsock2.h [0]. Adding something like the following within the HAVE_FTIME block would work...
#ifdef MS_WINDOWS
#include <Winsock2.h>
#endif I don't currently have time to dig deeper into this stuff, but test_queue hangs. Running the suite without test_queue leaves 31 other failures and something hangs in the background so the test never completes. That was all done with issue9079a.diff applied, plus my winsock include. [0] http://msdn.microsoft.com/en-us/library/ms740560(VS.85).aspx |
Interesting. As far as I can tell, struct timeval should not be used unless HAVE_GETTIMEOFDAY is defined: +#ifdef HAVE_GETTIMEOFDAY Brian, where do you get undefined symbol error? |
I think you used 'struct timeval *' in the function definition instead of '_PyTimeVal *'. |
Here are the errors I get: Error 104 error C2037: left of 'tv_sec' specifies undefined struct/union 'timeval' c:\python-dev\py3k\Python\pytime.c 46 pythoncore |
I think Reid nailed the issue. Fix is coming. |
issue9079b.diff should fix the Windows issue. |
I won't have time to review this, but I can say issue9079b.diff works fine on Windows. |
Updated the patch to reflect recent datetimemodule.c renaming to _datetimemodule.c. No other changes between issue9079b.diff and issue9079c.diff. I am going to commit issue9079c.diff soon unless someone wants more time to review. |
Committed issue9079c.diff as r83744. This commit does not provide time.gettimeofday() and therefore does not close the issue. |
A tuple is not the preferred type for a timestamp: Python uses float and is not going to use something different (the PEP-410 was just rejected). I don't see what we need a new function: there is always time.time(). I'm closing this issue because I consider it as done. |
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: