Avoid duplication of a hidden & inherited prototype's properties.

Avoid duplication of a hidden & inherited prototype's properties.

In Runtime_GetLocalPropertyNames(), the hidden prototypes of an object
are also consulted when deriving the property name set. However, if
given a function object and its template was inherited from the
template of one of its hidden prototypes, that hidden prototype's
property accessors will be present on the object already. Unwanted
duplicates will therefore appear.

Hence, go through the property names that the hidden prototypes
contribute and remove any already occurring ones.

Assumed to be a rare constellation, so the cost of this extra pass is
considered acceptable.

LOG=N
R=dcarney@chromium.org, jochen@chromium.org, rossberg@chromium.org
BUG=269562
Committed: https://code.google.com/p/v8/source/detail?r=18448

[sof, 2013-12-16 21:17:49]

Please take a look when you next have a spare moment.

The overlap between hidden prototypes and inherited function templates is
unfortunate. Addressing it here makes some sense to me, but there might be well
be more appropriate ways to address this one (and elsewhere.) Please let me know
if a trip back to the drawing board is warranted.

[jochen (gone - plz use gerrit), 2013-12-17 15:00:53]

looks sane

https://codereview.chromium.org/116533003/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/116533003/diff/20001/src/runtime.cc#newcode5733
src/runtime.cc:5733: if (!constructor->IsJSFunction()) {
nit. v8 style is if (...) return false; (in one line). if it doesn't fit in one
line, but the body is just one line, no need for { }

https://codereview.chromium.org/116533003/diff/20001/src/runtime.cc#newcode5740
src/runtime.cc:5740: return constructor_fun->shared()->
nit the -> should go on the next line

https://codereview.chromium.org/116533003/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/116533003/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:10009: Local<v8::FunctionTemplate> t1 =
v8::FunctionTemplate::New();
please pass an isolate to ::New (here and below)

[sof, 2013-12-17 15:34:40]

On 2013/12/17 15:00:53, jochen wrote:
> looks sane
> 
> https://codereview.chromium.org/116533003/diff/20001/src/runtime.cc
> File src/runtime.cc (right):
> 
>
https://codereview.chromium.org/116533003/diff/20001/src/runtime.cc#newcode5733
> src/runtime.cc:5733: if (!constructor->IsJSFunction()) {
> nit. v8 style is if (...) return false; (in one line). if it doesn't fit in
one
> line, but the body is just one line, no need for { }
> 

Thanks, will follow that. Done.

>
https://codereview.chromium.org/116533003/diff/20001/src/runtime.cc#newcode5740
> src/runtime.cc:5740: return constructor_fun->shared()->
> nit the -> should go on the next line
> 

Done.

> https://codereview.chromium.org/116533003/diff/20001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (right):
> 
>
https://codereview.chromium.org/116533003/diff/20001/test/cctest/test-api.cc#...
> test/cctest/test-api.cc:10009: Local<v8::FunctionTemplate> t1 =
> v8::FunctionTemplate::New();
> please pass an isolate to ::New (here and below)

Done.

[dcarney, 2013-12-19 19:46:12]

https://codereview.chromium.org/116533003/diff/40001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/116533003/diff/40001/src/runtime.cc#newcode5749
src/runtime.cc:5749: !IsInheritedApiFunctionOf(obj, proto)) {
filtering here is problematic as there could be properties higher up the chain
that should be included or even properties set on this proto that should be
included

you need to do the full chain walk here and filter correctly in
Runtime_GetLocalPropertyNames by not inserting duplicate names into the list. 
It'll be expensive, but you only need to pay the cost for objects with hidden
prototype links, which are rare.

https://codereview.chromium.org/116533003/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/116533003/diff/40001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:10040: ExpectInt32("names.length", 4);
'names' should include 'n1' and 'n2' as they are in the hidden prototype chain
only 'foo', 'bar', and 'bas' should be filtered

[rossberg, 2013-12-20 12:54:54]

I'm doubtful about this CL, see Dan's and my comments. I think the obvious
solution is to simply eliminate duplicates downstream.

https://codereview.chromium.org/116533003/diff/40001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/116533003/diff/40001/src/runtime.cc#newcode5732
src/runtime.cc:5732: Object *constructor =
HeapObject::cast(proto)->map()->constructor();
Note that .constructor is a mutable property, so I'm not sure this test is
actually correct.

[sof, 2013-12-20 15:13:49]

On 2013/12/20 12:54:54, rossberg wrote:
> I'm doubtful about this CL, see Dan's and my comments. I think the obvious
> solution is to simply eliminate duplicates downstream.
> 

thanks, will adjust. How far downstream is acceptable -- something for the
embedder entirely to handle, or are you referring to
Runtime_GetLocalPropertyNames() ?

[sof, 2013-12-22 08:39:55]

https://codereview.chromium.org/116533003/diff/40001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/116533003/diff/40001/src/runtime.cc#newcode5749
src/runtime.cc:5749: !IsInheritedApiFunctionOf(obj, proto)) {
On 2013/12/19 19:46:12, dcarney wrote:
> filtering here is problematic as there could be properties higher up the chain
> that should be included or even properties set on this proto that should be
> included
> 
> you need to do the full chain walk here and filter correctly in
> Runtime_GetLocalPropertyNames by not inserting duplicate names into the list. 
> It'll be expensive, but you only need to pay the cost for objects with hidden
> prototype links, which are rare.

Thanks, the initial version wasn't general enough nor complete. As you suggest,
I've switched to explicitly removing duplicates instead.

Spotting those dupes isn't without cost, but if we assume them being rare,
perhaps acceptable?

https://codereview.chromium.org/116533003/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/116533003/diff/40001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:10040: ExpectInt32("names.length", 4);
On 2013/12/19 19:46:12, dcarney wrote:
> 'names' should include 'n1' and 'n2' as they are in the hidden prototype chain
> only 'foo', 'bar', and 'bas' should be filtered

Eh, right :) Thanks, updated the pass conditions accordingly.

[dcarney, 2013-12-24 08:48:08]

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc#newcode5815
src/runtime.cc:5815: RUNTIME_ASSERT(names->get(j)->IsString());
I don't think these asserts are necessary, but the correct assert would be
IsName(), since this function supports returning symbols as well as strings. 
also, an ASSERT would make more sense here

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc#newcode5818
src/runtime.cc:5818: if (!names->get(k)->IsTheHole() &&
no need for a hole check here, see last comment

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc#newcode5822
src/runtime.cc:5822: if (name_from_hidden_proto->Equals(name)) {
all Name keys for properties are either symbols or internalized strings, so
Equals is overkill there, can just use == and merge the above if clause to make

name_from_hidden_proto == names->get(k) &&
isolate->heap()->hidden_string() != names->get(k)

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc#newcode5842
src/runtime.cc:5842: if (proto_with_hidden_properties > 0 || duplicates > 0) {
maybe merge these 2 variables into a single duplicate_properties variable

[dcarney, 2013-12-26 13:26:58]

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc#newcode5808
src/runtime.cc:5808: if (i > 0 && jsproto->map()->is_hidden_prototype()) {
this should just be i > 0

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc#newcode5842
src/runtime.cc:5842: if (proto_with_hidden_properties > 0 || duplicates > 0) {
actually, a better solution might be to call this unified variable
hidden_strings and insert the hidden string above instead of the hole. it would
reduce complexity

[sof, 2013-12-26 21:48:44]

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc#newcode5808
src/runtime.cc:5808: if (i > 0 && jsproto->map()->is_hidden_prototype()) {
On 2013/12/26 13:26:58, dcarney wrote:
> this should just be i > 0

Done.

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc#newcode5815
src/runtime.cc:5815: RUNTIME_ASSERT(names->get(j)->IsString());
On 2013/12/24 08:48:08, dcarney wrote:
> I don't think these asserts are necessary, but the correct assert would be
> IsName(), since this function supports returning symbols as well as strings. 
> also, an ASSERT would make more sense here

Gone; I kept the use of String (rather than Name.)

https://codereview.chromium.org/116533003/diff/120001/src/runtime.cc#newcode5842
src/runtime.cc:5842: if (proto_with_hidden_properties > 0 || duplicates > 0) {
On 2013/12/26 13:26:58, dcarney wrote:
> actually, a better solution might be to call this unified variable
> hidden_strings and insert the hidden string above instead of the hole. it
would
> reduce complexity

Nice redux; done.

[dcarney, 2013-12-27 09:32:22]

lgtm, if the comments below are addressed

https://codereview.chromium.org/116533003/diff/180001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/116533003/diff/180001/src/runtime.cc#newcode5815
src/runtime.cc:5815: String* name_from_hidden_proto =
String::cast(names->get(j));
You can't cast to String here. It would have to be Name, but in this case,
there's no need to cast whatsoever.  Just use Object*

https://codereview.chromium.org/116533003/diff/180001/src/runtime.cc#newcode5818
src/runtime.cc:5818: String* name = String::cast(names->get(k));
same here

https://codereview.chromium.org/116533003/diff/180001/src/runtime.cc#newcode5846
src/runtime.cc:5846: if (name == isolate->heap()->hidden_string()) continue;
would you please add a hidden_strings-- here and an ASSERT_EQ(0, hidden_strings)
after the loop.  It will be a good check that bookkeeping is correct in case of
future changes to this logic

[sof, 2013-12-27 10:52:42]

https://codereview.chromium.org/116533003/diff/180001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/116533003/diff/180001/src/runtime.cc#newcode5846
src/runtime.cc:5846: if (name == isolate->heap()->hidden_string()) continue;
On 2013/12/27 09:32:23, dcarney wrote:
> would you please add a hidden_strings-- here and an ASSERT_EQ(0,
hidden_strings)
> after the loop.  It will be a good check that bookkeeping is correct in case
of
> future changes to this logic

Done (under DEBUG.)

[dcarney, 2013-12-27 11:14:16]

looking good.

https://codereview.chromium.org/116533003/diff/250001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/116533003/diff/250001/src/runtime.cc#newcode5847
src/runtime.cc:5847: #ifdef DEBUG
If the compiler doesn't complain when you remove this #ifdef, please remove it. 
It's not typical of v8 code to leave it in, and the compiler will probably
remove the decrement in non debug mode anyway.

[sof, 2013-12-27 12:16:00]

https://codereview.chromium.org/116533003/diff/250001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/116533003/diff/250001/src/runtime.cc#newcode5847
src/runtime.cc:5847: #ifdef DEBUG
On 2013/12/27 11:14:17, dcarney wrote:
> If the compiler doesn't complain when you remove this #ifdef, please remove
it. 
> It's not typical of v8 code to leave it in, and the compiler will probably
> remove the decrement in non debug mode anyway.

Adjusted to follow local convention.

No warnings elicited + checked that the local release build generated the exact
same instruction sequence for the function, with & without. g++-4.7.3

[dcarney, 2013-12-27 12:39:56]

Great, thanks!
I'm not around to be able to land this for a while.
@rossberg, @jochen: maybe you can land this for me?

[sof, 2013-12-27 13:05:33]

On 2013/12/27 12:39:56, dcarney wrote:
> Great, thanks!
> I'm not around to be able to land this for a while.
> @rossberg, @jochen: maybe you can land this for me?

Thanks very much. No particular rush on my part, whenever there's time.

[jochen (gone - plz use gerrit), 2014-01-03 11:19:19]

Committed patchset #8 manually as r18448 (presubmit successful).

[sof, 2014-01-03 12:32:17]

On 2014/01/03 11:19:19, jochen wrote:
> Committed patchset #8 manually as r18448 (presubmit successful).

Thanks