AllocationSite objects weakly linked for traversal

The gc should be able to traverse all AllocationSites for decision making. The sites are threaded into a weak list. Special problems include:

* Allocations of AllocationSites occur in generated code, so generated code needs to be able to add to the list. For now I have a special hydrogen instruction, though it would be nice to use general purpose instructions.
* The snapshot contains AllocationSites, and these need to be re-threaded into the list on deserialization.

Something nice is that the AllocationSites are only created in old space, so a special new space visitor isn't required.

BUG=
R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=15715

[mvstanton, 2013-07-16 15:10:10]

Hi guys,
I worked with Michael the most on this, but it heavily uses code from Dmitry,
and Hannes is the primary "customer." Maybe full review from Michael, drive by
from Dmitry and driveby++ from Hannes?

I didn't do the platform ports. I will in the meantime look for a way to
eliminate the hydrogen instruction I added.
Thanks!
--Michael

[Michael Starzinger, 2013-07-16 18:14:45]

First round of comments. On a high level I don't particularly like the hackery
with the virtual object visitor for the serializer. But I have to think about
this some more to come up with a cleaner alternative. That's why a left a first
round of comments about the other parts of this change.

https://codereview.chromium.org/18173013/diff/11001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/18173013/diff/11001/src/heap.cc#newcode1789
src/heap.cc:1789: allocation_sites_list(),
nit: Indentation is off.

https://codereview.chromium.org/18173013/diff/11001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/18173013/diff/11001/src/heap.h#newcode2057
src/heap.h:2057: Object* allocation_sites_list_;
nit: Can we group these three fields together (removing new-lines) and maybe add
a short one-liner comment saying that these are heads of weak-lists?

https://codereview.chromium.org/18173013/diff/11001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/18173013/diff/11001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:3005: __ mov(Operand(temp, 0), object);
Since "temp" is clobbered before "object" is accessed, having "object" be
allocated as use-at-start here is bogus, as "temp == object" might hold.

https://codereview.chromium.org/18173013/diff/11001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/18173013/diff/11001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:309: default:
No default case for enums, better just fall through and have the UNREACHABLE and
the dummy return at the bottom after the switch. Thereby we will statically
catch this site when a new enum value is added.

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

https://codereview.chromium.org/18173013/diff/11001/src/objects-visiting-inl....
src/objects-visiting-inl.h:238: table_.Register(kVisitAllocationSite,
&VisitAllocationSite);
nit: Move the registration of this visitor up to after kVisitNativeContext for
consistency.

