Remove static initializers in v8.

Remove static initializers in v8.

This CL depends on CL 9447052 (adding CallOnce and LazyInstance).
It is based on a patch sent by Digit.

With this patch applied, we have only one static initializer left (in atomicops_internals_x86_gcc.cc). This static initializer populates a structure used by x86 atomic operations. It seems that we can hardly remove it. If possible, it will be removed in a next CL. 

This CL also modifies the presubmit script to check the number of static initializers.


BUG=1859

[fschneider, 2012-02-28 16:54:48]

This already looks quite good.

Can you rebase it on bleeding_edge? I refactored HashMap, which should simplify
this CL a bit.

http://codereview.chromium.org/9455088/diff/46/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/9455088/diff/46/src/arm/code-stubs-arm.cc#newc...
src/arm/code-stubs-arm.cc:7056: { REG(R6), REG(R4), REG(R7), EMIT_REMEMBERED_SET
},
It would be nice to use the same names here as well like r6, r4, r7. This way
they are easier to find when searching for all occurrences of a register.

Maybe another macro like
DECLARE_STUB(r6, r4, r7, EMIT_REMEMBERED_SET) would help.

http://codereview.chromium.org/9455088/diff/46/src/hashmap.h
File src/hashmap.h (right):

http://codereview.chromium.org/9455088/diff/46/src/hashmap.h#newcode47
src/hashmap.h:47: class HashMap {
I changed HashMap so that it does not have an allocator_ member, so I think
hashmap.h and hashmap.cc should be fine now.

http://codereview.chromium.org/9455088/diff/46/src/lithium-allocator.cc
File src/lithium-allocator.cc (right):

http://codereview.chromium.org/9455088/diff/46/src/lithium-allocator.cc#newco...
src/lithium-allocator.cc:56: &name::cache);                                     
  \
I'd move this initialization to lithium.cc since it rather belongs to where
LOperand is defined.

http://codereview.chromium.org/9455088/diff/46/src/lithium.h
File src/lithium.h (left):

http://codereview.chromium.org/9455088/diff/46/src/lithium.h#oldcode248
src/lithium.h:248: 
Accidental edit?

http://codereview.chromium.org/9455088/diff/46/src/lithium.h
File src/lithium.h (right):

http://codereview.chromium.org/9455088/diff/46/src/lithium.h#newcode263
src/lithium.h:263: SetUpCache();
Creation of LOperand is a really frequent operation. The call to SetupCache
should maybe moved to global V8 initialization in InitializeOncePerProcessImpl.

http://codereview.chromium.org/9455088/diff/46/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/9455088/diff/46/src/scopes.cc#newcode88
src/scopes.cc:88: VariableMap::VariableMap() : HashMap(Match,
LocalsMapAllocator.Pointer(), 8) {}
Not needed anymore because HashMap is already refactored to avoid a static
initializer.

http://codereview.chromium.org/9455088/diff/46/tools/check-static-initializer...
File tools/check-static-initializers.sh (right):

http://codereview.chromium.org/9455088/diff/46/tools/check-static-initializer...
tools/check-static-initializers.sh:36: v8_out=$(readlink -f $(dirname
$BASH_SOURCE)/../out)
Not sure if this works with the current build configuration on the buildbot
which I believe uses the SCons build. Somehow specifying the binary output
directory would be useful, I guess.

[Sven Panne, 2012-02-29 08:05:35]

http://codereview.chromium.org/9455088/diff/46/tools/check-static-initializer...
File tools/check-static-initializers.sh (right):

http://codereview.chromium.org/9455088/diff/46/tools/check-static-initializer...
tools/check-static-initializers.sh:36: v8_out=$(readlink -f $(dirname
$BASH_SOURCE)/../out)
A few points:

 * A non-shared scons build only generates a library for the preparser.

 * You typically have various build variants lying around, which get added
together and therefore things will almost always fail.

  * You can't use this for a presubmit script, because there is no guarantee
that the objects you find are the ones created from the current sources.

  * Just to be nitpicking: What is being counted here is *not* the number of
static initializers, it is the number of compilation units having at least 1
static initializer.

  * You can greatly simplify the whole script by just looking at the final
executable, doing something like "objdump --headers --section=.ctors d8" and
look at the size of the section.

[Philippe, 2012-02-29 10:24:45]

