Mark optimized code that used CHA for optimization so that lazy class 'finalization' does not inval…

runtime/vm/object.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/object.h"
 
 #include "include/dart_api.h"
 #include "platform/assert.h"
 #include "vm/assembler.h"
 #include "vm/cpu.h"
@@ skipping 2316 matching lines @@
   // Prefinalized classes have a VM internal representation and no Dart fields.
   // Their instance size  is precomputed and field offsets are known.
   if (!is_prefinalized()) {
     // Compute offsets of instance fields and instance size.
     CalculateFieldOffsets();
   }
   set_is_finalized();
 }
 
 
+// Helper class to handle an array of code weak properties. Implements
+// registration and disabling of stored code objects.
+class WeakCodeReferences : public ValueObject {

[Florian Schneider, 2014/02/05 10:36:31]

This class and it's two subclasses seem good candidates for using a template
parameter instead of inheritance+abstract methods to make things more tidy:
Instead of 3 you have only two classes (for T=Field and T=Class) and there is no
unnecessary vtables.

template<classname T>
class WeakCodeReferences : public ValueObject {

[srdjan, 2014/02/05 16:20:51]

Templates are hard to work with: harder on compiler errors, harder to read,
harder to debug. They also create more code (larger footprint). Therefore I tend
to avoid them if possible. The overhead of the virtual call seems negligible in
this case.
Please reconsider the suggestion.

[Florian Schneider, 2014/02/05 17:07:28]

I don't really care that much - so land it either way.

I prefer small tidy code over manully duplicated code. Many bugs arise from
manually copying code around.

Sure, the impact in this case is not noticable, but if there is a simple pattern
to avoid manual code duplication and the extra work of maintaining two copies of
the same code it, why not use it.

But ok, if you can avoid the code duplication otherwise (and without using a C
macro), I'm fine with that, too.

I just don't see the disadvantages to be relevant here: I don't buy the size
argument either, unless there is evidence that compilers will actually produce
bloated code here. I would not be surprised if the template version produces a
smaller binary.

You already have code duplication, or you can let the compiler duplicate the
code for you for the benefit of a cleaner, better maintainable source.

+ public:
+  explicit WeakCodeReferences(const Array& value) : array_(value) {}

[Florian Schneider, 2014/02/05 10:36:31]

explicit WeakCodeReferences(const T& owner)
    : owner_(owner) { }

+  virtual ~WeakCodeReferences() {}
+
+  void Register(const Code& value) {
+    if (!array_.IsNull()) {
+      // Try to find and reuse cleared WeakProperty to avoid allocating new one.
+      WeakProperty& weak_property = WeakProperty::Handle();
+      for (intptr_t i = 0; i < array_.Length(); i++) {
+        weak_property ^= array_.At(i);
+        if (weak_property.key() == Code::null()) {
+          // Empty property found. Reuse it.
+          weak_property.set_key(value);
+          return;
+        }
+      }
+    }
+
+    const WeakProperty& weak_property = WeakProperty::Handle(
+        WeakProperty::New(Heap::kOld));
+    weak_property.set_key(value);
+
+    intptr_t length = array_.IsNull() ? 0 : array_.Length();
+    const Array& new_array = Array::Handle(
+        Array::Grow(array_, length + 1, Heap::kOld));
+    new_array.SetAt(length, weak_property);
+    UpdateArrayTo(new_array);
+  }
+
+  virtual void UpdateArrayTo(const Array& array) = 0;

[Florian Schneider, 2014/02/05 10:36:31]

void UpdateArrayTo(const Array& array) {
  owner_.set_cha_codes(array);
}

The other virtual methods can be written in the same manner.

+  virtual void ReportDeoptimization(const Code& code) = 0;
+  virtual void ReportSwitchingCode(const Code& code) = 0;
+
+  static bool IsOptimizedCode(const Array& dependent_code, const Code& code) {

[Florian Schneider, 2014/02/05 17:07:28]

This checks for two things: the code is optimized and the code is in the array
of dependent code objects, so I'd rename it to:

s/IsOptimizedCode/IsDependentOptimizedCode/g

+    if (!code.is_optimized()) {
+      return false;
+    }
+    WeakProperty& weak_property = WeakProperty::Handle();
+    for (intptr_t i = 0; i < dependent_code.Length(); i++) {
+      weak_property ^= dependent_code.At(i);
+      if (code.raw() == weak_property.key()) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  void DisableCode() {
+    const Array& code_objects = Array::Handle(array_.raw());
+    if (code_objects.IsNull()) {
+      return;
+    }
+    UpdateArrayTo(Object::null_array());
+    // Disable all code on stack.
+    Code& code = Code::Handle();
+    {
+      DartFrameIterator iterator;
+      StackFrame* frame = iterator.NextFrame();
+      while (frame != NULL) {
+        code = frame->LookupDartCode();
+        if (IsOptimizedCode(code_objects, code)) {
+          ReportDeoptimization(code);
+          DeoptimizeAt(code, frame->pc());
+        }
+        frame = iterator.NextFrame();
+      }
+    }
+
+    // Switch functions that use dependent code to unoptimized code.
+    WeakProperty& weak_property = WeakProperty::Handle();
+    Function& function = Function::Handle();
+    for (intptr_t i = 0; i < code_objects.Length(); i++) {
+      weak_property ^= code_objects.At(i);
+      code ^= weak_property.key();
+      if (code.IsNull()) {
+        // Code was garbage collected already.
+        continue;
+      }
+
+      function ^= code.function();
+      // If function uses dependent code switch it to unoptimized.
+      if (function.CurrentCode() == code.raw()) {
+        ASSERT(function.HasOptimizedCode());
+        ReportSwitchingCode(code);
+        function.SwitchToUnoptimizedCode();
+      }
+    }
+  }
+
+ private:
+  const Array& array_;

[Florian Schneider, 2014/02/05 10:36:31]

Replace array_ with owner_.dependent_code() in all places and have

const T& owner_;

as the only member.

+  DISALLOW_COPY_AND_ASSIGN(WeakCodeReferences);
+};
+
+
+class CHACodeArray : public WeakCodeReferences {
+ public:
+  explicit CHACodeArray(const Class& cls)
+      : WeakCodeReferences(Array::Handle(cls.cha_codes())), cls_(cls) {
+  }
+
+  virtual void UpdateArrayTo(const Array& value) {
+    cls_.set_cha_codes(value);
+  }
+
+  virtual void ReportDeoptimization(const Code& code) {
+    if (FLAG_trace_deoptimization || FLAG_trace_deoptimization_verbose) {
+      Function& function = Function::Handle(code.function());
+      OS::PrintErr("Deoptimizing %s because CHA optimized (%s).\n",
+          function.ToFullyQualifiedCString(),
+          cls_.ToCString());
+    }
+  }
+
+  virtual void ReportSwitchingCode(const Code& code) {
+    if (FLAG_trace_deoptimization || FLAG_trace_deoptimization_verbose) {
+      Function& function = Function::Handle(code.function());
+      OS::PrintErr("Switching %s to unoptimized code because CHA invalid"
+                   " (%s)\n",
+                   function.ToFullyQualifiedCString(),
+                   cls_.ToCString());
+    }
+  }
+
+ private:
+  const Class& cls_;
+  DISALLOW_COPY_AND_ASSIGN(CHACodeArray);
+};
+
+
+void Class::RegisterCHACode(const Code& code) {
+  ASSERT(code.is_optimized());
+  CHACodeArray a(*this);
+  a.Register(code);
+}
+
+
+void Class::DisableCHAOptimizedCode() {
+  CHACodeArray a(*this);

[Florian Schneider, 2014/02/05 10:36:31]

WeakCodeReferences<Class> a(*this);

+  a.DisableCode();
+}
+
+
+void Class::set_cha_codes(const Array& cache) const {

[Florian Schneider, 2014/02/05 10:36:31]

s/set_cha_codes/set_dependent_code_/g

s/cha_codes/dependent_code/g

so that Field and Class have a compatible interface to this template class.

[srdjan, 2014/02/05 16:20:51]

Yes they would have 'compatible' interface but that design is fragile and
confusing since there is no 'dependency' relation.

[Florian Schneider, 2014/02/05 17:07:28]

Imo, fragile would imply that something bad could happen which is not the case:
you would just get a compile-time error about a unknown member.

+  StorePointer(&raw_ptr()->cha_codes_, cache.raw());
+}
+
+
 // Apply the members from the patch class to the original class.
 bool Class::ApplyPatch(const Class& patch, Error* error) const {
   ASSERT(error != NULL);
   ASSERT(!is_finalized());
   // Shared handles used during the iteration.
   String& member_name = String::Handle();
 
   const PatchClass& patch_class =
       PatchClass::Handle(PatchClass::New(*this, patch));
 
@@ skipping 4039 matching lines @@
   }
 }
 
 
 RawArray* Field::dependent_code() const {
   return raw_ptr()->dependent_code_;
 }
 
 
 void Field::set_dependent_code(const Array& array) const {
-  raw_ptr()->dependent_code_ = array.raw();
+  StorePointer(&raw_ptr()->dependent_code_, array.raw());
 }
 
 
-void Field::RegisterDependentCode(const Code& code) const {
-  const Array& dependent = Array::Handle(dependent_code());
+class FieldDependentArray : public WeakCodeReferences {
+ public:
+  explicit FieldDependentArray(const Field& field)
+      : WeakCodeReferences(Array::Handle(field.dependent_code())),
+                           field_(field) {}
 
-  if (!dependent.IsNull()) {
-    // Try to find and reuse cleared WeakProperty to avoid allocating new one.
-    WeakProperty& weak_property = WeakProperty::Handle();
-    for (intptr_t i = 0; i < dependent.Length(); i++) {
-      weak_property ^= dependent.At(i);
-      if (weak_property.key() == Code::null()) {
-        // Empty property found. Reuse it.
-        weak_property.set_key(code);
-        return;
-      }
+  virtual void UpdateArrayTo(const Array& value) {
+    field_.set_dependent_code(value);
+  }
+
+  virtual void ReportDeoptimization(const Code& code) {
+    if (FLAG_trace_deoptimization || FLAG_trace_deoptimization_verbose) {
+      Function& function = Function::Handle(code.function());
+          OS::PrintErr("Deoptimizing %s because guard on field %s failed.\n",
+          function.ToFullyQualifiedCString(),
+          field_.ToCString());
     }
   }
 
-  const WeakProperty& weak_property = WeakProperty::Handle(
-      WeakProperty::New(Heap::kOld));
-  weak_property.set_key(code);
-
-  intptr_t length = dependent.IsNull() ? 0 : dependent.Length();
-  const Array& new_dependent = Array::Handle(
-      Array::Grow(dependent, length + 1, Heap::kOld));
-  new_dependent.SetAt(length, weak_property);
-  set_dependent_code(new_dependent);
-}
-
-
-static bool IsDependentCode(const Array& dependent_code, const Code& code) {
-  if (!code.is_optimized()) {
-    return false;
-  }
-
-  WeakProperty& weak_property = WeakProperty::Handle();
-  for (intptr_t i = 0; i < dependent_code.Length(); i++) {
-    weak_property ^= dependent_code.At(i);
-    if (code.raw() == weak_property.key()) {
-      return true;
+  virtual void ReportSwitchingCode(const Code& code) {
+    if (FLAG_trace_deoptimization || FLAG_trace_deoptimization_verbose) {
+      Function& function = Function::Handle(code.function());
+      OS::PrintErr("Switching %s to unoptimized code because guard"
+                   " on field %s was violated.\n",
+                   function.ToFullyQualifiedCString(),
+                   field_.ToCString());
     }
   }
 
-  return false;
+ private:
+  const Field& field_;
+  DISALLOW_COPY_AND_ASSIGN(FieldDependentArray);
+};
+
+
+void Field::RegisterDependentCode(const Code& code) const {
+  ASSERT(code.is_optimized());
+  FieldDependentArray a(*this);
+  a.Register(code);
 }
 
 
 void Field::DeoptimizeDependentCode() const {
-  const Array& code_objects = Array::Handle(dependent_code());
-
-  if (code_objects.IsNull()) {
-    return;
-  }
-  set_dependent_code(Object::null_array());
-
-  // Deoptimize all dependent code on the stack.
-  Code& code = Code::Handle();
-  {
-    DartFrameIterator iterator;
-    StackFrame* frame = iterator.NextFrame();
-    while (frame != NULL) {
-      code = frame->LookupDartCode();
-      if (IsDependentCode(code_objects, code)) {
-        if (FLAG_trace_deoptimization || FLAG_trace_deoptimization_verbose) {
-          Function& function = Function::Handle(code.function());
-          OS::PrintErr("Deoptimizing %s because guard on field %s failed.\n",
-              function.ToFullyQualifiedCString(),
-              ToCString());
-        }
-        DeoptimizeAt(code, frame->pc());
-      }
-      frame = iterator.NextFrame();
-    }
-  }
-
-  // Switch functions that use dependent code to unoptimized code.
-  WeakProperty& weak_property = WeakProperty::Handle();
-  Function& function = Function::Handle();
-  for (intptr_t i = 0; i < code_objects.Length(); i++) {
-    weak_property ^= code_objects.At(i);
-    code ^= weak_property.key();
-    if (code.IsNull()) {
-      // Code was garbage collected already.
-      continue;
-    }
-
-    function ^= code.function();
-    // If function uses dependent code switch it to unoptimized.
-    if (function.CurrentCode() == code.raw()) {
-      ASSERT(function.HasOptimizedCode());
-      if (FLAG_trace_deoptimization || FLAG_trace_deoptimization_verbose) {
-        OS::PrintErr("Switching %s to unoptimized code because guard"
-                     " on field %s was violated.\n",
-                     function.ToFullyQualifiedCString(),
-                     ToCString());
-      }
-      function.SwitchToUnoptimizedCode();
-    }
-  }
+  FieldDependentArray a(*this);

[Florian Schneider, 2014/02/05 10:36:31]

WeakCodeReferences<Field> a(*this);

+  a.DisableCode();
 }
 
 
 bool Field::IsUninitialized() const {
   const Instance& value = Instance::Handle(raw_ptr()->value_);
   ASSERT(value.raw() != Object::transition_sentinel().raw());
   return value.raw() == Object::sentinel().raw();
 }
 
 
@@ skipping 10718 matching lines @@
   return "_MirrorReference";
 }
 
 
 void MirrorReference::PrintToJSONStream(JSONStream* stream, bool ref) const {
   Instance::PrintToJSONStream(stream, ref);
 }
 
 
 }  // namespace dart