Comments addressed, please have another look
http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc
File src/ia32/deoptimizer-ia32.cc (right):
http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#newcode57
src/ia32/deoptimizer-ia32.cc:57: Address original_reloc_payload =
code->relocation_start();
On 2011/02/02 08:33:31, Kevin Millikin wrote:
This needs a comment to the effect of "We will overwrite the code's
relocation
info in-place. Relocation info is written backward. The relocation
info is the
payload of a byte array."
A simpler way to deal with the padding at the end, which I like better
than
RoundUp is:
ByteArray* reloc_info = code->relocation_info();
Address end_address = reloc_info->address() + reloc_info->Size();
RelocInfoWriter writer(end_address, code->instruction_start());
Done.
http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#newcode80
src/ia32/deoptimizer-ia32.cc:80: Address entry_address =
On 2011/02/02 08:33:31, Kevin Millikin wrote:
Does this fit on one line? What if you just call it "entry"?
Done.
http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#newcode99
src/ia32/deoptimizer-ia32.cc:99: int reloc_size = reloc_end -
reloc_info_writer.pos();
On 2011/02/02 08:33:31, Kevin Millikin wrote:
This needs a comment to the effect that we will now move the new
relocation info
to the beginning of the original byte array and patch its size.
Done.
http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#newcode103
src/ia32/deoptimizer-ia32.cc:103:
code->relocation_info()->set_length(reloc_size);
On 2011/02/02 08:33:31, Kevin Millikin wrote:
Simply:
reloc_info->set_length(reloc_size);
if you name the reloc info as suggested above.
Done.
http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#newcode105
src/ia32/deoptimizer-ia32.cc:105: Address new_reloc_end =
On 2011/02/02 08:33:31, Kevin Millikin wrote:
This needs a comment to the effect that we'll put a non-live object in
the extra
space at the end of the former reloc info.
More simply, without RoundUp:
Address junk_address = reloc_info->address() + reloc_info->Size();
Done.
http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#newcode107
src/ia32/deoptimizer-ia32.cc:107: CHECK(new_reloc_end <= reloc_end);
On 2011/02/02 08:33:31, Kevin Millikin wrote:
I'm a little uncomfortable with having this a CHECK instead of an
ASSERT. I
guess if it's false then the memmove has stomped over some other
object.
Done.
http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#newcode115
src/ia32/deoptimizer-ia32.cc:115: while(reloc_end > new_reloc_end) {
On 2011/02/02 08:33:31, Kevin Millikin wrote:
This is a little simpler if you decrement first. I find it clearer if
it counts
up, though. I also like to leave explicit use of Memory:: functions
for the GC
and use higher level abstractions here:
HeapObject* filler_map = Heap::one_pointer_filler_map();
while (junk_address < end_address) {
HeapObject::FromAddress(junk_address)->set_map(filler_map);
junk_address += kPointerSize;
}
It's annoying that the code implicitly depends on kPointerSize and the
size of a
one word filler object being the same, but I guess that will never
change.
Well, it explicitly say one_POINTER_filler, not one_word_filler, so I
guess there could never be any confusion.
http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#newcode121
src/ia32/deoptimizer-ia32.cc:121: Address junk_data_start =
new_reloc_end + ByteArray::kHeaderSize;
On 2011/02/02 08:33:31, Kevin Millikin wrote:
I think this whole thing is
int size = end_address - junk_address;
HeapObject* junk_object = HeapObject::FromAddress(junk_address);
junk_object->set_map(Heap::byte_array_map());
ByteArray::cast(junk_object)->set_length(ByteArray::LengthFor(end -
junk));
Done.
http://codereview.chromium.org/6349043/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
