Ensure that generated code for object literals will call Runtime_DefineOrRedefineAccessorProperty o…

Ensure that generated code for object literals will call Runtime_DefineOrRedefineAccessorProperty only once per accessor property.

To do this, we collect all accessor properties in a first pass and emit code for
defining those properties afterwards in a second pass.

As a finger exercise, the table used for collecting accessors has a (subset of
an) STL-like iterator interface, including STL-like names and operators.
Although C++ is quite verbose here (as usual, but partly this is caused by our
current slightly clumsy classes/templates), things work out quite nicely and it
cleans up some confusion, e.g. a table entry is not an iterator etc.
Everything compiles into very efficient code, e.g. the loop condition 'it !=
accessor_table.end()' compiles into a single 'testl' instruction on ia32.
+1 for using standard APIs!

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

[rossberg, 2012-03-13 13:59:10]

While I agree that this is a nicer interface, I think full-codegen is the wrong
place. Can't you put the iterator interface into hashmap itself, so that other
uses can benefit from it in the future? You could also put the a generic version
of the value_type thingy there (although ultimately, we should simply templatise
TemplateHashMap properly).

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/full-codegen.h
File src/full-codegen.h (right):

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/full-codegen.h#n...
src/full-codegen.h:816: class Accessors: public ZoneObject {
This is just a simple pair, not abstracting anything, so a plain struct seems
more appropriate.

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/full-codegen.h#n...
src/full-codegen.h:833: Literal* first() const { return
reinterpret_cast<Literal*>(key); }
These can be static_cast's.

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/full-codegen.h#n...
src/full-codegen.h:853: return  map_ != other.map_ || entry_ != other.entry_;
Is the first condition necessary? I believe iterator comparison is undefined
anyway if pointing into different containers.

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/ia32/full-codege...
File src/ia32/full-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/ia32/full-codege...
src/ia32/full-codegen-ia32.cc:1416: __
push(Immediate(isolate()->factory()->null_value()));
Not this change, but 'undefined' rather than 'null' is what is typically used in
circumstances like this.

[Sven Panne, 2012-03-14 15:26:56]

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/full-codegen.h
File src/full-codegen.h (right):

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/full-codegen.h#n...
src/full-codegen.h:816: class Accessors: public ZoneObject {
On 2012/03/13 13:59:10, rossberg wrote:
> This is just a simple pair, not abstracting anything, so a plain struct seems
> more appropriate.

I'm perfectly fine with this, and as Uncle Bob says: "The quasi-encapsulation of
beans seems to make some OO purists feel better, but usually provides no other
benefit." :-)

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/full-codegen.h#n...
src/full-codegen.h:833: Literal* first() const { return
reinterpret_cast<Literal*>(key); }
On 2012/03/13 13:59:10, rossberg wrote:
> These can be static_cast's.

Done.

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/full-codegen.h#n...
src/full-codegen.h:853: return  map_ != other.map_ || entry_ != other.entry_;
On 2012/03/13 13:59:10, rossberg wrote:
> Is the first condition necessary? I believe iterator comparison is undefined
> anyway if pointing into different containers.

In a first version, I left the first part out, but feared a comment saying "This
is not correct for blablabla reasons.", so I added it. :-) Actually the first
test is even totally removed by gcc in the loop condition for the iteration over
the table, because it can statically see that it is always false.

Removing it again...

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/ia32/full-codege...
File src/ia32/full-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/9691040/diff/2001/src/ia32/full-codege...
src/ia32/full-codegen-ia32.cc:1416: __
push(Immediate(isolate()->factory()->null_value()));
On 2012/03/13 13:59:10, rossberg wrote:
> Not this change, but 'undefined' rather than 'null' is what is typically used
in
> circumstances like this.

Nope, 'undefined' won't work here, we really need a value which can't be an
accessor: 'undefined' can be one, but 'null' can not.

[rossberg, 2012-03-14 16:29:03]

LGTM, with small changes.

https://chromiumcodereview.appspot.com/9691040/diff/7013/src/full-codegen.h
File src/full-codegen.h (right):

https://chromiumcodereview.appspot.com/9691040/diff/7013/src/full-codegen.h#n...
src/full-codegen.h:820: Iterator lookup(Literal* literal) {
Nit: Shouldn't this be called 'find' and have the same interface?

On second thought, this wrapper class wouldn't even be necessary if the HashMap
itself was properly templatized and could work on arbitrary types, not just
pointers, like a real STL thingy. Then the accessor struct could be flattened
into the entry struct and needn't be allocated separately.

Such a change might or might not blow up code size -- would be worth a try (but
not in this CL :) ).

https://chromiumcodereview.appspot.com/9691040/diff/7013/src/hashmap.h
File src/hashmap.h (right):

https://chromiumcodereview.appspot.com/9691040/diff/7013/src/hashmap.h#newcod...
src/hashmap.h:301: struct value_type {
Hm, value_type doesn't seem to fit. I'd call this entry_type, and name the
fields 'key' and 'value'.

[Sven Panne, 2012-03-15 07:12:56]

https://chromiumcodereview.appspot.com/9691040/diff/7013/src/full-codegen.h
File src/full-codegen.h (right):

https://chromiumcodereview.appspot.com/9691040/diff/7013/src/full-codegen.h#n...
src/full-codegen.h:820: Iterator lookup(Literal* literal) {
On 2012/03/14 16:29:03, rossberg wrote:
> Nit: Shouldn't this be called 'find' and have the same interface?

In a language with lazy evaluation, yes. In C++ we would need a mechanism to
delay the creation of an Accessors instance somehow. Introducing a new abstract
class just for this or waiting for lambdas to be available everywhere seems to
be too much. :-)

In general, I think that mixing lookup and insertion is a bad idea from a
design/API point of view. STL has find and insert for this... *sigh*

Regarding proper templatization: We discussed this offline, and it was *you* who
proposed to do this in a stepwise manner and for pointers only initially. :-)
But I fully agree that this class is only a hint that its base class is poorly
designed, and we can change this in later CLs.

https://chromiumcodereview.appspot.com/9691040/diff/7013/src/hashmap.h
File src/hashmap.h (right):

https://chromiumcodereview.appspot.com/9691040/diff/7013/src/hashmap.h#newcod...
src/hashmap.h:301: struct value_type {
On 2012/03/14 16:29:03, rossberg wrote:
> Hm, value_type doesn't seem to fit. I'd call this entry_type, and name the
> fields 'key' and 'value'.

The whole point of this finger exercise was to be more STLish, and
value_type/first/second are the names STL uses, so I think I'll leave it as it
is.

[Vitaly Repeshko, 2012-03-15 07:59:26]

Drive by comments.

I don't think adding STL-like things to V8 is a good idea. At least for
consistency reasons. You may argue that C++ developers are expected to know (and
love) STL. Yet the rest of V8 code does not use STL. Making a container
implementation fully STL conformant is a lot of boilerplate code nobody wants.
(I think you can only justify that when you're creating a drop in replacement
for an existing container, e.g., Google's dense and sparse hash maps.) Not
making it fully conformant means it will still be surprising to engineers
expecting conformance. It seems so far V8 has been either avoiding using hash
maps by adding extra properties to various objects (e.g., see
CalculateEmitStore) or, if a hash map must be used, building ad hoc wrappers
with Google C++ style interfaces (Lookup, Insert, etc.).


-- Vitaly

https://chromiumcodereview.appspot.com/9691040/diff/11002/src/ia32/full-codeg...
File src/ia32/full-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/9691040/diff/11002/src/ia32/full-codeg...
src/ia32/full-codegen-ia32.cc:1511: // Emit code to define accessors, using only
a single call to the runtime for
Can this share at least some code with CalculateEmitStore (called above)?

[Sven Panne, 2012-03-15 09:08:44]

This isn't intended to be a full drop-in replacement for an STL container, it is
just templatizing things more to get more compile-time checking instead of
insane casts all over the code, and it is reducing our usual ad-hoc inconsistent
naming of things by using well-known names.

Folding things together is normally only a good idea if things are really,
really performance-critical, which is not the case here: We need a mapping from
the property names to their getters/setters only during a very short time span
(during a single method), so adding this info to the AST nodes feels plainly
wrong and increases the permanent memory usage.

In general we should not fold things together more than they already are, but do
the exact opposite and untangle things. Ask Andreas about his fun in the parser
while trying to implement modules... :-)