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
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
>
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.
Static initializers are not only banned by our style guide,
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
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 wrote:POD types that are initialized by constant data are OK. POD types that
> To be specific, it's non-POD static initializers that are banned, right?
are initialized by running code aren’t.
std::__ioinit
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.
"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
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.
>
Is this true? http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.16
says initialization only happens on first use.
>
>
> Cheers
>
> AGL
Ok, please ignore me, I'm wrong.
Cheers
AGL
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.
GCC (and Clang) do thread-safe initialization unless defeated with
-fno-threadsafe-statics.
Chrome does specify -fno-threadsafe-statics.
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";
}
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.
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.
On Thu, Sep 1, 2011 at 7:46 AM, Mark Mentovai <ma...@chromium.org> wrote:Sigurður Ásgeirsson wrote:GCC (and Clang) do thread-safe initialization unless defeated with
> 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.
-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
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:GCC (and Clang) do thread-safe initialization unless defeated with
> 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.
-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=69Will valgrind complain that those are never destructed? Do we care if it does?
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.
* 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.
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.
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.)
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.