Change the type inference for fields in dart2js

lib/compiler/implementation/js_backend/backend.dart

Index: lib/compiler/implementation/js_backend/backend.dart
diff --git a/lib/compiler/implementation/js_backend/backend.dart b/lib/compiler/implementation/js_backend/backend.dart
index 9006953008a06a8e154a994fd8e3534c36f98079..2994708845817a8f9e37c49fdc72b2b462d2e16b 100644
--- a/lib/compiler/implementation/js_backend/backend.dart
+++ b/lib/compiler/implementation/js_backend/backend.dart
@@ -223,6 +223,114 @@ class HTypeList {
       allUnknown ? "HTypeList.ALL_UNKNOWN" : "HTypeList $types";
 }
 
+class FieldTypesRegistry {
+  final JavaScriptBackend backend;
+  final Map<Element, HType> fieldInitializerTypeMap;
+  final Map<Element, HType> fieldConstructorTypeMap;
+  final Map<Element, HType> fieldTypeMap;

[ngeoffray, 2012/09/20 14:43:01]

Why do you need three maps and not one?

[Søren Gjesse, 2012/09/21 13:49:44]

I need to separate the types set by the initializers and the types set by the
constructors to completely override the initializer type if a type is set in the
constructor. This is to avoid the XXX_OR_NULL type if the initializer just
initialize to null.

+  final Map<Element, FunctionSet> optimizedFunctions;
+
+  FieldTypesRegistry(JavaScriptBackend backend)
+      : fieldInitializerTypeMap = new Map<Element, HType>(),
+        fieldConstructorTypeMap = new Map<Element, HType>(),
+        fieldTypeMap = new Map<Element, HType>(),
+        optimizedFunctions = new Map<Element, FunctionSet>(),
+        this.backend = backend;
+
+  Compiler get compiler => backend.compiler;
+
+  void scheduleRecompilation(Element field) {
+    FunctionSet optimized = optimizedFunctions[field];
+    if (optimized != null) {
+      optimized.forEach(backend.scheduleForRecompilation);
+    }
+    optimizedFunctions.remove(field);
+  }
+
+  void registerFieldType(Map<Element, HType> typeMap,
+                         Element field,
+                         HType type) {
+    assert(field.isField());
+    HType oldType = typeMap[field];
+    HType newType;
+
+    if (oldType != null) {
+      newType = oldType.union(type);
+    } else {
+      newType = type;
+    }
+    typeMap[field] = newType;
+    if (oldType != newType) {
+      scheduleRecompilation(field);
+    }
+  }
+
+  void registerFieldInitializer(Element field, HType type) {
+    registerFieldType(fieldInitializerTypeMap, field, type);
+  }
+
+  void registerFieldConstructor(Element field, HType type) {
+    registerFieldType(fieldConstructorTypeMap, field, type);
+  }
+
+  void registerFieldSetter(HFieldSet node, HTypeMap types) {
+    Element field = node.element;
+    HType initializerType = fieldInitializerTypeMap[field];
+    HType constructorType = fieldConstructorTypeMap[field];
+    HType setterType = fieldTypeMap[field];
+    HType type = types[node.value];
+    if (type == HType.UNKNOWN &&
+        initializerType == null &&
+        constructorType == null &&
+        setterType == null) {

[ngeoffray, 2012/09/20 14:43:01]

Not sure what all these checks really mean. If the type is unknown shouldn't you
invalidate the type in the maps?

[Søren Gjesse, 2012/09/21 13:49:44]

This checks whether there is any type information for the field. If not then we
don't register the type as having no information leads to type UNKNOWN. If type
information is revealed later i the compilation process we will start
registering it.

Added comment.

+      return;
+    }
+    registerFieldType(fieldTypeMap, field, type);
+  }
+
+  void addedDynamicSetter(Selector setter) {
+    optimizedFunctions.getKeys().forEach((Element field) {

[ngeoffray, 2012/09/20 14:43:01]

I would do optimizedFunctions.forEach((Element field, _) { ...});

[Søren Gjesse, 2012/09/21 13:49:44]

Done.

+      if (field.name == setter.name) {
+        if (compiler.codegenWorld.hasInvokedSetter(field, compiler)) {
+          scheduleRecompilation(field);
+        }
+      }
+    });
+  }
+
+  HType optimisticFieldType(Element field) {
+    assert(field.isField());
+    assert(field.isMember());
+    if (compiler.codegenWorld.hasInvokedSetter(field, compiler)) {
+      return HType.UNKNOWN;
+    }
+    HType initializerType = fieldInitializerTypeMap[field];
+    HType constructorType = fieldConstructorTypeMap[field];
+    if (initializerType === null && constructorType === null) {
+      return HType.UNKNOWN;
+    }
+    HType result = constructorType != null ? constructorType : initializerType;
+    HType type = fieldTypeMap[field];
+    if (type !== null) result = result.union(type);
+    return result;
+  }
+
+  void registerOptimization(FunctionElement element,

[ngeoffray, 2012/09/20 14:43:01]

registerOptimization -> registerOptimizedFunction

[Søren Gjesse, 2012/09/21 13:49:44]

Done.

+                            Element field,
+                            HType type) {
+    assert(field.isField());
+    optimizedFunctions.putIfAbsent(

[ngeoffray, 2012/09/20 14:43:01]

FunctionSet set = optimizedFunctions...
set.add(element)

[Søren Gjesse, 2012/09/21 13:49:44]

This pattern is used in many places. Is it the performance penalty by making two
lookups in the Map you don't like?

+        field, () => new FunctionSet(backend.compiler));
+    optimizedFunctions[field].add(element);
+  }
+
+  void dump() {
+    optimizedFunctions.getKeys().forEach((Element field) {
+      print("Inferred $field has type ${optimisticFieldType(field)}");
+    });
+  }
+}
+
 class ArgumentTypesRegistry {
   final JavaScriptBackend backend;
   final Map<Element, HTypeList> staticTypeMap;
@@ -421,24 +529,18 @@ class JavaScriptBackend extends Backend {
    */
   ClassElement jsIndexingBehaviorInterface;
 
-  final Map<Element, Map<Element, HType>> fieldInitializers;
-  final Map<Element, Map<Element, HType>> fieldConstructorSetters;
-  final Map<Element, Map<Element, HType>> fieldSettersType;
-
   final Map<Element, ReturnInfo> returnInfo;
 
   final List<Element> invalidateAfterCodegen;
   ArgumentTypesRegistry argumentTypes;
+  FieldTypesRegistry fieldTypes;
 
   List<CompilerTask> get tasks {
     return <CompilerTask>[builder, optimizer, generator, emitter];
   }
 
   JavaScriptBackend(Compiler compiler, bool generateSourceMap)
-      : fieldInitializers = new Map<Element, Map<Element, HType>>(),
-        fieldConstructorSetters = new Map<Element, Map<Element, HType>>(),
-        fieldSettersType = new Map<Element, Map<Element, HType>>(),
-        namer = new Namer(compiler),
+      : namer = new Namer(compiler),
         returnInfo = new Map<Element, ReturnInfo>(),
         invalidateAfterCodegen = new List<Element>(),
         super(compiler, constantSystem: JAVA_SCRIPT_CONSTANT_SYSTEM) {
@@ -447,6 +549,7 @@ class JavaScriptBackend extends Backend {
     optimizer = new SsaOptimizerTask(this);
     generator = new SsaCodeGeneratorTask(this);
     argumentTypes = new ArgumentTypesRegistry(this);
+    fieldTypes = new FieldTypesRegistry(this);
   }
 
   Element get cyclicThrowHelper {
@@ -512,106 +615,6 @@ class JavaScriptBackend extends Backend {
     emitter.assembleProgram();
   }
 
-  void updateFieldInitializers(Element field, HType propagatedType) {
-    assert(field.isField());
-    assert(field.isMember());
-    Map<Element, HType> fields =
-        fieldInitializers.putIfAbsent(
-          field.getEnclosingClass(), () => new Map<Element, HType>());
-    if (!fields.containsKey(field)) {
-      fields[field] = propagatedType;
-    } else {
-      fields[field] = fields[field].union(propagatedType);
-    }
-  }
-
-  HType typeFromInitializersSoFar(Element field) {
-    assert(field.isField());
-    assert(field.isMember());
-    if (!fieldInitializers.containsKey(field.getEnclosingClass())) {
-      return HType.CONFLICTING;
-    }
-    Map<Element, HType> fields = fieldInitializers[field.getEnclosingClass()];
-    return fields[field];
-  }
-
-  void updateFieldConstructorSetters(Element field, HType type) {
-    assert(field.isField());
-    assert(field.isMember());
-    Map<Element, HType> fields =
-        fieldConstructorSetters.putIfAbsent(
-            field.getEnclosingClass(), () => new Map<Element, HType>());
-    if (!fields.containsKey(field)) {
-      fields[field] = type;
-    } else {
-      fields[field] = fields[field].union(type);
-    }
-  }
-
-  // Check if this field is set in the constructor body.
-  bool hasConstructorBodyFieldSetter(Element field) {
-    ClassElement enclosingClass = field.getEnclosingClass();
-    if (!fieldConstructorSetters.containsKey(enclosingClass)) {
-      return false;
-    }
-    return fieldConstructorSetters[enclosingClass][field] != null;
-  }
-
-  // Provide an optimistic estimate of the type of a field after construction.
-  // If the constructor body has setters for fields returns HType.UNKNOWN.
-  // This only takes the initializer lists and field assignments in the
-  // constructor body into account. The constructor body might have method calls
-  // that could alter the field.
-  HType optimisticFieldTypeAfterConstruction(Element field) {
-    assert(field.isField());
-    assert(field.isMember());
-
-    ClassElement classElement = field.getEnclosingClass();
-    if (hasConstructorBodyFieldSetter(field)) {
-      // If there are field setters but there is only constructor then the type
-      // of the field is determined by the assignments in the constructor
-      // body.
-      var constructors = classElement.constructors;
-      if (constructors.head !== null && constructors.tail.isEmpty()) {
-        return fieldConstructorSetters[classElement][field];
-      } else {
-        return HType.UNKNOWN;
-      }
-    } else if (fieldInitializers.containsKey(classElement)) {
-      HType type = fieldInitializers[classElement][field];
-      return type == null ? HType.CONFLICTING : type;
-    } else {
-      return HType.CONFLICTING;
-    }
-  }
-
-  void updateFieldSetters(Element field, HType type) {
-    assert(field.isField());
-    assert(field.isMember());
-    Map<Element, HType> fields =
-        fieldSettersType.putIfAbsent(
-          field.getEnclosingClass(), () => new Map<Element, HType>());
-    if (!fields.containsKey(field)) {
-      fields[field] = type;
-    } else {
-      fields[field] = fields[field].union(type);
-    }
-  }
-
-  // Returns the type that field setters are setting the field to based on what
-  // have been seen during compilation so far.
-  HType fieldSettersTypeSoFar(Element field) {
-    assert(field.isField());
-    assert(field.isMember());
-    ClassElement enclosingClass = field.getEnclosingClass();
-    if (!fieldSettersType.containsKey(enclosingClass)) {
-      return HType.CONFLICTING;
-    }
-    Map<Element, HType> fields = fieldSettersType[enclosingClass];
-    if (!fields.containsKey(field)) return HType.CONFLICTING;
-    return fields[field];
-  }
-
   void scheduleForRecompilation(Element element) {
     if (compiler.phase == Compiler.PHASE_COMPILING) {
       invalidateAfterCodegen.add(element);
@@ -672,6 +675,12 @@ class JavaScriptBackend extends Backend {
         element, parameterTypes, defaultValueTypes);
   }
 
+  registerFieldTypesOptimization(FunctionElement element,
+                                 Element field,
+                                 HType type) {
+    fieldTypes.registerOptimization(element, field, type);
+  }
+
   void registerReturnType(FunctionElement element, HType returnType) {
     ReturnInfo info = returnInfo[element];
     if (info != null) {
@@ -706,6 +715,26 @@ class JavaScriptBackend extends Backend {
     });
   }
 
+  void registerFieldInitializer(Element field, HType type) {
+    fieldTypes.registerFieldInitializer(field, type);
+  }
+
+  void registerFieldConstructor(Element field, HType type) {
+    fieldTypes.registerFieldConstructor(field, type);
+  }
+
+  void registerFieldSetter(HFieldSet node, HTypeMap types) {
+    fieldTypes.registerFieldSetter(node, types);
+  }
+
+  void addedDynamicSetter(Selector setter) {
+    fieldTypes.addedDynamicSetter(setter);
+  }
+
+  HType optimisticFieldType(Element element) {
+    return fieldTypes.optimisticFieldType(element);
+  }
+
   SourceString getCheckedModeHelper(DartType type) {
     Element element = type.element;
     bool nativeCheck =