classification
Title: os.times() is bogus
Type: Stage:
Components: Extension Modules Versions: Python 2.6
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: loewis Nosy List: LambertDW, belopolsky, christian.heimes, giampaolo.rodola, gregory.p.smith, loewis, maltehelmert, ocean-city, pitrou, rbp
Priority: release blocker Keywords: patch

Created on 2004-10-04 16:34 by gvanrossum, last changed 2009-03-20 23:51 by loewis. This issue is now closed.

Files
File name Uploaded Description Edit
test_posix5.patch maltehelmert, 2008-02-24 20:54
os_times5.patch maltehelmert, 2008-10-19 00:12
Messages (65)
msg60572 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2004-10-04 16:34
At least on Linux, all values the tuple returned by
os.times() are bogus. The code always divides the
return value from the times() system call by HZ, but
according to the man page times(2) they are in clock
ticks. The man page adds:

NOTES
       The number of clock ticks per second can be
obtained using
              sysconf(_SC_CLK_TCK);
       In  POSIX-1996 the symbol CLK_TCK (defined in
<time.h>) is mentioned as
       obsolescent. It is obsolete now.

       [...]

       Note that clock(3) returns values of type
clock_t that are not measured
       in clock ticks but in CLOCKS_PER_SEC.

msg62596 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-20 18:51
On i386 Linux, HZ is #defined as sysconf(_SC_CLK_TCK)

/usr/include/asm-i386/param.h:7:#define HZ sysconf(_SC_CLK_TCK)

so times does the right thing.  On x86_64 HZ is defined as 100, but it 
is the same value as sysconf returns.

I could not find an authoritative statement in this regard, but it 
appears that on modern Linuxes posix_times implementation usin HZ is 
correct.


On the other hand, os.times would be more useful if it just returned a 
tuple of clock ticks.  I suggest implementing os._times returning 
integer clock ticks in posixmodule and reimplementing os.times in os.py 
in terms of sysconf and _times.
msg62742 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-23 15:14
I'm attaching a test script (test_times.py) that forks a child which
runs for 5 seconds, waits for the child, then prints the time taken by
the child according to os.times().

I have a machine where os.times() reproducably reports that 8.33 seconds
have been spent, although indeed only 5 seconds pass:

======================================
# time python test_times.py
8.33333333333

real    0m5.018s
user    0m5.008s
sys     0m0.008s
======================================

I don't know which characteristics of the machine are causing this.
msg62748 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-02-23 15:25
Well, 8.3333/5 equals 100/60. Go figure.
msg62750 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-02-23 15:30
test_times.py produces the correct value on Linux for me, but I see the
same bogus value as Malte on OSX.
msg62751 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-23 15:34
Here's three tests with different pythons on the same machine:
# ./python ../test_times.py
8.33333333333
# python ../test_times.py
8.33333333333
# python2.5 ../test_times.py
5.0

The first Python is current trunk, built just now.
The second Python is the vendor-installed (SuSE) Python 2.5.
The third Python is a Python 2.5 I installed manually from source some
time ago. Strange that it would differ from the second; it appears to be
the same revision as the second from the greeting message.

Anyway, I can reproduce the error in the trunk, which is good.
This is a 64-bit SuSE Linux machine (Xeon X5355 @ 2.66GHz).
msg62756 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-23 15:55
I'm attaching a patch against trunk that fixes the problem for me
(os_times.PATCH).

This uses the sysconf values when HAVE_SYSCONF is defined, and otherwise
falls back on the old behaviour (use HZ if that is defined, 60 
otherwise).

I'm not sure if this is stylistically ok (#ifdefs inside a function,
etc.), so I'd appreciate comments.

Should I add a test case for the test suite?
msg62758 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-23 15:58
Another comment: Since the fallback value of 60 was wrong in the past,
it may likely be wrong in the future. Should that fallback be removed
and replaced by a compile-time error? And is the "HZ" fallback necessary
at all? I don't know enough about Posix to know whether or not
HAVE_SYSCONF should be implied by HAVE_TIMES.
msg62767 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-23 16:26
Here is a patch that adds a test case to test_posix.py. This is ignored
on Windows, as it requires fork.

There is a trade-off: If WAIT_TIME isn't large enough, small
irregularities in the process scheduler might cause this to fail. If it
is too large, the unit tests will take long for everyone.

Currently, WAIT_TIME is 0.1 seconds and the tolerance in the assertion
is about 5%, which works well for me. Not sure how this behaves on very
busy machines.
msg62775 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-02-23 17:13
Well first I can't reproduce the bug on my machine :)
However the two patches do not produce any regression either.
I have some questions:
1. isn't 0.1 for WAIT_TIME a bit too low? 1.0 would probably be less
fragile IMHO
2. why do you fork() in test_times, rather than simply compute the
increase in the first and second return values of os.times()?
msg62777 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-23 17:21
Using 1.0 would certainly be more robust. I wasn't sure if a slow-down
of "make test" by 1 second just for this one bug would be acceptable.

