From d3a3d021d946adf445d2641d7da06784d23109d6 Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Fri, 18 Oct 2019 16:22:44 +0200 Subject: [PATCH 1/5] refactor(ivy): correct typings in instantiateAllDirectives --- packages/core/src/render3/di.ts | 4 ++-- packages/core/src/render3/di_setup.ts | 10 +++++----- packages/core/src/render3/instructions/shared.ts | 14 +++++++------- packages/core/src/render3/interfaces/injector.ts | 4 ++-- packages/core/src/render3/interfaces/node.ts | 5 +++++ 5 files changed, 21 insertions(+), 16 deletions(-) diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index 8b63e30d0c47a..a095dc47f58f7 100644 --- a/packages/core/src/render3/di.ts +++ b/packages/core/src/render3/di.ts @@ -19,7 +19,7 @@ import {getFactoryDef} from './definition'; import {NG_ELEMENT_ID, NG_FACTORY_DEF} from './fields'; import {DirectiveDef, FactoryFn} from './interfaces/definition'; import {NO_PARENT_INJECTOR, NodeInjectorFactory, PARENT_INJECTOR, RelativeInjectorLocation, RelativeInjectorLocationFlags, TNODE, isFactory} from './interfaces/injector'; -import {AttributeMarker, TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeProviderIndexes, TNodeType} from './interfaces/node'; +import {AttributeMarker, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TNode, TNodeProviderIndexes, TNodeType} from './interfaces/node'; import {isComponentDef, isComponentHost} from './interfaces/type_checks'; import {DECLARATION_VIEW, INJECTOR, LView, TData, TVIEW, TView, T_HOST} from './interfaces/view'; import {assertNodeOfPossibleTypes} from './node_assert'; @@ -528,7 +528,7 @@ export function locateDirectiveOrProvider( * instantiates the `injectable` and caches the value. */ export function getNodeInjectable( - tData: TData, lView: LView, index: number, tNode: TElementNode): any { + tData: TData, lView: LView, index: number, tNode: TDirectiveHostNode): any { let value = lView[index]; if (isFactory(value)) { const factory: NodeInjectorFactory = value; diff --git a/packages/core/src/render3/di_setup.ts b/packages/core/src/render3/di_setup.ts index 7540090cba1b5..9716a70fa1eeb 100644 --- a/packages/core/src/render3/di_setup.ts +++ b/packages/core/src/render3/di_setup.ts @@ -15,7 +15,7 @@ import {diPublicInInjector, getNodeInjectable, getOrCreateNodeInjectorForNode} f import {ɵɵdirectiveInject} from './instructions/all'; import {DirectiveDef} from './interfaces/definition'; import {NodeInjectorFactory} from './interfaces/injector'; -import {TContainerNode, TElementContainerNode, TElementNode, TNodeProviderIndexes} from './interfaces/node'; +import {TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TNodeProviderIndexes} from './interfaces/node'; import {isComponentDef} from './interfaces/type_checks'; import {LView, TData, TVIEW, TView} from './interfaces/view'; import {getLView, getPreviousOrParentTNode} from './state'; @@ -205,7 +205,7 @@ function indexOf(item: any, arr: any[], begin: number, end: number) { */ function multiProvidersFactoryResolver( this: NodeInjectorFactory, _: undefined, tData: TData, lData: LView, - tNode: TElementNode): any[] { + tNode: TDirectiveHostNode): any[] { return multiResolve(this.multi !, []); } @@ -216,7 +216,7 @@ function multiProvidersFactoryResolver( */ function multiViewProvidersFactoryResolver( this: NodeInjectorFactory, _: undefined, tData: TData, lData: LView, - tNode: TElementNode): any[] { + tNode: TDirectiveHostNode): any[] { const factories = this.multi !; let result: any[]; if (this.providerFactory) { @@ -254,8 +254,8 @@ function multiResolve(factories: Array<() => any>, result: any[]): any[] { */ function multiFactory( factoryFn: ( - this: NodeInjectorFactory, _: undefined, tData: TData, lData: LView, tNode: TElementNode) => - any, + this: NodeInjectorFactory, _: undefined, tData: TData, lData: LView, + tNode: TDirectiveHostNode) => any, index: number, isViewProvider: boolean, isComponent: boolean, f: () => any): NodeInjectorFactory { const factory = new NodeInjectorFactory(factoryFn, isViewProvider, ɵɵdirectiveInject); diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 5fa7c649b2ee0..4852d490c1272 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -23,7 +23,7 @@ import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags, re import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer} from '../interfaces/container'; import {ComponentDef, ComponentTemplate, DirectiveDef, DirectiveDefListOrFactory, PipeDefListOrFactory, RenderFlags, ViewQueriesFunction} from '../interfaces/definition'; import {INJECTOR_BLOOM_PARENT_SIZE, NodeInjectorFactory} from '../interfaces/injector'; -import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, PropertyAliasValue, PropertyAliases, TAttributes, TContainerNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TProjectionNode, TViewNode} from '../interfaces/node'; +import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, PropertyAliasValue, PropertyAliases, TAttributes, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TProjectionNode, TViewNode} from '../interfaces/node'; import {RComment, RElement, RText, Renderer3, RendererFactory3, isProceduralRenderer} from '../interfaces/renderer'; import {SanitizerFn} from '../interfaces/sanitization'; import {isComponentDef, isComponentHost, isContentQueryHost, isLContainer, isRootView} from '../interfaces/type_checks'; @@ -1090,19 +1090,19 @@ export function resolveDirectives( /** * Instantiate all the directives that were previously resolved on the current node. */ -function instantiateAllDirectives(tView: TView, lView: LView, tNode: TNode) { +function instantiateAllDirectives(tView: TView, lView: LView, tNode: TDirectiveHostNode) { const start = tNode.directiveStart; const end = tNode.directiveEnd; if (!tView.firstTemplatePass) { - getOrCreateNodeInjectorForNode( - tNode as TElementNode | TContainerNode | TElementContainerNode, lView); + getOrCreateNodeInjectorForNode(tNode, lView); } for (let i = start; i < end; i++) { const def = tView.data[i] as DirectiveDef; if (isComponentDef(def)) { - addComponentLogic(lView, tNode, def as ComponentDef); + assertNodeOfPossibleTypes(tNode, TNodeType.Element); + addComponentLogic(lView, tNode as TElementNode, def); } - const directive = getNodeInjectable(tView.data, lView !, i, tNode as TElementNode); + const directive = getNodeInjectable(tView.data, lView, i, tNode); postProcessDirective(lView, tNode, directive, def, i - start); } } @@ -1306,7 +1306,7 @@ function baseResolveDirective(tView: TView, viewData: LView, def: DirectiveDe viewData.push(nodeInjectorFactory); } -function addComponentLogic(lView: LView, hostTNode: TNode, def: ComponentDef): void { +function addComponentLogic(lView: LView, hostTNode: TElementNode, def: ComponentDef): void { const native = getNativeByTNode(hostTNode, lView) as RElement; const tView = getOrCreateTView(def); diff --git a/packages/core/src/render3/interfaces/injector.ts b/packages/core/src/render3/interfaces/injector.ts index ad2ef16d059df..a4324225394ed 100644 --- a/packages/core/src/render3/interfaces/injector.ts +++ b/packages/core/src/render3/interfaces/injector.ts @@ -10,7 +10,7 @@ import {InjectionToken} from '../../di/injection_token'; import {InjectFlags} from '../../di/interface/injector'; import {Type} from '../../interface/type'; -import {TElementNode} from './node'; +import {TDirectiveHostNode} from './node'; import {LView, TData} from './view'; export const TNODE = 8; @@ -230,7 +230,7 @@ export class NodeInjectorFactory { /** * The TNode of the same element injector. */ - tNode: TElementNode) => any, + tNode: TDirectiveHostNode) => any, /** * Set to `true` if the token is declared in `viewProviders` (or if it is component). */ diff --git a/packages/core/src/render3/interfaces/node.ts b/packages/core/src/render3/interfaces/node.ts index a598252390fbf..352f6eaa6fb14 100644 --- a/packages/core/src/render3/interfaces/node.ts +++ b/packages/core/src/render3/interfaces/node.ts @@ -602,6 +602,11 @@ export interface TProjectionNode extends TNode { projection: number; } +/** + * An union type representing all TNode types that can host a directive. + */ +export type TDirectiveHostNode = TElementNode | TContainerNode | TElementContainerNode; + /** * This mapping is necessary so we can set input properties and output listeners * properly at runtime when property names are minified or aliased. From 5de5337db9686c9049db2a975264a5501cf7a99a Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Fri, 18 Oct 2019 16:35:28 +0200 Subject: [PATCH 2/5] refactor(ivy): in-line postProcessDirective to avoid repeated isComponentDef checks --- .../core/src/render3/instructions/shared.ts | 38 +++++++++---------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 4852d490c1272..d25c001790c02 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -1096,14 +1096,27 @@ function instantiateAllDirectives(tView: TView, lView: LView, tNode: TDirectiveH if (!tView.firstTemplatePass) { getOrCreateNodeInjectorForNode(tNode, lView); } + for (let i = start; i < end; i++) { const def = tView.data[i] as DirectiveDef; - if (isComponentDef(def)) { - assertNodeOfPossibleTypes(tNode, TNodeType.Element); - addComponentLogic(lView, tNode as TElementNode, def); + const isComponent = isComponentDef(def); + + if (isComponent) { + ngDevMode && assertNodeOfPossibleTypes(tNode, TNodeType.Element); + addComponentLogic(lView, tNode as TElementNode, def as ComponentDef); } + const directive = getNodeInjectable(tView.data, lView, i, tNode); - postProcessDirective(lView, tNode, directive, def, i - start); + + postProcessBaseDirective(lView, tNode, directive); + if (tNode.initialInputs !== null) { + setInputsFromAttrs(lView, i - start, directive, def, tNode); + } + + if (isComponent) { + const componentView = getComponentLViewByIndex(tNode.index, lView); + componentView[CONTEXT] = directive; + } } } @@ -1169,23 +1182,6 @@ export function generateExpandoInstructionBlock( ])).push(elementIndex, providerCount, directiveCount); } -/** - * Process a directive on the current node after its creation. - */ -function postProcessDirective( - lView: LView, hostTNode: TNode, directive: T, def: DirectiveDef, - directiveDefIdx: number): void { - postProcessBaseDirective(lView, hostTNode, directive); - if (hostTNode.initialInputs !== null) { - setInputsFromAttrs(lView, directiveDefIdx, directive, def, hostTNode); - } - - if (isComponentDef(def)) { - const componentView = getComponentLViewByIndex(hostTNode.index, lView); - componentView[CONTEXT] = directive; - } -} - /** * A lighter version of postProcessDirective() that is used for the root component. */ From d64c6760cbc01275351f042dabe615c5372269be Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Mon, 21 Oct 2019 10:38:21 +0200 Subject: [PATCH 3/5] perf(ivy): avoid repeated native node retrieval and patching Before this change instantiating multiple directives on the same host node would result in repeated RNode retrieval and patching. This commint re-organises code around directive instance creation so the host node processing (common to all directives) happens once and only once. As the additional benefit the directive instantiation logic gets centralised in one function (at the expense of patching logic duplication for root node). --- .../core/src/render3/instructions/shared.ts | 45 ++++++++----------- .../hello_world/bundle.golden_symbols.json | 3 -- .../bundling/todo/bundle.golden_symbols.json | 6 --- 3 files changed, 18 insertions(+), 36 deletions(-) diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index d25c001790c02..d291bb7a4c148 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -24,7 +24,7 @@ import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer} from '../interfaces/c import {ComponentDef, ComponentTemplate, DirectiveDef, DirectiveDefListOrFactory, PipeDefListOrFactory, RenderFlags, ViewQueriesFunction} from '../interfaces/definition'; import {INJECTOR_BLOOM_PARENT_SIZE, NodeInjectorFactory} from '../interfaces/injector'; import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, PropertyAliasValue, PropertyAliases, TAttributes, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TProjectionNode, TViewNode} from '../interfaces/node'; -import {RComment, RElement, RText, Renderer3, RendererFactory3, isProceduralRenderer} from '../interfaces/renderer'; +import {RComment, RElement, RNode, RText, Renderer3, RendererFactory3, isProceduralRenderer} from '../interfaces/renderer'; import {SanitizerFn} from '../interfaces/sanitization'; import {isComponentDef, isComponentHost, isContentQueryHost, isLContainer, isRootView} from '../interfaces/type_checks'; import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_VIEW, ExpandoInstructions, FLAGS, HEADER_OFFSET, HOST, INJECTOR, InitPhaseState, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, TData, TVIEW, TView, T_HOST} from '../interfaces/view'; @@ -529,10 +529,9 @@ export function executeContentQueries(tView: TView, tNode: TNode, lView: LView) /** * Creates directive instances. */ -export function createDirectivesInstances( - tView: TView, lView: LView, tNode: TElementNode | TContainerNode | TElementContainerNode) { +export function createDirectivesInstances(tView: TView, lView: LView, tNode: TDirectiveHostNode) { if (!getBindingsEnabled()) return; - instantiateAllDirectives(tView, lView, tNode); + instantiateAllDirectives(tView, lView, tNode, getNativeByTNode(tNode, lView)); if ((tNode.flags & TNodeFlags.hasHostBindings) === TNodeFlags.hasHostBindings) { invokeDirectivesHostBindings(tView, lView, tNode); } @@ -543,7 +542,8 @@ export function createDirectivesInstances( * to LView in the same order as they are loaded in the template with load(). */ export function saveResolvedLocalsInData( - viewData: LView, tNode: TNode, localRefExtractor: LocalRefExtractor = getNativeByTNode): void { + viewData: LView, tNode: TDirectiveHostNode, + localRefExtractor: LocalRefExtractor = getNativeByTNode): void { const localNames = tNode.localNames; if (localNames) { let localIndex = tNode.index + 1; @@ -1018,17 +1018,20 @@ function warnAboutUnknownProperty(propName: string, tNode: TNode): void { /** * Instantiate a root component. */ -export function instantiateRootComponent( - tView: TView, viewData: LView, def: ComponentDef): T { +export function instantiateRootComponent(tView: TView, lView: LView, def: ComponentDef): T { const rootTNode = getPreviousOrParentTNode(); if (tView.firstTemplatePass) { if (def.providersResolver) def.providersResolver(def); generateExpandoInstructionBlock(tView, rootTNode, 1); - baseResolveDirective(tView, viewData, def); + baseResolveDirective(tView, lView, def); } const directive = - getNodeInjectable(tView.data, viewData, viewData.length - 1, rootTNode as TElementNode); - postProcessBaseDirective(viewData, rootTNode, directive); + getNodeInjectable(tView.data, lView, lView.length - 1, rootTNode as TElementNode); + attachPatchData(directive, lView); + const native = getNativeByTNode(rootTNode, lView); + if (native) { + attachPatchData(native, lView); + } return directive; } @@ -1090,13 +1093,16 @@ export function resolveDirectives( /** * Instantiate all the directives that were previously resolved on the current node. */ -function instantiateAllDirectives(tView: TView, lView: LView, tNode: TDirectiveHostNode) { +function instantiateAllDirectives( + tView: TView, lView: LView, tNode: TDirectiveHostNode, native: RNode) { const start = tNode.directiveStart; const end = tNode.directiveEnd; if (!tView.firstTemplatePass) { getOrCreateNodeInjectorForNode(tNode, lView); } + attachPatchData(native, lView); + for (let i = start; i < end; i++) { const def = tView.data[i] as DirectiveDef; const isComponent = isComponentDef(def); @@ -1107,8 +1113,8 @@ function instantiateAllDirectives(tView: TView, lView: LView, tNode: TDirectiveH } const directive = getNodeInjectable(tView.data, lView, i, tNode); + attachPatchData(directive, lView); - postProcessBaseDirective(lView, tNode, directive); if (tNode.initialInputs !== null) { setInputsFromAttrs(lView, i - start, directive, def, tNode); } @@ -1182,21 +1188,6 @@ export function generateExpandoInstructionBlock( ])).push(elementIndex, providerCount, directiveCount); } -/** - * A lighter version of postProcessDirective() that is used for the root component. - */ -function postProcessBaseDirective(lView: LView, hostTNode: TNode, directive: T): void { - ngDevMode && assertLessThanOrEqual( - getBindingIndex(), lView[TVIEW].bindingStartIndex, - 'directives should be created before any bindings'); - attachPatchData(directive, lView); - const native = getNativeByTNode(hostTNode, lView); - if (native) { - attachPatchData(native, lView); - } -} - - /** * Matches the current node against all available selectors. * If a component is matched (at most one), it is returned in first position in the array. diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index 2b3924e09d71c..46ff0fb4bfb6c 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -392,9 +392,6 @@ { "name": "noSideEffects" }, - { - "name": "postProcessBaseDirective" - }, { "name": "refreshChildComponents" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index cebecb6d3b561..90a2e991ee749 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -1088,12 +1088,6 @@ { "name": "patchConfig" }, - { - "name": "postProcessBaseDirective" - }, - { - "name": "postProcessDirective" - }, { "name": "readPatchedData" }, From bc73ebef22e42b90d42c80c6390c001b34d545b8 Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Tue, 22 Oct 2019 16:42:23 +0200 Subject: [PATCH 4/5] perf(ivy): avoid repeated tNode.initialInputs reads --- packages/core/src/render3/instructions/shared.ts | 9 +++++---- .../bundling/cyclic_import/bundle.golden_symbols.json | 6 ------ 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index d291bb7a4c148..c5cd3cde6d266 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -1103,6 +1103,7 @@ function instantiateAllDirectives( attachPatchData(native, lView); + const initialInputs = tNode.initialInputs; for (let i = start; i < end; i++) { const def = tView.data[i] as DirectiveDef; const isComponent = isComponentDef(def); @@ -1115,8 +1116,8 @@ function instantiateAllDirectives( const directive = getNodeInjectable(tView.data, lView, i, tNode); attachPatchData(directive, lView); - if (tNode.initialInputs !== null) { - setInputsFromAttrs(lView, i - start, directive, def, tNode); + if (initialInputs !== null) { + setInputsFromAttrs(lView, i - start, directive, def, tNode, initialInputs !); } if (isComponent) { @@ -1348,8 +1349,8 @@ export function elementAttributeInternal( * @param tNode The static data for this node */ function setInputsFromAttrs( - lView: LView, directiveIndex: number, instance: T, def: DirectiveDef, tNode: TNode): void { - const initialInputData = tNode.initialInputs as InitialInputData; + lView: LView, directiveIndex: number, instance: T, def: DirectiveDef, tNode: TNode, + initialInputData: InitialInputData): void { const initialInputs: InitialInputs|null = initialInputData ![directiveIndex]; if (initialInputs !== null) { const setInput = def.setInput; diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json index 4ea80638e4f3e..897f619d3421f 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -533,12 +533,6 @@ { "name": "objectToClassName" }, - { - "name": "postProcessBaseDirective" - }, - { - "name": "postProcessDirective" - }, { "name": "refreshChildComponents" }, From 8093da1b79c054ff3fab5ad78ec28049dd3a17ca Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Thu, 31 Oct 2019 15:26:44 +0100 Subject: [PATCH 5/5] fixup! perf(ivy): avoid repeated tNode.initialInputs reads Co-Authored-By: Kara --- packages/core/src/render3/interfaces/node.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/render3/interfaces/node.ts b/packages/core/src/render3/interfaces/node.ts index 352f6eaa6fb14..1b095bcddd636 100644 --- a/packages/core/src/render3/interfaces/node.ts +++ b/packages/core/src/render3/interfaces/node.ts @@ -603,7 +603,7 @@ export interface TProjectionNode extends TNode { } /** - * An union type representing all TNode types that can host a directive. + * A union type representing all TNode types that can host a directive. */ export type TDirectiveHostNode = TElementNode | TContainerNode | TElementContainerNode;