Remove calls to JSObject::SetLocalPropertyIgnoreAttributesTrampoline within objects.cc

Remove calls to JSObject::SetLocalPropertyIgnoreAttributesTrampoline within objects.cc

This includes handlifing:
-SetHiddenPropertiesHashTable
-ObjectHashSet::Add/Remove
-ObjectHashTable::Put

And splitting the following methods which previously took "allow creation" enum arguments to into side-effect-free getters and GetOrCreate*-handlfied getters.

-GetHash (now GetHash & handlified GetOrCreateHash)
-GetIdentityHash (now GetIdentityHash & handlified GetOrCreateIdentityHash)
-GetHiddenPropertiesHashTable (now GetHiddenPropertiesHashTable & handlified GetOrCreateaHiddenPropertiesHashTable)

BUG=v8:2877
R=mstarzinger@chromium.org

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

[rafaelw, 2013-10-29 05:30:23]

Michael,

I went ahead a took a crack at this. This isn't really ready for a full review
(having run tests or passed presubmit checks), but I'd like to know if this is
the general right direction.

Guidance appreciated.

[rafaelw, 2013-10-29 19:14:51]

Ok. Ready for review. PTAL.

https://codereview.chromium.org/48913008/diff/100001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/48913008/diff/100001/src/objects.h#newcode4068
src/objects.h:4068: static Handle<ObjectHashSet> EnsureSetCapacity(
Note that I've named this "EnsureSetCapacity" because I can't figure out how to
get the C++ compiler to stop complaining because in the CALL_HEAP_FUNCTION
implementation, it seems to be trying to invoke the static method (forgetting
about the inherted templatized method from HashTable.

I'm hoping you know what the solution to this is. Same problem below with
ShrinkSet.

https://codereview.chromium.org/48913008/diff/100001/test/cctest/test-diction...
File test/cctest/test-dictionary.cc (right):

https://codereview.chromium.org/48913008/diff/100001/test/cctest/test-diction...
test/cctest/test-dictionary.cc:178: int gc_count = isolate->heap()->gc_count();
I just took a guess at how to address this test.

https://codereview.chromium.org/48913008/diff/100001/test/cctest/test-diction...
test/cctest/test-dictionary.cc:182: // Calling Remove() should not cause GC
ever.
This comment doesn't seem true. I believe it's always true for this test, but
Remove() can clearly allocate more memory via Shrink.

[Michael Starzinger, 2013-11-04 10:29:10]

Looking good already. Just a couple of minor comments.

https://codereview.chromium.org/48913008/diff/100001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/48913008/diff/100001/src/objects.h#newcode1515
src/objects.h:1515: static Handle<Object> GetHash(Handle<Object> object,
Isolate* isolate,
nit: The current implementation as a wrapper requires the isolate parameter.
Once the implementation is fully handlified the isolate parameter is no longer
needed. Let's add a TODO to keep that in mind.

https://codereview.chromium.org/48913008/diff/100001/src/objects.h#newcode4066
src/objects.h:4066: static bool Contains(Handle<ObjectHashSet> table,
Handle<Object> key);
The ObjectHashSet::Contains method should never cause an allocation, it should
be possible to leave this one un-handlified.

https://codereview.chromium.org/48913008/diff/100001/src/objects.h#newcode4068
src/objects.h:4068: static Handle<ObjectHashSet> EnsureSetCapacity(
On 2013/10/29 19:14:51, rafaelw wrote:
> Note that I've named this "EnsureSetCapacity" because I can't figure out how
to
> get the C++ compiler to stop complaining because in the CALL_HEAP_FUNCTION
> implementation, it seems to be trying to invoke the static method (forgetting
> about the inherted templatized method from HashTable.
> 
> I'm hoping you know what the solution to this is. Same problem below with
> ShrinkSet.

One way to address that I can think of would be to add an explicit up-cast into
the implementation of ObjectHashSet::EnsureSetCapacity like this ...

Handle<HashTable<ObjectHashTableShape<1>, Object*> > table_base = table;

Unfortunately this will lead to access violations because
HashTable::EnsureSetCapacity is protected. But I think we can bake ObjectHashSet
a friend of HashTable to resolve this.

https://codereview.chromium.org/48913008/diff/100001/test/cctest/test-diction...
File test/cctest/test-dictionary.cc (right):

https://codereview.chromium.org/48913008/diff/100001/test/cctest/test-diction...
test/cctest/test-dictionary.cc:178: int gc_count = isolate->heap()->gc_count();
On 2013/10/29 19:14:51, rafaelw wrote:
> I just took a guess at how to address this test.

Yes, I think using the GC-count makes sense.

https://codereview.chromium.org/48913008/diff/100001/test/cctest/test-diction...
test/cctest/test-dictionary.cc:182: // Calling Remove() should not cause GC
ever.
On 2013/10/29 19:14:51, rafaelw wrote:
> This comment doesn't seem true. I believe it's always true for this test, but
> Remove() can clearly allocate more memory via Shrink.

You are right, the comment only holds for a HashSet of this particular size.
Maybe we should adapt the comment accordingly.

[rafaelw, 2013-11-04 10:58:06]

https://codereview.chromium.org/48913008/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/48913008/diff/100001/src/objects.cc#newcode15764
src/objects.cc:15764: new_table->set(table->EntryToIndex(entry), *key);
One question, this probably doesn't matter, but it seemed strange to me that the
original code was:

table->set(EntryToIndex(entry), key);

seems like it should be

table->set(table->EntryToIndex(entry), key);

or here,

new_table->set(new_table->EntryToIndex(entry), *key);

[Michael Starzinger, 2013-11-04 13:46:41]

https://codereview.chromium.org/48913008/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/48913008/diff/100001/src/objects.cc#newcode15764
src/objects.cc:15764: new_table->set(table->EntryToIndex(entry), *key);
On 2013/11/04 10:58:06, rafaelw wrote:
> One question, this probably doesn't matter, but it seemed strange to me that
the
> original code was:
> 
> table->set(EntryToIndex(entry), key);
> 
> seems like it should be
> 
> table->set(table->EntryToIndex(entry), key);
> 
> or here,
> 
> new_table->set(new_table->EntryToIndex(entry), *key);

Hmm, HashTable::EntryToIndex() should be a static function. It shouldn't need a
receiver object.

[rafaelw, 2013-11-04 15:12:00]

PTAL

https://codereview.chromium.org/48913008/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/48913008/diff/100001/src/objects.cc#newcode15764
src/objects.cc:15764: new_table->set(table->EntryToIndex(entry), *key);
That makes sense. The enclosing methods are now static as well, so these calls
are now explicitly namespaced with the classname (e.g.
ObjectHashSet::EntryToIndex, in this case).

On 2013/11/04 13:46:42, Michael Starzinger wrote:
> On 2013/11/04 10:58:06, rafaelw wrote:
> > One question, this probably doesn't matter, but it seemed strange to me that
> the
> > original code was:
> > 
> > table->set(EntryToIndex(entry), key);
> > 
> > seems like it should be
> > 
> > table->set(table->EntryToIndex(entry), key);
> > 
> > or here,
> > 
> > new_table->set(new_table->EntryToIndex(entry), *key);
> 
> Hmm, HashTable::EntryToIndex() should be a static function. It shouldn't need
a
> receiver object.

https://codereview.chromium.org/48913008/diff/100001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/48913008/diff/100001/src/objects.h#newcode1515
src/objects.h:1515: static Handle<Object> GetHash(Handle<Object> object,
Isolate* isolate,
On 2013/11/04 10:29:11, Michael Starzinger wrote:
> nit: The current implementation as a wrapper requires the isolate parameter.
> Once the implementation is fully handlified the isolate parameter is no longer
> needed. Let's add a TODO to keep that in mind.

Done (although this todo is now on GetOrCreateHash

https://codereview.chromium.org/48913008/diff/100001/src/objects.h#newcode4066
src/objects.h:4066: static bool Contains(Handle<ObjectHashSet> table,
Handle<Object> key);
On 2013/11/04 10:29:11, Michael Starzinger wrote:
> The ObjectHashSet::Contains method should never cause an allocation, it should
> be possible to leave this one un-handlified.

Done.

https://codereview.chromium.org/48913008/diff/100001/src/objects.h#newcode4068
src/objects.h:4068: static Handle<ObjectHashSet> EnsureSetCapacity(
Done. Applied to ObjectHash(Set/Table)::(EnsureCapacity/Shrink)

On 2013/11/04 10:29:11, Michael Starzinger wrote:
> On 2013/10/29 19:14:51, rafaelw wrote:
> > Note that I've named this "EnsureSetCapacity" because I can't figure out how
> to
> > get the C++ compiler to stop complaining because in the CALL_HEAP_FUNCTION
> > implementation, it seems to be trying to invoke the static method
(forgetting
> > about the inherted templatized method from HashTable.
> > 
> > I'm hoping you know what the solution to this is. Same problem below with
> > ShrinkSet.
> 
> One way to address that I can think of would be to add an explicit up-cast
into
> the implementation of ObjectHashSet::EnsureSetCapacity like this ...
> 
> Handle<HashTable<ObjectHashTableShape<1>, Object*> > table_base = table;
> 
> Unfortunately this will lead to access violations because
> HashTable::EnsureSetCapacity is protected. But I think we can bake
ObjectHashSet
> a friend of HashTable to resolve this.

https://codereview.chromium.org/48913008/diff/100001/test/cctest/test-diction...
File test/cctest/test-dictionary.cc (right):

https://codereview.chromium.org/48913008/diff/100001/test/cctest/test-diction...
test/cctest/test-dictionary.cc:182: // Calling Remove() should not cause GC
ever.
On 2013/10/29 19:14:51, rafaelw wrote:
> This comment doesn't seem true. I believe it's always true for this test, but
> Remove() can clearly allocate more memory via Shrink.

Done.

https://codereview.chromium.org/48913008/diff/170002/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/48913008/diff/170002/src/objects.cc#newcode1047
src/objects.cc:1047: ASSERT(IsJSReceiver());
Note that I've changed this for consistency with GetOrCreateHash below, which
cannot return Smi::FromInt(0) after an unreachable, because no Isolate is
available at that point to create a handle.

https://codereview.chromium.org/48913008/diff/170002/src/objects.cc#newcode4808
src/objects.cc:4808: Handle<Object> hash =
JSObject::GetOrCreateIdentityHash(object);
I *think* this is right. It's a bit hard for me to follow the
CALL_AND_RETRY_OR_DIE logic. Please verify.

[Michael Starzinger, 2013-11-04 17:58:58]

LGTM, just a couple of nits.

https://codereview.chromium.org/48913008/diff/170002/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/48913008/diff/170002/src/objects.cc#newcode4802
src/objects.cc:4802: JSObject::SetHiddenProperty(
nit: The explicit name-space prefix "JSObject::" shouldn't be necessary as we
are already in the same class. Applies a couple of times below as well.

https://codereview.chromium.org/48913008/diff/170002/src/objects.cc#newcode4808
src/objects.cc:4808: Handle<Object> hash =
JSObject::GetOrCreateIdentityHash(object);
On 2013/11/04 15:12:01, rafaelw wrote:
> I *think* this is right. It's a bit hard for me to follow the
> CALL_AND_RETRY_OR_DIE logic. Please verify.

AFAICT, the return value of JSObject::GetOrCreateIdentityHash should always be a
Smi. So it seems to me JSObject::GetIdentityHash(Handle<JSObject>) is deprecated
by now and the calls to it can be replaced with
JSObject::GetOrCreateIdentityHash() instead.

I only see one call-site in api.cc and that one should actually used
JSReceiver::GetOrCreateIdentityHash() instead so that we can avoid making
JSObject::GetOrCreateIdentityHash() public.

https://codereview.chromium.org/48913008/diff/170002/src/objects.cc#newcode4962
src/objects.cc:4962: Object* inline_value;
nit: The local "inline_value" variable is (almost) not used, let's drop it.

https://codereview.chromium.org/48913008/diff/170002/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/48913008/diff/170002/src/objects.h#newcode1507
src/objects.h:1507: MUST_USE_RESULT Object* GetHash();
nit: I think we can drop the MUST_USE_RESULT here, it's not necessarily a bug if
the return value is no used.

https://codereview.chromium.org/48913008/diff/170002/src/objects.h#newcode2007
src/objects.h:2007: inline MUST_USE_RESULT Object* GetIdentityHash();
nit: Likewise.

https://codereview.chromium.org/48913008/diff/170002/test/cctest/test-diction...
File test/cctest/test-dictionary.cc (right):

https://codereview.chromium.org/48913008/diff/170002/test/cctest/test-diction...
test/cctest/test-dictionary.cc:218: // Calling Put() should request GC by
returning a failure.
nit: Let's move the comment up one line, easier to read.

[rafaelw, 2013-11-05 09:56:54]

https://codereview.chromium.org/48913008/diff/170002/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/48913008/diff/170002/src/objects.cc#newcode4802
src/objects.cc:4802: JSObject::SetHiddenProperty(
On 2013/11/04 17:58:58, Michael Starzinger wrote:
> nit: The explicit name-space prefix "JSObject::" shouldn't be necessary as we
> are already in the same class. Applies a couple of times below as well.

Done.

https://codereview.chromium.org/48913008/diff/170002/src/objects.cc#newcode4808
src/objects.cc:4808: Handle<Object> hash =
JSObject::GetOrCreateIdentityHash(object);
On 2013/11/04 17:58:58, Michael Starzinger wrote:
> On 2013/11/04 15:12:01, rafaelw wrote:
> > I *think* this is right. It's a bit hard for me to follow the
> > CALL_AND_RETRY_OR_DIE logic. Please verify.
> 
> AFAICT, the return value of JSObject::GetOrCreateIdentityHash should always be
a
> Smi. So it seems to me JSObject::GetIdentityHash(Handle<JSObject>) is
deprecated
> by now and the calls to it can be replaced with
> JSObject::GetOrCreateIdentityHash() instead.
> 
> I only see one call-site in api.cc and that one should actually used
> JSReceiver::GetOrCreateIdentityHash() instead so that we can avoid making
> JSObject::GetOrCreateIdentityHash() public.

Done.

https://codereview.chromium.org/48913008/diff/170002/src/objects.cc#newcode4962
src/objects.cc:4962: Object* inline_value;
On 2013/11/04 17:58:58, Michael Starzinger wrote:
> nit: The local "inline_value" variable is (almost) not used, let's drop it.

Done.

https://codereview.chromium.org/48913008/diff/170002/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/48913008/diff/170002/src/objects.h#newcode1507
src/objects.h:1507: MUST_USE_RESULT Object* GetHash();
On 2013/11/04 17:58:58, Michael Starzinger wrote:
> nit: I think we can drop the MUST_USE_RESULT here, it's not necessarily a bug
if
> the return value is no used.

Done.

https://codereview.chromium.org/48913008/diff/170002/src/objects.h#newcode2007
src/objects.h:2007: inline MUST_USE_RESULT Object* GetIdentityHash();
On 2013/11/04 17:58:58, Michael Starzinger wrote:
> nit: Likewise.

Done.

https://codereview.chromium.org/48913008/diff/170002/test/cctest/test-diction...
File test/cctest/test-dictionary.cc (right):

https://codereview.chromium.org/48913008/diff/170002/test/cctest/test-diction...
test/cctest/test-dictionary.cc:218: // Calling Put() should request GC by
returning a failure.
On 2013/11/04 17:58:58, Michael Starzinger wrote:
> nit: Let's move the comment up one line, easier to read.

Done.

[rafaelw, 2013-11-05 11:47:22]

Committed patchset #11 manually as r17477 (presubmit successful).