Regarding the fork, when I first encountered this bug, it was in the
context of measuring the runtime of child processes, so that's what I
tried to reproduce. But looking at the code, the bug should occur just
as well with the running process itself. So you're right; one could just
busy-wait for a second and then look at times()[0] and times()[1].
msg62792 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-23 18:24
Note: My original unit test fails to take account that previous unit
tests may have spawned child processes. The correct behaviour is of
course to call os.times() before and after the fork and compute the
difference.

I'm not uploading a modified patch because from previous comments it
looks like a different test will be used anyway.
msg62796 - (view) Author: Rodrigo Bernardo Pimentel (rbp) (Python committer) Date: 2008-02-23 19:18
Malte, Antoine and I discussed this a bit on #python-dev and concluded
that the correct behaviour should be trying sysconf first, then HZ, or
raise an exception if not even HZ is available (since whichever static
value we chose would be misleading anyway).

I'm attaching a patch (modified from Malte's) which implements that.
msg62799 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-23 19:33
Attaching a new test (test_posix2.PATCH) that doesn't fork and fixes the
problem with the previous test not taking previously elapsed time into
account. This supersedes test_posix.PATCH.

I left the wait time at 0.1; if we stay within the same process, this
should be large enough. A busy wait loop for 0.1 seconds should easily
get the 5% precision required by the assertAlmostEqual.
msg62801 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-23 19:40
I was wrong -- 0.1 isn't enough, because os.times() typically has 0.01s
resolution, so we can easily get 0.1 vs. 0.11 which will fail the
assertion. Cranked up the WAIT_TIME to 0.3 in the attached patch
(test_posix3.PATCH). Sorry for the noise.
msg62802 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-23 19:43
Sorry, but the test was still wrong because I misunderstood how
assertAlmostEqual works. Attaching a fourth (final?) test.
msg62806 - (view) Author: Rodrigo Bernardo Pimentel (rbp) (Python committer) Date: 2008-02-23 20:06
Malte noticed that my previous patch won't compile when HAVE_SYSCONFIG
and HZ are not defined. My bad, silly mistake.

I've attached a new version, which compiles and has been tested on all
three cases (with test_posix4.PATCH). Please, someone with privileges
remove os_times2.PATCH.
msg62811 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-23 21:15
os_times3.PATCH works for me on Mac OS 10.4 and RHEL.  I have a few 
comments on the patch:

1. sysconf return type is long, not clock_t
2. If sysconf is present, but _SC_CLK_TCK is not supported, it will 
return -1.  In this case we should fall back to system HZ if available.
3. Use -1 instead of NULL as the invalid value. NULL has too strong 
connotation with pointers.
4. On systems where fixed HZ is correct calling sysconf(_SC_CLK_TCK) on 
every times call is an overkill.

I would suggest that instead of patching posixmodule.c an appropriate 
system-dependent value for HZ should be defined in configure.h.  
Unfortunately I am not familiar enough with autoconf to prepare a patch.
msg62866 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 02:13
Alexander, regarding your comments:

1. sysconf in general returns a long because it can return all sorts of
information, but os.times() returns clock_t items, so the _SC_CLK_TCK
value must comfortably fit into a clock_t. It's preferable to cast into
a clock_t immediately rather than doing a conversion for each of the
ensuing divisions.

2. Do you have indications that such platforms exist? In that case,
indeed the patch should be adapted. Is that -1 return value documented
somewhere?

3. I agree; 0 or -1 would be better.

4. You're right about the overhead, but someone (amk?) measured it and
it's only 5% compared to the old buggy behaviour. It's still possible to
do a million calls to os.times() from Python in a second, which is
plenty fast enough. Clearly the speed could be improved, but it doesn't
appear worth the complications to me.
msg62869 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 02:40
> 1. .. It's preferable to cast into a clock_t immediately rather than
> doing a conversion for each of the ensuing divisions.

If that's your motivation, then you should cast to double instead. 
However, I would leave it to compiler to do micro-optimizations like 
these. I am not aware of a standard that says that clock_t must be wider 
than long.  I agree that it is unlikely to produces wrong results given 
that we are realistically talking about 50-1000 range, but on some 
platforms you may see a warning.

> 2. .. Is that -1 return value documented somewhere?

Yes, see man sysconf on a sufficiently unix-like system or http://www.opengroup.org/onlinepubs/009695399/functions/sysconf.html

> 4. You're right about the overhead, but someone (amk?) measured it and
> it's only 5% compared to the old buggy behaviour. It's still possible 
to
> do a million calls to os.times() from Python in a second, which is
> plenty fast enough. Clearly the speed could be improved, but it
> doesn't appear worth the complications to me.

5% is a lot and IIRC os.times is used by some standard python profilers 
and 5% slowdown will affect people.

What I suggest is a simpler solution than your patch:

(1) Define USE_SYSTEM_HZ in config.h (will require some autoconf 
hacking).