Thanks for reviewing this CL!

I also made some new changes in isolate.{h,cc}, v8.cc. With my initial change, I
had sometimes some tests timing out (deadlock actually) because of an unexpected
situation:
Isolate::Current() called Isolate::EnsureDefaultIsolate() which acquired the
process wide mutex. Right after the mutex was acquired, the thread was preempted
because of a SIGPROF (see platform_linux.cc). The signal handler (running in the
same thread) also called Isolate::Current() which indirectly tried to acquire
the same mutex. Obviously this mutex was already acquired by the same thread.
You know the end of the story :) That was quite unfortunate.

http://codereview.chromium.org/9455088/diff/46/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/9455088/diff/46/src/arm/code-stubs-arm.cc#newc...
src/arm/code-stubs-arm.cc:7056: { REG(R6), REG(R4), REG(R7), EMIT_REMEMBERED_SET
},
On 2012/02/28 16:54:48, fschneider wrote:
> It would be nice to use the same names here as well like r6, r4, r7. This way
> they are easier to find when searching for all occurrences of a register.
> 
> Maybe another macro like
> DECLARE_STUB(r6, r4, r7, EMIT_REMEMBERED_SET) would help.

I agree with your point.

I am probably missing something but I don't see how DECLARE_STUB() could help me
here. The problem is that the corresponding constants are named kRegisterRxCode.
Here I can make them rx instead of Rx but I would have to rename the constants
to something like kRegister_rx_code for example which looks a bit odd (but I can
live with that :).

What do you think?

http://codereview.chromium.org/9455088/diff/46/src/d8.cc
File src/d8.cc (left):

http://codereview.chromium.org/9455088/diff/46/src/d8.cc#oldcode94
src/d8.cc:94: DumbLineEditor() : LineEditor(LineEditor::DUMB, "dumb") { }
I didn't notice the side effect in LineEditor's constructor. It actually sets a
static variable.

I reverted this change.

http://codereview.chromium.org/9455088/diff/46/src/hashmap.h
File src/hashmap.h (right):

http://codereview.chromium.org/9455088/diff/46/src/hashmap.h#newcode47
src/hashmap.h:47: class HashMap {
On 2012/02/28 16:54:48, fschneider wrote:
> I changed HashMap so that it does not have an allocator_ member, so I think
> hashmap.h and hashmap.cc should be fine now.

Done.

http://codereview.chromium.org/9455088/diff/46/src/lithium-allocator.cc
File src/lithium-allocator.cc (right):

http://codereview.chromium.org/9455088/diff/46/src/lithium-allocator.cc#newco...
src/lithium-allocator.cc:56: &name::cache);                                     
  \
On 2012/02/28 16:54:48, fschneider wrote:
> I'd move this initialization to lithium.cc since it rather belongs to where
> LOperand is defined.

Done.

http://codereview.chromium.org/9455088/diff/46/src/lithium.h
File src/lithium.h (right):

http://codereview.chromium.org/9455088/diff/46/src/lithium.h#newcode263
src/lithium.h:263: SetUpCache();
On 2012/02/28 16:54:48, fschneider wrote:
> Creation of LOperand is a really frequent operation. The call to SetupCache
> should maybe moved to global V8 initialization in
InitializeOncePerProcessImpl.

Done.

http://codereview.chromium.org/9455088/diff/46/tools/check-static-initializer...
File tools/check-static-initializers.sh (right):

http://codereview.chromium.org/9455088/diff/46/tools/check-static-initializer...
tools/check-static-initializers.sh:36: v8_out=$(readlink -f $(dirname
$BASH_SOURCE)/../out)
On 2012/02/29 08:05:35, Sven Panne wrote:
> A few points:
> 
>  * A non-shared scons build only generates a library for the preparser.
> 
>  * You typically have various build variants lying around, which get added
> together and therefore things will almost always fail.
> 
>   * You can't use this for a presubmit script, because there is no guarantee
> that the objects you find are the ones created from the current sources.
> 
>   * Just to be nitpicking: What is being counted here is *not* the number of
> static initializers, it is the number of compilation units having at least 1
> static initializer.
> 
>   * You can greatly simplify the whole script by just looking at the final
> executable, doing something like "objdump --headers --section=.ctors d8" and
> look at the size of the section.

