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#newcode7056
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#newcode56
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-initializers.sh
File tools/check-static-initializers.sh (right):
http://codereview.chromium.org/9455088/diff/46/tools/check-static-initializers.sh#newcode36
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.
http://codereview.chromium.org/9455088/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
