[sldev] QueryPerformanceCounter() & related issues VWR-940, VWR-962, VWR975

[Paul TBBle Hampson]

On Sat, Jun 02, 2007 at 04:44:34AM -0700, Dzonatas wrote:
> Paul TBBle Hampson wrote:
> >There's no copyright statement or similar on that page, and I'm
> >pretty sure that's been copied from somewhere itself.

> It has a governmental address, which it is common to assume public
> domain. They are pretty simple.

Assuming something in copyright isn't exactly iron-clad.

In this case, the person who's personal site this is, is also working on
ZeptOS which comes from the same address and carries a GPL license. [1]

So that assumption has no basis here.

On the other hand, the simplicity was asserted by the person who wrote
the PowerPC assembly routine I used in my patch. And authorative as he
might be for his code, I'm not sure he can speak for anyone else who's
implemented functionally identical code.

> I started to sort out the related contributions. Attached is a patch
> for review... more works need to be done on it.

> Main change I did was move rdtsc and likewise code into the header
> file, llfasttimers.h, so that code can now assemble in-line. Eh...
> saves a little overhead.

As per coding standard, please provide profiling justification for this.

It's already hard enough to make sense of as it is, and moving things
around like that makes it _very_ hard to review a patch. Once the code
is done and working, profile and inline it as neccessary.

You don't mention this, but I assume this is based on your previously-
posted patch, since it removes some of the LL_GNUC #define usage you
added in that patch.

> @@ -156,9 +160,11 @@
>  ////////////////////////////////////////////////////////////////////////////
>  F64 CProcessor::GetCPUFrequency(unsigned int uiMeasureMSecs)
>  {
> -#ifndef PROCESSOR_FREQUENCY_MEASURE_AVAILABLE
> -	return 0;
> -#else
> +#if LL_LINUX
> +	extern LLCPUInfo gSysCPU;
> +	return boost::lexical_cast<F64>(gSysCPU.getInfoLine( "cpu MHz" )) * 1000000;
> +
> +#elif LL_WINDOWS // PROCESSOR_FREQUENCY_MEASURE_AVAILABLE
>  	// If there are invalid measure time parameters, zero msecs for example,
>  	// we've to exit the function
>  	if (uiMeasureMSecs < 1)

cpu MHz only exists on x86 and AMD64. It's "clock" on PowerPC.

/proc might not be the best place to fetch the current CPU speed from,
/sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq is prolly more
reliable, /sys/ is designed for code to process, but I dunno if it's
actually widespead enough. My i386 box doesn't have it, but it's also
much too slow to run SLViewer...

Frankly, I'm getting more and more uncomfortable with the idea of
measuring the CPU Frequency by rdtscing a _Delay call, whatever that is.

Mind you, the cpu MHz output from /proc/cpuinfo isn't actually any more
reliable, since that's basically what the kernel does during boot on
i386, I believe, and prolly AMD64 too. But at least at that stage,
there's nothing else going on. I dunno if win32 allows you to get kicked
off the CPU unexpectedly, but if that happened in the middle of a Delay,
that'd blow the measurement. I'm guessing that doesn't happen or it
wouldn't have been written like that. ^_^ Although that's then 50ms of
wasted CPU time...

(On my G4 1.5Ghz PowerCPU, the "clock" line is 1499.999000MHz, but I
think that's actually due to rounding on the reads from the timing
data, rather than calibrating it from a loop during boot.)

 ##else
>  U64 LLFastTimer::countsPerSecond()
>  {
> @@ -108,6 +62,8 @@
>  	{
>  		CProcessor proc;
>  		sCPUClockFrequency = proc.GetCPUFrequency(50);
> +		if(!sCPUClockFrequency)
> +			sCPUClockFrequency = 1000000.0;

>  	return U64(sCPUClockFrequency);

I think this rather confusingly ties "has a high-performance counter"
code to the "has a sensible GetCPUFrequency value" code.

This is fine on win32 (has both), Linux x86 (has both), Linux AMD64 (has
both), and PowerPC (Special-cased with #define, never hits this).

Also the high performance counters falling back to gettime of day, will
be correct.

But it took me two readings to realise that, and I suggest it's a rather
tangled way of approaching the problem.

That is also prolly related to the complete lack of comments giving any
kind of context to these numbers. That's could alternatively be solved
by rearranging this file to keep the countsPerSecond and related
get_cpu_clock_count methods together.

I considered doing that in my patch, but I didn't want to produce a
hard-to-read mess of #defines for patch review, especially when the code
was not as feature-complete as I'd like and I'd had no feedback on the
direction I was taking.

It also needs a comment in the LLFastTimer class definition to the
effect that sCPUClockFrequency is CPUFrequency at first call on i386
Linux, AMD64 Linux and Win32, undefined on PowerPC Linux, and 1 million
on platforms that use gettimeofday to implement fast-timers. That's of
course true of the upstream code as well, as it happens, apart from the
last, arbitrary possible value you've introduced here.

It really out to be renamed, methinks, and used either for all the
countsPerSecond methods, only the ones where GetCPUFrequency is
expensive (ie. win32, under your proposed implementation)

The former makes sense under your implementation, and the latter makes
sense under mine, where I've already split up the different platforms,
even the ones that happen to do the same thing. ('cause I expected
someone would want to implement a similar thing to the linux
PowerPC/AMD64 split in the MacOS X code, and wanted to make sure the
patches didn't turn into a mess of #define changes at the same time)

===

Mind you, I haven't actually looked to see how this sort of thing is
going to handle changes in the timebase on x86 and AMD64 arches, anyway,
but using the static cpu MHz from /proc/cpuinfo at program start can't
be the right solution. As it is, chaning the CPU frequency on win32 or
Linux x86 or Linux AMD64 is going to have a proportional effect on the
results of the llfasttimers.

Bleh. At least on Apple PowerPC hardware, I can rely on the timebase
staying constant, even if I pull out the AC adaptor while it's
happening and my CPU drop to 700Mhz.

Then again, I'd prolly like some of the low-speed CPU effects to kick
in then, so GetCPUFrequency prolly _should_ return the current CPU
speed, but I doubt the featuremanager checks it after startup.

That smells like a different issue to me at this point.

[1] http://www-unix.mcs.anl.gov/zeptoos/docs/license.php

-- 
-----------------------------------------------------------
Paul "TBBle" Hampson, B.Sc, LPI, MCSE
On-hiatus Asian Studies student, ANU
The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361)
Paul.Hampson at Pobox.com

Of course Pacman didn't influence us as kids. If it did,
we'd be running around in darkened rooms, popping pills and
listening to repetitive music.
 -- Kristian Wilson, Nintendo, Inc, 1989

License: http://creativecommons.org/licenses/by/2.1/au/
-----------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.secondlife.com/pipermail/sldev/attachments/20070603/27735318/attachment.pgp