424917 - Performance regression with studio 12 compiler

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

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]

Only P2 because we aren't building officially with Studio 12 yet. But we may do so eventually.

Comment 2

[Julien Pierre]

Attachment: Add required option

Also see Sun CR 6637542 for what the compiler guys had to say.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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]

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]

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)]

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]

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)]

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]

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]

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.