Ensure that generated code for object literals will call...

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!

Review URL: https://chromiumcodereview.appspot.com/9691040

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@11051 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/arm/full-codegen-arm.cc

@@ -1410,6 +1410,16 @@ void FullCodeGenerator::VisitRegExpLiteral(RegExpLiteral* expr) {
 }
 
 
+void FullCodeGenerator::EmitAccessor(Expression* expression) {
+  if (expression == NULL) {
+    __ LoadRoot(r1, Heap::kNullValueRootIndex);
+    __ push(r1);
+  } else {
+    VisitForStackValue(expression);
+  }
+}
+
+
 void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   Comment cmnt(masm_, "[ ObjectLiteral");
   Handle<FixedArray> constant_properties = expr->constant_properties();
@@ -1445,6 +1455,7 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   // marked expressions, no store code is emitted.
   expr->CalculateEmitStore();
 
+  AccessorTable accessor_table(isolate()->zone());
   for (int i = 0; i < expr->properties()->length(); i++) {
     ObjectLiteral::Property* property = expr->properties()->at(i);
     if (property->IsCompileTimeValue()) continue;
@@ -1493,27 +1504,29 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
         }
         break;
       case ObjectLiteral::Property::GETTER:
+        accessor_table.find(key)->second->getter = value;
+        break;
       case ObjectLiteral::Property::SETTER:
-        // Duplicate receiver on stack.
-        __ ldr(r0, MemOperand(sp));
-        __ push(r0);
-        VisitForStackValue(key);
-        if (property->kind() == ObjectLiteral::Property::GETTER) {
-          VisitForStackValue(value);
-          __ LoadRoot(r1, Heap::kNullValueRootIndex);
-          __ push(r1);
-        } else {
-          __ LoadRoot(r1, Heap::kNullValueRootIndex);
-          __ push(r1);
-          VisitForStackValue(value);
-        }
-        __ mov(r0, Operand(Smi::FromInt(NONE)));
-        __ push(r0);
-        __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
+        accessor_table.find(key)->second->setter = value;
         break;
     }
   }
 
+  // Emit code to define accessors, using only a single call to the runtime for
+  // each pair of corresponding getters and setters.
+  for (AccessorTable::Iterator it = accessor_table.begin();
+       it != accessor_table.end();
+       ++it) {
+    __ ldr(r0, MemOperand(sp));  // Duplicate receiver.
+    __ push(r0);
+    VisitForStackValue(it->first);
+    EmitAccessor(it->second->getter);
+    EmitAccessor(it->second->setter);
+    __ mov(r0, Operand(Smi::FromInt(NONE)));
+    __ push(r0);
+    __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
+  }
+
   if (expr->has_function()) {
     ASSERT(result_saved);
     __ ldr(r0, MemOperand(sp));

src/ast.h

@@ -1366,6 +1366,12 @@ class ObjectLiteral: public MaterializedLiteral {
     kHasFunction = 1 << 1
   };
 
+  struct Accessors: public ZoneObject {
+    Accessors() : getter(NULL), setter(NULL) { }
+    Expression* getter;
+    Expression* setter;
+  };
+
  protected:
   template<class> friend class AstNodeFactory;
 

src/full-codegen.h

@@ -470,6 +470,8 @@ class FullCodeGenerator: public AstVisitor {
                                  Label* done);
   void EmitVariableLoad(VariableProxy* proxy);
 
+  void EmitAccessor(Expression* expression);
+
   // Expects the arguments and the function already pushed.
   void EmitResolvePossiblyDirectEval(int arg_count);
 
@@ -804,6 +806,28 @@ class FullCodeGenerator: public AstVisitor {
 };
 
 
+// A map from property names to getter/setter pairs allocated in the zone.
+class AccessorTable: public TemplateHashMap<Literal,
+                                            ObjectLiteral::Accessors,
+                                            ZoneListAllocationPolicy> {
+ public:
+  explicit AccessorTable(Zone* zone) :
+      TemplateHashMap<Literal,
+                      ObjectLiteral::Accessors,
+                      ZoneListAllocationPolicy>(Literal::Match),
+      zone_(zone) { }
+
+  Iterator lookup(Literal* literal) {
+    Iterator it = find(literal, true);
+    if (it->second == NULL) it->second = new(zone_) ObjectLiteral::Accessors();
+    return it;
+  }
+
+ private:
+  Zone* zone_;
+};
+
+
 } }  // namespace v8::internal
 
 #endif  // V8_FULL_CODEGEN_H_

