eliminating static initializers

[Evan Martin]

Static initializers are not only banned by our style guide, we've seen
them impact startup performance for Chrome OS. This is the reason the
"sizes" bot goes red when new code adds static initializers. I wrote
a lot more about this here:
http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html
.

It turns out it's relatively easy to dump some of the sources of
static initialization by scripting gdb.
I have dumped a report here:
http://paste.ubuntu.com/678316/
It shows a list of functions called by the static initializers used in
each file.
If you have a moment, please scan the list for files you are familiar
with and consider removing them.
If you see a file listed by no functions called, that means the static
initialization is likely limited to just code within that file.

PS: I have pending patches to fix xid_map_ and most of the __ioinit
instances in that report, don't worry about those.

[Adam Barth]

As a point of comparison, WebKit (at least on Apple Mac) has a build
step that breaks compile if you add a static initializer. If we get
the Chromium static initializer count down near zero, we should
consider doing something similar.

Adam

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

[Paweł Hajdan, Jr.]

On Tue, Aug 30, 2011 at 14:35, Evan Martin <ev...@chromium.org> wrote:

It turns out it's relatively easy to dump some of the sources of
static initialization by scripting gdb.
I have dumped a report here:
 http://paste.ubuntu.com/678316/
It shows a list of functions called by the static initializers used in
each file.
If you have a moment, please scan the list for files you are familiar
with and consider removing them.
If you see a file listed by no functions called, that means the static
initialization is likely limited to just code within that file.

I'm interested in fixing those. How about filing a metabug for this and individual bugs for each files? That would help avoiding duplication of work. If that seems like too much bureaucracy, how about a spreadsheet?

[Peter Kasting]

On Tue, Aug 30, 2011 at 2:35 PM, Evan Martin <ev...@chromium.org> wrote:

Static initializers are not only banned by our style guide,

To be specific, it's non-POD static initializers that are banned, right?

I notice that e.g. HistoryQuickProvider::kMaxNonInliningScore is on the list, which is an int.  Shouldn't that be OK?

PK 

[Elliot Glaysher (Chromium)]

That'll still still require running code at startup instead of having
it be part of the image, right? I fixed a similar case when I just
moved it to a file local constant.

-- Elliot

[Mark Mentovai]

Peter Kasting wrote:
> To be specific, it's non-POD static initializers that are banned, right?

POD types that are initialized by constant data are OK. POD types that
are initialized by running code aren’t.

> I notice that e.g. HistoryQuickProvider::kMaxNonInliningScore is on the
> list, which is an int.  Shouldn't that be OK?

From chrome/browser/autocomplete/history_quick_provider.cc:

const int HistoryQuickProvider::kMaxNonInliningScore =
AutocompleteResult::kLowestDefaultScore - 1;

Because the value of AutocompleteResult::kLowestDefaultScore is not
known at compile time (it’s defined in
chrome/browser/autocomplete/autocomplete.cc), the only way to
initialize HistoryQuickProvider::kMaxNonInliningScore is by run-time
static initializer.

[Peter Kasting]

On Tue, Aug 30, 2011 at 3:37 PM, Mark Mentovai <ma...@chromium.org> wrote:

Peter Kasting wrote:
> To be specific, it's non-POD static initializers that are banned, right?

POD types that are initialized by constant data are OK. POD types that
are initialized by running code aren’t.

I see.  The Google style guide doesn't make this clear so I added a note to our style guide (although I don't know how clear I was).  I'll fix this.

PK

[Gregg Tavares (wrk)]

Any idea where

    std::__ioinit

is coming from?

Is it any reference to printf? or any reference to logging.h? It's in a lot of modules

[Nico Weber]

See http://code.google.com/p/chromium/issues/detail?id=94794 , Evan's
working on these I think.

Nico

[Evan Martin]

