149154 - Use -xstrconst to avoid heavy string overhead

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Nidheesh Dubey]

There's a lot of string overhead in many objects:

	libgfx_gtk.so strings:
	  53 nsFontMetricsGTK.cpp
	  52 , %s %d
	  44 file %s: line %d: assertion `%s' failed.
	  40 nsRenderingContextGTK.cpp
	  12 mTranMatrix != NULL
	  12 mSurface != NULL
	  ...

	libjavaplugin_oji.so strings:
	  62 Invalid signature: %s
	  36 remotejni: %s
	  34 JavaVM5 %s
	  20 JavaPluginFactory5:%s
	  18 JavaPluginInstance5:%s
	  12 JavaVM5 %s %X
	  10 Internal error: Null jvm manager
	  10 CSecureJNIEnv %s
	  ...

     Each string typically requires an individual relocation, plus the
     duplication bloats the object.  -xstrconst can reduce some of this,
     as it moves strings to the text segment *and* uniquifies them too.
     Note that this option can't remove string duplicates seem between
     different relocatable objects, that's a trick we're still working
     with the compiler folks to achieve.

Comment 1

[Roland Mainz]

(Dumb ?!) question:
Are "combined" strings created by "-xstrconst" put into read-only memory (e.g.
write access to these strings --> SIGSEGV) ?

If the answer is "no" we may face all kinds of hard-to-track issues (I would not
worry much if there is a gcc option which can be turned "on" for gcc-based
builds at the same time, too - which would mean that both Sun Workshop and gcc
builds would suffer from the same issue if someone accidently writes to these
shared strings) ...

Comment 2

[Roland Mainz]

cls:
Do you know if gcc has a matching option which does the same as WS's
"-xstrconst" ?

Comment 3

[Roland Mainz]

bbaetz/bz:
Any idea about the question in comment #2 ?

Comment 4

[Roland Mainz]

It looks gcc 3.x does this by default: See
http://gcc.gnu.org/onlinedocs/gcc-3.0.4/gcc_9.html#IDX1273

Can anyone confirm this for gcc 2.95.x, please ?

Comment 5

[Roland Mainz]

I tried a small example program to test whether -xstrconst may hurt:
-- snip --
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main( void )
{
    const char *s1 = "hello";
    const char *s2 = "hello";
    puts( "start." );

    puts("* Intial values:");
    printf("s1='%s'\n", s1);
    printf("s2='%s'\n", s2);
    
    puts("* Modifying s1...");
    strcpy((char *)s1, "foo");
    printf("s1='%s'\n", s1);
    printf("s2='%s'\n", s2);

    puts( "stop." );
    
    return( EXIT_SUCCESS );
}
-- snip --

Regardless if -xstrconst is enabled or not this program crashes like this:
-- snip --
(process id 27626)
start.
* Intial values:
s1='hello'
s2='hello'
* Modifying s1...
signal SEGV (access to address exceeded protections) in strcpy at 0xff2b6e50
0xff2b6e50: strcpy+0x0324:      st      %i3, [%i0]
Current function is main (optimized)
   18       strcpy((char *)s1, "foo");
(dbx) where
  [1] strcpy(0x10730, 0x10784, 0x10730, 0x666f6f00, 0xff335eec, 0x0), at
0xff2b6e50
=>[2] main() (optimized), at 0x106d4 (line ~18) in "helloworld.c"
-- snip --

Therefore writes to |const|-strings seems to be catched on Solaris/SPARC.

Comment 6

[Roland Mainz]

Taking myself...

Comment 7

[Bradley Baetz (:bbaetz)]

gcc definately puts them into a read only section, and has done for ages. On
windows, VC6 doesn't, but ISTR seeing a bug because it does on VC7, and some
win-only code doesn't support that.

g++ merges constant strings in the same file weven w/o optimistaion; and a
really quick test says that under optimisation it (or rather, the linker) merges
them accross different files.

Running strings over the same lib in my g++-3.2 -O2 debug build has only a few
strings there more than a few times, and almost all of those are bogus (PlY^P,
for example). Probably debug stuff, which is very verbose in g++.

Those which look valid and appear more than once are:

      4                     desired=%d, scaled=%d, bitmap=%d
      2 XGetGeometry
      2 rejecting font '%s' (via reject pattern)
      2 rejecting font '%s' (via hardcoded workaround for bug 103159)
      2 rejecting font '%s' (via accept pattern)
      2 FT_Get_Sfnt_Table
      2 bitmap scaled font: %s

Comment 8

[Roland Mainz]

Nidheesh Dubey:
It looks that "xstrconst" is /opt/SUNWspro/bin/cc only and that
/opt/SUNWspro/bin/CC does that functionality by default in standard mode.

Can you confirm that ?

Comment 9

[Nidheesh Dubey]

That is right CC by default uses strconst. Only cc requires that.

nidheesh

Comment 10

[Roland Mainz]

Nidheesh Dubey wrote:
> That is right CC by default uses strconst. Only cc requires that.

Mozilla has - with the exception of NSPR, NSS, XprintUtil and XlibRgb - nearly
zero C files - most of the code is C++ ...
I could only imagine that NSPR and NSS would get a (very minor) gain from this
change.

Do you think it's worth the effort ? 

Comment 11

[Boris Zbarsky [:bzbarsky]]

You forgot the JS engine, which is all pure C, iirc.  Not to mention the PLDHash
implementation. And likely a few other performance-critical things.

Comment 12

[Roland Mainz]

Boris Zbarsky wrote:
> You forgot the JS engine, which is all pure C, iirc.  Not to mention the 
> PLDHash implementation. And likely a few other performance-critical things.

Mhhh, any idea if there are many string contants in js/ and PLDHash ?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Probably none in the latter.  May be some in the former..  Might want to ask
someone who knows that code, like Brendan...

Comment 14

[Roland Mainz]

I just finished a test build, using "-xstrconst" and "-xbuiltin=%all" seems to
be save...

Comment 15

[Roland Mainz]

Attachment: Patch for 2002-10-12-08-trunk

The patch adds -xstrconst and -xbuiltin=%all to both configure.in and NSPR's
configure.in (and removing the obsolete |-Qoption cg -xstrconst| from NSPR
configure.in per bug 149154 comment #9) ...

Comment 16

[Roland Mainz]

CC:'ing wtc since the patch affects NSPR, too...

Comment 17

[Roland Mainz]

Requesting r=/a= ...

Comment 18

[hacker formerly known as [e-mail]]

Comment on attachment 103103 [details] [diff] [review]
Patch for 2002-10-12-08-trunk

r=cls

Comment 19

[Wan-Teh Chang]

Comment on attachment 103103 [details] [diff] [review]
Patch for 2002-10-12-08-trunk

In mozilla/nsprpub/configure.in, you have:

>-        CC="$CC -xstrconst"
>-        CXX="$CXX -Qoption cg -xstrconst"
>+        CC="$CC -xstrconst -xbuiltin=%all"
>+        CXX="$CXX -xbuiltin=%all"

1. What is the "-xbuiltin=%all" option?  It is not
supported by Workshop 4.2 and 5.0 compilers.

2. The change to CXX is wrong because it removes
the -Qoption cg -xstrconst flags that we are using.

Comment 20

[Roland Mainz]

Wan-Teh Chang wrote:
> (From update of attachment 103103 [details] [diff] [review])
> In mozilla/nsprpub/configure.in, you have:
>
> >-        CC="$CC -xstrconst"
> >-        CXX="$CXX -Qoption cg -xstrconst"
> >+        CC="$CC -xstrconst -xbuiltin=%all"
> >+        CXX="$CXX -xbuiltin=%all"
>
> 1. What is the "-xbuiltin=%all" option? 

It allows that the compiler can replace some functions like |strcpy|/|memcpy|
with compiler builtins and allowed the optimizer to figure-out what these
functions do.
AFAIK it's one of the rare options in Sun Workshop/Forte/ONE which is not
causing trouble, even in the FCS releases.

> It is not
> supported by Workshop 4.2 and 5.0 compilers.

Read 
http://groups.google.com/groups?hl=en&lr=&ie=UTF-8&safe=off&selm=an2obu%24aqn1%40ripley.netscape.com
Support for Workshop 4.2 and 5.0 has been dropped (4.2 did not work anyway since
a long time and 5.0 was a mess with anything beyond -xO2 (=compiler bug h*ll)).

> 2. The change to CXX is wrong because it removes
> the -Qoption cg -xstrconst flags that we are using.

1. Did you read comment #9 ? Per [e-mail] it's on by default.
2. Does NSPR have any C++ source files at all ?

Comment 21

[Wan-Teh Chang]

Roland, cls, I suggest that you go ahead and check
in the mozilla/configure.in change first.

I need to get a second opinion on adding -xbuiltin=%all
to NSPR.  There are still NSPR customers using Workshop
4.2 and 5.0 compilers.  Adding -xbuiltin=%all to NSPR
now would require an autoconf test.  I should be able
to just add -xbuiltin=%all without an autoconf test when
NSPR also requires Forte 6 Update 2 as the minimum compiler
version.

I didn't read comment #9.  Sorry.  It should be
harmless to leave the -Qoption cg -xstrconst flag
there.

Comment 22

[Roland Mainz]

Wan-Teh Chang wrote:
> Roland, cls, I suggest that you go ahead and check
> in the mozilla/configure.in change first.

;-((

OK

> I need to get a second opinion on adding -xbuiltin=%all
> to NSPR.  There are still NSPR customers using Workshop
> 4.2 and 5.0 compilers. 

For what product ?

> Adding -xbuiltin=%all to NSPR
> now would require an autoconf test.  I should be able
> to just add -xbuiltin=%all without an autoconf test when
> NSPR also requires Forte 6 Update 2 as the minimum compiler
> version.
>
> I didn't read comment #9.  Sorry.  It should be
> harmless to leave the -Qoption cg -xstrconst flag
> there.

Sure, but getting some stuff cleaned-up would be nice...

Comment 23

[Roland Mainz]

Attachment: Same patch as previous one exclduing the NSPR part

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

a=roc+moz for trunk checkin

Comment 25

[Roland Mainz]

OK, the patch for the Mozilla tree has been checked-in by cls.

----

wtc:
Should I file a seperate bug for the NSPR part of the patch and close this one
or should I reassign this one to you ?

Comment 26

[Wan-Teh Chang]

Roland, please open a NSPR bug and close this one.  Thanks.

Comment 27

[Roland Mainz]

Filed bug 175413 for the NSPR-specific part of this bug.

----

Marking bug as FIXED per comment #25.