I changed the script to work on d8 instead after a SCons build.
I reverted the changes that I previously made in presubmit.py.
I still use nm as ctors is not always the right name (init_array on ARM).

Also, d8 contains some static initializers which I didn't remove since it is not
affecting clients.

[Philippe, 2012-03-05 15:53:09]

On 2012/02/29 10:24:45, Philippe wrote:
> Thanks for reviewing this CL!
> 
> I also made some new changes in isolate.{h,cc}, v8.cc. With my initial change,
I
> had sometimes some tests timing out (deadlock actually) because of an
unexpected
> situation:
> Isolate::Current() called Isolate::EnsureDefaultIsolate() which acquired the
> process wide mutex. Right after the mutex was acquired, the thread was
preempted
> because of a SIGPROF (see platform_linux.cc). The signal handler (running in
the
> same thread) also called Isolate::Current() which indirectly tried to acquire
> the same mutex. Obviously this mutex was already acquired by the same thread.
> You know the end of the story :) That was quite unfortunate.
> 
> http://codereview.chromium.org/9455088/diff/46/src/arm/code-stubs-arm.cc
> File src/arm/code-stubs-arm.cc (right):
> 
>
http://codereview.chromium.org/9455088/diff/46/src/arm/code-stubs-arm.cc#newc...
> src/arm/code-stubs-arm.cc:7056: { REG(R6), REG(R4), REG(R7),
EMIT_REMEMBERED_SET
> },
> On 2012/02/28 16:54:48, fschneider wrote:
> > It would be nice to use the same names here as well like r6, r4, r7. This
way
> > they are easier to find when searching for all occurrences of a register.
> > 
> > Maybe another macro like
> > DECLARE_STUB(r6, r4, r7, EMIT_REMEMBERED_SET) would help.
> 
> I agree with your point.
> 
> I am probably missing something but I don't see how DECLARE_STUB() could help
me
> here. The problem is that the corresponding constants are named
kRegisterRxCode.
> Here I can make them rx instead of Rx but I would have to rename the constants
> to something like kRegister_rx_code for example which looks a bit odd (but I
can
> live with that :).
> 
> What do you think?
> 
> http://codereview.chromium.org/9455088/diff/46/src/d8.cc
> File src/d8.cc (left):
> 
> http://codereview.chromium.org/9455088/diff/46/src/d8.cc#oldcode94
> src/d8.cc:94: DumbLineEditor() : LineEditor(LineEditor::DUMB, "dumb") { }
> I didn't notice the side effect in LineEditor's constructor. It actually sets
a
> static variable.
> 
> I reverted this change.
> 
> http://codereview.chromium.org/9455088/diff/46/src/hashmap.h
> File src/hashmap.h (right):
> 
> http://codereview.chromium.org/9455088/diff/46/src/hashmap.h#newcode47
> src/hashmap.h:47: class HashMap {
> On 2012/02/28 16:54:48, fschneider wrote:
> > I changed HashMap so that it does not have an allocator_ member, so I think
> > hashmap.h and hashmap.cc should be fine now.
> 
> Done.
> 
> http://codereview.chromium.org/9455088/diff/46/src/lithium-allocator.cc
> File src/lithium-allocator.cc (right):
> 
>
http://codereview.chromium.org/9455088/diff/46/src/lithium-allocator.cc#newco...
> src/lithium-allocator.cc:56: &name::cache);                                   
 
