js accessor creation on Template

js accessor creation on Template

R=svenpanne@chromium.org
BUG=

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

[dcarney, 2013-08-20 15:28:56]

here's a first round of this
it more or less works, but I'm going to add a bunch more tests once the approach
etc is commented on/lgtm'd

[Sven Panne, 2013-08-21 10:46:25]

First round of comments.

https://chromiumcodereview.appspot.com/22903012/diff/1/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/22903012/diff/1/src/api.cc#newcode958
src/api.cc:958: static void TemplateSet(i::Isolate* isolate,
I don't understand what this function is for... o_O

https://chromiumcodereview.appspot.com/22903012/diff/1/src/api.cc#newcode970
src/api.cc:970: if (data[i].IsEmpty()) {
Nit: Using a ternary ?: to calculate the value and uncoditionally adding it
makes things a bit clearer (i.e. the loop is actually a "map" in the FP sense).

https://chromiumcodereview.appspot.com/22903012/diff/1/src/apinatives.js
File src/apinatives.js (right):

https://chromiumcodereview.appspot.com/22903012/diff/1/src/apinatives.js#newc...
src/apinatives.js:120: %DefineJsAccessor(obj, properties, i);
Hmmm, this is somehow inconsistent with the 'length == 3' case above: There we
do the parsing of the properties array in JavaScript, here we do it on the
native side. Perhaps we should parse it in JavaScript in both cases.

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:4443: ACCESSORS_TO_SMI(AccessorPair, flag, kFlagOffset)
Can we rename this from plain "flag" to something more specific like
"access_flags"? (OK, this collides with the new method below, but you get the
idea.)

The name "flag" contains roughly as much information as "foo" or "hurz". :-)

As discussed offline: What were the results of measuring the number of
AccessorPairs in Chrome on typical web sites/applications?

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc#newcode516
src/objects.cc:516: } else if (obj->IsAccessorPair()) {
Just curious: Can we reach this code path with Foreign callbacks? Is it OK that
they are not handled at all? Depending on that, we might want to ASSERT
something here, but I am not sure what. ;-)

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc#newcode582
src/objects.cc:582: } else if (obj->IsAccessorPair()) {
Same here.

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc#newcode...
src/objects.cc:3375: } else if (obj->IsAccessorPair()) {
... and here.

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc#newcode...
src/objects.cc:6213: ASSERT_EQ(v8::DEFAULT, access_control);
Perhaps we should document this restriction in v8.h.

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.h#newcode9677
src/objects.h:9677: //   * a pointer to a map: a transition used to ensure map
sharing
Extend the comment, covering the access flag part.

[dcarney, 2013-08-21 10:59:33]

https://chromiumcodereview.appspot.com/22903012/diff/1/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/22903012/diff/1/src/api.cc#newcode958
src/api.cc:958: static void TemplateSet(i::Isolate* isolate,
On 2013/08/21 10:46:25, Sven Panne wrote:
> I don't understand what this function is for... o_O

I'm stashing multiple kinds of properties in a single NeanderArray
this just helps me do it

https://chromiumcodereview.appspot.com/22903012/diff/1/src/api.cc#newcode970
src/api.cc:970: if (data[i].IsEmpty()) {
On 2013/08/21 10:46:25, Sven Panne wrote:
> Nit: Using a ternary ?: to calculate the value and uncoditionally adding it
> makes things a bit clearer (i.e. the loop is actually a "map" in the FP
sense).

okay

https://chromiumcodereview.appspot.com/22903012/diff/1/src/apinatives.js
File src/apinatives.js (right):

https://chromiumcodereview.appspot.com/22903012/diff/1/src/apinatives.js#newc...
src/apinatives.js:120: %DefineJsAccessor(obj, properties, i);
On 2013/08/21 10:46:25, Sven Panne wrote:
> Hmmm, this is somehow inconsistent with the 'length == 3' case above: There we
> do the parsing of the properties array in JavaScript, here we do it on the
> native side. Perhaps we should parse it in JavaScript in both cases.

okay

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:4443: ACCESSORS_TO_SMI(AccessorPair, flag, kFlagOffset)
On 2013/08/21 10:46:25, Sven Panne wrote:
> Can we rename this from plain "flag" to something more specific like
> "access_flags"? (OK, this collides with the new method below, but you get the
> idea.)
> 
> The name "flag" contains roughly as much information as "foo" or "hurz". :-)

okay, but all the flag fields everywhere else are called flag

> 
> As discussed offline: What were the results of measuring the number of
> AccessorPairs in Chrome on typical web sites/applications?

I haven't done it yet. I'm doing this first, and will extend the class hierarchy
if necessary in a follow up cl

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc#newcode516
src/objects.cc:516: } else if (obj->IsAccessorPair()) {
On 2013/08/21 10:46:25, Sven Panne wrote:
> Just curious: Can we reach this code path with Foreign callbacks? Is it OK
that
> they are not handled at all? Depending on that, we might want to ASSERT
> something here, but I am not sure what. ;-)

there's no access overrides for foreign callbacks, so we can't ASSERT anything,
they just fall through

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc#newcode...
src/objects.cc:6213: ASSERT_EQ(v8::DEFAULT, access_control);
On 2013/08/21 10:46:25, Sven Panne wrote:
> Perhaps we should document this restriction in v8.h.

it's not a restriction, i just want to get this working

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.h#newcode9677
src/objects.h:9677: //   * a pointer to a map: a transition used to ensure map
sharing
On 2013/08/21 10:46:25, Sven Panne wrote:
> Extend the comment, covering the access flag part.

okay

[dcarney, 2013-08-22 08:34:00]

okay, i think i've addressed everything and things seem more or less to work....

[Sven Panne, 2013-08-22 08:52:50]

LGTM with a nit, but perhaps wait for Michael's review, too.

https://codereview.chromium.org/22903012/diff/16/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/22903012/diff/16/src/runtime.cc#newcode1966
src/runtime.cc:1966: if (getter->IsUndefined()) {
Tiny nit: Can we extract this if/then/else into a small static helper function?
This would remove the copy-n-paste + avoid the assignments to getter/setter
(which I effectively consider parameters of Runtime_SetAccessorProperty).

[dcarney, 2013-08-22 10:41:32]

nit fixed

will wait for michael if he wants in on this

[dcarney, 2013-08-26 11:59:32]

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