On Tue, Aug 30, 2011 at 3:12 PM, Paweł Hajdan, Jr.
<phajd...@chromium.org> wrote:
> I'm interested in fixing those. How about filing a metabug for this and
> individual bugs for each files? That would help avoiding duplication of
> work. If that seems like too much bureaucracy, how about a spreadsheet?

Filed http://code.google.com/p/chromium/issues/detail?id=94925
A bunch of the cases in my initial report have been fixed, so I'll put
up a new report on the bug to aid in dividing it up.

[Thiago Farina]

Evan,

Do function static members create static initializers?

In crbug.com/83856, it reports
gfx::NativeThemeLinux::instance()::s_native_theme, but Dave said that
function static members don't create static initializers.

[Adam Langley]

On Thu, Sep 1, 2011 at 10:15 AM, Thiago Farina <tfa...@chromium.org> wrote:
> In crbug.com/83856, it reports
> gfx::NativeThemeLinux::instance()::s_native_theme, but Dave said that
> function static members don't create static initializers.

"static const unsigned kFoo = 6;" doesn't create a static initialiser,
but you have "static NativeThemeLinux s_native_theme;". That has to
call the constructor of NativeThemeLinux, so that's a static
initializer. The answer is to heap allocate it.

Cheers

AGL

[William Chan (陈智昌)]

On Thu, Sep 1, 2011 at 7:45 PM, Thiago Farina <tfa...@chromium.org> wrote:
> Evan,
>
> Do function static members create static initializers?

I do not believe that they should. I'd be curious to see Evan's gdb
script. Perhaps he's also picking up static destructors, which is also
a problem due to global destructor ordering not being defined (and
slows down Chrome shutdown). The code for s_native_theme should not be
destroying NativeThemeLinux using a static destructor. If it needs to
be destroyed on shutdown, it should use a LazyInstance. Otherwise, it
should do something like:

static NativeThemeLinux* s_native_theme = new NativeThemeLinux;
return s_native_theme;

or use LeakyLazyInstance.

>
> In crbug.com/83856, it reports
> gfx::NativeThemeLinux::instance()::s_native_theme, but Dave said that
> function static members don't create static initializers.
>

[Thiago Farina]

I indeed did, but see what Dave that. http://codereview.chromium.org/7778036/#msg2

[Thiago Farina]

On Sep 1, 11:27 am, Thiago Farina <tfa...@chromium.org> wrote:
> I indeed did, but see what Dave that.http://codereview.chromium.org/7778036/#msg2

Sorry for the typo here, it should read: "...Dave said about that".

[William Chan (陈智昌)]

Is this true? http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.16
says initialization only happens on first use.

>
>
> Cheers
>
> AGL

[Sigurður Ásgeirsson]

Initialization of function local static members MUST be deferred until first invocation of the containing function per the C++ specification. This does therefore not create a static initializer that runs at program start.

Please note however, that the C++ runtime does not make the initialization threadsafe on any compiler/CRT that I'm aware of, so unless the containing function is only used from one thread (or under an external lock), the initialization is racy.

Note also that non POD static locals are destroyed at program shutdown. The Visual Studio runtime does this by registering an atexit() call for each such static local.

[Adam Langley]

On Thu, Sep 1, 2011 at 10:33 AM, Sigurður Ásgeirsson <si...@chromium.org> wrote:
> Initialization of function local static members MUST be deferred until first
> invocation of the containing function per the C++ specification

Ok, please ignore me, I'm wrong.

Cheers

AGL

[William Chan (陈智昌)]

On Thu, Sep 1, 2011 at 8:03 PM, Sigurður Ásgeirsson <si...@chromium.org> wrote:
> Initialization of function local static members MUST be deferred until first
> invocation of the containing function per the C++ specification. This does
> therefore not create a static initializer that runs at program start.
> Please note however, that the C++ runtime does not make the initialization
> threadsafe on any compiler/CRT that I'm aware of, so unless the containing