src/hashmap.h

@@ -36,15 +36,15 @@ namespace v8 {
 namespace internal {
 
 template<class AllocationPolicy>
-class TemplateHashMap {
+class TemplateHashMapImpl {
  public:
   typedef bool (*MatchFun) (void* key1, void* key2);
 
   // initial_capacity is the size of the initial hash map;
   // it must be a power of 2 (and thus must not be 0).
-  TemplateHashMap(MatchFun match, uint32_t initial_capacity = 8);
+  TemplateHashMapImpl(MatchFun match, uint32_t initial_capacity = 8);
 
-  ~TemplateHashMap();
+  ~TemplateHashMapImpl();
 
   // HashMap entries are (key, value, hash) triplets.
   // Some clients may not need to use the value slot
@@ -99,10 +99,10 @@ class TemplateHashMap {
   void Resize();
 };
 
-typedef TemplateHashMap<FreeStoreAllocationPolicy> HashMap;
+typedef TemplateHashMapImpl<FreeStoreAllocationPolicy> HashMap;
 
 template<class P>
-TemplateHashMap<P>::TemplateHashMap(MatchFun match,
+TemplateHashMapImpl<P>::TemplateHashMapImpl(MatchFun match,
                     uint32_t initial_capacity) {
   match_ = match;
   Initialize(initial_capacity);
@@ -110,13 +110,13 @@ TemplateHashMap<P>::TemplateHashMap(MatchFun match,
 
 
 template<class P>
-TemplateHashMap<P>::~TemplateHashMap() {
+TemplateHashMapImpl<P>::~TemplateHashMapImpl() {
   P::Delete(map_);
 }
 
 
 template<class P>
-typename TemplateHashMap<P>::Entry* TemplateHashMap<P>::Lookup(
+typename TemplateHashMapImpl<P>::Entry* TemplateHashMapImpl<P>::Lookup(
     void* key, uint32_t hash, bool insert) {
   // Find a matching entry.
   Entry* p = Probe(key, hash);
@@ -146,7 +146,7 @@ typename TemplateHashMap<P>::Entry* TemplateHashMap<P>::Lookup(
 
 
 template<class P>
-void TemplateHashMap<P>::Remove(void* key, uint32_t hash) {
+void TemplateHashMapImpl<P>::Remove(void* key, uint32_t hash) {
   // Lookup the entry for the key to remove.
   Entry* p = Probe(key, hash);
   if (p->key == NULL) {
@@ -206,7 +206,7 @@ void TemplateHashMap<P>::Remove(void* key, uint32_t hash) {
 
 
 template<class P>
-void TemplateHashMap<P>::Clear() {
+void TemplateHashMapImpl<P>::Clear() {
   // Mark all entries as empty.
   const Entry* end = map_end();
   for (Entry* p = map_; p < end; p++) {
@@ -217,13 +217,14 @@ void TemplateHashMap<P>::Clear() {
 
 
 template<class P>
-typename TemplateHashMap<P>::Entry* TemplateHashMap<P>::Start() const {
+typename TemplateHashMapImpl<P>::Entry* TemplateHashMapImpl<P>::Start() const {
   return Next(map_ - 1);
 }
 
 
 template<class P>
-typename TemplateHashMap<P>::Entry* TemplateHashMap<P>::Next(Entry* p) const {
+typename TemplateHashMapImpl<P>::Entry* TemplateHashMapImpl<P>::Next(Entry* p)
+    const {
   const Entry* end = map_end();
   ASSERT(map_ - 1 <= p && p < end);
   for (p++; p < end; p++) {
@@ -236,7 +237,7 @@ typename TemplateHashMap<P>::Entry* TemplateHashMap<P>::Next(Entry* p) const {
 
 
 template<class P>
-typename TemplateHashMap<P>::Entry* TemplateHashMap<P>::Probe(void* key,
+typename TemplateHashMapImpl<P>::Entry* TemplateHashMapImpl<P>::Probe(void* key,
                                                             uint32_t hash) {
   ASSERT(key != NULL);
 
@@ -258,7 +259,7 @@ typename TemplateHashMap<P>::Entry* TemplateHashMap<P>::Probe(void* key,
 
 
 template<class P>
-void TemplateHashMap<P>::Initialize(uint32_t capacity) {
+void TemplateHashMapImpl<P>::Initialize(uint32_t capacity) {
   ASSERT(IsPowerOf2(capacity));
   map_ = reinterpret_cast<Entry*>(P::New(capacity * sizeof(Entry)));
   if (map_ == NULL) {
@@ -271,7 +272,7 @@ void TemplateHashMap<P>::Initialize(uint32_t capacity) {
 
 
 template<class P>
-void TemplateHashMap<P>::Resize() {
+void TemplateHashMapImpl<P>::Resize() {
   Entry* map = map_;
   uint32_t n = occupancy_;
 
@@ -290,6 +291,50 @@ void TemplateHashMap<P>::Resize() {
   P::Delete(map);
 }
 
+
+// A hash map for pointer keys and values with an STL-like interface.
+template<class Key, class Value, class AllocationPolicy>
+class TemplateHashMap: private TemplateHashMapImpl<AllocationPolicy> {
+ public:
+  STATIC_ASSERT(sizeof(Key*) == sizeof(void*));  // NOLINT
+  STATIC_ASSERT(sizeof(Value*) == sizeof(void*));  // NOLINT
+  struct value_type {
+    Key* first;
+    Value* second;
+  };
+
+  class Iterator {
+   public:
+    Iterator& operator++() {
+      entry_ = map_->Next(entry_);
+      return *this;
+    }
+
+    value_type* operator->() { return reinterpret_cast<value_type*>(entry_); }
+    bool operator!=(const Iterator& other) { return  entry_ != other.entry_; }
+
+   private:
+    Iterator(const TemplateHashMapImpl<AllocationPolicy>* map,
+             typename TemplateHashMapImpl<AllocationPolicy>::Entry* entry) :
+        map_(map), entry_(entry) { }
+
+    const TemplateHashMapImpl<AllocationPolicy>* map_;
+    typename TemplateHashMapImpl<AllocationPolicy>::Entry* entry_;
+
+    friend class TemplateHashMap;
+  };
+
+  TemplateHashMap(
+      typename TemplateHashMapImpl<AllocationPolicy>::MatchFun match)
+    : TemplateHashMapImpl<AllocationPolicy>(match) { }
+
+  Iterator begin() const { return Iterator(this, this->Start()); }
+  Iterator end() const { return Iterator(this, NULL); }
+  Iterator find(Key* key, bool insert = false) {
+    return Iterator(this, Lookup(key, key->Hash(), insert));
+  }
+};
+
 } }  // namespace v8::internal
 
 #endif  // V8_HASHMAP_H_

src/ia32/full-codegen-ia32.cc

@@ -1411,6 +1411,15 @@ void FullCodeGenerator::VisitRegExpLiteral(RegExpLiteral* expr) {
 }
 
 
+void FullCodeGenerator::EmitAccessor(Expression* expression) {
+  if (expression == NULL) {
+    __ push(Immediate(isolate()->factory()->null_value()));
+  } else {
+    VisitForStackValue(expression);
+  }
+}
+
+
 void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   Comment cmnt(masm_, "[ ObjectLiteral");
   Handle<FixedArray> constant_properties = expr->constant_properties();
@@ -1445,6 +1454,7 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   // marked expressions, no store code is emitted.
   expr->CalculateEmitStore();
 
+  AccessorTable accessor_table(isolate()->zone());
   for (int i = 0; i < expr->properties()->length(); i++) {
     ObjectLiteral::Property* property = expr->properties()->at(i);
     if (property->IsCompileTimeValue()) continue;
@@ -1456,6 +1466,8 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
       result_saved = true;
     }
     switch (property->kind()) {
+      case ObjectLiteral::Property::CONSTANT:
+        UNREACHABLE();
       case ObjectLiteral::Property::MATERIALIZED_LITERAL:
         ASSERT(!CompileTimeValue::IsCompileTimeValue(value));
         // Fall through.
@@ -1487,24 +1499,28 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
           __ Drop(3);
         }
         break;
-      case ObjectLiteral::Property::SETTER:
       case ObjectLiteral::Property::GETTER:
-        __ push(Operand(esp, 0));  // Duplicate receiver.
-        VisitForStackValue(key);
-        if (property->kind() == ObjectLiteral::Property::GETTER) {
-          VisitForStackValue(value);
-          __ push(Immediate(isolate()->factory()->null_value()));
-        } else {
-          __ push(Immediate(isolate()->factory()->null_value()));
-          VisitForStackValue(value);
-        }
-        __ push(Immediate(Smi::FromInt(NONE)));
-        __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
+        accessor_table.lookup(key)->second->getter = value;
+        break;
+      case ObjectLiteral::Property::SETTER:
+        accessor_table.lookup(key)->second->setter = value;
         break;
-      default: UNREACHABLE();
     }
   }
 
+  // Emit code to define accessors, using only a single call to the runtime for
+  // each pair of corresponding getters and setters.
+  for (AccessorTable::Iterator it = accessor_table.begin();
+       it != accessor_table.end();
+       ++it) {
+    __ push(Operand(esp, 0));  // Duplicate receiver.
+    VisitForStackValue(it->first);
+    EmitAccessor(it->second->getter);
+    EmitAccessor(it->second->setter);
+    __ push(Immediate(Smi::FromInt(NONE)));
+    __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
+  }
+
   if (expr->has_function()) {
     ASSERT(result_saved);
     __ push(Operand(esp, 0));

src/mips/full-codegen-mips.cc

@@ -1421,6 +1421,16 @@ void FullCodeGenerator::VisitRegExpLiteral(RegExpLiteral* expr) {
 }
 
 
+void FullCodeGenerator::EmitAccessor(Expression* expression) {
+  if (expression == NULL) {
+    __ LoadRoot(a1, Heap::kNullValueRootIndex);
+    __ push(a1);
+  } else {
+    VisitForStackValue(expression);
+  }
+}
+
+
 void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   Comment cmnt(masm_, "[ ObjectLiteral");
   Handle<FixedArray> constant_properties = expr->constant_properties();
@@ -1456,6 +1466,7 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   // marked expressions, no store code is emitted.
   expr->CalculateEmitStore();
 
+  AccessorTable accessor_table(isolate()->zone());
   for (int i = 0; i < expr->properties()->length(); i++) {
     ObjectLiteral::Property* property = expr->properties()->at(i);
     if (property->IsCompileTimeValue()) continue;
@@ -1505,27 +1516,29 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
         }
         break;
       case ObjectLiteral::Property::GETTER:
+        accessor_table.find(key)->second->getter = value;
+        break;
       case ObjectLiteral::Property::SETTER:
-        // Duplicate receiver on stack.
-        __ lw(a0, MemOperand(sp));
-        __ push(a0);
-        VisitForStackValue(key);
-        if (property->kind() == ObjectLiteral::Property::GETTER) {
-          VisitForStackValue(value);
-          __ LoadRoot(a1, Heap::kNullValueRootIndex);
-          __ push(a1);
-        } else {
-          __ LoadRoot(a1, Heap::kNullValueRootIndex);
-          __ push(a1);
-          VisitForStackValue(value);
-        }
-        __ li(a0, Operand(Smi::FromInt(NONE)));
-        __ push(a0);
-        __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
+        accessor_table.find(key)->second->setter = value;
         break;
     }
   }
 
+  // Emit code to define accessors, using only a single call to the runtime for
+  // each pair of corresponding getters and setters.
+  for (AccessorTable::Iterator it = accessor_table.begin();
+       it != accessor_table.end();
+       ++it) {
+    __ lw(a0, MemOperand(sp));  // Duplicate receiver.
+    __ push(a0);
+    VisitForStackValue(it->first);
+    EmitAccessor(it->second->getter);
+    EmitAccessor(it->second->setter);
+    __ li(a0, Operand(Smi::FromInt(NONE)));
+    __ push(a0);
+    __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
+  }
+
   if (expr->has_function()) {
     ASSERT(result_saved);
     __ lw(a0, MemOperand(sp));

src/x64/full-codegen-x64.cc

@@ -1377,6 +1377,15 @@ void FullCodeGenerator::VisitRegExpLiteral(RegExpLiteral* expr) {
 }
 
 
+void FullCodeGenerator::EmitAccessor(Expression* expression) {
+  if (expression == NULL) {
+    __ PushRoot(Heap::kNullValueRootIndex);
+  } else {
+    VisitForStackValue(expression);
+  }
+}
+
+
 void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   Comment cmnt(masm_, "[ ObjectLiteral");
   Handle<FixedArray> constant_properties = expr->constant_properties();
@@ -1411,6 +1420,7 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   // marked expressions, no store code is emitted.
   expr->CalculateEmitStore();
 
+  AccessorTable accessor_table(isolate()->zone());
   for (int i = 0; i < expr->properties()->length(); i++) {
     ObjectLiteral::Property* property = expr->properties()->at(i);
     if (property->IsCompileTimeValue()) continue;
@@ -1455,23 +1465,28 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
           __ Drop(3);
         }
         break;
-      case ObjectLiteral::Property::SETTER:
       case ObjectLiteral::Property::GETTER:
-        __ push(Operand(rsp, 0));  // Duplicate receiver.
-        VisitForStackValue(key);
-        if (property->kind() == ObjectLiteral::Property::GETTER) {
-          VisitForStackValue(value);
-          __ PushRoot(Heap::kNullValueRootIndex);
-        } else {
-          __ PushRoot(Heap::kNullValueRootIndex);
-          VisitForStackValue(value);
-        }
-        __ Push(Smi::FromInt(NONE));
-        __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
+        accessor_table.find(key)->second->getter = value;
+        break;
+      case ObjectLiteral::Property::SETTER:
+        accessor_table.find(key)->second->setter = value;
         break;
     }
   }
 
+  // Emit code to define accessors, using only a single call to the runtime for
+  // each pair of corresponding getters and setters.
+  for (AccessorTable::Iterator it = accessor_table.begin();
+       it != accessor_table.end();
+       ++it) {
+    __ push(Operand(rsp, 0));  // Duplicate receiver.
+    VisitForStackValue(it->first);
+    EmitAccessor(it->second->getter);
+    EmitAccessor(it->second->setter);
+    __ Push(Smi::FromInt(NONE));
+    __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
+  }
+
   if (expr->has_function()) {
     ASSERT(result_saved);
     __ push(Operand(rsp, 0));

src/zone.h

@@ -240,7 +240,7 @@ class ZoneSplayTree: public SplayTree<Config, ZoneListAllocationPolicy> {
 };
 
 
-typedef TemplateHashMap<ZoneListAllocationPolicy> ZoneHashMap;
+typedef TemplateHashMapImpl<ZoneListAllocationPolicy> ZoneHashMap;
 
 } }  // namespace v8::internal