perf(view): increase view instantiation speed 40%

mhevery · dsalsbury · commit 8a1c158d4486 · 2014-08-16T10:36:06.000-07:00
1) Stop using futures when the value is already cached 2) Remove adding nodes to fake parent during view instantiation Closes dart-archive#1358
diff --git a/lib/core_dom/directive.dart b/lib/core_dom/directive.dart
@@ -22,7 +22,7 @@ class NodeAttrs {
 
   NodeAttrs(this.element);
 
-  operator [](String attrName) => element.attributes[attrName];
+  operator [](String attrName) => element.getAttribute(attrName);
 
   void operator []=(String attrName, String value) {
     if (_mustacheAttrs.containsKey(attrName)) {
@@ -31,7 +31,7 @@ class NodeAttrs {
     if (value == null) {
       element.attributes.remove(attrName);
     } else {
-      element.attributes[attrName] = value;
+      element.setAttribute(attrName, value);
     }
 
     if (_observers != null && _observers.containsKey(attrName)) {
@@ -86,9 +86,18 @@ class NodeAttrs {
  * ShadowRoot is ready.
  */
 class TemplateLoader {
-  final async.Future<dom.Node> template;
+  async.Future<dom.Node> _template;
+  List<async.Future> _futures;
+  final dom.Node _shadowRoot;
 
-  TemplateLoader(this.template);
+  TemplateLoader(this._shadowRoot, this._futures);
+
+  async.Future<dom.Node> get template {
+    if (_template == null) {
+      _template = async.Future.wait(_futures).then((_) => _shadowRoot);
+    }
+    return _template;
+  }
 }
 
 class _MustacheAttr {
diff --git a/lib/core_dom/shadow_dom_component_factory.dart b/lib/core_dom/shadow_dom_component_factory.dart
@@ -1,6 +1,7 @@
 part of angular.core.dom_internal;
 
 var _ComponentFactory_call = traceCreateScope('ComponentFactory#call()');
+var _ComponentFactory_styles = traceCreateScope('ComponentFactory#styles()');
 
 abstract class ComponentFactory {
   BoundComponentFactory bind(DirectiveRef ref, directives);
@@ -13,7 +14,7 @@ abstract class BoundComponentFactory {
   List<Key> get callArgs;
   Function call(dom.Element element);
 
-  static async.Future<ViewFactory> _viewFuture(
+  static async.Future<ViewFactory> _viewFactoryFuture(
         Component component, ViewCache viewCache, DirectiveMap directives) {
     if (component.template != null) {
       return new async.Future.value(viewCache.fromHtml(component.template, directives));
@@ -67,20 +68,27 @@ class BoundShadowDomComponentFactory implements BoundComponentFactory {
   Component get _component => _ref.annotation as Component;
 
   String _tag;
-  async.Future<Iterable<dom.StyleElement>> _styleElementsFuture;
-  async.Future<ViewFactory> _viewFuture;
+  async.Future<List<dom.StyleElement>> _styleElementsFuture;
+  List<dom.StyleElement> _styleElements;
+  async.Future<ViewFactory> _shadowViewFactoryFuture;
+  ViewFactory _shadowViewFactory;
 
   BoundShadowDomComponentFactory(this._componentFactory, this._ref, this._directives) {
     _tag = _component.selector.toLowerCase();
-    _styleElementsFuture = async.Future.wait(_component.cssUrls.map(_styleFuture));
+    _styleElementsFuture = async.Future.wait(_component.cssUrls.map(_urlToStyle))
+        ..then((stylesElements) => _styleElements = stylesElements);
 
-    _viewFuture = BoundComponentFactory._viewFuture(
+    _shadowViewFactoryFuture = BoundComponentFactory._viewFactoryFuture(
         _component,
+        // TODO(misko): Why do we create a new one per Component. This kind of defeats the caching.
         new PlatformViewCache(_componentFactory.viewCache, _tag, _componentFactory.platform),
         _directives);
+    if (_shadowViewFactoryFuture != null) {
+      _shadowViewFactoryFuture.then((viewFactory) => _shadowViewFactory = viewFactory);
+    }
   }
 
-  async.Future<dom.StyleElement> _styleFuture(cssUrl) {
+  async.Future<dom.StyleElement> _urlToStyle(cssUrl) {
     Http http = _componentFactory.http;
     TemplateCache templateCache = _componentFactory.templateCache;
     WebPlatform platform = _componentFactory.platform;
@@ -109,7 +117,7 @@ class BoundShadowDomComponentFactory implements BoundComponentFactory {
 
           // If the css shim is required, it means that scoping does not
           // work, and adding the style to the head of the document is
-          // preferrable.
+          // preferable.
           if (platform.cssShimRequired) {
             dom.document.head.append(styleElement);
             return null;
@@ -128,47 +136,57 @@ class BoundShadowDomComponentFactory implements BoundComponentFactory {
             EventHandler eventHandler) {
       var s = traceEnter(_ComponentFactory_call);
       try {
-        var shadowDom = element.createShadowRoot()
+        var shadowScope = scope.createChild(new HashMap()); // Isolate
+        ComponentDirectiveInjector shadowInjector;
+        dom.ShadowRoot shadowRoot = element.createShadowRoot();
+        shadowRoot
           ..applyAuthorStyles = _component.applyAuthorStyles
           ..resetStyleInheritance = _component.resetStyleInheritance;
 
-        var shadowScope = scope.createChild(new HashMap()); // Isolate
+        List<async.Future> futures = <async.Future>[];
+        TemplateLoader templateLoader = new TemplateLoader(shadowRoot, futures);
+        shadowInjector = new ShadowDomComponentDirectiveInjector(
+            injector, injector.appInjector, shadowScope, templateLoader, shadowRoot);
+        shadowInjector.bindByKey(_ref.typeKey, _ref.factory, _ref.paramKeys,
+            _ref.annotation.visibility);
+        dom.Node firstViewNode = null;
+
+        // Load ngBase CSS
+        if (_component.useNgBaseCss == true && baseCss.urls.isNotEmpty) {
+          if (baseCss.styles == null) {
+            futures.add(async.Future
+                .wait(baseCss.urls.map(_urlToStyle))
+                .then((List<dom.StyleElement> cssList) {
+                  baseCss.styles = cssList;
+                  _insertCss(cssList, shadowRoot, shadowRoot.firstChild);
+                }));
+          } else {
+            _insertCss(baseCss.styles, shadowRoot, shadowRoot.firstChild);
+          }
+        }
 
-        async.Future<Iterable<dom.StyleElement>> cssFuture;
-        if (_component.useNgBaseCss == true) {
-          cssFuture = async.Future.wait([async.Future.wait(baseCss.urls.map(_styleFuture)), _styleElementsFuture]).then((twoLists) {
-            assert(twoLists.length == 2);return []
-              ..addAll(twoLists[0])
-              ..addAll(twoLists[1]);
-          });
-        } else {
-          cssFuture = _styleElementsFuture;
+        if (_styleElementsFuture != null) {
+          if (_styleElements == null) {
+            futures.add(_styleElementsFuture .then((List<dom.StyleElement> styles) =>
+                _insertCss(styles, shadowRoot, firstViewNode)));
+          } else {
+            _insertCss(_styleElements, shadowRoot);
+          }
         }
 
-        ComponentDirectiveInjector shadowInjector;
 
-        TemplateLoader templateLoader = new TemplateLoader(cssFuture.then((Iterable<dom.StyleElement> cssList) {
-          cssList.where((styleElement) => styleElement != null).forEach((styleElement) {
-            shadowDom.append(styleElement.clone(true));
-          });
-          if (_viewFuture != null) {
-            return _viewFuture.then((ViewFactory viewFactory) {
-              if (shadowScope.isAttached) {
-                shadowDom.nodes.addAll(viewFactory.call(shadowInjector.scope, shadowInjector).nodes);
-              }
-              return shadowDom;
-            });
+        if (_shadowViewFactoryFuture != null) {
+          if (_shadowViewFactory == null) {
+            futures.add(_shadowViewFactoryFuture.then((ViewFactory viewFactory) =>
+                firstViewNode = _insertView(viewFactory, shadowRoot, shadowScope, shadowInjector)));
+          } else {
+            _insertView(_shadowViewFactory, shadowRoot, shadowScope, shadowInjector);
           }
-          return shadowDom;
-        }));
-
-        var probe;
-        shadowInjector = new ShadowDomComponentDirectiveInjector(injector, injector.appInjector, shadowScope, templateLoader, shadowDom);
-        shadowInjector.bindByKey(_ref.typeKey, _ref.factory, _ref.paramKeys, _ref.annotation.visibility);
+        }
 
         if (_componentFactory.config.elementProbeEnabled) {
-          probe = _componentFactory.expando[shadowDom] = shadowInjector.elementProbe;
-          shadowScope.on(ScopeEvent.DESTROY).listen((ScopeEvent) => _componentFactory.expando[shadowDom] = null);
+          ElementProbe probe = _componentFactory.expando[shadowRoot] = shadowInjector.elementProbe;
+          shadowScope.on(ScopeEvent.DESTROY).listen((ScopeEvent) => _componentFactory.expando[shadowRoot] = null);
         }
 
         var controller = shadowInjector.getByKey(_ref.typeKey);
@@ -181,6 +199,36 @@ class BoundShadowDomComponentFactory implements BoundComponentFactory {
       }
     };
   }
+
+  _insertCss(List<dom.StyleElement> cssList,
+             dom.ShadowRoot shadowRoot,
+             [dom.Node insertBefore = null]) {
+    var s = traceEnter(_ComponentFactory_styles);
+    for(int i = 0; i < cssList.length; i++) {
+      var styleElement = cssList[i];
+      if (styleElement != null) {
+        shadowRoot.insertBefore(styleElement.clone(true), insertBefore);
+      }
+    }
+    traceLeave(s);
+  }
+
+  dom.Node _insertView(ViewFactory viewFactory,
+              dom.ShadowRoot shadowRoot,
+              Scope shadowScope,
+              ShadowDomComponentDirectiveInjector shadowInjector) {
+    dom.Node first = null;
+    if (shadowScope.isAttached) {
+      View shadowView = viewFactory.call(shadowScope, shadowInjector);
+      List<dom.Node> shadowViewNodes = shadowView.nodes;
+      for (var j = 0; j < shadowViewNodes.length; j++) {
+        var node = shadowViewNodes[j];
+        if (j == 0) first = node;
+        shadowRoot.append(node);
+      }
+    }
+    return first;
+  }
 }
 
 class _ComponentAssetKey {
diff --git a/lib/core_dom/transcluding_component_factory.dart b/lib/core_dom/transcluding_component_factory.dart
@@ -87,13 +87,14 @@ class BoundTranscludingComponentFactory implements BoundComponentFactory {
   final DirectiveMap _directives;
 
   Component get _component => _ref.annotation as Component;
-  async.Future<ViewFactory> _viewFuture;
+  async.Future<ViewFactory> _viewFactoryFuture;
+  ViewFactory _viewFactory;
 
   BoundTranscludingComponentFactory(this._f, this._ref, this._directives) {
-    _viewFuture = BoundComponentFactory._viewFuture(
-        _component,
-        _f.viewCache,
-        _directives);
+    _viewFactoryFuture = BoundComponentFactory._viewFactoryFuture(_component, _f.viewCache, _directives);
+    if (_viewFactoryFuture != null) {
+      _viewFactoryFuture.then((viewFactory) => _viewFactory = viewFactory);
+    }
   }
 
   List<Key> get callArgs => _CALL_ARGS;
@@ -110,51 +111,39 @@ class BoundTranscludingComponentFactory implements BoundComponentFactory {
             ViewCache viewCache, Http http, TemplateCache templateCache,
             DirectiveMap directives, NgBaseCss baseCss, EventHandler eventHandler) {
 
-      DirectiveInjector childInjector;
-      var childInjectorCompleter; // Used if the ViewFuture is available before the childInjector.
-
-      var component = _component;
+      List<async.Future> futures = [];
       var contentPort = new ContentPort(element);
-
-      // Append the component's template as children
-      var elementFuture;
-
-      if (_viewFuture != null) {
-        elementFuture = _viewFuture.then((ViewFactory viewFactory) {
-          contentPort.pullNodes();
-          if (childInjector != null) {
-            element.nodes.addAll(
-                viewFactory.call(childInjector.scope, childInjector).nodes);
-            return element;
-          } else {
-            childInjectorCompleter = new async.Completer();
-            return childInjectorCompleter.future.then((childInjector) {
-              element.nodes.addAll(
-                  viewFactory.call(childInjector.scope, childInjector).nodes);
-              return element;
-            });
-          }
-        });
-      } else {
-        elementFuture = new async.Future.microtask(() => contentPort.pullNodes());
-      }
-      TemplateLoader templateLoader = new TemplateLoader(elementFuture);
-
+      TemplateLoader templateLoader = new TemplateLoader(element, futures);
       Scope shadowScope = scope.createChild(new HashMap());
-
-      childInjector = new ShadowlessComponentDirectiveInjector(injector, injector.appInjector,
-          eventHandler, shadowScope, templateLoader, new ShadowlessShadowRoot(element),
-          contentPort);
+      DirectiveInjector childInjector = new ShadowlessComponentDirectiveInjector(
+          injector, injector.appInjector, eventHandler, shadowScope, templateLoader,
+          new ShadowlessShadowRoot(element), contentPort);
       childInjector.bindByKey(_ref.typeKey, _ref.factory, _ref.paramKeys, _ref.annotation.visibility);
 
-      if (childInjectorCompleter != null) {
-        childInjectorCompleter.complete(childInjector);
-      }
-
       var controller = childInjector.getByKey(_ref.typeKey);
-      shadowScope.context[component.publishAs] = controller;
+      shadowScope.context[_component.publishAs] = controller;
       BoundComponentFactory._setupOnShadowDomAttach(controller, templateLoader, shadowScope);
+
+      if (_viewFactoryFuture != null && _viewFactory == null) {
+        futures.add(_viewFactoryFuture.then((ViewFactory viewFactory) =>
+            _insert(viewFactory, element, childInjector, contentPort)));
+      } else {
+        scope.rootScope.runAsync(() {
+          _insert(_viewFactory, element, childInjector, contentPort);
+        });
+      }
       return controller;
     };
   }
+
+  _insert(ViewFactory viewFactory, dom.Element element, DirectiveInjector childInjector,
+          ContentPort contentPort) {
+    contentPort.pullNodes();
+    if (viewFactory != null) {
+      var viewNodes = viewFactory.call(childInjector.scope, childInjector).nodes;
+      for(var i = 0; i < viewNodes.length; i++) {
+        element.append(viewNodes[i]);
+      }
+    }
+  }
 }
diff --git a/lib/core_dom/view_factory.dart b/lib/core_dom/view_factory.dart
@@ -94,10 +94,12 @@ class ViewFactory implements Function {
     }
     elementInjectors[elementBinderIndex] = elementInjector;
 
-    if (tagged.textBinders != null) {
-      for (var k = 0; k < tagged.textBinders.length; k++) {
-        TaggedTextBinder taggedText = tagged.textBinders[k];
-        var childNode = boundNode.childNodes[taggedText.offsetIndex];
+    var textBinders = tagged.textBinders;
+    if (textBinders != null && textBinders.length > 0) {
+      var childNodes = boundNode.childNodes;
+      for (var k = 0; k < textBinders.length; k++) {
+        TaggedTextBinder taggedText = textBinders[k];
+        var childNode = childNodes[taggedText.offsetIndex];
         taggedText.binder.bind(view, scope, elementInjector, childNode, eventHandler, animate);
       }
     }
@@ -113,15 +115,6 @@ class ViewFactory implements Function {
       dom.Node node = nodeList[i];
       NodeLinkingInfo linkingInfo = nodeLinkingInfos[i];
 
-      // if node isn't attached to the DOM, create a parent for it.
-      var parentNode = node.parentNode;
-      var fakeParent = false;
-      if (parentNode == null) {
-        fakeParent = true;
-        parentNode = new dom.DivElement();
-        parentNode.append(node);
-      }
-
       if (linkingInfo.isElement) {
         if (linkingInfo.containsNgBinding) {
           var tagged = elementBinders[elementBinderIndex];
@@ -149,11 +142,6 @@ class ViewFactory implements Function {
         }
         elementBinderIndex++;
       }
-
-      if (fakeParent) {
-        // extract the node from the parentNode.
-        nodeList[i] = parentNode.nodes[0];
-      }
     }
     return view;
   }
diff --git a/lib/core_dom/web_platform.dart b/lib/core_dom/web_platform.dart
@@ -42,7 +42,7 @@ class WebPlatform {
       //
       // TODO Remove the try-catch once https://github.com/angular/angular.dart/issues/1189 is fixed.
       try {
-        root.querySelectorAll("*").forEach((n) => n.attributes[selector] = "");
+        root.querySelectorAll("*").forEach((dom.Element n) => n.setAttribute(selector, ""));
       } catch (e, s) {
         print("WARNING: Failed to set up Shadow DOM shim for $selector.\n$e\n$s");
       }
@@ -69,6 +69,7 @@ class PlatformViewCache implements ViewCache {
     if (selector != null && selector != "" && platform.shadowDomShimRequired) {
       // By adding a comment with the tag name we ensure the template html is unique per selector
       // name when used as a key in the view factory cache.
+      //TODO(misko): This will always be miss, since we never put it in cache under such key.
       viewFactory = viewFactoryCache.get("<!-- Shimmed template for: <$selector> -->$html");
     } else {
       viewFactory = viewFactoryCache.get(html);
diff --git a/lib/directive/ng_base_css.dart b/lib/directive/ng_base_css.dart
@@ -12,10 +12,14 @@ part of angular.directive;
     selector: '[ng-base-css]',
     visibility: Visibility.CHILDREN)
 class NgBaseCss {
+  List<dom.StyleElement> styles;
   List<String> _urls = const [];
 
   @NgAttr('ng-base-css')
-  set urls(v) => _urls = v is List ? v : [v];
+  set urls(v) {
+    _urls = v is List ? v : [v];
+    styles = null;
+  }
 
   List<String> get urls => _urls;
 }
diff --git a/test/core_dom/compiler_spec.dart b/test/core_dom/compiler_spec.dart
diff --git a/test/directive/ng_base_css_spec.dart b/test/directive/ng_base_css_spec.dart
