Add infrastructure to keep track of references to prototypes.

Add infrastructure to keep track of references to prototypes.

There are no users of this infrastructure yet, so it's behind an off-by-default flag.

Committed: https://crrev.com/60dafcaab278825cebdfb998370be3cfe10acf8c
Cr-Commit-Position: refs/heads/master@{#25829}

[Jakob Kummerow, 2014-11-27 17:16:16]

jkummerow@chromium.org changed reviewers:
+ verwaest@chromium.org

[Jakob Kummerow, 2014-11-27 17:16:17]

Toon: PTAL.
Ulan: FYI (WeakFixedArray definition in src/objects*).

I can remove the FLAG_trace_weak_arrays if you don't like it.

[Toon Verwaest, 2014-12-01 15:23:54]

https://codereview.chromium.org/768633002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode8112
src/objects.cc:8112: Handle<FixedArray>::cast(array)->set(index +
kReservedFieldCount, *cell);
So kReservedFieldCount is just an internal header? What about naming it
kFirstIndex like in the other arrays?

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode8158
src/objects.cc:8158: int first_index = array->last_used_index();
Seems like you would want to start at last_used_index+1?

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode8168
src/objects.cc:8168: if (i == array->length()) i = 0;
i = (i + 1) % array->length();

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode9791
src/objects.cc:9791: if (!WeakFixedArray::StoreAnywhere(array, user)) {
Handle<WFA> new_array = WFA::Add(array, user);
must_store = !new_array.is_identical_to(array);

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode9808
src/objects.cc:9808: Handle<Object> maybe_array =
JSObject::GetProperty(&it).ToHandleChecked();
JSObject::GetProperty(prototype, symbol);

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode9837
src/objects.cc:9837: if (!back->IsMap()) return true;
That would include maps that aren't part of a transition tree right? Including
slow-mode maps.

https://codereview.chromium.org/768633002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/768633002/diff/1/src/objects.h#newcode2601
src/objects.h:2601: static bool StoreAnywhere(Handle<WeakFixedArray> array,
What about combining StoreAnywhere with GrowAndStore into
Handle<WFA> Add(Handle<WFA>, ...)?

https://codereview.chromium.org/768633002/diff/1/src/objects.h#newcode2607
src/objects.h:2607: void Delete(Handle<HeapObject> value);
And then rename to Remove to match Add.

https://codereview.chromium.org/768633002/diff/1/src/objects.h#newcode2627
src/objects.h:2627: friend class Factory;  // For kReservedFieldCount.
Just move the method onto WeakFixedArray rather than having this indirection
over the factory. Just like DescriptorArray::Allocate. In that case, you don't
need to friend anything.

[Jakob Kummerow, 2014-12-02 12:32:15]

Thanks for the review.

https://codereview.chromium.org/768633002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode8112
src/objects.cc:8112: Handle<FixedArray>::cast(array)->set(index +
kReservedFieldCount, *cell);
On 2014/12/01 15:23:54, Toon Verwaest wrote:
> So kReservedFieldCount is just an internal header? What about naming it
> kFirstIndex like in the other arrays?

Done.

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode8158
src/objects.cc:8158: int first_index = array->last_used_index();
On 2014/12/01 15:23:54, Toon Verwaest wrote:
> Seems like you would want to start at last_used_index+1?

No; the last element may have been added and then removed again, so it's best to
start searching at its index.

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode8168
src/objects.cc:8168: if (i == array->length()) i = 0;
On 2014/12/01 15:23:54, Toon Verwaest wrote:
> i = (i + 1) % array->length();

Done.

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode9791
src/objects.cc:9791: if (!WeakFixedArray::StoreAnywhere(array, user)) {
On 2014/12/01 15:23:54, Toon Verwaest wrote:
> Handle<WFA> new_array = WFA::Add(array, user);
> must_store = !new_array.is_identical_to(array);

Done.

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode9808
src/objects.cc:9808: Handle<Object> maybe_array =
JSObject::GetProperty(&it).ToHandleChecked();
On 2014/12/01 15:23:54, Toon Verwaest wrote:
> JSObject::GetProperty(prototype, symbol);

Done.

https://codereview.chromium.org/768633002/diff/1/src/objects.cc#newcode9837
src/objects.cc:9837: if (!back->IsMap()) return true;
On 2014/12/01 15:23:54, Toon Verwaest wrote:
> That would include maps that aren't part of a transition tree right? Including
> slow-mode maps.

Done -- blacklisted dictionary maps. On the other hand, these rules obviously
depend on what we need the information for, so we anyway might have to adapt
them over time as necessary.

https://codereview.chromium.org/768633002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/768633002/diff/1/src/objects.h#newcode2601
src/objects.h:2601: static bool StoreAnywhere(Handle<WeakFixedArray> array,
On 2014/12/01 15:23:54, Toon Verwaest wrote:
> What about combining StoreAnywhere with GrowAndStore into
> Handle<WFA> Add(Handle<WFA>, ...)?

Done.

https://codereview.chromium.org/768633002/diff/1/src/objects.h#newcode2607
src/objects.h:2607: void Delete(Handle<HeapObject> value);
On 2014/12/01 15:23:54, Toon Verwaest wrote:
> And then rename to Remove to match Add.

Done.

https://codereview.chromium.org/768633002/diff/1/src/objects.h#newcode2627
src/objects.h:2627: friend class Factory;  // For kReservedFieldCount.
On 2014/12/01 15:23:54, Toon Verwaest wrote:
> Just move the method onto WeakFixedArray rather than having this indirection
> over the factory. Just like DescriptorArray::Allocate. In that case, you don't
> need to friend anything.

Done. I've merged CopyFrom into it, to avoid redundant initialization.

[ulan, 2014-12-15 15:40:35]

ulan@chromium.org changed reviewers:
+ ulan@chromium.org

[ulan, 2014-12-15 15:40:36]

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

https://codereview.chromium.org/768633002/diff/20001/src/objects.cc#newcode8174
src/objects.cc:8174: return;  // StoreAnywhere() makes sure there are no
duplicates.
What is StoreAnywhere()?

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

https://codereview.chromium.org/768633002/diff/20001/src/objects.h#newcode2602
src/objects.h:2602: bool may_be_duplicate = false);
Please use enum with descriptive name instead of bool. E.g. ALWAYS_ADD,
ADD_IF_NOT_FOUND.

https://codereview.chromium.org/768633002/diff/20001/src/objects.h#newcode2607
src/objects.h:2607: inline int length() const;
Nit: I personally don't like non-virtual overrides because they make code harder
to read: to see what x->get(i) does, one needs to remember whether x is
WeakFixedArray or FixedArray. I would use x->Get(i) and x->Length() for
WeakFixedArray methods. Feel free to change or leave as is.

https://codereview.chromium.org/768633002/diff/20001/src/runtime/runtime-obje...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/768633002/diff/20001/src/runtime/runtime-obje...
src/runtime/runtime-object.cc:999: if
(isolate->IsInternallyUsedPropertyName(name_from_hidden_proto)) {
Shouldn't this also check (filter & STRING) == 0?

[Jakob Kummerow, 2014-12-15 16:18:49]

Thanks for the review. Will now rebase and use Map::WeakCellForMap().

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

https://codereview.chromium.org/768633002/diff/20001/src/objects.cc#newcode8174
src/objects.cc:8174: return;  // StoreAnywhere() makes sure there are no
duplicates.
On 2014/12/15 15:40:36, ulan wrote:
> What is StoreAnywhere()?

An outdated comment. Fixed.

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

https://codereview.chromium.org/768633002/diff/20001/src/objects.h#newcode2602
src/objects.h:2602: bool may_be_duplicate = false);
On 2014/12/15 15:40:36, ulan wrote:
> Please use enum with descriptive name instead of bool. E.g. ALWAYS_ADD,
> ADD_IF_NOT_FOUND.

Done.

https://codereview.chromium.org/768633002/diff/20001/src/objects.h#newcode2607
src/objects.h:2607: inline int length() const;
On 2014/12/15 15:40:36, ulan wrote:
> Nit: I personally don't like non-virtual overrides because they make code
harder
> to read: to see what x->get(i) does, one needs to remember whether x is
> WeakFixedArray or FixedArray. I would use x->Get(i) and x->Length() for
> WeakFixedArray methods. Feel free to change or leave as is.

Done. (I don't see the need, but I'm happy to make you happy.)

https://codereview.chromium.org/768633002/diff/20001/src/runtime/runtime-obje...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/768633002/diff/20001/src/runtime/runtime-obje...
src/runtime/runtime-object.cc:999: if
(isolate->IsInternallyUsedPropertyName(name_from_hidden_proto)) {
On 2014/12/15 15:40:36, ulan wrote:
> Shouldn't this also check (filter & STRING) == 0?

No, we don't need to do that any more, because calling
IsInternallyUsedPropertyName() on actually retrieved properties is a more
precise check than asking JSObject::HasHiddenProperties().

[ulan, 2014-12-15 16:32:02]

lgtm

[Jakob Kummerow, 2014-12-15 19:26:11]

The CQ bit was checked by jkummerow@chromium.org

[commit-bot: I haz the power, 2014-12-15 19:26:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/768633002/80001

[commit-bot: I haz the power, 2014-12-15 19:57:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-17 10:13:46]

Patchset 5 (id:??) landed as
https://crrev.com/60dafcaab278825cebdfb998370be3cfe10acf8c
Cr-Commit-Position: refs/heads/master@{#25829}