Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio and SPARC specifics)

[Olav Sandstaa]

Hi Peter,

Peter Gulutzan wrote:
> Hi Olav (and Marc):
>
> Regardless of the other Solaris difficulty, it's time
> to change mysys/my_timer_cycles.il and include/my_rdtsc.h
> and mysys/my_rdtsc.c as proposed in my earlier email (extracted below).
> I enclose new copies of each file. I am asking Marc
> to replace the old copies with these, and I am asking
> Olav to confirm that this is acceptable.
>   

Thanks for the updated versions of these files. I have looked at the 
changes you have done and these seem to be fine and resolves all my 
earlier comments (with the exception of the safe_mutex linking issue 
which you stated that Marc had some ideas for). Thanks for addressing 
these issues.

I have applied the changes, had a new look at the code  and done some 
more experiment with compiling . And have some new comments:

* I run into a linking issue when trying to link a simple test program 
on Solaris 10 (AMD64) using Sun Studio compiler. It turned out that 
inline assembly routines in my_timer_cycles.il was not used at all. As 
you are aware of the file my_timer_cycles.il must be included on the 
command line in compiling in order to get the code inlined, something like:

       cc ..... my_timer_cycles.il my_rdtsc.c

This seems not to happen on Solaris 10 (but worked fine on a SPARC 
machine running Solaris 9). I might be wrong but I think the following 
in configure.in needs to be updated:

# When compiling with Sun Studio compiler on SPARC assembly code for
# Interlock operations needs to be included. This has been implemented
# as "inline templates" in a separate file
RDTSC_SPARC_ASSEMBLY=""
case $host in
  sparc-sun-solaris2.9)
    case $CXX_VERSION in
      *Sun*C++*)
        RDTSC_SPARC_ASSEMBLY="my_timer_cycles.il"
        ;;
    esac
    ;;
esac

This test limits the inclusion of the inline assembly to only happen on 
Solaris 9. I think it would be OK to remove the test for host (case 
$host in...) and just check for compiler version?

The comment in configure.in could probably also be updated to not 
include anything about Interlocked operations and SPARC.

* (Minor comment): I think you have two identical assembly 
implementations of the my_timer_cycles() for i386 when using Sun Studio 
C compiler:

    1. The first implementation is very early in my_timer_cycles():

#elif defined(__SUNPRO_C) && defined(__i386)
  __asm("rdtsc");

   2. The second implementation is using the inline assembly file:

#elif (defined(__SUNPRO_CC) || defined(__SUNPRO_C)) && defined(__i386) 
&& defined(_ILP32)
  return (my_timer_cycles_il_i386());

I expect that both works (I only tested with the first one) and with the 
exception of that "multiple implementations of the same functionality 
are bad and hard to maintain" this likely causes no issues.

* (Minor issue) On SPARC 32 bit the compiler did not use the inline 
assembly code but used gethrtime() instead. The reason for this is that 
__sparcv8plus was not defined. I normally  run configure with -m32 to 
get 32 bits code on SPARC - I could have included __sparcv8plus when 
running configure (I guess). Would an alternative to explicitly check  
for __sparcv8plus and __sparcv9 be to check the size of a void*? If 
sizeof(void*) is 4 then it use the 32 bit code, otherwise use the 64 bit 
code?

* (Irrelevant) comment/question/observation: in my_timer_nanonseconds() 
the function clock_gettime() will be used on Solaris (you seem to prefer 
this over gethrtime() - which probably is fine) but in my_timer_cycles() 
only gethrtime() will be used on Solaris if there is no assembly code 
for the function (ie. you prefer gethrtime() instead of clock_gettime(). 
(feel free to ignore this - it was just an observation).

Best regards,
Olav


> And, regarding the other Solaris difficulty, Marc has
> some ideas. Olav, please bring a Solaris laptop to the
> Falcon conference. We can all get together and figure it
> out there. I hope that will be the last thing we need to
> do for this round.
>
> /Peter
>
>
> On 2009-05-15 Peter Gulutzan wrote:
>
> <cut>
>   
>> I think we could fix the above-described issues thus:
>>
>> Add to my_timer_cycles.il:
>> .inline my_timer_cycles_il_x86_64,0
>> rdtsc
>> shlq $32,%rdx
>> orq %rdx,%rax
>> .end
>>
>> In my_timer_cycles.il add line breaks and change the comment.
>>
>> Add to my_rdtsc.h:
>> #define MY_TIMER_ROUTINE_ASM_SUNPRO_X86_64       27
>>
>> In my_my_rdtsc.c change all occurrences of
>> "defined(__SUNPRO_CC)", except for the "extern" declarations,
>> to "(defined(__SUNPRO_CC) || defined(__SUNPRO_C))".
>>
>> Add to rdtsc3.c:
>> /* after the other "extern" declarations */
>> #elif defined(__SUNPRO_CC) && defined(__x86_64) &&
> defined(_LP64)
>> extern "C" ulonglong my_timer_cycles_il_x86_64();
>> ...
>> /* in my_timer_cycles */
>> #elif (defined(__SUNPRO_CC) || defined(__SUNPRO_C)) && defined(__x86_64)
>> && defined(_LP64)
>>   return (my_timer_cycles_il_x86_64());
>> ...
>> /* in my_timer_init */
>> #elif (defined(__SUNPRO_CC) || defined(__SUNPRO_C)) && defined(__x86_64)
>> && defined(_LP64)
>>   mti->cycles_routine= MY_TIMER_ROUTINE_ASM_SUNPRO_X86_64;
>>
>> In rdtsc3.c after the "extern" declarations, add non-extern
>> SUNPRO_C declarations, for example
>> #elif defined(__SUNPRO_C) && defined(__x86_64) && defined(_LP64)
>> ulonglong my_timer_cycles_il_x86_64();
>>
>> Quick tests with the above changes applied to rdtsc3.c,
>> on sol10-amd64-a (which has "cc: Sun C 5.8 2005/10/13"), with
>> cc -O -xarch=native64 -o rdtsc3 rdtsc3.c my_timer_cycles.il
>> /usr/lib/64/librt.so
>> look fine. I could test our other Sun boxes any time.
>>
>> But I'm not proposing do that yet, because you described
>> your analysis as "initial comments".
>>
>> I hope we'll be able to say that Performance Schema might
>> work on all the Sun platforms listed on this page:
>> http://www.mysql.com/support/supportedplatforms/enterprise.html
>>
>>   
>>     
>
>
>   
> ------------------------------------------------------------------------
>
>