(2) Define Py_HZ as system HZ on the systems where HZ can be trusted. 
(Some systems already define HZ as sysconf(_SC_CLK_TCK)) and fix the 
system definition appropriately where necessary.

(3) Replace HZ with Py_HZ in posixmodule.c

The advantage is that the systems where os.times is not broken will not 
be affected.

BTW, does anyone know if sysconf(_SC_CLK_TCK)) result can change during 
process lifetime?
msg62870 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 02:54
Aha, I should read my own sources:

"The value shall not change during the lifetime of the calling process, 
[XSI]   except that sysconf(_SC_OPEN_MAX) may return different values 
before and after a call to setrlimit() which changes the RLIMIT_NOFILE 
soft limit." 
<http://www.opengroup.org/onlinepubs/009695399/functions/sysconf.html>

So we can consider making ticks_per_sec static and initializing it in 
posixinit.
msg62871 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 03:09
FWIW, the following minimal patch fixed the bug on MacOS X and does not 
affect Linux:

===================================================================
--- Modules/posixmodule.c       (revision 61014)
+++ Modules/posixmodule.c       (working copy)
@@ -5923,7 +5923,7 @@
 
 #ifdef HAVE_TIMES
 #ifndef HZ
-#define HZ 60 /* Universal constant :-) */
+#define HZ sysconf(_SC_CLK_TCK)
 #endif /* HZ */
 
 #if defined(PYCC_VACPP) && defined(PYOS_OS2)
msg62895 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-02-24 12:43
Hi Alexander,

> 5% is a lot and IIRC os.times is used by some standard python profilers 
> and 5% slowdown will affect people.

Profiled runs are always slower than non-profiled runs, so I'm not
convinced it is very important. You use profiling only when you need it,
not in normal production conditions.

However, fetching the value only once and then caching it is a valid
approach, so you can produce an updated patch for that if you want :-)
msg62910 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 15:29
Alexander, speed-wise your patch is worse than the original one on
systems where HZ isn't predefined, because it calls sysconf 5 times in
each call to os.times, rather than only once per call.

If you worry about speed, the approach outlined in Antoine's last
message makes most sense to me. Doing this in the configure script
appears dangerous to me; it is well possible that the clock tick value
changes over time on the same machine (e.g. after a kernel upgrade), so
this should be determined upon process initialization, not before
compilation.

Also, I don't think that the HZ value should be preferred to the sysconf
value if both are available, as all times man pages I could check
mention sysconf as the correct way to do this, not HZ. (Some of them
state that HZ is used on "older systems".)

Finally, your patch assumes that HAVE_TIMES implies HAVE_SYSCONF; is
that guaranteed? In particular, it's not clear to me what happens on
Windows (see comment at the top of the file). I also have no idea how
any of the earlier patches behaves on Windows, unfortunately.
msg62911 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-02-24 15:39
I suggest that you define "static int hz" and assign a value to the var
in INITFUNC().

#ifdef HZ
hz = HZ;
#elif defined(HAVE_SYSCONF)
hz = sysconf(_SC_CLK_TCK);
#else
hz = 60; /* It's 50Hz in Europe */
#endif
msg62912 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 15:46
Tiran, that's the general approach we should follow, yes.

But the people who discussed this on #python-dev all felt a bit queasy
about the "60" fallback -- this is what caused the bug in the first
place on Guido's and my machine. (A value of 60 was assumed; 100 would
have been correct.) Having no such fallback would be preferable, unless
it's necessary.

You use Windows, right? Can you test if that fallback is necessary
there? As far as I can tell, it should not be necessary on a more
UNIX-ish system.
msg62913 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 15:53
Never mind, on Windows a different code path is chosen.
I'm now working on a patch that computes the hz value during module
initialization. So should I keep the 60 magic value? What is the
justification?
msg62914 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-02-24 15:57
I don't *use* Windows except for some computer games. But I'm doing some
development for Python on Windows in a VMWare box. I'm going to check HZ
for you. Give me a couple of minutes to boot the image.

How do you like this idea. Since HZ is only used in posix_times it's
fine to cache it in a local static var.

