MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Olav Sandstaa Date:June 10 2009 8:49pm
Subject:Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio and
SPARC specifics)
View as plain text  
Hi Peter,

I have some responses and a few more comments.

Peter Gulutzan wrote:
> Hi Olav,
>
> Olav Sandstaa wrote:
>   
>> 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.
>>
>>     
>
> If I understand correctly (and I usually don't), the proposal is:
> in configure.in:
>   

You did understand correctly.

> Remove:
> "
> # 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
> "
> Add:
> "
> # When compiling with Sun Studio C / C++ we need to include
> # my_timer_cycles.il, an "inline templates" separate file,
> # on the command line. It has assembly code, "rd %tick" for
> # SPARC or "rdtsc" for x86.
> RDTSC_SPARC_ASSEMBLY=""
> case $CXX_VERSION in
>   *Sun*C*)
>     RDTSC_SPARC_ASSEMBLY="my_timer_cycles.il"
>     ;;
> esac
> case $CXX_VERSION in
>   *Sun*C++*)
>     RDTSC_SPARC_ASSEMBLY="my_timer_cycles.il"
>     ;;
> esac
> "
> This is certainly more correct, since the question
> isn't what the host is (gcc can run on SPARC, Sun Studio
> can run on non-SPARC, etc.), but what the compiler is.
> I have a few trivial random thoughts about this change:
> * The name RDTSC_SPARC_ASSEMBLY is harmless but the .il
>   now has both SPARC and x86 assembly code.
>   

Good point. It might be a good idea to change the name 
RDTSC_SPARC_ASSEMBLY to something that did not include SPARC.

> * The requirement *Sun*C* is harmless but I'll guess that
>   Sun Studio compilers always deliver "Sun C...", that is,
>   maybe there's a more exact way to specify here.
>   

I think this is fine and is also used a few other places in configure. 
It has worked until now - but it is hard to predict if the "Sun" in the 
compiler name might be changed in the future to something else :-) I do 
not know about a more exact way to specify this.

> * Putting the .il file on the command line is harmless,
>   even for old Sun C versions where we don't use the code
>   (notice that in my_rdtsc.c we say we won't use the code
>   if "defined(__SunOS_5_7)").
>   

I agree that this is harmless - but this made me think of another 
potential issue. Including the .il file on the command line might be 
fine for older Sun C versions but where will they find the 
implementation of e.g. my_timer_cycles_il_sparc64? I guess using eg. Sun 
Studio 10 will lead to a linking issue for my_timer_cycles_il_sparc64? 
(I have not verified this but can do a test compile tomorrow).

> Do you agree with the above configure.in change?
>   

Yes (given that one of the CXX_VERSION being replaced by CC_VERSION as 
you pointed out in your second email). I have compiled with this change 
on a few Solaris platforms (Solaris 10 on SPARC 64 and Solaris 10 on 
AMD64) and this works as expected.

>   
>> * (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.
>>
>>     
>
> Yes, the second implementation is a new addition and I didn't
> notice that it should have worked anyway. In an attempt to avoid
> changing the code, I'll just suggest to Marc: you might add:
> "
> /* This is probably redundant for __SUNPRO_C. */
> "
> before "return (my_timer_cycles_il_i386());".
>   

Fine with me - although I had preferred that the first implementation 
had been removed.
>   
>> * (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?
>>
>>     
>
> I don't think so. It's more than a 32-bit / 64-bit distinction.
> As I understand things: with SPARC v8 or earlier, these inline
> assembly instructions just won't work. You need SPARC v8+ or later.
> Now, should the output of a BUILD (e.g. the binary distributed to
> customers) work with SPARC v8, as well as SPARC v8+? I fear so.
>   

Does our binaries actually work on a SPARV v8? Do we test it? (another 
related questions: does any of the Solaris version we are supposed to 
support run on SPARC v8? I do not know).

Unless told otherwise (and I do not know what our official build script 
does) the Sun Studio compiler will produce code for SPARC V9.

> Our page is vague, it just says "SPARC".
> http://dev.mysql.com/doc/refman/5.1/en/connector-mxj-install-platforms.html
> Therefore you cannot define __sparcv8plus on the command line
> unless you're doing a custom build, and without __sparcv8plus
> it's not safe to use the instructions, so you end up with gethrtime.
>   

I tried to raise the same question when I did some SPARC assembly for 
Falcon. I probably asked the wrong people since the answer was 
relatively vague but I concluded that for Falcon (and thus mysql-6.0) it 
was sufficient to support sparc v8 plus/v9.
> However, when we finally decide we don't need to support SPARC v8,
> or when somebody decides to do a custom build and knowingly adds
> __sparcv9 or __sparcv8plus, my_rdtsc.c will smoothly switch to
> "rd %tick". At least, that's my hopeful logic and intent.
>   

I think your logic is fine and should work.

About decide to not needing to support SPARC v8: I think that decision 
should be taken now. As the code is now a default build on SPARC using 
Sun Studio will likely not have any benefit from you excellent work on 
providing ultra fast assembly implementation of the cycle timer code 
(the default is to build for 32 bit memory model on SPARC).

A small fact about SPARC v8: The last processor Sun produced with the 
SPARC V8 instruction set was the super fast SuperSPARC II running at the 
blazing speed of 90 MHz (in 1994).
(my last SPARC V8 machine run even faster: it had a 125 MHz hyperSPARC....)

>> * (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).
>>     
>
> It's an interesting observation. I don't ignore it,
> I pretend it's a feature. This way the user
> on that platform has a way to choose whether the timer
> will be clock_gettime() or gethrtime().
>   

Great - and you even have provided code to give the user data about the 
overhead of these two methods. So she has both the opportunity to choose 
and the data to make the right choice :-)

Olav

Thread
Review of WL#2373 "Use cycle counter for timing" (Sun Studio and SPARCspecifics)Olav Sandstaa15 May
  • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Peter Gulutzan15 May
    • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Peter Gulutzan9 Jun
      • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Olav Sandstaa9 Jun
        • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Peter Gulutzan9 Jun
          • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Peter Gulutzan9 Jun
          • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Olav Sandstaa10 Jun
            • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Peter Gulutzan12 Jun
              • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Olav Sandstaa16 Jun
                • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Peter Gulutzan17 Jun
                  • Re: Review of WL#2373 "Use cycle counter for timing"Peter Gulutzan27 Oct
                    • RE: Review of WL#2373 "Use cycle counter for timing"Vladislav Vaintroub27 Oct
                      • Re: Review of WL#2373 "Use cycle counter for timing"Peter Gulutzan27 Oct
                    • Re: Review of WL#2373 "Use cycle counter for timing"Olav Sandstaa28 Oct
                      • RE: Review of WL#2373 "Use cycle counter for timing"Vladislav Vaintroub28 Oct
                    • Re: Review of WL#2373 "Use cycle counter for timing"Kay Röpke2 Nov
            • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Peter Gulutzan8 Dec
              • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Olav Sandstaa9 Dec
                • RE: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Vladislav Vaintroub9 Dec
                  • Re: Review of WL#2373 "Use cycle counter for timing" (Sun Studio andSPARC specifics)Peter Gulutzan10 Dec