>   \
> On 2012/02/28 16:54:48, fschneider wrote:
> > I'd move this initialization to lithium.cc since it rather belongs to where
> > LOperand is defined.
> 
> Done.
> 
> http://codereview.chromium.org/9455088/diff/46/src/lithium.h
> File src/lithium.h (right):
> 
> http://codereview.chromium.org/9455088/diff/46/src/lithium.h#newcode263
> src/lithium.h:263: SetUpCache();
> On 2012/02/28 16:54:48, fschneider wrote:
> > Creation of LOperand is a really frequent operation. The call to SetupCache
> > should maybe moved to global V8 initialization in
> InitializeOncePerProcessImpl.
> 
> Done.
> 
>
http://codereview.chromium.org/9455088/diff/46/tools/check-static-initializer...
> File tools/check-static-initializers.sh (right):
> 
>
http://codereview.chromium.org/9455088/diff/46/tools/check-static-initializer...
> tools/check-static-initializers.sh:36: v8_out=$(readlink -f $(dirname
> $BASH_SOURCE)/../out)
> On 2012/02/29 08:05:35, Sven Panne wrote:
> > A few points:
> > 
> >  * A non-shared scons build only generates a library for the preparser.
> > 
> >  * You typically have various build variants lying around, which get added
> > together and therefore things will almost always fail.
> > 
> >   * You can't use this for a presubmit script, because there is no guarantee
> > that the objects you find are the ones created from the current sources.
> > 
> >   * Just to be nitpicking: What is being counted here is *not* the number of
> > static initializers, it is the number of compilation units having at least 1
> > static initializer.
> > 
> >   * You can greatly simplify the whole script by just looking at the final
> > executable, doing something like "objdump --headers --section=.ctors d8" and
> > look at the size of the section.
> 
> I changed the script to work on d8 instead after a SCons build.
> I reverted the changes that I previously made in presubmit.py.
> I still use nm as ctors is not always the right name (init_array on ARM).
> 
> Also, d8 contains some static initializers which I didn't remove since it is
not
> affecting clients.

This CL is still alive :)

[fschneider, 2012-03-07 12:01:34]

I think the CL is almost there. A few more comments:

http://codereview.chromium.org/9455088/diff/21035/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/9455088/diff/21035/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4135: map = Register::from_code(kRegisterNoneCode);
Why do you need to replace no_reg here?

http://codereview.chromium.org/9455088/diff/21035/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4886: __ LeaveExitFrame(false,
Register::from_code(kRegisterNoneCode));
Why do you need to replace no_reg here?

http://codereview.chromium.org/9455088/diff/21035/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/9455088/diff/21035/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:7036: #define REG(Name) { kRegister ## Name ## Code
}
Yes, I'd rather prefer kRegister_eax_Code in this case, so that the names used
here match the rest of the register names.

http://codereview.chromium.org/9455088/diff/21035/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/9455088/diff/21035/src/isolate.cc#newcode410
src/isolate.cc:410: return
global_state.Pointer()->default_isolate->stack_guard();
Here you use Pointer() and further down Get(). It would be nice to be
consistent. i.e. use Pointer() only if a pointer is actually needed, otherwise
Get().

[Philippe, 2012-03-07 13:14:17]

http://codereview.chromium.org/9455088/diff/21035/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/9455088/diff/21035/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4135: map = Register::from_code(kRegisterNoneCode);
On 2012/03/07 12:01:34, fschneider wrote:
> Why do you need to replace no_reg here?

I don't need this change indeed. I fixed this place and the other ones.

http://codereview.chromium.org/9455088/diff/21035/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4886: __ LeaveExitFrame(false,
Register::from_code(kRegisterNoneCode));
On 2012/03/07 12:01:34, fschneider wrote:
> Why do you need to replace no_reg here?

Done.

http://codereview.chromium.org/9455088/diff/21035/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/9455088/diff/21035/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:7036: #define REG(Name) { kRegister ## Name ## Code
}
On 2012/03/07 12:01:34, fschneider wrote:
> Yes, I'd rather prefer kRegister_eax_Code in this case, so that the names used
> here match the rest of the register names.

Done.

http://codereview.chromium.org/9455088/diff/21035/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/9455088/diff/21035/src/isolate.cc#newcode410
src/isolate.cc:410: return
global_state.Pointer()->default_isolate->stack_guard();
On 2012/03/07 12:01:34, fschneider wrote:
> Here you use Pointer() and further down Get(). It would be nice to be
> consistent. i.e. use Pointer() only if a pointer is actually needed, otherwise
> Get().

I initially made LazyInstance::Get() return a const reference as opposed to a
reference to avoid violating the Google C++ guidelines.
To be clear, Get() returns a const reference and Pointer() returns a non-const
pointer.

The logic here, which is not obvious, is that Pointer() is used (as opposed to
Get()) whenever we need a pointer or need to call a non-const method or access
to a mutable field.

In that particular case, stack_guard() is non-const, therefore default_isolate
must be accessed in a non-const way (using Pointer()).

[fschneider, 2012-03-12 11:48:01]

lgtm

[fschneider, 2012-03-12 11:50:41]

On 2012/03/12 11:48:01, fschneider wrote:
> lgtm

I'll land this together with http://codereview.chromium.org/9447052/ since those
two really belong together.