https://codereview.chromium.org/18173013/diff/11001/src/objects-visiting-inl....
src/objects-visiting-inl.h:338: void
StaticMarkingVisitor<StaticVisitor>::VisitAllocationSite(
Having this separate visitor function is obsolete, we can register the
FixedBodyVisitor directly while initializing.

https://codereview.chromium.org/18173013/diff/11001/src/objects-visiting.h
File src/objects-visiting.h (right):

https://codereview.chromium.org/18173013/diff/11001/src/objects-visiting.h#ne...
src/objects-visiting.h:424: INLINE(static void VisitAllocationSite(Map* map,
HeapObject* object));
This is obsolete, see comment in the objects-visiting-inl.h file.

https://codereview.chromium.org/18173013/diff/11001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/18173013/diff/11001/src/objects.h#newcode7479
src/objects.h:7479: static const int kSize = kWeakNextOffset + kPointerSize;
nit: The offset and size calculation should go to the bottom after the
dispatched behavior.

https://codereview.chromium.org/18173013/diff/11001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/18173013/diff/11001/src/serialize.cc#newcode594
src/serialize.cc:594: Add(address, LAZY_DEOPTIMIZATION, 63 + entry,
"lazy_deopt");
These references need to start at 64 now, s/63/64/ here.

[mvstanton, 2013-07-16 19:34:57]

Thanks Michael, PTAL.
--Michael

https://codereview.chromium.org/18173013/diff/11001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/18173013/diff/11001/src/heap.cc#newcode1789
src/heap.cc:1789: allocation_sites_list(),
On 2013/07/16 18:14:46, Michael Starzinger wrote:
> nit: Indentation is off.
weird, how did that happen? Done.

https://codereview.chromium.org/18173013/diff/11001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/18173013/diff/11001/src/heap.h#newcode2057
src/heap.h:2057: Object* allocation_sites_list_;
On 2013/07/16 18:14:46, Michael Starzinger wrote:
> nit: Can we group these three fields together (removing new-lines) and maybe
add
> a short one-liner comment saying that these are heads of weak-lists?

Done.

https://codereview.chromium.org/18173013/diff/11001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/18173013/diff/11001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:3005: __ mov(Operand(temp, 0), object);
On 2013/07/16 18:14:46, Michael Starzinger wrote:
> Since "temp" is clobbered before "object" is accessed, having "object" be
> allocated as use-at-start here is bogus, as "temp == object" might hold.

Awesome, thank you for catching that. Done.

https://codereview.chromium.org/18173013/diff/11001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/18173013/diff/11001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:309: default:
On 2013/07/16 18:14:46, Michael Starzinger wrote:
> No default case for enums, better just fall through and have the UNREACHABLE
and
> the dummy return at the bottom after the switch. Thereby we will statically
> catch this site when a new enum value is added.

Right on, thanks. Done.

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

https://codereview.chromium.org/18173013/diff/11001/src/objects-visiting-inl....
src/objects-visiting-inl.h:238: table_.Register(kVisitAllocationSite,
&VisitAllocationSite);
On 2013/07/16 18:14:46, Michael Starzinger wrote:
> nit: Move the registration of this visitor up to after kVisitNativeContext for
> consistency.

Done.

https://codereview.chromium.org/18173013/diff/11001/src/objects-visiting-inl....
src/objects-visiting-inl.h:338: void
StaticMarkingVisitor<StaticVisitor>::VisitAllocationSite(
On 2013/07/16 18:14:46, Michael Starzinger wrote:
> Having this separate visitor function is obsolete, we can register the
> FixedBodyVisitor directly while initializing.

Done.

https://codereview.chromium.org/18173013/diff/11001/src/objects-visiting.h
File src/objects-visiting.h (right):

https://codereview.chromium.org/18173013/diff/11001/src/objects-visiting.h#ne...
src/objects-visiting.h:424: INLINE(static void VisitAllocationSite(Map* map,
HeapObject* object));
On 2013/07/16 18:14:46, Michael Starzinger wrote:
> This is obsolete, see comment in the objects-visiting-inl.h file.

Done.

https://codereview.chromium.org/18173013/diff/11001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/18173013/diff/11001/src/objects.h#newcode7479
src/objects.h:7479: static const int kSize = kWeakNextOffset + kPointerSize;
On 2013/07/16 18:14:46, Michael Starzinger wrote:
> nit: The offset and size calculation should go to the bottom after the
> dispatched behavior.

Hmm. I actually moved these constants to the top of the file per request by
Hannes to make this new class finally conform to the coding guide of putting
constants at the top. But it makes sense that file-wise convention trumps that.
Done.

https://codereview.chromium.org/18173013/diff/11001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/18173013/diff/11001/src/serialize.cc#newcode594
src/serialize.cc:594: Add(address, LAZY_DEOPTIMIZATION, 63 + entry,
"lazy_deopt");
On 2013/07/16 18:14:46, Michael Starzinger wrote:
> These references need to start at 64 now, s/63/64/ here.

Yikes! thanks! Done.

[Michael Starzinger, 2013-07-17 08:48:56]

Yep, looking good already. Some comment about the virtual object visitor as we
discussed offline. Once those are addressed, I am fine with the change.

https://codereview.chromium.org/18173013/diff/17002/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/18173013/diff/17002/src/objects.h#newcode9758
src/objects.h:9758: virtual void VisitAllocationSite(HeapObject* site) {
As discussed offline: It should be possible to get rid of this virtual visitor
function, since the serializer doesn't necessarily need special handling for
allocation sites. And the deserializer has a special dispatch anyways. That
would make the virtual visitor simpler again.

https://codereview.chromium.org/18173013/diff/17002/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/18173013/diff/17002/src/serialize.cc#newcode788
src/serialize.cc:788: if (obj->IsAllocationSite()) {
Can we leave a TODO here saying that for now we rebuild the list during
deserialization, but in the future we should just deserialize the whole linked
list and somehow relink the head during deserialization. The relinking of the
head is the last unsolved problem to get that working.

https://codereview.chromium.org/18173013/diff/17002/src/serialize.cc#newcode1590
src/serialize.cc:1590: void
Serializer::ObjectSerializer::VisitAllocationSite(HeapObject* site) {
This should be obsolete AFAICT, see comment in the objects.h file as well.

[mvstanton, 2013-07-17 09:56:16]

Hi Michael, addressed issues per our conversation. I'm happy to be able to avoid
changing ObjectVisitor.

I also added platform ports. Note that on x64 and ARM I didn't need an extra
temp register.
Thanks,
--michael

https://codereview.chromium.org/18173013/diff/17002/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/18173013/diff/17002/src/objects.h#newcode9758
src/objects.h:9758: virtual void VisitAllocationSite(HeapObject* site) {
On 2013/07/17 08:48:57, Michael Starzinger wrote:
> As discussed offline: It should be possible to get rid of this virtual visitor
> function, since the serializer doesn't necessarily need special handling for
> allocation sites. And the deserializer has a special dispatch anyways. That
> would make the virtual visitor simpler again.

Done.

https://codereview.chromium.org/18173013/diff/17002/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/18173013/diff/17002/src/serialize.cc#newcode788
src/serialize.cc:788: if (obj->IsAllocationSite()) {
On 2013/07/17 08:48:57, Michael Starzinger wrote:
> Can we leave a TODO here saying that for now we rebuild the list during
> deserialization, but in the future we should just deserialize the whole linked
> list and somehow relink the head during deserialization. The relinking of the
> head is the last unsolved problem to get that working.

Done, check the wording on my comment, I wrote it before your text here and
yours is probably better.

I also changed the name of the method to be more specific.

https://codereview.chromium.org/18173013/diff/17002/src/serialize.cc#newcode1590
src/serialize.cc:1590: void
Serializer::ObjectSerializer::VisitAllocationSite(HeapObject* site) {
On 2013/07/17 08:48:57, Michael Starzinger wrote:
> This should be obsolete AFAICT, see comment in the objects.h file as well.

Done.

[Michael Starzinger, 2013-07-17 11:25:30]

LGTM from my end with minor final nits.

https://codereview.chromium.org/18173013/diff/36001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/18173013/diff/36001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5434: store_field_(store_field),
nit: Indentation is off.

https://codereview.chromium.org/18173013/diff/36001/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/18173013/diff/36001/src/serialize.h#newcode494
src/serialize.h:494: Isolate* isolate() { return isolate_; }
nit: It looks like this accessor became obsolete, let's drop it again.

[mvstanton, 2013-07-17 11:49:00]

Thanks very much!
--Michael

https://codereview.chromium.org/18173013/diff/36001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/18173013/diff/36001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5434: store_field_(store_field),
On 2013/07/17 11:25:30, Michael Starzinger wrote:
> nit: Indentation is off.

Done.

https://codereview.chromium.org/18173013/diff/36001/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/18173013/diff/36001/src/serialize.h#newcode494
src/serialize.h:494: Isolate* isolate() { return isolate_; }
On 2013/07/17 11:25:30, Michael Starzinger wrote:
> nit: It looks like this accessor became obsolete, let's drop it again.

Done.

[mvstanton, 2013-07-17 11:50:37]

Committed patchset #5 manually as r15715 (presubmit successful).