static PyObject *
posix_times(PyObject *self, PyObject *noargs)
{
	static int hz = -1;
	struct tms t;
	clock_t c;

	if (hz == -1) {
		/* Py_HZ may call sysconf(), cache the value */
		hz = Py_HZ;
	}
	errno = 0;
	...
msg62917 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 17:03
Here is an updated patch (os_times4.PATCH) that incorporates Christian's
suggestions. The patch includes the new unit test, so test_posix?.PATCH
need not be applied separately.

I again made the unit test a bit more lenient to allow an absolute error
of 0.02 seconds, as there may be systems where the clock tick
granularity is only 1/60 seconds, and then the old tolerance of 0.015
seconds would be too low.

This patch prefers sysconf where it is available; this is what "man 2
times" asks to do. If sysconf is available but produces an error, that
error is raised. (Errors should never pass silently.) HZ is only used if
sysconf is not available. If neither sysconf nor HZ is available, a
compile-time error is raised -- in that case, HAVE_TIMES shouldn't have
been defined in the first place.

I also timed this; there is no discernible change compared to the old
behaviour.

The patch fixes the buggy behaviour on my 64-bit Linux box and makes no
difference on my 32-bit Linux box. The new unit test passes on both
machines.
msg62918 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 17:59
Here is my take (posixmodule.diff).  When ticks per seconds cannot be 
determined, python should not provide times rather than refuse to build.  
Initialization moved to the init function.
msg62919 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 18:14
I'd prefer a noisy compile error, since in situations where times is
available but unusable, HAVE_TIMES shouldn't have been #defined in the
first place. (That is, I'd see that as a bug in the configure script.)

But this is turning into a bikeshed discussion. I care more about the
bug being fixed than about how precisely it is fixed. For the record,
Alexander's patch fixes the bug on my Linux box, too, so I'm fine with
either patch.

If Alexander's patch ends up being applied, I suggest ripping the new
unit test from os_times4.PATCH, since the timing in test_posix4.PATCH is
fragile as mentioned above.

Either way, this looks ready to be closed to me.
msg62920 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 18:40
> I'd prefer a noisy compile error ..

That would be fine if you could verify that none of the currently 
supported platforms will be affected. I would still feel uneasy about 
refusing to build python simply because os.times is not ported to a 
platform.

> HAVE_TIMES shouldn't have been #defined in the
> first place. (That is, I'd see that as a bug in
> the configure script.)

No, defined HAVE_TIMES only tell you that the system has 'times' 
function in the C library.  It is not intended to mean that os.times is 
implementable.

Personally, I would still prefer a one-line change that I proposed 
above.  It is obviously better than the current smiley code and if it 
happens to fix the platforms where errant behavior was observed, it is 
worth applying even if theoretically it may be wrong.

In any case, there is plenty of material here for a developer to step in 
and close the issue.
msg62921 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-02-24 18:43
IMO, if there is no available way to compute HZ, a NotImplementedError
should be raised rather than using the 60 magic value.
msg62922 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 18:51
>> I'd prefer a noisy compile error ..
>
> That would be fine if you could verify that none of the currently 
> supported platforms will be affected. I would still feel uneasy about 
> refusing to build python simply because os.times is not ported to a 
> platform.

Unless I'm missing something, your suggested one-line change fails to
compile in exactly the same cases -- if HAVE_TIMES is defined, but HZ
and sysconf unavailable -- but with a worse error message.

> HAVE_TIMES shouldn't have been #defined in the
> first place. (That is, I'd see that as a bug in
> the configure script.)

> No, defined HAVE_TIMES only tell you that the system has 'times' 
> function in the C library.  It is not intended to mean that os.times
> is implementable.

Sure, but if times is in the standard library, but its output is
uninterpretable, then there's something wrong going on that needs to be
fixed rather than swept under the rug.

> Personally, I would still prefer a one-line change that I proposed 
> above.  It is obviously better than the current smiley code and if it 
> happens to fix the platforms where errant behavior was observed, it is 
> worth applying even if theoretically it may be wrong.

You complained in msg62869 about the original patch that calling sysconf
on every call leads to an unacceptable slowdown. Your one-line patch
calls sysconf five times on each call when HZ is not defined.
msg62923 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 18:53
s/standard library/system library/, of course.
Also, the original code is wrong in preferring HZ over the sysconf value.
msg62924 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 18:55
Antoine, none of the recently proposed patches uses the 60 magic value.

Alexander's patch doesn't define times in that case (leading to an
AttributeError when trying to call os.times) while my patch complains
about the bogus environment during compilation.
msg62925 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-02-24 18:58
I think it's better to make it a runtime error (upon invocation of
os.times()), rather than a compile-time error. But it's quite
theoretical until we find a system where the error does occur, anyway :)
msg62926 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 19:07
> You complained in msg62869 about the original patch that calling 
sysconf
> on every call leads to an unacceptable slowdown. Your one-line patch
> calls sysconf five times on each call when HZ is not defined.

That's why I said that was a matter of personal preference.  I use 3 
different systems: i386 linux, x86_64 linux and Mac OS X and the last is 
not mission critical.  On i386 linux HZ is already defined as sysconf 
call, on x86_64 HZ is defined as 100 and it happens to be the correct 
value.  On MacOS X, HZ is not defined, and os.times exhibits the bug. 
One-line patch will make MacOS X behave the same as i386 Linux, which is 
fine for me, but I will oppose any changes that affect x86_64 linux 
performance.

Does anyone know of a platform where HZ is defined to a wrong value?  WE 
may want to undef or redefine HZ on such platforms.
msg62927 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-02-24 19:11
> but I will oppose any changes that affect x86_64 linux 
performance.

