Recording array buffer views.

Recording array buffer views.

R=hpayer@chromium.org
BUG=

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

[Dmitry Lomov (no reviews), 2013-05-21 13:54:56]

Hannes/Michael,

this is work in progress - please check if the approach looks good to you.

[Michael Starzinger, 2013-05-21 15:48:04]

From a high-level perspective this approach should work. So it is heading into
the right direction I think.

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

https://codereview.chromium.org/15562008/diff/1/src/objects-visiting-inl.h#ne...
src/objects-visiting-inl.h:414: if (array_buffer->next() == Smi::FromInt(0)) {
This might cause an array buffer to be enqueued twice if it is at the end of the
list , because the list-end sentinel is also Smi(0).

[Dmitry Lomov (no reviews), 2013-05-21 21:25:52]

On second thoughts, I think that most of this is not entirely correct. The
better approach would be to treat list of views of array buffer (as well as a
list of array buffers) as "true" weak pointers and walk them at the end of
marking and on scavenging and compacting using WeakObjectRetainer, in the manner
of Heap::ProcessWeakReferences. I'll be back with a different CL.

It looks like JSWeakMaps should really use the same mechanism.

[Dmitry Lomov (no reviews), 2013-05-22 15:39:49]

Please take another look. This is incomplete:
- no tests
- no actual API for array buffer neutering.

but let me know if there is anything I have missed.

[Dmitry Lomov (no reviews), 2013-06-04 15:15:52]

Hannes/Michael,
please take a look.
This does not have actual implementation of neutering API yet - this is a big
enough CL already. 

Thanks much!
Dmitry

https://codereview.chromium.org/15562008/diff/15001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/15562008/diff/15001/src/heap.cc#newcode1632
src/heap.cc:1632: static Object* VisitWeakList(Object* list,
This is a unified weak list visitor. 
I hope to switch ProcessNativeContexts to it as well, but in subsequent CL.

[Hannes Payer (out of office), 2013-06-06 09:33:32]

First rounds of comments. The implementation looks good. Can we make the tests
more complicated, see comments?

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

https://codereview.chromium.org/15562008/diff/20001/src/objects.h#newcode8831
src/objects.h:8831: // [next]: linked list of typed arrays over the same array
buffer.
should be called weak_next

https://codereview.chromium.org/15562008/diff/20001/test/cctest/test-weaktype...
File test/cctest/test-weaktypedarrays.cc (right):

https://codereview.chromium.org/15562008/diff/20001/test/cctest/test-weaktype...
test/cctest/test-weaktypedarrays.cc:134: "var ab2 = new ArrayBuffer(256);");
indent

https://codereview.chromium.org/15562008/diff/20001/test/cctest/test-weaktype...
test/cctest/test-weaktypedarrays.cc:167: 
Can we add a third typed array to the week list and delete, once the first, once
the middle, and once the third element? To get all corner cases of the list
implementation.

[Michael Starzinger, 2013-06-06 09:53:18]

Just a bunch of nits.

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

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting-inl....
src/objects-visiting-inl.h:125: 
nit: Add a second empty newline.

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting-inl....
src/objects-visiting-inl.h:143: 
nit: Drop the third empty newline.

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting-inl....
src/objects-visiting-inl.h:199: 
nit: Drop the second empty newline.

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting-inl....
src/objects-visiting-inl.h:470: 
nit: Add a second empty newline.

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting.cc
File src/objects-visiting.cc (right):

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting.cc#n...
src/objects-visiting.cc:154: return GetVisitorIdForSize(kVisitJSObject,
nit: Indentation is off.

[Michael Starzinger, 2013-06-06 12:03:11]

One high-level comment: I don't think JSTypedArray or JSArrayBuffer objects can
have in-object properties. So their instance size should be statically known,
which makes the visitors a lot simpler than they are currently implemented.

[Dmitry Lomov (no reviews), 2013-06-06 13:18:34]

Thanks a lot for review - please take another look.

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

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting-inl....
src/objects-visiting-inl.h:125: 
On 2013/06/06 09:53:18, Michael Starzinger wrote:
> nit: Add a second empty newline.

Done.

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting-inl....
src/objects-visiting-inl.h:143: 
On 2013/06/06 09:53:18, Michael Starzinger wrote:
> nit: Drop the third empty newline.

Done.

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting-inl....
src/objects-visiting-inl.h:199: 
On 2013/06/06 09:53:18, Michael Starzinger wrote:
> nit: Drop the second empty newline.

Done.

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting-inl....
src/objects-visiting-inl.h:470: 
On 2013/06/06 09:53:18, Michael Starzinger wrote:
> nit: Add a second empty newline.

Done.

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting.cc
File src/objects-visiting.cc (right):

https://codereview.chromium.org/15562008/diff/20001/src/objects-visiting.cc#n...
src/objects-visiting.cc:154: return GetVisitorIdForSize(kVisitJSObject,
On 2013/06/06 09:53:18, Michael Starzinger wrote:
> nit: Indentation is off.

Done.

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

https://codereview.chromium.org/15562008/diff/20001/src/objects.h#newcode8831
src/objects.h:8831: // [next]: linked list of typed arrays over the same array
buffer.
On 2013/06/06 09:33:35, Hannes Payer wrote:
> should be called weak_next

Done.

https://codereview.chromium.org/15562008/diff/20001/test/cctest/test-weaktype...
File test/cctest/test-weaktypedarrays.cc (right):

https://codereview.chromium.org/15562008/diff/20001/test/cctest/test-weaktype...
test/cctest/test-weaktypedarrays.cc:134: "var ab2 = new ArrayBuffer(256);");
On 2013/06/06 09:33:35, Hannes Payer wrote:
> indent

Done.

https://codereview.chromium.org/15562008/diff/20001/test/cctest/test-weaktype...
test/cctest/test-weaktypedarrays.cc:167: 
Done.
I have added this to *FromScript tests - it is easier to create and clean up
references at will in that context. 
On 2013/06/06 09:33:35, Hannes Payer wrote:
> Can we add a third typed array to the week list and delete, once the first,
once
> the middle, and once the third element? To get all corner cases of the list
> implementation.

[Hannes Payer (out of office), 2013-06-07 10:43:50]

lgtm

[Dmitry Lomov (no reviews), 2013-06-07 10:52:23]

Committed patchset #6 manually as r15000 (presubmit successful).

[Michael Starzinger, 2013-06-07 11:02:57]

LGTM from my end as well, even though you didn't seem to bother. :)

[Dmitry Lomov (no reviews), 2013-06-07 11:12:58]

On 2013/06/07 11:02:57, Michael Starzinger wrote:
> LGTM from my end as well, even though you didn't seem to bother. :)

Sorry about, I treated R= line as an 'or' not as 'and'. Rest assured, I do care
about your opinion!
Next time I'll designate one person as an R= and put others on CC:.

Thanks,
Dmitry