Fix lazy initialization of static fields (issue 2472).

runtime/vm/parser.cc

Index: runtime/vm/parser.cc
===================================================================
--- runtime/vm/parser.cc	(revision 6422)
+++ runtime/vm/parser.cc	(working copy)
@@ -688,6 +688,11 @@
 }
 
 
+// TODO(regis): Implement support for non-const final static fields (currently
+// supported "final" fields are actually const fields).
+// TODO(regis): Since a const variable is implicitly final,
+// rename ParseStaticConstGetter to ParseStaticFinalGetter and
+// rename kConstImplicitGetter to kImplicitFinalGetter.
 SequenceNode* Parser::ParseStaticConstGetter(const Function& func) {
   TRACE_PARSER("ParseStaticConstGetter");
   ParamList params;
@@ -699,8 +704,12 @@
   OpenFunctionBlock(func);
   AddFormalParamsToScope(&params, current_block_->scope);
 
+  const String& field_name = *ExpectIdentifier("field name expected");
+  const Class& field_class = Class::Handle(func.owner());
+  const Field& field =
+      Field::ZoneHandle(field_class.LookupStaticField(field_name));
+
   // Static const fields must have an initializer.
-  ExpectIdentifier("field name expected");
   ExpectToken(Token::kASSIGN);
 
   // We don't want to use ParseConstExpr() here because we don't want
@@ -710,11 +719,82 @@
   // leave the evaluation to the getter function.
   const intptr_t expr_pos = token_index_;
   AstNode* expr = ParseExpr(kAllowConst);
-  if (expr->EvalConstExpr() == NULL) {
-    ErrorMsg(expr_pos, "initializer must be a compile time constant");
+  // TODO(regis): Implement support for const fields.
+  if (field.is_final()) {  // TODO(regis): Should be field.is_const().

[srdjan, 2012/04/11 19:52:51]

You could already add is_const() tester to field, which would return true for
final fields.

[regis, 2012/04/11 21:11:24]

Done.

+    // This getter will only be called once at compile time.
+    if (expr->EvalConstExpr() == NULL) {
+      ErrorMsg(expr_pos, "initializer must be a compile time constant");
+    }
+    ReturnNode* return_node = new ReturnNode(token_index_, expr);
+    current_block_->statements->Add(return_node);
+  } else {
+    // This getter may be called each time the static field is accessed.
+    // The following generated code lazily initializes the field:
+    // if (field.value == transition_sentinel) {

[srdjan, 2012/04/11 19:52:51]

=== (strict equal used)

[regis, 2012/04/11 21:11:24]

Done.

+    //   field.value = null;

[srdjan, 2012/04/11 19:52:51]

Why set it to null? If we catch the exception, next acces will be then valid?

[regis, 2012/04/11 21:11:24]

Yes, I spoke earlier with Gilad about it. We should only get one exception.
Added comment:
    // Set field to null prior to throwing exception, so that subsequent
    // accesses to the field do not throw again, since initializers should only
    // be executed once.

+    //   throw("circular dependency in field initialization");
+    // }
+    // if (field.value == sentinel) {

[srdjan, 2012/04/11 19:52:51]

===

[regis, 2012/04/11 21:11:24]

Done.

+    //   field.value = transition_sentinel;
+    //   field.value = expr;
+    // }
+    // return field.value;  // Type check is executed here in checked mode.
+
+    // TODO(regis): Remove this check once we support proper const fields.
+    if (expr->EvalConstExpr() == NULL) {
+      ErrorMsg(expr_pos, "initializer must be a compile time constant");
+    }
+
+    // Generate code checking for circular dependency in field initialization.
+    AstNode* compare_circular = new ComparisonNode(
+        token_index_,
+        Token::kEQ_STRICT,
+        new LoadStaticFieldNode(token_index_, field),
+        new LiteralNode(token_index_,
+                        Instance::ZoneHandle(Object::transition_sentinel())));
+    SequenceNode* report_circular = new SequenceNode(token_index_, NULL);
+    report_circular->Add(
+        new StoreStaticFieldNode(
+            token_index_,
+            field,
+            new LiteralNode(token_index_, Instance::ZoneHandle())));
+    // TODO(regis): Exception to throw is not specified by spec.
+    const String& circular_error = String::ZoneHandle(
+        String::NewSymbol("circular dependency in field initialization"));
+    report_circular->Add(
+        new ThrowNode(token_index_,
+                      new LiteralNode(token_index_, circular_error),
+                      NULL));
+    AstNode* circular_check =
+        new IfNode(token_index_, compare_circular, report_circular, NULL);
+    current_block_->statements->Add(circular_check);
+
+    // Generate code checking for uninitialized field.
+    AstNode* compare_uninitialized = new ComparisonNode(
+        token_index_,
+        Token::kEQ_STRICT,
+        new LoadStaticFieldNode(token_index_, field),
+        new LiteralNode(token_index_,
+                        Instance::ZoneHandle(Object::sentinel())));
+    SequenceNode* initialize_field = new SequenceNode(token_index_, NULL);
+    initialize_field->Add(
+        new StoreStaticFieldNode(
+            token_index_,
+            field,
+            new LiteralNode(
+                token_index_,
+                Instance::ZoneHandle(Object::transition_sentinel()))));
+    initialize_field->Add(new StoreStaticFieldNode(token_index_, field, expr));
+    AstNode* uninitialized_check =
+        new IfNode(token_index_, compare_uninitialized, initialize_field, NULL);
+    current_block_->statements->Add(uninitialized_check);
+
+    // Generate code returning the field value.
+    ReturnNode* return_node =
+        new ReturnNode(token_index_,
+                       new LoadStaticFieldNode(token_index_, field));
+    current_block_->statements->Add(return_node);
   }
-  ReturnNode* return_node = new ReturnNode(token_index_, expr);
-  current_block_->statements->Add(return_node);
   return CloseBlock();
 }
 
@@ -6175,19 +6255,17 @@
 
 AstNode* Parser::GenerateStaticFieldLookup(const Field& field,
                                            intptr_t ident_pos) {
-  // Run static field initializer first if necessary.
-  // May return an exception throwing ast node.
-  AstNode* throw_exception = RunStaticFieldInitializer(field);
-  if (throw_exception != NULL) {
-    return throw_exception;
+  AstNode* initializer = RunStaticFieldInitializer(field);

[srdjan, 2012/04/11 19:52:51]

initializer -> call_getter ?

[regis, 2012/04/11 21:11:24]

No, the returned ast is the initializer. If the field is already initialized,
null is returned.

+  if (initializer != NULL) {
+    return initializer;
   }
-  // Access the field.
-  if (field.is_final()) {
+  if (field.is_final()) {  // TODO(regis): Should be field.is_const().

[srdjan, 2012/04/11 19:52:51]

Add is_const() to Field

[regis, 2012/04/11 21:11:24]

Done.

+    ASSERT(field.value() != Object::sentinel());
+    ASSERT(field.value() != Object::transition_sentinel());
     return new LiteralNode(ident_pos, Instance::ZoneHandle(field.value()));
-  } else {
-    return new LoadStaticFieldNode(ident_pos,
-                                   Field::ZoneHandle(field.raw()));
   }
+  // No initializer. Access the field directly.
+  return new LoadStaticFieldNode(ident_pos, Field::ZoneHandle(field.raw()));
 }
 
 
@@ -6648,64 +6726,68 @@
 }
 
 
-// Returns null on success.
-// Returns a throw node if evaluation of the static initializer results in an
-// unhandled exception.
 AstNode* Parser::RunStaticFieldInitializer(const Field& field) {

[srdjan, 2012/04/11 19:52:51]

Add comment what you are returning.

[regis, 2012/04/11 21:11:24]

Done.

+  // TODO(regis): Implement support for non-const final static fields.
   ASSERT(field.is_static());
   const Instance& value = Instance::Handle(field.value());
   if (value.raw() == Object::transition_sentinel()) {
-    ErrorMsg("circular dependency while initializing static field '%s'",
-             String::Handle(field.name()).ToCString());
-
+    if (field.is_final()) {  // TODO(regis): Should be field.is_const().
+      ErrorMsg("circular dependency while initializing static field '%s'",
+               String::Handle(field.name()).ToCString());
+    } else {

[srdjan, 2012/04/11 19:52:51]

Add comment like: call static getter so that it can throw the exception.

[regis, 2012/04/11 21:11:24]

Done.

+      return new StaticGetterNode(token_index_,
+                                  Class::ZoneHandle(field.owner()),
+                                  String::ZoneHandle(field.name()));
+    }
   } else if (value.raw() == Object::sentinel()) {
     // This field has not been referenced yet and thus the value has
-    // not been evaluated. Call the static getter method to evaluate
-    // the expression and canonicalize the value.
-
-    field.set_value(Instance::Handle(Object::transition_sentinel()));
-    const String& field_name = String::Handle(field.name());
-    const String& getter_name =
-        String::Handle(Field::GetterName(field_name));
-    const Class& cls = Class::Handle(field.owner());
-    GrowableArray<const Object*> arguments;  // no arguments.
-    const int kNumArguments = 0;  // no arguments.
-    const Array& kNoArgumentNames = Array::Handle();
-    const Function& func =
-        Function::Handle(Resolver::ResolveStatic(cls,
-                                                 getter_name,
-                                                 kNumArguments,
-                                                 kNoArgumentNames,
-                                                 Resolver::kIsQualified));
-    ASSERT(!func.IsNull());
-    ASSERT(func.kind() == RawFunction::kConstImplicitGetter);
-    Object& const_value = Object::Handle(
-        DartEntry::InvokeStatic(func, arguments, kNoArgumentNames));
-    if (const_value.IsError()) {
-      Error& error = Error::Handle();
-      error ^= const_value.raw();
-      if (const_value.IsUnhandledException()) {
-        field.set_value(Instance::Handle());
-        // It is a compile-time error if evaluation of a compile-time constant
-        // would raise an exception.
-        if (field.is_final()) {
+    // not been evaluated. If the field is const, call the static getter method
+    // to evaluate the expression and canonicalize the value.
+    if (field.is_final()) {  // TODO(regis): Should be field.is_const().
+      field.set_value(Instance::Handle(Object::transition_sentinel()));
+      const String& field_name = String::Handle(field.name());
+      const String& getter_name =
+          String::Handle(Field::GetterName(field_name));
+      const Class& cls = Class::Handle(field.owner());
+      GrowableArray<const Object*> arguments;  // no arguments.
+      const int kNumArguments = 0;  // no arguments.
+      const Array& kNoArgumentNames = Array::Handle();
+      const Function& func =
+          Function::Handle(Resolver::ResolveStatic(cls,
+                                                   getter_name,
+                                                   kNumArguments,
+                                                   kNoArgumentNames,
+                                                   Resolver::kIsQualified));
+      ASSERT(!func.IsNull());
+      ASSERT(func.kind() == RawFunction::kConstImplicitGetter);
+      Object& const_value = Object::Handle(
+          DartEntry::InvokeStatic(func, arguments, kNoArgumentNames));
+      if (const_value.IsError()) {
+        Error& error = Error::Handle();
+        error ^= const_value.raw();
+        if (const_value.IsUnhandledException()) {
+          field.set_value(Instance::Handle());
+          // It is a compile-time error if evaluation of a compile-time constant
+          // would raise an exception.
           AppendErrorMsg(error, token_index_,
                          "error initializing final field '%s'",
                          String::Handle(field.name()).ToCString());
         } else {
-          return GenerateRethrow(token_index_, const_value);
+          Isolate::Current()->long_jump_base()->Jump(1, error);
         }
-      } else {
-        Isolate::Current()->long_jump_base()->Jump(1, error);
       }
+      ASSERT(const_value.IsNull() || const_value.IsInstance());
+      Instance& instance = Instance::Handle();
+      instance ^= const_value.raw();
+      if (!instance.IsNull()) {
+        instance ^= instance.Canonicalize();
+      }
+      field.set_value(instance);
+    } else {

[srdjan, 2012/04/11 19:52:51]

If I understand correctly, we will now always call the getter for non-final
static fields, previously we loaded directly from the field. Please run
benchmarks before/after this change to see if there are any performance
degradations.

As I understand it, this also introduces lazy initialization of static fields.

[regis, 2012/04/11 21:11:24]

As we discussed, it would be comparing apples and oranges, since the previous
behavior was not correct. Let's watch Golem and see if this causes a slowdown.
In that case, it will be easy for the optimizing compiler to replace calls to
the getter with direct field accesses if the field is initialized. Note that we
still access the field directly if it has been initialized by the time we
compile the field access.

+      return new StaticGetterNode(token_index_,
+                                  Class::ZoneHandle(field.owner()),
+                                  String::ZoneHandle(field.name()));
     }
-    ASSERT(const_value.IsNull() || const_value.IsInstance());
-    Instance& instance = Instance::Handle();
-    instance ^= const_value.raw();
-    if (!instance.IsNull()) {
-      instance ^= instance.Canonicalize();
-    }
-    field.set_value(instance);
   }
   return NULL;
 }