Alexander, as I said I'm really curious about any situation where
os.times() is so performance-critical that a 5% slowdown for that
function call is unacceptable. Even when a profiler is involved,
os.times() is not the only overhead added by profiling, there is also
all the bookkeeping needed for recording various statistics... Perhaps
by profiling the profiler we would have an answer :)
msg62928 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 19:22
Alexander, your one-line patch *does* affect performance on my 64-bit
Linux machine in a worse way than any other proposed patch by calling
sysconf five times. HZ may be defined on your machine, but it isn't on
my (Xeon) machine.

I checked man pages on four different Linuxes (32 bit and 64 bit; SuSE,
Fedora, Ubuntu; recent or six years old). All of them state that using
the sysconf value is the right thing to do. This is also stated in the
man page excerpt in Guido's original bug report.

Neither your latest patch (posixmodule.diff) not my latest patch
(os_times4.PATCH) affects performance; they both only call sysconf once
and then used a cached value.

I'm perfectly fine with your posixmodule.diff, which also meets
Antoine's criteria. I suggest we apply that patch, along with the unit
test from os_times4.PATCH, and be done with it.
msg62929 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 19:47
>  HZ may be defined on your machine, but it isn't on my (Xeon) machine.

Are you sure?  Please note that HZ will not show up in grep -w HZ 
/usr/include because the right header file further up the tree.

On Solaris:
/usr/include/sys/param.h:#define        HZ              
((clock_t)_sysconf(_SC_CLK_TCK))

On 32-bit Linux:
/usr/include/asm-i386/param.h:#define HZ sysconf(_SC_CLK_TCK)

On 64-bit Linux:
/usr/include/asm-x86_64/param.h:#define HZ 100

Did you try to run posixmodule.c through gcc -E on your system?

I should not play devil's advocate and argue against my own patch, but 
there are two issues:

1. If a system provides non-standard HZ, is it to be preferred to 
sysconf(..)?  Are there systems with correct HZ but no sysconf?

2. Is the added complexity of #ifdef dance justified for the performance 
improvements on some platforms?  I know it looks like I am flip-flopping 
here, but I just don't want to change anything on the platforms where 
os.times is not broken and use a solution that is know to work on some 
platforms to fix the bug where it shows up.
msg62930 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 20:11
If I remove the "#define 60" line in an unmodified posixmodule.c from
trunk, I get the following compiler error:

gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O3 -Wall
-Wstrict-prototypes  -I. -IInclude -I./Include   -DPy_BUILD_CORE  -c
./Modules/posixmodule.c -o Modules/posixmodule.o
./Modules/posixmodule.c: In function posix_times:
./Modules/posixmodule.c:5964: error: HZ undeclared (first use in this
function)

So yes, HZ isn't available there. It sure is defined *somewhere* (grep
shows a definition in /usr/include/asm-x86_64/param.h), but it isn't
anywhere Python would pick it up.

And I don't really see why it should when the man pages all argue that
using HZ for times is for "older system" (this is a man page from 2002,
no less!). Can you measure a performance difference between your patch
and the old buggy behaviour? I couldn't, on any machine, and I'd be very
surprised if it existed. There is no significant difference between
dividing by a constant and diving by a static module variable, and, as
Antoine rightly suggests, any such difference would be completely lost
in the noise considering the hefty cost of the other operations.

Regarding your two issues:
1. Yes, the sysconf value should take precedence over HZ, since this is
what "man 2 times" says you should use.
2. Personally, I'd be just as fine with the original patch that doesn't
have the code complexity of the value caching, but I'm fine with
anything that fixes the bug. Keeping the old behaviour where possible
"for old time's sake" seems a bad idea -- there were at least two broken
platforms (Mac OS and Xeon), and there may be others. Using the
documented behaviour (sysconf) where available is a much better
solution; honestly, sticking to using HZ as a default without support
for that from any documentation has a cargo-cult programming smell to me.

I don't know if there are platforms that have times, but neither sysconf
nor HZ. That sounds very strange to me, but of course I can't rule it
out. But if there are, it is likely that os.times was broken before on
these platforms -- it was broken on two platforms that I wouldn't
consider minor. In that case, it will still be broken, but at least now
we have a unit tests that detects this.
msg62931 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 20:13
In the first line of my previous message, I mean "#define HZ 60", of course.
msg62932 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-02-24 20:37
Guys, please don't waste too much time on this issue! The tracker still
has more than 1,700 open issues for to debate on. While I enjoy the
fact, that you three are enthusiastic, I strongly feel the urge to
re-route your men power. This bug isn't important enough to waste your
precious time on it.

My opinion as junior core developer is:

sysconf(_SC_CLK_TCK) is the winner and it should be used instead of HZ
when available. A default value should not be used because it will lead
to wrong data. Wrong results are worse than no results.

Since calls to sysconf seem to cost several CPU cycles "clk_tck" should
be cached somehow. I prefer a local static variable in the function but
a static var on file level is fine, too.

The compilation of Python must not fail. When neither HZ nor sysconf is
available but HAVE_TIMES is defined then the function must not be
included. Either you skip the function plus undef HAVE_TIMES or you add
some code to configure.in that does it earlier. The C89 standard doesn't
define #warn so that not an option, too. But configure is allowed fail
for a broken system.

