From 5424b4d6e25e1d547d57af97bca6822c99ed8bf6 Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Thu, 14 Aug 2014 19:52:27 -0700 Subject: [PATCH] perf(view): increase view instantiation speed 40% 1) Stop using futures when the value is already cached 2) Remove adding nodes to fake parent during view instantiation Closes #1358 --- lib/core_dom/directive.dart | 17 ++- .../shadow_dom_component_factory.dart | 128 ++++++++++++------ .../transcluding_component_factory.dart | 74 +++++----- lib/core_dom/view_factory.dart | 24 +--- lib/core_dom/web_platform.dart | 3 +- lib/directive/ng_base_css.dart | 6 +- lib/ng_tracing_scopes.dart | 7 + test/core_dom/compiler_spec.dart | 4 +- test/directive/ng_base_css_spec.dart | 25 +++- 9 files changed, 173 insertions(+), 115 deletions(-) diff --git a/lib/core_dom/directive.dart b/lib/core_dom/directive.dart index 8738fe273..aa32724ce 100644 --- 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 template; + async.Future _template; + List _futures; + final dom.Node _shadowRoot; - TemplateLoader(this.template); + TemplateLoader(this._shadowRoot, this._futures); + + async.Future 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 index 221701575..b48a15fde 100644 --- a/lib/core_dom/shadow_dom_component_factory.dart +++ b/lib/core_dom/shadow_dom_component_factory.dart @@ -11,7 +11,7 @@ abstract class BoundComponentFactory { List get callArgs; Function call(dom.Element element); - static async.Future _viewFuture( + static async.Future _viewFactoryFuture( Component component, ViewCache viewCache, DirectiveMap directives) { if (component.template != null) { return new async.Future.value(viewCache.fromHtml(component.template, directives)); @@ -67,20 +67,27 @@ class BoundShadowDomComponentFactory implements BoundComponentFactory { Component get _component => _ref.annotation as Component; String _tag; - async.Future> _styleElementsFuture; - async.Future _viewFuture; + async.Future> _styleElementsFuture; + List _styleElements; + async.Future _shadowViewFactoryFuture; + ViewFactory _shadowViewFactory; BoundShadowDomComponentFactory(this._componentFactory, this._ref, this._directives, this._injector) { _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 _styleFuture(cssUrl) { + async.Future _urlToStyle(cssUrl) { Http http = _componentFactory.http; TemplateCache templateCache = _componentFactory.templateCache; WebPlatform platform = _componentFactory.platform; @@ -109,7 +116,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,50 +135,59 @@ class BoundShadowDomComponentFactory implements BoundComponentFactory { EventHandler _) { var s = traceEnter(View_createComponent); 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 futures = []; + TemplateLoader templateLoader = new TemplateLoader(shadowRoot, futures); + var eventHandler = new ShadowRootEventHandler( + shadowRoot, injector.getByKey(EXPANDO_KEY), injector.getByKey(EXCEPTION_HANDLER_KEY)); + shadowInjector = new ComponentDirectiveInjector( + injector, this._injector, eventHandler, shadowScope, templateLoader, shadowRoot, null); + 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 cssList) { + baseCss.styles = cssList; + _insertCss(cssList, shadowRoot, shadowRoot.firstChild); + })); + } else { + _insertCss(baseCss.styles, shadowRoot, shadowRoot.firstChild); + } + } - async.Future> 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 styles) => + _insertCss(styles, shadowRoot, firstViewNode))); + } else { + _insertCss(_styleElements, shadowRoot); + } } - ComponentDirectiveInjector shadowInjector; - TemplateLoader templateLoader = new TemplateLoader(cssFuture.then((Iterable 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; - var eventHandler = new ShadowRootEventHandler( - shadowDom, injector.getByKey(EXPANDO_KEY), injector.getByKey(EXCEPTION_HANDLER_KEY)); - shadowInjector = new ComponentDirectiveInjector(injector, _injector, eventHandler, shadowScope, - templateLoader, shadowDom, null); - 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); @@ -185,6 +201,36 @@ class BoundShadowDomComponentFactory implements BoundComponentFactory { } }; } + + _insertCss(List cssList, + dom.ShadowRoot shadowRoot, + [dom.Node insertBefore = null]) { + var s = traceEnter(View_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, + ComponentDirectiveInjector shadowInjector) { + dom.Node first = null; + if (shadowScope.isAttached) { + View shadowView = viewFactory.call(shadowScope, shadowInjector); + List 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 index a3c2b4f60..3cf74aeee 100644 --- a/lib/core_dom/transcluding_component_factory.dart +++ b/lib/core_dom/transcluding_component_factory.dart @@ -88,13 +88,14 @@ class BoundTranscludingComponentFactory implements BoundComponentFactory { final Injector _injector; Component get _component => _ref.annotation as Component; - async.Future _viewFuture; + async.Future _viewFactoryFuture; + ViewFactory _viewFactory; BoundTranscludingComponentFactory(this._f, this._ref, this._directives, this._injector) { - _viewFuture = BoundComponentFactory._viewFuture( - _component, - _f.viewCache, - _directives); + _viewFactoryFuture = BoundComponentFactory._viewFactoryFuture(_component, _f.viewCache, _directives); + if (_viewFactoryFuture != null) { + _viewFactoryFuture.then((viewFactory) => _viewFactory = viewFactory); + } } List get callArgs => _CALL_ARGS; @@ -107,56 +108,45 @@ class BoundTranscludingComponentFactory implements BoundComponentFactory { _component.cssUrls.isEmpty); var element = node as dom.Element; + var component = _component; return (DirectiveInjector injector, Scope scope, 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 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 ComponentDirectiveInjector(injector, this._injector, - eventHandler, shadowScope, templateLoader, new ShadowlessShadowRoot(element), - contentPort); + DirectiveInjector childInjector = new ComponentDirectiveInjector( + injector, this._injector, 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; if (controller is ScopeAware) controller.scope = shadowScope; 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 index 4c9952bcc..4ced0cd00 100644 --- a/lib/core_dom/view_factory.dart +++ b/lib/core_dom/view_factory.dart @@ -86,10 +86,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); } } @@ -104,15 +106,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]; @@ -138,11 +131,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 index f40845622..f30aa8c3d 100644 --- 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("$html"); } else { viewFactory = viewFactoryCache.get(html); diff --git a/lib/directive/ng_base_css.dart b/lib/directive/ng_base_css.dart index 74e41406d..34b9eade6 100644 --- 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 styles; List _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 get urls => _urls; } diff --git a/lib/ng_tracing_scopes.dart b/lib/ng_tracing_scopes.dart index ba70c0861..ba77b27e3 100644 --- a/lib/ng_tracing_scopes.dart +++ b/lib/ng_tracing_scopes.dart @@ -171,6 +171,13 @@ final View_create = traceCreateScope('View#create(ascii html)'); */ final View_createComponent = traceCreateScope('View#createComponent()'); +/** + * Name: `View#styles()` + * + * Designates where styles are inserted into component. + */ +final View_styles = traceCreateScope('View#styles()'); + /** * Name: `Directive#create(ascii name)` * diff --git a/test/core_dom/compiler_spec.dart b/test/core_dom/compiler_spec.dart index 1c6584bb5..01a7da5c5 100644 --- a/test/core_dom/compiler_spec.dart +++ b/test/core_dom/compiler_spec.dart @@ -912,7 +912,7 @@ void main() { })); /* - This test is dissabled becouse I (misko) thinks it has no real use case. It is easier + This test is disabled because I (misko) thinks it has no real use case. It is easier to understand in terms of ng-repeat @@ -926,7 +926,7 @@ void main() { the DirectChild between tabs and pane. It is not clear to me (misko) that there is a use case for getting hold of the - tranrscluding directive such a ng-repeat. + transcluding directive such a ng-repeat. */ xit('should reuse controllers for transclusions', async((Logger log) { _.compile('
view
'); diff --git a/test/directive/ng_base_css_spec.dart b/test/directive/ng_base_css_spec.dart index bc56214be..b1b807ee4 100644 --- a/test/directive/ng_base_css_spec.dart +++ b/test/directive/ng_base_css_spec.dart @@ -23,22 +23,35 @@ main() => describe('NgBaseCss', () { ..bind(_NoBaseCssComponent); }); - it('should load css urls from ng-base-css', async((TestBed _, MockHttpBackend backend) { + it('should load css urls from ng-base-css', async((TestBed _, MockHttpBackend backend, + DirectiveMap directiveMap) { backend ..expectGET('simple.css').respond(200, '.simple{}') ..expectGET('simple.html').respond(200, '
Simple!
') ..expectGET('base.css').respond(200, '.base{}'); - var element = e('
ignore
'); - _.compile(element); + NgBaseCss ngBaseCss = new NgBaseCss(); + ngBaseCss.urls = 'base.css'; + DirectiveInjector directiveInjector = new DirectiveInjector( + null, _.injector, null, null, null, null, null); + directiveInjector.bind(NgBaseCss, toValue: ngBaseCss, visibility: Visibility.CHILDREN); + var elements = es('
ignore
'); + ViewFactory viewFactory = _.compiler(elements, directiveMap); + View view = viewFactory.call(_.rootScope, directiveInjector); microLeap(); backend.flush(); microLeap(); - expect(element.children[0].shadowRoot).toHaveHtml( - '
Simple!
' - ); + expect((view.nodes[0].firstChild as Element).shadowRoot).toHaveHtml( + '
Simple!
'); + expect(ngBaseCss.styles.first.innerHtml).toEqual('.base{}'); + + // Now it should be sync + view = viewFactory.call(_.rootScope, directiveInjector); + expect((view.nodes[0].firstChild as Element).shadowRoot).toHaveHtml( + '
Simple!
'); + })); it('ng-base-css should overwrite parent ng-base-csses', async((TestBed _, MockHttpBackend backend) {