The approach LGTM, but it needs a couple of comments about what hte
approach is.
I also want to use higher level abstractions (e.g., HeapObject::Size and
HeapObject::set_map).
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();
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());
http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#newcode80
src/ia32/deoptimizer-ia32.cc:80: Address entry_address =
Does this fit on one line? What if you just call it "entry"?
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();
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.
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);
Simply:
reloc_info->set_length(reloc_size);
if you name the reloc info as suggested above.
http://codereview.chromium.org/6349043/diff/1/src/ia32/deoptimizer-ia32.cc#newcode105
src/ia32/deoptimizer-ia32.cc:105: Address new_reloc_end =
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();
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);
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.
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) {
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.
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;
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));
http://codereview.chromium.org/6349043/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