Christian
msg62934 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 20:47
Christian, I agree on all points. Alexander's patch posixmodule.diff
satisfies those requirements.

I suggest also adding the unit test from os_times4.PATCH (but not the
changes to posixmodule.c in that patch), as this will help identify
misbehaving platforms in the future.
msg62935 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 20:49
Thanks Cristian for your intervention.  This bug is clearly an 
aberration: how many GvR reported bugs do you know that stayed open for 
3+ years?

I think posixmodule.diff satisfies all your requirements and was not 
opposed by anyone except yours truly.  Do you need anything else to be 
done before you can accept the patches.  It looks like test_posix4.PATCH 
+ posixmodule.diff should be enough.
msg62936 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-02-24 20:54
Great, we're approaching closure! :-)

One final small thing: As said somewhere above, the allowed discrepancy
in test_posix4.PATCH is a bit too low for machines with only 60 ticks
per second. I uploaded a slightly more generous test_posix5.PATCH instead.

So posixmodule.diff + test_posix5.PATCH.

This is the same as what I mentioned above (posixmodule.diff + only the
unit test from os_times4.PATCH).
msg63160 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2008-03-01 06:12
How abount also check CLK_TCK? I know it's obsolute but maybe
there are platforms without sysconf() and with CLK_TCK.
msg63291 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-03-05 18:39
I think it's better only to only add another fallback if the unit tests
show that such platforms exist. Avoiding cruft is important, too. After
all, sysconf is a standard POSIX API, and from my (admittedly limited)
research was already available in that form back in 1988.

So my suggestion is to apply the current patches.
msg72605 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-09-05 17:03
I agree with Christian's most recent comment.

However without BDFL intervention I think its too late in the 2.6/3.0 
release cycle to rush this fix in.  It can wait for 2.6.1/3.0.1.

I won't have time to look at it for several days myself, but I'm 
assigning the bug to me so that it doesn't sit idle longer than it 
already has; feel free to steal it if someone else intends to fix it 
fast.
msg72612 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-09-05 17:38
Yes, please move this to 3.0.1 / 2.6.1.  The hard part appears to be
making sure that it compiles *and* is correct on all conceivable
platforms...
msg74301 - (view) Author: David W. Lambert (LambertDW) Date: 2008-10-04 03:24
I don't know what is "HZ", but if it's "hertz" then a division is 
necessary.

         total_clocks
time = -----------------
       clocks_per_second

otherwise there's no hope.
msg74355 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-10-05 23:50
David,

not sure what you are commenting on. Are you commenting on one of the
patches? The patches do contain those divisions, of course; you can also
run the attached unit test to verify that the patches work for you.
msg74961 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-10-18 22:37
Malte proposed this patch for inclusion into Python 2.5. In its current
form, I'm skeptical about doing so, as it has the potential for breakage.

IIUC, the patch requires that HZ is defined if HAVE_TIMES is defined and
HAVE_SYSCONF is not; plus it requires _SC_CLK_TCK to be defined if
HAVE_SYSCONF is defined. For 2.5, such additional constraints are not
acceptable. Instead, the patch should guarantee compilation of
posixmodule if 2.5.2 allowed compilation, no matter how confused the
system is. If some of the prerequisites are not met, it is OK if either
os.times is absent, or produces bogus results.
msg74962 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-10-18 23:31
Martin,

compilation indeed breaks if sysconf is available but _SC_CLK_TCK is
not. My Unix-foo is not sufficient to confidently say that this is
impossible, so my suggestion is to add defined(_SC_CLK_TCK) to the
condition of this #ifdef branch. For what it's worth, this also appears
to be the way Perl does it (perl.c, lines 384-385):

  #if defined(HAS_SYSCONF) && defined(_SC_CLK_TCK) && !defined(__BEOS__)
      PL_clocktick = sysconf(_SC_CLK_TCK);

In the other case you mention, where neither sysconf nor HZ are
available, the old default of 60 could be used instead. A noisy error
appears safer to me to avoid future similar bugs, but I see that this is
a bad idea for Python 2.5.x.

I'll prepare a modified patch.
msg74963 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-10-19 00:12
OK, modified and simplified patch attached (os_times5.PATCH).

The patch and unit test (in test_posix5.PATCH) apply cleanly against the
trunk. "make test" passes on two machines I tried, including a 64-bit
Linux machine where the new unit test fails without the patch. The
caveat is that with the machines I have, I can't exercise all #ifdef
paths. However, the logic looks simple enough now.
msg74965 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-10-19 07:12
> compilation indeed breaks if sysconf is available but _SC_CLK_TCK is
> not. My Unix-foo is not sufficient to confidently say that this is
> impossible

To make such a statement, one would need knowledge of all operating
system releases that have ever been made, including releases that didn't
make it to the public. It might be that POSIX mandates _SC_CLK_TCK,
but that would be irrelevant, as systems might chose not to comply with
POSIX in this aspect.

