Go into slow case when encountering object initialization on the top level to...

src/parser.cc

Index: src/parser.cc
===================================================================
--- src/parser.cc	(revision 1358)
+++ src/parser.cc	(working copy)
@@ -69,6 +69,10 @@
                              Handle<String> name,
                              int start_position, bool is_expression);
 
+  // The minimum number of contiguous assignment that is needed to
+  // be treated as an initialization block.
+  static const int kMinInitializationBlock = 3;

[Mads Ager (chromium), 2009/02/25 13:11:07]

Is this limit based on benchmarking?  We should make sure to only perform the
two runtime calls if that overhead is less than the instance descriptor copying
overhead.

[olehougaard, 2009/02/26 07:31:44]

It's based on benchmarking. I've added a comment to that effect.

+
  protected:
 
   enum Mode {
@@ -1215,6 +1219,110 @@
 }
 
 
+// An InitializationBlockFinder finds and marks sequences of statements of the
+// form x.y.z.a = ...; x.y.z.b = ...; etc.
+class InitializationBlockFinder {
+ public:
+  InitializationBlockFinder()
+    : first_in_block_(NULL), last_in_block_(NULL), block_size_(0) {}
+
+  ~InitializationBlockFinder() {
+    if (InBlock()) EndBlock();
+  }
+
+  void Update(Statement* stat) {
+    Assignment* assignment = AsAssignment(stat);
+    if (InBlock()) {
+      if (BlockContinues(assignment)) {
+        UpdateBlock(assignment);
+      } else {
+        EndBlock();
+      }
+    }
+    if (!InBlock() && (assignment != NULL)) StartBlock(assignment);

[Mads Ager (chromium), 2009/02/25 13:11:07]

In BlockContinues, you check if the operation is Token::ASSIGN.  You should do
that here (or in StartBlock) as well?

Please add a test case that hits this.

[olehougaard, 2009/02/26 07:31:44]

Fixed.

+  }
+
+ private:
+  static Assignment* AsAssignment(Statement* stat) {
+    if (stat == NULL) return NULL;
+    ExpressionStatement* exp_stat = stat->AsExpressionStatement();
+    if (exp_stat == NULL) return NULL;
+    return exp_stat->expression()->AsAssignment();
+  }
+
+  // Returns true if the expressions appear to denote the same object.
+  // In the context of initialization blocks, we only consider expressions
+  // of the form 'x.y.z'.
+  static bool SameObject(Expression* e1, Expression* e2);
+
+  // Returns true if the expressions appear to denote different properties
+  // of the same object.
+  static bool PropertyOfSameObject(Expression* e1, Expression* e2) {
+    Property* p1 = e1->AsProperty();
+    Property* p2 = e2->AsProperty();
+    if (p1 == NULL || p2 == NULL) return false;
+    return SameObject(p1->obj(), p2->obj());
+  }
+
+  bool BlockContinues(Assignment* assignment) {
+    if (assignment == NULL || first_in_block_ == NULL) return false;
+    if (assignment->op() != Token::ASSIGN) return false;

[iposva, 2009/02/25 12:54:45]

You check here that only ASSIGN statements are added when the block continues,
but you do not check for the initial node to be an ASSIGN statement anywhere I
can tell.

[olehougaard, 2009/02/26 07:31:44]

Fixed.

+    return PropertyOfSameObject(first_in_block_->target(),
+                                assignment->target());
+  }
+
+  void StartBlock(Assignment* assignment) {
+    first_in_block_ = assignment;
+    last_in_block_ = assignment;
+    block_size_ = 1;
+  }
+
+  void UpdateBlock(Assignment* assignment) {
+    last_in_block_ = assignment;
+    ++block_size_;
+  }
+
+  void EndBlock() {
+    if (block_size_ >= Parser::kMinInitializationBlock) {
+      first_in_block_->mark_block_start();
+      last_in_block_->mark_block_end();
+    }
+    last_in_block_ = first_in_block_ = NULL;
+    block_size_ = 0;
+  }
+
+  bool InBlock() { return first_in_block_ != NULL; }
+
+  Assignment* first_in_block_;
+  Assignment* last_in_block_;
+  int block_size_;
+
+  DISALLOW_COPY_AND_ASSIGN(InitializationBlockFinder);
+};
+
+
+bool InitializationBlockFinder::SameObject(Expression* e1, Expression* e2) {

[iposva, 2009/02/25 12:54:45]

What made you put this method outside the class declaration? Since this is a
private class I would just put all of the code in the declaration, or at least
pull all non-trivial (aka 1-liners) out to be consistent.

[olehougaard, 2009/02/26 07:31:44]

I've put all the code in the declaration for consistency.

+  VariableProxy* v1 = e1->AsVariableProxy();
+  VariableProxy* v2 = e2->AsVariableProxy();
+  if (v1 != NULL && v2 != NULL) {

[iposva, 2009/02/25 12:54:45]

In general I find code more readable when () are added around boolean
expressions such as

if ((v1 != NULL) && (v2 != NULL))

[olehougaard, 2009/02/26 07:31:44]

Fixed where found.

+    return v1->name()->Equals(*v2->name());
+  }
+  Property* p1 = e1->AsProperty();
+  Property* p2 = e2->AsProperty();
+  if (p1 == NULL || p2 == NULL) return false;
+  Literal* key1 = p1->key()->AsLiteral();
+  Literal* key2 = p2->key()->AsLiteral();
+  if (key1 == NULL || key2 == NULL) return false;
+  if (!key1->handle()->IsString() || !key2->handle()->IsString()) {
+    return false;
+  }
+  String* name1 = String::cast(*key1->handle());
+  String* name2 = String::cast(*key2->handle());
+  if (!name1->Equals(name2)) return false;
+  return SameObject(p1->obj(), p2->obj());
+}
+
+
 void* Parser::ParseSourceElements(ZoneListWrapper<Statement>* processor,
                                   int end_token,
                                   bool* ok) {
@@ -1228,9 +1336,15 @@
   TargetScope scope(this);
 
   ASSERT(processor != NULL);
+  InitializationBlockFinder block_finder;
   while (peek() != end_token) {
     Statement* stat = ParseStatement(NULL, CHECK_OK);
-    if (stat && !stat->IsEmpty()) processor->Add(stat);
+    if (stat == NULL || stat->IsEmpty()) continue;
+    // We find and mark the initialization blocks on top level code only.
+    // The optimizations of initialization blocks may result in a performance
+    // degradation in any other scopes.

[Mads Ager (chromium), 2009/02/25 13:11:07]

You should write here that the reason for only doing this in top level code is
that map transitions are not used in this case leading to different maps for all
objects going through this series of assignments.

[olehougaard, 2009/02/26 07:31:44]

Fixed.

+    if (top_scope_->is_global_scope()) block_finder.Update(stat);
+    processor->Add(stat);
   }
   return 0;
 }