On 2014/03/06 11:13:32, ulan wrote:
Un-overlapped and added VisitCodeNextPointer, PTAL.


https://codereview.chromium.org/181833004/diff/20001/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):


https://codereview.chromium.org/181833004/diff/20001/src/objects-visiting-inl.h#newcode936
src/objects-visiting-inl.h:936: !is_optimized_code()) {
On 2014/03/05 08:58:02, titzer wrote:
> On 2014/03/04 12:31:12, Michael Starzinger wrote:
> > As discussed offline: AFAICT, currently we can afford to not have the
fields
> > overlap as the padding is big enough on 32bit and 64bit architectures. So
> > "un-overlapping" them woukd definitely make things easier to read.
>
> +1 unoverlap. I am sorry for overlapping them in the first place.
Nope, it is my fault. I remember introducing this field and overlapping it.
Sorry for that.


https://codereview.chromium.org/181833004/diff/20001/src/objects-visiting-inl.h#newcode936
src/objects-visiting-inl.h:936: !is_optimized_code()) {
On 2014/03/04 12:31:12, Michael Starzinger wrote:
> As discussed offline: AFAICT, currently we can afford to not have the fields > overlap as the padding is big enough on 32bit and 64bit architectures. So
> "un-overlapping" them woukd definitely make things easier to read.

Done.

https://codereview.chromium.org/181833004/diff/20001/src/objects.h
File src/objects.h (right):


https://codereview.chromium.org/181833004/diff/20001/src/objects.h#newcode10690
src/objects.h:10690: enum WeakPointerMode {
On 2014/03/04 12:31:12, Michael Starzinger wrote:
> As discussed offline: I am not a particular fan of having this mode, because
it
> assumes that all visitors either visit _all_ or _none_ weak pointers and I
am
> not sure this hold in general.
>
> Alternatively I would suggest having a special visitor method (e.g.
> VisitCodeNextPointer) that the concrete visitor can override to handle this
slot
> specially. This could be applied to both, the virtual as well as the static
> visitor I think.

Done. Thanks for suggestion, it is cleaner indeed.

https://codereview.chromium.org/181833004/diff/20001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):


https://codereview.chromium.org/181833004/diff/20001/test/cctest/test-heap.cc#newcode3719
test/cctest/test-heap.cc:3719: TEST(NextCodeLinkIsWeak) {
It assumes two things:
- next_code_link is used for linking
- the optimized code list is a stack (the last optimized code object points to
the previously optimized code)

Alternative to this would be to manage the list manually. Then I would have to
- optimize the two functions
- remove the optimized code objects from the context[OPTIMIZED_CODE_LIST] list
- add two code objects to the list manually.

I agree that this is less brittle in the sense that it wouldn't break if the
real list management changes. But then there is a risk that the list
management
in this test and the real list management diverge and the test no longer tests
the implementation.

Agree with your above statement. But it's easy just to have two tests.

https://codereview.chromium.org/181833004/

--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
--- You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

Reply via email to