It is threadsafe on gcc
(http://stackoverflow.com/questions/1270927/are-function-static-variables-thread-safe-in-gcc)
but we disable it to be compatible with a certain lame Windows
compiler.

> function is only used from one thread (or under an external lock), the
> initialization is racy.

Yes, and that's why you should use LazyInstance.

> Note also that non POD static locals are destroyed at program shutdown. The
> Visual Studio runtime does this by registering an atexit() call for each
> such static local.

And more importantly note that the destruction order is not defined.

[Mark Mentovai]

Sigurður Ásgeirsson wrote:
> Initialization of function local static members MUST be deferred until first
> invocation of the containing function per the C++ specification. This does
> therefore not create a static initializer that runs at program start.
> Please note however, that the C++ runtime does not make the initialization
> threadsafe on any compiler/CRT that I'm aware of, so unless the containing
> function is only used from one thread (or under an external lock), the
> initialization is racy.

GCC (and Clang) do thread-safe initialization unless defeated with
-fno-threadsafe-statics.

Chrome does specify -fno-threadsafe-statics.

[John Abd-El-Malek]

As others said, people should use LazyInstance for this. However, if code is running in another library without access to base, or for some very odd reason it needs to run before AtExitManager (which really shouldn't happen), then this pattern is useful: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/googleurl/src/gurl.cc&exact_package=chromium&q=gurl.cc&type=cs&l=69

[Thiago Farina]

On Sep 1, 12:54 pm, John Abd-El-Malek <j...@chromium.org> wrote:
> On Thu, Sep 1, 2011 at 7:46 AM, Mark Mentovai <ma...@chromium.org> wrote:
> > Sigurður Ásgeirsson wrote:
> > > Initialization of function local static members MUST be deferred until
> > first
> > > invocation of the containing function per the C++ specification. This
> > does
> > > therefore not create a static initializer that runs at program start.
> > > Please note however, that the C++ runtime does not make the
> > initialization
> > > threadsafe on any compiler/CRT that I'm aware of, so unless the
> > containing
> > > function is only used from one thread (or under an external lock), the
> > > initialization is racy.
>
> > GCC (and Clang) do thread-safe initialization unless defeated with
> > -fno-threadsafe-statics.
>
> > Chrome does specify -fno-threadsafe-statics.
>
> As others said, people should use LazyInstance for this. However, if code is
> running in another library without access to base, or for some very odd
> reason it needs to run before AtExitManager (which really shouldn't happen),

> then this pattern is useful:http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/googleurl/src...
>
>
I've uploaded a patch which uses LazyInstance here http://codereview.chromium.org/7778036/.
Please see patch set 3.

[David Michael]

chrome/common/chrome_content_client.cc has a few showing up. I'm not
really sure why, though... I have a CL that should fix it, but can
anybody explain why this sort of code would require a static
initializer to run?

// header:
class Foo {
static const char* kString;
};
/// body:
namespace {
const char* kString = "Blah";
}
const char* Foo::kString = ::kString;

Here's my CL to fix it by replacing each of these static members with
a static function (and cleaning up the fact that the c-string pointers
aren't properly const).

http://codereview.chromium.org/7824011/

I think this approach is no worse, but I'm curious if it's worth the
trouble at all (there are a ton of similar ones, e.g. in
autofill_country.cc).

Thanks!
-Dave

> I've uploaded a patch which uses LazyInstance herehttp://codereview.chromium.org/7778036/.

[Mark Mentovai]

David Michael wrote:
> chrome/common/chrome_content_client.cc has a few showing up. I'm not
> really sure why, though...  I have a CL that should fix it, but can
> anybody explain why this sort of code would require a static
> initializer to run?

Gladly!

> // header:
> class Foo {
>  static const char* kString;
> };
> /// body:
> namespace {
>  const char* kString = "Blah";
> }
> const char* Foo::kString = ::kString;

In this case, the actual chars in ::kString are const, but the pointer
itself is not, therefore ::kString itself is not a compile-time
constant. Someone can come along and say |kString =
"SomeOtherConstantString"| before Foo::kString is supposed to be
initialized.

You can write:

namespace {
const char* const kString = "Blah";
}

or:

namespace {
const char kString[] = "Blah";
}

[David Michael]

So the same problem would exist for an e.g., an integral type?

class Foo {

  static int x;

};

int Foo::x = 4;

I recognized the incorrect const-ness, but didn't realize it required static initialization code if it was non-const; I guess maybe it depends on where it lives in memory. So fixing the c-string to be properly const:

const char* const x = "";

or

const char const[] y = "";

Will fix a lot of these cases, I think.

[Mark Mentovai]

David Michael wrote:
> So the same problem would exist for an e.g., an integral type?
> class Foo {
>   static int x;
> };
> int Foo::x = 4;

No, because 4 is a compile-time constant.

The crucial element here is whether the initializer (in these
examples, the thing to the right of the =) is a compile-time constant
or not. The constness of the thing on the left is irrelevant.

This results in static initializers (no namespaces or classes required):

int x = 0;
int y = x; // x is not const, x may change before it’s time to initialize y

This does not:

const int y = 0;
int y = x; // whether or not y is const is insignificant

int is no different from any pointer here.

“How might x change?” Like this:

int MutateX();

int x = 0;
int m = MutateX();
const int y = x; // illustrative: y's constness is unimportant

int MutateX() {
++x;
return -1;
}

> I recognized the incorrect const-ness, but didn't realize it required static
> initialization code if it was non-const; I guess maybe it depends on where
> it lives in memory. So fixing the c-string to be properly const:
> const char* const x = "";
> or
> const char const[] y = "";
> Will fix a lot of these cases, I think.

Yes, it absolutely will. Looking at your change, you don’t need to
turn those things into functions, you just need to get the constness
right.

[John Abd-El-Malek]

+some infrastructure folks

Can we make this graph go red on _any_ increase, not just a percentage? That would help us make sure no more statics leak in as we fix existing usage. As I understand it now, this only goes red when a bunch of statics go in at the same time.

http://build.chromium.org/f/chromium/perf/linux-release/sizes/report.html?history=1000&header=chrome-si&graph=chrome-si&rev=-1

On Tue, Aug 30, 2011 at 2:35 PM, Evan Martin <ev...@chromium.org> wrote:

Static initializers are not only banned by our style guide, we've seen
them impact startup performance for Chrome OS.  This is the reason the
"sizes" bot goes red when new code adds static initializers.  I wrote
a lot more about this here:
http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html
.

It turns out it's relatively easy to dump some of the sources of
static initialization by scripting gdb.
I have dumped a report here:
 http://paste.ubuntu.com/678316/
It shows a list of functions called by the static initializers used in
each file.
If you have a moment, please scan the list for files you are familiar
with and consider removing them.
If you see a file listed by no functions called, that means the static
initialization is likely limited to just code within that file.

PS: I have pending patches to fix xid_map_ and most of the __ioinit
instances in that report, don't worry about those.

[John Bates]

On Thu, Sep 1, 2011 at 8:54 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Sep 1, 2011 at 7:46 AM, Mark Mentovai <ma...@chromium.org> wrote:

Sigurður Ásgeirsson wrote:
> Initialization of function local static members MUST be deferred until first
> invocation of the containing function per the C++ specification. This does
> therefore not create a static initializer that runs at program start.
> Please note however, that the C++ runtime does not make the initialization
> threadsafe on any compiler/CRT that I'm aware of, so unless the containing
> function is only used from one thread (or under an external lock), the
> initialization is racy.

GCC (and Clang) do thread-safe initialization unless defeated with
-fno-threadsafe-statics.

Chrome does specify -fno-threadsafe-statics.

As others said, people should use LazyInstance for this. However, if code is running in another library without access to base, or for some very odd reason it needs to run before AtExitManager (which really shouldn't happen), then this pattern is useful: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/googleurl/src/gurl.cc&exact_package=chromium&q=gurl.cc&type=cs&l=69

Will valgrind complain that those are never destructed? Do we care if it does?

[John Abd-El-Malek]

On Fri, Sep 2, 2011 at 1:37 PM, John Bates <jba...@google.com> wrote:

On Thu, Sep 1, 2011 at 8:54 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Sep 1, 2011 at 7:46 AM, Mark Mentovai <ma...@chromium.org> wrote:

Sigurður Ásgeirsson wrote:
> Initialization of function local static members MUST be deferred until first
> invocation of the containing function per the C++ specification. This does
> therefore not create a static initializer that runs at program start.
> Please note however, that the C++ runtime does not make the initialization
> threadsafe on any compiler/CRT that I'm aware of, so unless the containing
> function is only used from one thread (or under an external lock), the
> initialization is racy.

GCC (and Clang) do thread-safe initialization unless defeated with
-fno-threadsafe-statics.

Chrome does specify -fno-threadsafe-statics.

As others said, people should use LazyInstance for this. However, if code is running in another library without access to base, or for some very odd reason it needs to run before AtExitManager (which really shouldn't happen), then this pattern is useful: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/googleurl/src/gurl.cc&exact_package=chromium&q=gurl.cc&type=cs&l=69

Will valgrind complain that those are never destructed? Do we care if it does?

It won't because the objects are still reachable.

[William Chan (陈智昌)]

We care if valgrind complains because we want valgrind to catch real
leaks. But as jam@ notes, it won't complain, since it's still
reachable. It's actually desirable for them not to be destructed,
because why slow down shutdown to delete stuff? That's only useful if
the destructor has necessary side effects like persisting data to disk
or something.

[John Bates]

With chrome-frame this design would leak into the owner process memory.

[William Chan (陈智昌)]

Evan and I discussed this earlier in the week. It's true for all
unloadable modules, like plugins and CF. Here are a few counterpoints:

* My understanding is CF is split into two DLLs, one stub one that's
loaded into the host process's address space and chrome.dll. The stub
only uses some parts from base/, right? Or am I spouting nonsense? I
only vaguely understand the CF DLL model.
* For unloadable modules, the solution is indeed to delete the memory,
usually via some module Shutdown() function. This at least allows us
to control destruction order, rather than relying on static
destructors.

But for normal Chrome, these concerns are unnecessary and as noted
previously, just slow down shutdown.

Btw, how often does CF actually get unloaded from the host process's
address space? The obvious, easy (but hacky) solution is just not to
unload it.

[Chase Phillips]

Done.  Now the Linux sizes step will turn red on even just one additional static initializer increase.  If this holds steady, I'll enable it on the rest of the static initializer tests.  I'll send a separate email to chromium-dev to make sure everyone's aware.

[John Abd-El-Malek]

awesome, thanks!

On Friday, September 16, 2011, Chase Phillips <c...@google.com> wrote:
> Done.  Now the Linux sizes step will turn red on even just one additional static initializer increase.  If this holds steady, I'll enable it on the rest of the static initializer tests.  I'll send a separate email to chromium-dev to make sure everyone's aware.
> On Fri, Sep 2, 2011 at 11:39 AM, John Abd-El-Malek <j...@chromium.org> wrote:
>>
>> +some infrastructure folks
>> Can we make this graph go red on _any_ increase, not just a percentage? That would help us make sure no more statics leak in as we fix existing usage. As I understand it now, this only goes red when a bunch of statics go in at the same time.

>> http://build.chromium.org/f/chromium/perf/linux-release/sizes/report.html?history=1000&header=chrome-si&graph=chrome-si&rev=-1 <http://build.chromium.org/f/chromium/perf/linux-release/sizes/report.html?history=1000&header=chrome-si&graph=chrome-si&rev=-1>

>>
>> On Tue, Aug 30, 2011 at 2:35 PM, Evan Martin <ev...@chromium.org> wrote:
>>>
>>> Static initializers are not only banned by our style guide, we've seen
>>> them impact startup performance for Chrome OS.  This is the reason the
>>> "sizes" bot goes red when new code adds static initializers.  I wrote
>>> a lot more about this here:

>>> http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html <http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html>

[Evan Martin]

On Tue, Aug 30, 2011 at 2:35 PM, Evan Martin <ev...@chromium.org> wrote:
> Static initializers are not only banned by our style guide, we've seen
> them impact startup performance for Chrome OS.  This is the reason the
> "sizes" bot goes red when new code adds static initializers.  I wrote
> a lot more about this here:
> http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html

> .
>
> It turns out it's relatively easy to dump some of the sources of
> static initialization by scripting gdb.
> I have dumped a report here:
>  http://paste.ubuntu.com/678316/
> It shows a list of functions called by the static initializers used in
> each file.

Just to follow up on this, I checked in the program that dumps the
static initializer list in at
tools/linux/dump-static-initializers.py
. You can run it for instructions.

[John Abd-El-Malek]

Two things happened today that surprised me:

-I found out that this script measures LazyInstance. I thought that LazyInstance usage is allowed since that object is just POD. Are we sure we want to enforce this? Most of the code is written with the assumption that these are ok when we need a global (obviously good to avoid a global unless we can, but when we do, I thought this is the recommended pattern to use.)

-turns out that the script only measures the number of files that have this. so if there's a file with LazyInstance in it already, I can add n objects with complex constructors and it won't be caught. Alternatively, a file with two LazyInstances that gets split into two files turns the tree red.

[William Chan (陈智昌)]

On Thu, Oct 6, 2011 at 5:21 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Two things happened today that surprised me:

-I found out that this script measures LazyInstance. I thought that LazyInstance usage is allowed since that object is just POD. Are we sure we want to enforce this? Most of the code is written with the assumption that these are ok when we need a global (obviously good to avoid a global unless we can, but when we do, I thought this is the recommended pattern to use.)

This is the pattern to use. It's an unfortunate side effect of gcc (fixed in 4.6) that this generates a static initializer (an empty one). Pretty harmless except for the fact that it happens on startup and forces segments of memory to get paged in. Oops.

[Robert Shield]

While this is true in theory, in practice the Chrome Frame module is pinned and never unloaded from the host Internet Explorer process until that process terminates. This is done because some of the patching Chrome Frame does cannot be safely undone.

[Robert Shield]

On Fri, Sep 2, 2011 at 6:22 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

Evan and I discussed this earlier in the week. It's true for all
unloadable modules, like plugins and CF. Here are a few counterpoints:

* My understanding is CF is split into two DLLs, one stub one that's
loaded into the host process's address space and chrome.dll. The stub
only uses some parts from base/, right? Or am I spouting nonsense? I
only vaguely understand the CF DLL model.

That's not quite right. The CF DLL loaded in IE launches a separate chrome.exe process which loads chrome.dll. Chrome.dll is not loaded into IE.

The CF DLL itself uses code from many places the Chrome code base, including /base, /chrome/app, /chrome/common, /chrome/test, /net, /ipc and several other places. 

 

* For unloadable modules, the solution is indeed to delete the memory,
usually via some module Shutdown() function. This at least allows us
to control destruction order, rather than relying on static
destructors.

But for normal Chrome, these concerns are unnecessary and as noted
previously, just slow down shutdown.

Btw, how often does CF actually get unloaded from the host process's
address space? The obvious, easy (but hacky) solution is just not to
unload it.

That's what we do. Once CF has patched all the relevant IE bits, it turns out to be *very* hard to unload cleanly without breaking many of the other things that tend to run inside IE processes, so we don't.

[William Chan (陈智昌)]

Great! Glad someone knowledgeable about CF chimed in. I think with this noted, then unless there are other consumers of chrome code that need to be unloadable (Chrome's PDF reader? I dunno), we should just leak LazyInstances/Singletons unless cleanup really needs to do something important like persist data to disk.