> In the other case you mention, where neither sysconf nor HZ are
> available, the old default of 60 could be used instead. 

That would probably be safest, although I could also accept that
os.times becomes unavailable on such a system.
msg77746 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-12-13 15:15
Committed os_times5.patch into 2.5 branch as r67739

Applying this to the other branches still needs to be done.
msg77759 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-12-13 18:34
should the unit test in test_posix5.patch have been removed?  (regardless, 
its still on the bug tracker via the history links)
msg77766 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-12-13 21:52
> should the unit test in test_posix5.patch have been removed?  

I see. I wish that people would a) always provide complete patches in
a single file, and b) delete files themselves that have been superceded
by others. In any case, I have re-attached the file; thanks for pointing
this out.
msg77797 - (view) Author: Malte Helmert (maltehelmert) Date: 2008-12-14 12:36
> I see. I wish that people would a) always provide complete patches in
> a single file, and b) delete files themselves that have been
> superceded by others. In any case, I have re-attached the file;
> thanks for pointing this out.

Regarding b), I did that as far as I could (i.e., deleted my own files
that were superseded).

I left the unit test as a separate file because I wasn't too sure if it
would get incorporated -- it looks a bit flaky because it relies on
timing issues, and it's also one of the longer-running tests as it
spends (comparatively) a lot of time in a busy loop. I wouldn't be
surprised if it breaks on some machines sometimes, especially ones with
low clock granularity.

Thanks for the comment though, I'll keep it in mind in the future.
msg77818 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-12-14 19:48
> I wouldn't be
> surprised if it breaks on some machines sometimes, especially ones with
> low clock granularity.

Now that I look at the test, I see what you mean. I think I will reject
it.
msg78472 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-12-29 18:26
Malte, thanks again for the patch; committed into the various branches
as r68018, r68019, r68020, r68021.

As annunciated, I reject the test; I don't think there is a reasonable
way to test for this bug.
msg83872 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2009-03-20 22:29
Is this fixed in Python 2.6.1?
We have encountered some problems on both OS X and FreeBSD by using 2.6.1:
http://code.google.com/p/psutil/issues/detail?id=40
msg83893 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-03-20 23:51
> Is this fixed in Python 2.6.1?

