Hi Vladislav,
I have added cc:commits@stripped.
Vladislav Vaintroub wrote:
> Hi Peter, Marc.
>
> my_rdtsc.c is in fact almost the same as rdtsc3.c and I'll repeat most of what I
> said
> And I'll also add 2 nagging point, about use of GetTickCount
>
> Major nagging points
>
> 1) my_timer_milliseconds uses GetTickCount(). GetTickCount() returns milliseconds
> since the start of the system, which is what the function needs. But, it returns it in 4
> byte integer. 4 byte integer will overflow in 49.7 days. I do not think we want huge or
> negative elapsed times, not even once in 49.7 days. Because of wrap around, I strongly
> believe using this function is not appropriate.
>
> Could you consider something else?
>
> For example, using GetSystemTimeAsFileType(), the functionality can be implemented as
> 3liner:
>
> FILETIME ft;
> GetSystemTimeAsFileTime( &ft );
> return ((ulonglong)ft.dwLowDateTime +
> (((ulonglong)ft.dwHighDateTime) << 32))/10000;
>
>
Okay.
We won't avoid rollover/overflow and we take no
measures to check whether it happens. However,
your proposed code works fine in a test, and I
think we can just shift GetTickCount elsewhere,
see the reply about 'ticks'.
> 2) my_timer_ticks implementation on Windows is dummy (always returns 0) .
> Will something go wrong? I suspect yes, as otherwise there would not be a need for
> this routine.
We don't expect anything to go wrong. Sometimes
a platform has no appropriate function. I think
that's not a big deal and not a surprise.
It is quite possible that there is no need for
a ticks routine -- we include it because maybe
some user will find such a need, but if that
doesn't happen, we might decide to remove it.
But I propose to put GetTickCount as a 'ticks'
routine for now, because:
* There is a way to do this with Vista or
Windows Server 2008 without fear of overflow:
use GetTickCount64.
* In effect this gives users a choice whether to
pick GetSystemTimeAsFileTime or GetTickcount.
I think that's good because GetSystemTimeAsFileTime
has some problems, which I'll discuss later.
> By the way, why does this routine prefers Posix times() instead of ANSI clock() in
> non-Windows implementation?
>
>
The comment about this in rdtsc3.c itself is:
" clock() -- We don't use because it would overflow frequently."
> 3) Please simplify Windows preprocessor directives. There is no need for
> #if defined(_WIN32) || defined(_WIN64)
>
> ,as _WIN32 is a preprocessor constant that is always defined for 32 and both 64 bit
> compilers. ( see http://msdn.microsoft.com/en-us/library/b0084kay(VS.80).aspx )
>
> - Please replace all occurrences of the above with #if defined (_WIN32)
>
You are right, and your reference to Microsoft documentation
is proof.
This causes me to ask you about some existing MySQL code.
The answers won't affect a review of Performance Schema
but might satisfy a curious onlooker.
1. In include/my_global.h:
"
#if defined(_WIN32) || defined(_WIN64) || defined(__WIN32__) ||
defined(WIN32)
#include <config-win.h>
"
This has the same redundancy that I used.
Should such code be regarded as a bug?
2. In include/mysql.h:
"
#if (defined(_WIN32) || defined(_WIN64)) && !defined(__WIN__)
#define __WIN__
#endif
"
And grep tells me that "#ifdef __WIN__" happens often.
But "#ifdef _WIN32" is far more common.
So is there a rule for when to say "#ifdef __WIN__"?
> - Please replace occurrences of #if !defined(_WIN32) && !defined(_WIN64)
> with #if !defined(_WIN32)
>
> - please replace occurrences of #if (defined(_WIN32)||defined(_WIN64)) &&
> (defined(_M_IX86)||defined(_M_X64))
> with #if defined(_WIN32)
> the code in this #ifdef only does QueryPerformanceCounters() or GetTickCount(), it
> will work on Itanium too.
>
>
This was deliberate. Generally speaking, the "#if ..."s
are supposed to restrict to platforms I could test on
(MySQL Developer Machines), plus some code that is known
to work with MySQL because it appears elsewhere in MySQL
sources (such as the NetWare code). That doesn't mean my
tests are good and the "#if ..."s are good, it just means
one could check my results.
So: Have you tested QueryPerformanceCounter()
and GetTickCount() on a Windows Itanium machine,
using exactly the same code that my_rdtsc.c has?
Until I get a positive answer, I'll believe that
"&& (defined(_M_IX86)||defined(_M_X64))" is safer.
> (Very) minor nagging point:
>
>
> 4) my_timer_cycles
>
> #elif defined(_WIN64) && defined(_M_X64)
> /* For 64-bit Windows: unsigned __int64 __rdtsc(); */
> return (ulonglong) __rdtsc();
>
> Pointless ulonglong cast here.
>
>
No problem.
I propose these changes, tell me if they're satisfactory.
1. Remove _WIN64 where unnecessary (already done).
2. change gettimer_milliseconds function:
REMOVE:
#elif (defined(_WIN32)||defined(_WIN64)) &&
(defined(_M_IX86)||defined(_M_X64))
return (ulonglong) GetTickCount();
ADD:
#elif defined(_WIN32)) && (defined(_M_IX86)||defined(_M_X64))
FILETIME ft;
GetSystemTimeAsFileTime( &ft );
return ((ulonglong)ft.dwLowDateTime +
(((ulonglong)ft.dwHighDateTime) << 32))/10000;
3. change gettimer_ticks function:
ADD:
#elif defined(_WIN32) && (defined(_M_IX86)||defined(_M_X64)) &&
(_WIN32_WINNT >= 0x0600)
return (ulonglong) GetTickCount64();
#elif defined(_WIN32) && (defined(_M_IX86)||defined(_M_X64))
return (ulonglong) GetTickCount();
4. change list of #defines:
ADD:
#define MY_TIMER_ROUTINE_GETSYSTEMTIMEASFILETIME 26
#define MY_TIMER_ROUTINE_GETTICKCOUNT64 27
5. change my_timer_init /* milliseconds */:
REMOVE:
#elif (defined(_WIN32)||defined(_WIN64)) &&
(defined(_M_IX86)||defined(_M_X64))
mti->milliseconds_routine= MY_TIMER_ROUTINE_GETTICKCOUNT;
ADD:
#elif defined(_WIN32)) && (defined(_M_IX86)||defined(_M_X64))
mti->milliseconds_routine= MY_TIMER_ROUTINE_GETSYSTEMTIMEASFILETIME;
6. change my_timer_init /* ticks */
ADD:
#elif defined(_WIN32) && (defined(_M_IX86)||defined(_M_X64)) &&
(_WIN32_WINNT >= 0x0600)
mti->ticks_routine= MY_TIMER_ROUTINE_GETTICKCOUNT64;
#elif defined(_WIN32) && (defined(_M_IX86)||defined(_M_X64))
mti->ticks_routine= MY_TIMER_ROUTINE_GETTICKCOUNT;
The "&& (_WIN32_WINNT >= 0x0600)" is a Microsoft recommendation,
but I'm uncertain about its style, and haven't tested with Vista.
Yes, Vista and SQL Server 2008 are platforms that don't
meet my earlier criteria, they're not in MySQL Developer Machines.
But surely somebody exists who can check this out.
After making those changes in rdtsc3.c, I tried it out with
two Windows platforms. Here are results:
[win2003-x86, 32-bit, Microsoft Compiler Version 13]
mysqldev@win2003-x86:~/pgulutzan> ./rdtsc3.exe
cycles nanoseconds microseconds milliseconds ticks
------------- ------------- ------------- ------------- -------------
4 0 14 26 15
3189450000 0 3189450000 1024 1023
1 0 1296 15 1
88 0 964 240 36
[win2003-amd64, 64-bit, Microsoft Compiler Version 14]
-bash-3.2$ ./rdtsc3.exe
cycles nanoseconds microseconds milliseconds ticks
------------- ------------- ------------- ------------- -------------
5 0 14 26 15
1792000000 0 1792000000 946 969
1 0 241 16 1
1 0 244 6 4
In the above, the "milliseconds" column is saying that
routine = GetSystemTimeAsFileTime
resolution = 15 or 16 milliseconds
overhead = 240 or 6 cycles
And the "tick" column is saying that
routine = GetTickCount
resolution = 1 tick i.e. 15 milliseconds
overhead = 36 or 4 cycles
Incidentally, last year there was a bug:
Bug#33748 my_getsystime, my_micro_time and my_micro_time_and_time broken
on windows
I commented at the time that GetSystemTimeAsFileTime
is imprecise. Once the WL#2373 code is pushed, we will
have better ways to do some things in my_getsystime.c.
--
Peter Gulutzan
Database Group / MySQL www.mysql.com
Sun Microsystems of Canada Inc.
Edmonton, AB, Canada
