[Isolates] StaticVisitor::VisitPointer gets Heap* as additional argument

During GC, the StaticVisitor is massively used, adding a Heap* parameter makes it possible to remove Isolate::Current() lookup in some very hot and cheap operations (like marking), giving measurable result.

Committed: http://code.google.com/p/v8/source/detail?r=5516

[yurys, 2010-09-22 08:54:23]

LGTM with comments.

http://codereview.chromium.org/3397021/diff/1/5
File src/mark-compact.cc (right):

http://codereview.chromium.org/3397021/diff/1/5#newcode677
src/mark-compact.cc:677: RootMarkingVisitor(Heap* heap)
Should be "explicit".

http://codereview.chromium.org/3397021/diff/1/5#newcode1473
src/mark-compact.cc:1473: PointersToNewGenUpdatingVisitor(Heap* heap) :
heap_(heap) { }
ditto.

http://codereview.chromium.org/3397021/diff/1/6
File src/objects-visiting.h (right):

http://codereview.chromium.org/3397021/diff/1/6#newcode394
src/objects-visiting.h:394: HEAP,
Code is a HeapObject, you can call GetHeap() instead of HEAP here.

http://codereview.chromium.org/3397021/diff/1/7
File src/x64/assembler-x64-inl.h (right):

http://codereview.chromium.org/3397021/diff/1/7#newcode358
src/x64/assembler-x64-inl.h:358: StaticVisitor::VisitPointer(HEAP,
target_object_address());
We could try to get the Heap pointer from the target_object when it's a
HeapObject but probably the performance gain won't worth the effort.

[Dmitry Titov, 2010-09-23 02:23:51]

I've made more changes, perhaps it's better if one of you guys took another
look:

On 2010/09/22 08:54:23, Yury Semikhatsky wrote:
> http://codereview.chromium.org/3397021/diff/1/5#newcode677
> src/mark-compact.cc:677: RootMarkingVisitor(Heap* heap)
> Should be "explicit".

Done.

> http://codereview.chromium.org/3397021/diff/1/5#newcode1473
> src/mark-compact.cc:1473: PointersToNewGenUpdatingVisitor(Heap* heap) :
> heap_(heap) { }
> ditto.

Done.

> File src/objects-visiting.h (right):
> 
> http://codereview.chromium.org/3397021/diff/1/6#newcode394
> src/objects-visiting.h:394: HEAP,
> Code is a HeapObject, you can call GetHeap() instead of HEAP here.

I couldn't use GetHeap() here because it is used during GC when GetHeap() can
not be used. Instead, I've added a Heap* parameter to Code::CodeIterateBody().
It is also passed to static version of RelocInfo::Visit().

> http://codereview.chromium.org/3397021/diff/1/7
> File src/x64/assembler-x64-inl.h (right):
> 
> http://codereview.chromium.org/3397021/diff/1/7#newcode358
> src/x64/assembler-x64-inl.h:358: StaticVisitor::VisitPointer(HEAP,
> target_object_address());
> We could try to get the Heap pointer from the target_object when it's a
> HeapObject but probably the performance gain won't worth the effort.

Same as above - getting the Heap* pointer passed from the caller.


Also, 2 fixes:
1. test_spaces.cc fix for Page test. Test constructs the Page object
artificially and needs to init heap_ field now (regression from my previous CL)
2. The Code alignment shifted on x64 after I've added Heap pointer to Page
header. This only happens on x64 because Page uses MAP_POINTER_ALIGN to align
its header and it is 32 bytes on 32-bit host and 8 bytes on 64-bit host, but
code alignment is always required to be 32 bytes (thus some asserts fail). I'm
not sure why we need generated code blocks to be 32 bytes aligned (is it a guess
on cache line size?), likely for the different reasons then Maps that need this
to be able to produce an index of a map in the page during GC. So I left it
32-byte aligned on 32-bit host and 8 byte-aligned on 64-bit host. Wonder if this
is right.

[Vitaly Repeshko, 2010-09-23 17:41:45]

On 2010/09/23 02:23:51, Dmitry Titov wrote:
> 2. The Code alignment shifted on x64 after I've added Heap pointer to Page
> header. This only happens on x64 because Page uses MAP_POINTER_ALIGN to align
> its header and it is 32 bytes on 32-bit host and 8 bytes on 64-bit host, but
> code alignment is always required to be 32 bytes (thus some asserts fail). I'm
> not sure why we need generated code blocks to be 32 bytes aligned (is it a
guess
> on cache line size?), likely for the different reasons then Maps that need
this
> to be able to produce an index of a map in the page during GC. So I left it
> 32-byte aligned on 32-bit host and 8 byte-aligned on 64-bit host. Wonder if
this
> is right.

I think we should avoid doing semantical changes. Cache line size is 64, so
using our old alignment we only waste 25% on average. Please add
CODE_POINTER_ALIGN macro like MAP_POINTER_ALIGN and make Page use both. We
should port it back to bleeding_edge.

[Dmitry Titov, 2010-09-23 22:05:06]

Got LGTM from Vitaly on IM so it's landed.
Will port the CODE_POINTER_ALIGN to bleeding_edge in a separate patch.