This is really great, it's good to finally have the snapshots working in the
official V8 repo.
The changes you made to use the root array more in generated code will
reduce
the size of the reloc info which is good, but for the cases where it is not
easy
to use the root array I decided to move the handling of that into the
serializer, rather than the macro assembler.
In the end I decided it would be easier to put your changes and my comments
into
a new changelist. Please take a look at
https://chromiumcodereview.appspot.com/9722020
http://codereview.chromium.org/9372063/diff/7001/src/mips/macro-assembler-mips.cc
File src/mips/macro-assembler-mips.cc (right):
http://codereview.chromium.org/9372063/diff/7001/src/mips/macro-assembler-mips.cc#newcode793
src/mips/macro-assembler-mips.cc:793: int32_t index =
FindRootIndex(*(reinterpret_cast<Object**>(j.imm32_)));
On 2012/03/12 12:40:00, kalmard wrote:
On 2012/02/29 14:38:13, Erik Corry wrote:
> This will be too slow. I suggest you make it into
> ASSERT(FindRootIndex(...) == kInvalidRootIndex) and then fix the
callers to
use
> LoadRoot.
The FindRootIndex() lookup only happens at serialization (e.g. Build)
time.
Performance is not an issue. Clarified this in the code via function
name
change.
In that case you should assert that the Serializer::enabled()
Please see the main comment about the LoadRoot change.
http://codereview.chromium.org/9372063/diff/17001/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (right):
http://codereview.chromium.org/9372063/diff/17001/src/mips/assembler-mips.cc#newcode855
src/mips/assembler-mips.cc:855: }
This is gone.
http://codereview.chromium.org/9372063/diff/17001/src/mips/assembler-mips.h
File src/mips/assembler-mips.h (right):
http://codereview.chromium.org/9372063/diff/17001/src/mips/assembler-mips.h#newcode1012
src/mips/assembler-mips.h:1012: bool
SerializingTryLoadFromRoot(RelocInfo::Mode rmode);
I got rid of this.
http://codereview.chromium.org/9372063/diff/17001/src/mips/code-stubs-mips.cc
File src/mips/code-stubs-mips.cc (right):
http://codereview.chromium.org/9372063/diff/17001/src/mips/code-stubs-mips.cc#newcode3959
src/mips/code-stubs-mips.cc:3959: __ Branch(throw_termination_exception,
eq,
This all fits on one line now.
http://codereview.chromium.org/9372063/diff/17001/src/mips/code-stubs-mips.cc#newcode7086
src/mips/code-stubs-mips.cc:7086: RelocInfo::CODE_TARGET),
CONSTANT_SIZE);
I fixed some strange line breaking here.
http://codereview.chromium.org/9372063/diff/17001/src/mips/ic-mips.cc
File src/mips/ic-mips.cc (right):
http://codereview.chromium.org/9372063/diff/17001/src/mips/ic-mips.cc#newcode775
src/mips/ic-mips.cc:775: __ CheckMap(scratch1, scratch2,
Heap::kNonStrictArgumentsElementsMapRootIndex,
Arguments should be all on one line or one per line here and in some
other places.
http://codereview.chromium.org/9372063/diff/17001/src/mips/macro-assembler-mips.cc
File src/mips/macro-assembler-mips.cc (right):
http://codereview.chromium.org/9372063/diff/17001/src/mips/macro-assembler-mips.cc#newcode766
src/mips/macro-assembler-mips.cc:766: if (heap->InNewSpace(heap_object))
return kInvalidRootIndex;
Since your comment indicated that this function is only used in the
serializer, it function should contain an assertion that the serializer
is active. That would serve both as a check and a sort of comment. In
the event I removed this function though.
http://codereview.chromium.org/9372063/diff/17001/src/mips/macro-assembler-mips.cc#newcode792
src/mips/macro-assembler-mips.cc:792: } else if
(SerializingTryLoadFromRoot(j.rmode_) && mode == OPTIMIZE_SIZE) {
I took out this change.
http://codereview.chromium.org/9372063/diff/17001/src/mips/macro-assembler-mips.h
File src/mips/macro-assembler-mips.h (right):
http://codereview.chromium.org/9372063/diff/17001/src/mips/macro-assembler-mips.h#newcode253
src/mips/macro-assembler-mips.h:253: int FindRootIndex(Object*
heap_object);
I got rid of this.
http://codereview.chromium.org/9372063/diff/17001/src/serialize.cc
File src/serialize.cc (right):
http://codereview.chromium.org/9372063/diff/17001/src/serialize.cc#newcode782
src/serialize.cc:782: (addr - 3 * kPointerSize)
This should be kInstructionSize (and I moved the functionality into the
MIPS assembler instead of this ifdef).
http://codereview.chromium.org/9372063/diff/17001/src/serialize.cc#newcode859
src/serialize.cc:859: reinterpret_cast<Address>(current);
\
It turns out this code can be simplified a little for all architecture,
since the kFromCode instructions are only emitted if the
current_was_incremented handling needs to be special in some way, and
the PATCH_SITE_ADJUST ugliness can be moved into the assembler.
http://codereview.chromium.org/9372063/diff/17001/src/serialize.cc#newcode1014
src/serialize.cc:1014: ALL_SPACES(kBackref, kFromCode, kStartOfObject)
The order is not important here so I moved these into the ifdef above
for a tiny code size improvement without an increase in the number of
ifdefs.
http://codereview.chromium.org/9372063/diff/17001/test/cctest/cctest.status
File test/cctest/cctest.status (left):
http://codereview.chromium.org/9372063/diff/17001/test/cctest/cctest.status#oldcode89
test/cctest/cctest.status:89:
Yay!
http://codereview.chromium.org/9372063/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