Please try to answer this question yourself in the future.
Python 2.6.1 was released on Dec 4th
(http://www.python.org/download/releases/2.6.1/), and the
commits were made on Dec 29 (as seen both in the time stamp
on my message, and the *linked* svnview pages).
History
Date User Action Args
2009-03-20 23:51:18loewissetmessages: + msg83893
2009-03-20 22:29:42giampaolo.rodolasetnosy: + giampaolo.rodola
messages: + msg83872
2008-12-29 18:26:50loewissetstatus: open -> closed
messages: + msg78472
2008-12-20 22:55:51loewissetmessages: - msg77803
2008-12-20 14:37:15loewissetversions: - Python 2.5, Python 2.4, Python 2.5.3
2008-12-20 02:41:56loewissetpriority: deferred blocker -> release blocker
2008-12-14 19:48:29loewissetmessages: + msg77818
2008-12-14 16:13:55gvanrossumsetnosy: - gvanrossum
2008-12-14 16:13:36gvanrossumsetnosy: gvanrossum, loewis, gregory.p.smith, belopolsky, maltehelmert, pitrou, ocean-city, christian.heimes, LambertDW, rbp
messages: + msg77803
2008-12-14 12:36:23maltehelmertsetmessages: + msg77797
2008-12-13 21:52:01loewissetmessages: + msg77766
2008-12-13 21:50:00loewissetfiles: + test_posix5.patch
2008-12-13 18:34:47gregory.p.smithsetmessages: + msg77759
2008-12-13 15:18:20loewissetfiles: - test_posix5.patch
2008-12-13 15:18:17loewissetfiles: - posixmodule.diff
2008-12-13 15:18:13loewissetfiles: - os_times3.PATCH
2008-12-13 15:18:09loewissetfiles: - os_times2.PATCH
2008-12-13 15:15:16loewissetpriority: release blocker -> deferred blocker
assignee: gregory.p.smith -> loewis
messages: + msg77746
resolution: accepted
2008-12-10 09:14:25loewissetkeywords: - easy
2008-12-10 09:14:12loewissetpriority: normal -> release blocker
2008-10-19 07:16:47loewissetversions: + Python 2.5.3
2008-10-19 07:12:18loewissetmessages: + msg74965
2008-10-19 00:12:15maltehelmertsetfiles: + os_times5.patch
messages: + msg74963
2008-10-18 23:31:09maltehelmertsetmessages: + msg74962
2008-10-18 22:37:49loewissetnosy: + loewis
messages: + msg74961
2008-10-18 19:58:10maltehelmertsetfiles: - os_times4.PATCH
2008-10-05 23:51:16maltehelmertsetfiles: - test_posix4.PATCH
2008-10-05 23:51:07maltehelmertsetfiles: - test_posix3.PATCH
2008-10-05 23:50:57maltehelmertsetfiles: - test_posix2.PATCH
2008-10-05 23:50:49maltehelmertsetfiles: - test_posix.PATCH
2008-10-05 23:50:32maltehelmertsetfiles: - os_times.PATCH
2008-10-05 23:50:23maltehelmertsetfiles: - test_times.py
2008-10-05 23:50:11maltehelmertsetmessages: + msg74355
2008-10-04 03:24:00LambertDWsetnosy: + LambertDW
messages: + msg74301
2008-09-05 17:38:41gvanrossumsetmessages: + msg72612
2008-09-05 17:03:32gregory.p.smithsetassignee: gregory.p.smith
messages: + msg72605
nosy: + gregory.p.smith
2008-03-05 18:39:37maltehelmertsetmessages: + msg63291
2008-03-01 06:12:21ocean-citysetnosy: + ocean-city
messages: + msg63160
2008-02-24 20:54:50maltehelmertsetfiles: + test_posix5.patch
messages: + msg62936
2008-02-24 20:49:23belopolskysetmessages: + msg62935
2008-02-24 20:47:26maltehelmertsetmessages: + msg62934
2008-02-24 20:37:02christian.heimessetmessages: + msg62932
2008-02-24 20:13:14maltehelmertsetmessages: + msg62931
2008-02-24 20:12:00maltehelmertsetmessages: + msg62930
2008-02-24 19:47:14belopolskysetmessages: + msg62929
2008-02-24 19:22:46maltehelmertsetmessages: + msg62928
2008-02-24 19:11:05pitrousetmessages: + msg62927
2008-02-24 19:07:33belopolskysetmessages: + msg62926
2008-02-24 18:58:59pitrousetmessages: + msg62925
2008-02-24 18:55:45maltehelmertsetmessages: + msg62924
2008-02-24 18:53:25maltehelmertsetmessages: + msg62923
2008-02-24 18:52:00maltehelmertsetmessages: + msg62922
2008-02-24 18:43:21pitrousetmessages: + msg62921
2008-02-24 18:40:11belopolskysetmessages: + msg62920
2008-02-24 18:14:36maltehelmertsetmessages: + msg62919
2008-02-24 17:59:40belopolskysetfiles: + posixmodule.diff
keywords: + patch
messages: + msg62918
2008-02-24 17:03:09maltehelmertsetfiles: + os_times4.PATCH
messages: + msg62917
2008-02-24 15:57:56christian.heimessetmessages: + msg62914
2008-02-24 15:53:50maltehelmertsetmessages: + msg62913
2008-02-24 15:46:22maltehelmertsetmessages: + msg62912
2008-02-24 15:39:24christian.heimessetnosy: + christian.heimes
messages: + msg62911
2008-02-24 15:29:44maltehelmertsetmessages: + msg62910
2008-02-24 12:43:56pitrousetmessages: + msg62895
2008-02-24 03:09:14belopolskysetmessages: + msg62871
2008-02-24 02:54:08belopolskysetmessages: + msg62870
2008-02-24 02:40:04belopolskysetmessages: + msg62869
2008-02-24 02:13:37maltehelmertsetmessages: + msg62866
2008-02-23 21:15:12belopolskysetmessages: + msg62811
2008-02-23 20:06:20rbpsetfiles: + os_times3.PATCH
messages: + msg62806
2008-02-23 19:43:35maltehelmertsetfiles: + test_posix4.PATCH
messages: + msg62802
2008-02-23 19:40:09maltehelmertsetfiles: + test_posix3.PATCH
messages: + msg62801
2008-02-23 19:33:00maltehelmertsetfiles: + test_posix2.PATCH
messages: + msg62799
2008-02-23 19:18:39rbpsetfiles: + os_times2.PATCH
nosy: + rbp
messages: + msg62796
versions: + Python 2.6, Python 2.5
2008-02-23 18:24:08maltehelmertsetmessages: + msg62792
2008-02-23 17:21:23maltehelmertsetmessages: + msg62777
2008-02-23 17:13:59pitrousetnosy: + pitrou
messages: + msg62775
2008-02-23 16:26:14maltehelmertsetfiles: + test_posix.PATCH
messages: + msg62767
2008-02-23 15:58:29maltehelmertsetmessages: + msg62758
2008-02-23 15:55:27maltehelmertsetfiles: + os_times.PATCH
messages: + msg62756
2008-02-23 15:34:01maltehelmertsetmessages: + msg62751
2008-02-23 15:30:16gvanrossumsetmessages: + msg62750
2008-02-23 15:26:00gvanrossumsetmessages: + msg62748
2008-02-23 15:14:31maltehelmertsetfiles: + test_times.py
nosy: + maltehelmert
messages: + msg62742
2008-02-20 18:51:53belopolskysetnosy: + belopolsky
messages: + msg62596
2008-02-19 23:34:45akuchlingsetkeywords: + easy
2004-10-04 16:34:35gvanrossumcreate