Last Comment Bug 424917 - Performance regression with studio 12 compiler
: Performance regression with studio 12 compiler
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: x86 SunOS
: P2 normal (vote)
: 3.12
Assigned To: Julien Pierre
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-24 22:06 PDT by Julien Pierre
Modified: 2008-06-19 16:23 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Add required option (563 bytes, patch)
2008-03-24 22:08 PDT, Julien Pierre
nelson: review+
Details | Diff | Splinter Review

Description Julien Pierre 2008-03-24 22:06:34 PDT
This problem occurs on x64. The compiler defaults changed and the RSA performance on AMD64 drops by 43% with the current compiler flags. We need to add -xprefetch=no to get back to the same level as we were getting with studio 11.
Comment 1 Julien Pierre 2008-03-24 22:07:33 PDT
Only P2 because we aren't building officially with Studio 12 yet. But we may do so eventually.
Comment 2 Julien Pierre 2008-03-24 22:08:34 PDT
Created attachment 311508 [details] [diff] [review]
Add required option

Also see Sun CR 6637542 for what the compiler guys had to say.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2008-03-25 10:27:12 PDT
Comment on attachment 311508 [details] [diff] [review]
Add required option

Questions and observations:
> 	    ASFLAGS += -xarch=generic64 -K PIC
>+            CFLAGS += -xprefetch=no

For consistency with the rest of this makefile, 
I'd say you want to use SOL_CFLAGS rather than CFLAGS.

Do you only want to do this in this Makefile?
Why not in coreconf?

Do you want to do this for all Solaris sparc builds,
including those with other versions of Studio?  
Do other versions of Studio ignore this option?
Comment 4 Julien Pierre 2008-03-25 15:07:26 PDT
Nelson,

In the absence of regular performance regression testing, I can't really say if we want to do this in all of coreconf or just in this Makefile. Basically, the compiler guys say this can help in some cases and hurt in others. And the compiler isn't smart enough to automatically figure out which ones :-( I do know we want do set this for freebl (actually, mpi) on AMD64 because I have personally tested this. I have not tested performance recently on Sparc.

I'm OK with using SOL_CFLAGS instead of CFLAGS to set this.
Comment 5 Julien Pierre 2008-03-25 18:48:36 PDT
Nelson,

I also verified that -xprefetch=no was supported by studio 11 (and I believe it is the default there and has no effect).
Comment 6 Nelson Bolyard (seldom reads bugmail) 2008-03-26 18:27:30 PDT
How about studio 10?  Do we claim to still support that compiler?
What about earlier compilers?

I'd really like it if this workaround could be applied only to the compiler
version(s) that really need it.  Basically, I don't want this workaround to 
have any detrimental effects on any other compilers.  
Comment 7 Julien Pierre 2008-03-26 18:31:58 PDT
We don't claim to support studio 10 anymore. We don't have different Makefile macros for the different versions of Studio compilers on Solaris.
As I wrote the patch, even though it applies to both Studio 11 and 12, the switch has no effect on studio 11, and does what's needed for studio 12.
Comment 8 Nelson Bolyard (seldom reads bugmail) 2008-03-26 20:29:39 PDT
Comment on attachment 311508 [details] [diff] [review]
Add required option

OK, as long as support for Studio 10 is a non-issue, this patch is OK.
Comment 9 Julien Pierre 2008-03-27 17:35:58 PDT
Thanks for the review, Nelson. I made the change on the trunk with SOL_CFLAGS .

Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.92; previous revision: 1.91
done
Comment 10 Julien Pierre 2008-06-19 16:23:30 PDT
Christophe,

To test this fix, use :
rsaperf -n none -p 30 -t x
(where x is the number of cores/CPUs)
with and without the fix.


Note You need to log in before you can comment on or make changes to this bug.