static initializers

[David Moore]

On linux we have approximately 1200 modules with at least one static initializer (there may be more than one such initializer in each module). The bulk are explained by the inclusion of header files, leaving around 200 that come from using static variables whose values can't be determined at compile time. Somewhere between 50 and 100 are in chromium code proper, with the rest coming from various third party libraries. Usage of these goes against our style guidelines.

Here's the details:

I started investigating this because on ChromeOS with current hardware we spend about 400ms getting to main() of Chrome. This is because about 26MB needs to be paged in. The data all shows that we're pinned I/O wise during this time, and it fits the read speed of about 70MB/s that we see on the hardware we're testing with. I believe that a significant part of this to come from the code and data paging necessary to run the static initializers, although I'm not sure yet just how much. We have a solution at the OS level that will allow this data to be read during boot without slowing down other boot tasks, but even then less data to read is better.

A few uses / patterns explained a significant percentage:

1. Including <iostream>
   This file has this in it:
   // For construction of filebuffers for cout, cin, cerr, clog et. al.
   static ios_base::Init __ioinit;
   I believe this is to allow people to use buffered I/O within their own static initializers. But any file that includes iostream in any way gets a static initializer. Commenting this out reduced the number of files with at least one to about 600, and the data read to 24.5MB.
   
   
2. Including any header that contains a use of scoped_ptr_malloc<>
   We have 9 uses in header files. This template includes this declaration:
3. static FreeProc const free_;
   When I remove this declaration (and I'm sure break much of chrome) it further reduces the number of files with at least one static initializer to about 225, and the data read to 19.5MB.
   
   
4. Statically declaring a SkColor variable with the result of SkColorSetRGB() or SkColorSetARGB()
   Even though SkColorSetARGB() is declared as inline the compiler still generates a static initializer. If I replace it with a macro it doesn't. That reduces the files with at least one to about 200, with little affect on the amount of data read.

I've spot checked the remaining 200 and they appear to fall into a few camps:

1. Using std::string or std::wstring when we should be using char or wchar_t for string constants.
2. Using a static class instance of another kind as a variable, when it should instead be contained within a static function with a local static variable
3. Using a static scaler but setting its value to the result of a function call that can't be computed at compile time, such as string16.

I've attached the list of these files. This list only has the filenames and includes third party things as well.

Here's what I think we should do:

1. Find solutions to the 3 patterns above. Perhaps we're including these things way more aggressively than we need to. Or maybe there's a way to avoid the statics in the first place.
2. Fix the remaining things. Most of the changes (at least w/in the chromium source) are straightforward. I can do it, or the people who know the code better can.
3. Get better about looking for these things in code reviews and reject any changes that contain code that produces static initializers. I believe webkit already does this.
4. Add tools to the perfbots that can track the number of static initializers that we have in the code. I haven't looked at the Windows or Mac side of this, but it's relatively easy to find them in the ELF format, using readelf and looking for the .ctors section.

Dave

[Peter Kasting]

On Wed, Jul 14, 2010 at 10:51 AM, David Moore <dave...@chromium.org> wrote:

1. Get better about looking for these things in code reviews and reject any changes that contain code that produces static initializers. I believe webkit already does this.

I believe WebKit actually won't compile if you add these, although I'm not sure.

PK

[Amit Joshi]

Tracking bug with some more information: http://code.google.com/p/chromium/issues/detail?id=48097

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Antoine Labour]

On Wed, Jul 14, 2010 at 10:51 AM, David Moore <dave...@chromium.org> wrote:

On linux we have approximately 1200 modules with at least one static initializer (there may be more than one such initializer in each module). The bulk are explained by the inclusion of header files, leaving around 200 that come from using static variables whose values can't be determined at compile time. Somewhere between 50 and 100 are in chromium code proper, with the rest coming from various third party libraries. Usage of these goes against our style guidelines.

Here's the details:

I started investigating this because on ChromeOS with current hardware we spend about 400ms getting to main() of Chrome. This is because about 26MB needs to be paged in. The data all shows that we're pinned I/O wise during this time, and it fits the read speed of about 70MB/s that we see on the hardware we're testing with. I believe that a significant part of this to come from the code and data paging necessary to run the static initializers, although I'm not sure yet just how much. We have a solution at the OS level that will allow this data to be read during boot without slowing down other boot tasks, but even then less data to read is better.

A few uses / patterns explained a significant percentage:

1. Including <iostream>
   This file has this in it:
   // For construction of filebuffers for cout, cin, cerr, clog et. al.
   static ios_base::Init __ioinit;
   I believe this is to allow people to use buffered I/O within their own static initializers. But any file that includes iostream in any way gets a static initializer. Commenting this out reduced the number of files with at least one to about 600, and the data read to 24.5MB.
   
   
2. Including any header that contains a use of scoped_ptr_malloc<>
   We have 9 uses in header files. This template includes this declaration:
3. static FreeProc const free_;
   When I remove this declaration (and I'm sure break much of chrome) it further reduces the number of files with at least one static initializer to about 225, and the data read to 19.5MB.

I understand that it generates data, but why is this one generating initializers ? It's a POD, it should be resolved at link time. What does the initializer look like ?

1. 
   
2. Statically declaring a SkColor variable with the result of SkColorSetRGB() or SkColorSetARGB()
   Even though SkColorSetARGB() is declared as inline the compiler still generates a static initializer. If I replace it with a macro it doesn't. That reduces the files with at least one to about 200, with little affect on the amount of data read.

Have you tried marking those 2 functions const and see if it helps ?

Antoine

 

I've spot checked the remaining 200 and they appear to fall into a few camps:

1. Using std::string or std::wstring when we should be using char or wchar_t for string constants.
2. Using a static class instance of another kind as a variable, when it should instead be contained within a static function with a local static variable
3. Using a static scaler but setting its value to the result of a function call that can't be computed at compile time, such as string16.

I've attached the list of these files. This list only has the filenames and includes third party things as well.

Here's what I think we should do:

1. Find solutions to the 3 patterns above. Perhaps we're including these things way more aggressively than we need to. Or maybe there's a way to avoid the statics in the first place.
2. Fix the remaining things. Most of the changes (at least w/in the chromium source) are straightforward. I can do it, or the people who know the code better can.
3. Get better about looking for these things in code reviews and reject any changes that contain code that produces static initializers. I believe webkit already does this.
4. Add tools to the perfbots that can track the number of static initializers that we have in the code. I haven't looked at the Windows or Mac side of this, but it's relatively easy to find them in the ELF format, using readelf and looking for the .ctors section.

Dave

--
Chromium OS Developers mailing list: chromiu...@chromium.org

View archives, change email options, or unsubscribe:

http://groups.google.com/a/chromium.org/group/chromium-os-dev?hl=en

[Evan Martin]

On Wed, Jul 14, 2010 at 10:57 AM, Peter Kasting <pkas...@google.com> wrote:

http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/StaticConstructors.h
is all I could find. It provides a macro that is used in place of
statics that has the effect of a static without an initializer being
involved.

24 // We need to avoid having static constructors. We achieve this
25 // with two separate methods for GCC and MSVC. Both methods prevent
the static
26 // initializers from being registered and called on program
startup. On GCC, we
27 // declare the global objects with a different type that can be POD default
28 // initialized by the linker/loader. On MSVC we use a special
compiler feature
29 // to have the CRT ignore our static initializers. The constructors
will never
30 // be called and the objects will be left uninitialized.
31 //
32 // With both of these approaches, we must define and explicitly call an init
33 // routine that uses placement new to create the objects and overwrite the
34 // uninitialized placeholders.
35 //
36 // This is not completely portable, but is what we have for now without
37 // changing how a lot of code accesses these global objects.

[Adam Barth]

On Wed, Jul 14, 2010 at 11:17 AM, Evan Martin <ev...@chromium.org> wrote:
> On Wed, Jul 14, 2010 at 10:57 AM, Peter Kasting <pkas...@google.com> wrote:
>> On Wed, Jul 14, 2010 at 10:51 AM, David Moore <dave...@chromium.org>
>> wrote:
>>>
>>> Get better about looking for these things in code reviews and reject any
>>> changes that contain code that produces static initializers. I believe
>>> webkit already does this.
>>
>> I believe WebKit actually won't compile if you add these, although I'm not
>> sure.
>
> http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/StaticConstructors.h
> is all I could find.  It provides a macro that is used in place of
> statics that has the effect of a static without an initializer being
> involved.

There's also a step in the build (at least on mac) that checks for
static initializers and errors out if any are present.

Adam

[Adam Barth]

I could be wrong, but I think it just runs this script:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/check-for-global-initializers

Adam

[David Moore]

Adding const didn't change anything. I've reduced it to all code in one file that looks like this:

typedef unsigned int SkColor;

typedef unsigned char uint8_t;

static const inline SkColor SkColorSetARGB(uint8_t a, uint8_t r, uint8_t g, uint8_t b) {

  return (a << 24) | (r << 16) | (g << 8) | (b << 0);

}

static const SkColor foo = SkColorSetARGB(0, 1, 2, 3);

The initializer is:

   0x080e5342 <+0>:     push   %ebp

   0x080e5343 <+1>:     mov    %esp,%ebp

   0x080e5345 <+3>:     mov    $0xffff,%edx

   0x080e534a <+8>:     mov    $0x1,%eax

   0x080e534f <+13>:    pop    %ebp

   0x080e5350 <+14>:    jmp    0x80e5320 <_Z41__static_initialization_and_destruction_0ii>

which jumps to this:

   0x080e5320 <+0>:     push   %ebp

   0x080e5321 <+1>:     mov    %esp,%ebp

   0x080e5323 <+3>:     sub    $0x1,%eax

   0x080e5326 <+6>:     je     0x80e532a <_Z41__static_initialization_and_destruction_0ii+10>

   0x080e5328 <+8>:     pop    %ebp

   0x080e5329 <+9>:     ret

   0x080e532a <+10>:    cmp    $0xffff,%edx

   0x080e5330 <+16>:    jne    0x80e5328 <_Z41__static_initialization_and_destruction_0ii+8>

   0x080e5332 <+18>:    movl   $0x10203,0xa8c22c4

   0x080e533c <+28>:    pop    %ebp

   0x080e533d <+29>:    lea    0x0(%esi),%esi

   0x080e5340 <+32>:    ret

All the static initializers have that bit of prologue...I guess they're guarding other code jumping to them for some reason.

If use this instead 

#define SkColorSetARGB(a, r, g, b) \

  static_cast<SkColor>((a << 24) | (r << 16) | (g << 8) | (b << 0))

I get no static initializer.

Dave

[Antoine Labour]

On Wed, Jul 14, 2010 at 1:03 PM, David Moore <dave...@chromium.org> wrote:

Adding const didn't change anything. I've reduced it to all code in one file that looks like this:

typedef unsigned int SkColor;

typedef unsigned char uint8_t;

static const inline SkColor SkColorSetARGB(uint8_t a, uint8_t r, uint8_t g, uint8_t b) {

  return (a << 24) | (r << 16) | (g << 8) | (b << 0);

}

To make the function const, you need to declare as:

static inline SkColor SkColorSetARGB(uint8_t a, uint8_t r, uint8_t g, uint8_t b) const {

  return (a << 24) | (r << 16) | (g << 8) | (b << 0);

}

(in your version, const applies to SkColor so it doesn't do anything).

Not sure it'll help at all, and a macro is probably just as good, I'm just curious.

Antoine

[Peter Kasting]

On Wed, Jul 14, 2010 at 1:22 PM, Antoine Labour <pi...@chromium.org> wrote:

To make the function const, you need to declare as:

static inline SkColor SkColorSetARGB(uint8_t a, uint8_t r, uint8_t g, uint8_t b) const

This is an illegal declaration.  SkColoSetARGB() is not a member function and therefore cannot be declared const.

PK 

[Antoine Labour]

My bad, that's not valid in C++. I thought it was the way to declare it as pure in c99, and that they kept that in C++, the latter is definitely not the case and I can't find a reference to the former. So ignore me.

Antoine

[Jochen Eisinger]

gcc supports the function attributes const and pure (const being pure
and additionally does not access global objects). If it would help to
reduce the amount of static initializers, we could add a macro
CONST_FUNCTION or similar that would evaluate to
__attribute__((const)) if compiled with gcc and to nothing otherwise.

>
> --
> Chromium Developers mailing list: chromi...@chromium.org

> View archives, change email options, or unsubscribe:

> http://groups.google.com/a/chromium.org/group/chromium-dev
>

[Jochen Eisinger]

[this time from the correct email address]

gcc supports the function attributes const and pure (const being pure

and additionally does not access global objects). If it would help to
reduce the amount of static initializers, we could add a macro
CONST_FUNCTION or similar that would evaluate to
__attribute__((const)) if compiled with gcc and to nothing otherwise.

>
> --
> Chromium Developers mailing list: chromi...@chromium.org

> View archives, change email options, or unsubscribe:

> http://groups.google.com/a/chromium.org/group/chromium-dev
>

[Jonathan Dixon]

On 14 July 2010 18:51, David Moore <dave...@chromium.org> wrote:

On linux we have approximately 1200 modules with at least one static initializer (there may be more than one such initializer in each module). The bulk are explained by the inclusion of header files, leaving around 200 that come from using static variables whose values can't be determined at compile time. Somewhere between 50 and 100 are in chromium code proper, with the rest coming from various third party libraries. Usage of these goes against our style guidelines.

Here's the details:

I started investigating this because on ChromeOS with current hardware we spend about 400ms getting to main() of Chrome. This is because about 26MB needs to be paged in. The data all shows that we're pinned I/O wise during this time, and it fits the read speed of about 70MB/s that we see on the hardware we're testing with. I believe that a significant part of this to come from the code and data paging necessary to run the static initializers, although I'm not sure yet just how much. We have a solution at the OS level that will allow this data to be read during boot without slowing down other boot tasks, but even then less data to read is better.

A few uses / patterns explained a significant percentage:

1. Including <iostream>
   This file has this in it:
   // For construction of filebuffers for cout, cin, cerr, clog et. al.
   static ios_base::Init __ioinit;
   I believe this is to allow people to use buffered I/O within their own static initializers. But any file that includes iostream in any way gets a static initializer. Commenting this out reduced the number of files with at least one to about 600, and the data read to 24.5MB.
   
   
2. Including any header that contains a use of scoped_ptr_malloc<>
   We have 9 uses in header files. This template includes this declaration:
3. static FreeProc const free_;
   When I remove this declaration (and I'm sure break much of chrome) it further reduces the number of files with at least one static initializer to about 225, and the data read to 19.5MB.
   
   

My experience is scoped_ptr_malloc<> is a bit disappointing to use. Every time I have needed it, I just have a plain old static function I want to bind it to, yet I need to code up a functor class just to call it. It's even more disappointing to find this class has a knock-on cost on app startup time. Perhaps a version for use with a plain function would help (in both simplicity of use, and efficiency),

Alternatively, with the existing versions could we alter it to just instantiate the functor class instance when needed?

~scoped_ptr_malloc() {

-    free_(ptr_);

+    FreeProc()(ptr_);   }

this could have bad side-effects (as the functor will be instantiated more times). If so, maybe some use of Singleton<> could address that

 

 

2. Statically declaring a SkColor variable with the result of SkColorSetRGB() or SkColorSetARGB()
   Even though SkColorSetARGB() is declared as inline the compiler still generates a static initializer. If I replace it with a macro it doesn't. That reduces the files with at least one to about 200, with little affect on the amount of data read.

I've spot checked the remaining 200 and they appear to fall into a few camps:

1. Using std::string or std::wstring when we should be using char or wchar_t for string constants.
2. Using a static class instance of another kind as a variable, when it should instead be contained within a static function with a local static variable
3. Using a static scaler but setting its value to the result of a function call that can't be computed at compile time, such as string16.

I've attached the list of these files. This list only has the filenames and includes third party things as well.

Here's what I think we should do:

1. Find solutions to the 3 patterns above. Perhaps we're including these things way more aggressively than we need to. Or maybe there's a way to avoid the statics in the first place.
2. Fix the remaining things. Most of the changes (at least w/in the chromium source) are straightforward. I can do it, or the people who know the code better can.
3. Get better about looking for these things in code reviews and reject any changes that contain code that produces static initializers. I believe webkit already does this.
4. Add tools to the perfbots that can track the number of static initializers that we have in the code. I haven't looked at the Windows or Mac side of this, but it's relatively easy to find them in the ELF format, using readelf and looking for the .ctors section.

Dave

[Jonathan Dixon]

On 14 July 2010 22:18, Jonathan Dixon <jo...@chromium.org> wrote:

My experience is scoped_ptr_malloc<> is a bit disappointing to use. Every time I have needed it, I just have a plain old static function I want to bind it to, yet I need to code up a functor class just to call it. It's even more disappointing to find this class has a knock-on cost on app startup time. Perhaps a version for use with a plain function would help ...

In case it's any help, I started playing around with this idea: http://codereview.chromium.org/2804042

As I expected, this mostly results in deletion of functor-defining code, with a few tricks needed to adapt FreeXxx() functions that take void* or return something other than void. (My template meta foo is not the best; there maybe cleaner ways to handle some of these conversions)

This is incomplete, but has some merit. The biggest issue I see is the end result would need to be renamed to something other than scoped_ptr_malloc to avoid massive confusion with all the other copies of it that are out there. (Even within chrome we have half a dozen duplicate copies of this class)