From 2deac0a41234180530986d698fd043114011f058 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Wed, 10 Apr 2019 11:54:59 -0700 Subject: [PATCH] perf(ivy): cache multiple reads to an element's stylingContext (#29818) Because the styling context may be stored in a different location and be apart of a sub array, reading the styling context each time a host binding is evaluated is costly. This patch ensures that the active styling context is cached so that multiple interactions with styling bindings can easily retrieve the styling context efficiently. PR Close #29818 --- packages/core/src/debug/debug_node.ts | 6 +-- .../core/src/render3/instructions/element.ts | 9 ++-- .../core/src/render3/instructions/styling.ts | 52 ++++++++++++------- packages/core/src/render3/players.ts | 4 +- packages/core/src/render3/state.ts | 6 +++ packages/core/src/render3/styling/state.ts | 30 +++++++++++ packages/core/src/render3/styling/util.ts | 4 +- .../cyclic_import/bundle.golden_symbols.json | 5 +- .../hello_world/bundle.golden_symbols.json | 3 ++ .../bundling/todo/bundle.golden_symbols.json | 12 +++++ 10 files changed, 102 insertions(+), 29 deletions(-) create mode 100644 packages/core/src/render3/styling/state.ts diff --git a/packages/core/src/debug/debug_node.ts b/packages/core/src/debug/debug_node.ts index 88b1ccebcb..722009eb21 100644 --- a/packages/core/src/debug/debug_node.ts +++ b/packages/core/src/debug/debug_node.ts @@ -13,7 +13,7 @@ import {TElementNode, TNode, TNodeFlags, TNodeType} from '../render3/interfaces/ import {StylingIndex} from '../render3/interfaces/styling'; import {LView, NEXT, PARENT, TData, TVIEW, T_HOST} from '../render3/interfaces/view'; import {getProp, getValue, isClassBasedValue} from '../render3/styling/class_and_style_bindings'; -import {getStylingContext} from '../render3/styling/util'; +import {getStylingContextFromLView} from '../render3/styling/util'; import {getComponent, getContext, getInjectionTokens, getInjector, getListeners, getLocalRefs, isBrowserEvents, loadLContext, loadLContextFromNode} from '../render3/util/discovery_utils'; import {INTERPOLATION_DELIMITER, isPropMetadataString, renderStringify} from '../render3/util/misc_utils'; import {findComponentView} from '../render3/util/view_traversal_utils'; @@ -288,7 +288,7 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme const element = this.nativeElement; if (element) { const lContext = loadLContextFromNode(element); - const stylingContext = getStylingContext(lContext.nodeIndex, lContext.lView); + const stylingContext = getStylingContextFromLView(lContext.nodeIndex, lContext.lView); if (stylingContext) { for (let i = StylingIndex.SingleStylesStartPosition; i < stylingContext.length; i += StylingIndex.Size) { @@ -317,7 +317,7 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme const element = this.nativeElement; if (element) { const lContext = loadLContextFromNode(element); - const stylingContext = getStylingContext(lContext.nodeIndex, lContext.lView); + const stylingContext = getStylingContextFromLView(lContext.nodeIndex, lContext.lView); if (stylingContext) { for (let i = StylingIndex.SingleStylesStartPosition; i < stylingContext.length; i += StylingIndex.Size) { diff --git a/packages/core/src/render3/instructions/element.ts b/packages/core/src/render3/instructions/element.ts index a250911ddd..bc2ce97089 100644 --- a/packages/core/src/render3/instructions/element.ts +++ b/packages/core/src/render3/instructions/element.ts @@ -13,13 +13,14 @@ import {registerPostOrderHooks} from '../hooks'; import {TAttributes, TNodeFlags, TNodeType} from '../interfaces/node'; import {RElement, isProceduralRenderer} from '../interfaces/renderer'; import {SanitizerFn} from '../interfaces/sanitization'; +import {StylingContext} from '../interfaces/styling'; import {BINDING_INDEX, QUERIES, RENDERER, TVIEW} from '../interfaces/view'; import {assertNodeType} from '../node_assert'; import {appendChild} from '../node_manipulation'; import {applyOnCreateInstructions} from '../node_util'; import {decreaseElementDepthCount, getActiveDirectiveId, getElementDepthCount, getIsParent, getLView, getNamespace, getPreviousOrParentTNode, getSelectedIndex, increaseElementDepthCount, setIsParent, setPreviousOrParentTNode} from '../state'; import {getInitialClassNameValue, getInitialStyleStringValue, initializeStaticContext, patchContextWithStaticAttrs, renderInitialClasses, renderInitialStyles} from '../styling/class_and_style_bindings'; -import {getStylingContext, hasClassInput, hasStyleInput} from '../styling/util'; +import {getStylingContextFromLView, hasClassInput, hasStyleInput} from '../styling/util'; import {NO_CHANGE} from '../tokens'; import {attrsStylingIndexOf, setUpAttributes} from '../util/attrs_utils'; import {renderStringify} from '../util/misc_utils'; @@ -156,13 +157,15 @@ export function ɵɵelementEnd(): void { // this is fired at the end of elementEnd because ALL of the stylingBindings code // (for directives and the template) have now executed which means the styling // context can be instantiated properly. + let stylingContext: StylingContext|null = null; if (hasClassInput(previousOrParentTNode)) { - const stylingContext = getStylingContext(previousOrParentTNode.index, lView); + stylingContext = getStylingContextFromLView(previousOrParentTNode.index, lView); setInputsForProperty( lView, previousOrParentTNode.inputs !['class'] !, getInitialClassNameValue(stylingContext)); } if (hasStyleInput(previousOrParentTNode)) { - const stylingContext = getStylingContext(previousOrParentTNode.index, lView); + stylingContext = + stylingContext || getStylingContextFromLView(previousOrParentTNode.index, lView); setInputsForProperty( lView, previousOrParentTNode.inputs !['style'] !, getInitialStyleStringValue(stylingContext)); diff --git a/packages/core/src/render3/instructions/styling.ts b/packages/core/src/render3/instructions/styling.ts index cb89610c14..5ca901decb 100644 --- a/packages/core/src/render3/instructions/styling.ts +++ b/packages/core/src/render3/instructions/styling.ts @@ -6,15 +6,17 @@ * found in the LICENSE file at https://angular.io/license */ import {StyleSanitizeFn} from '../../sanitization/style_sanitizer'; +import {assertEqual} from '../../util/assert'; import {TNode, TNodeType} from '../interfaces/node'; import {PlayerFactory} from '../interfaces/player'; -import {FLAGS, HEADER_OFFSET, LViewFlags, RENDERER, RootContextFlags} from '../interfaces/view'; +import {FLAGS, HEADER_OFFSET, LView, LViewFlags, RENDERER, RootContextFlags} from '../interfaces/view'; import {getActiveDirectiveId, getActiveDirectiveSuperClassDepth, getLView, getPreviousOrParentTNode, getSelectedIndex} from '../state'; import {getInitialClassNameValue, renderStyling, updateClassProp as updateElementClassProp, updateContextWithBindings, updateStyleProp as updateElementStyleProp, updateStylingMap} from '../styling/class_and_style_bindings'; import {ParamsOf, enqueueHostInstruction, registerHostDirective} from '../styling/host_instructions_queue'; import {BoundPlayerFactory} from '../styling/player_factory'; import {DEFAULT_TEMPLATE_DIRECTIVE_INDEX} from '../styling/shared'; -import {allocateOrUpdateDirectiveIntoContext, createEmptyStylingContext, forceClassesAsString, forceStylesAsString, getStylingContext, hasClassInput, hasStyleInput} from '../styling/util'; +import {getCachedStylingContext, setCachedStylingContext} from '../styling/state'; +import {allocateOrUpdateDirectiveIntoContext, createEmptyStylingContext, forceClassesAsString, forceStylesAsString, getStylingContextFromLView, hasClassInput, hasStyleInput} from '../styling/util'; import {NO_CHANGE} from '../tokens'; import {renderStringify} from '../util/misc_utils'; import {getRootContext} from '../util/view_traversal_utils'; @@ -170,9 +172,9 @@ export function ɵɵelementStyleProp( index: number, styleIndex: number, value: string | number | String | PlayerFactory | null, suffix?: string | null, forceOverride?: boolean): void { const valueToAdd = resolveStylePropValue(value, suffix); + const stylingContext = getStylingContext(index, getLView()); updateElementStyleProp( - getStylingContext(index + HEADER_OFFSET, getLView()), styleIndex, valueToAdd, - DEFAULT_TEMPLATE_DIRECTIVE_INDEX, forceOverride); + stylingContext, styleIndex, valueToAdd, DEFAULT_TEMPLATE_DIRECTIVE_INDEX, forceOverride); } /** @@ -206,9 +208,7 @@ export function ɵɵelementHostStyleProp( const directiveStylingIndex = getActiveDirectiveStylingIndex(); const hostElementIndex = getSelectedIndex(); - const lView = getLView(); - const stylingContext = getStylingContext(hostElementIndex + HEADER_OFFSET, lView); - + const stylingContext = getStylingContext(hostElementIndex, getLView()); const valueToAdd = resolveStylePropValue(value, suffix); const args: ParamsOf = [stylingContext, styleIndex, valueToAdd, directiveStylingIndex, forceOverride]; @@ -259,9 +259,9 @@ export function ɵɵelementClassProp( const input = (value instanceof BoundPlayerFactory) ? (value as BoundPlayerFactory) : booleanOrNull(value); + const stylingContext = getStylingContext(index, getLView()); updateElementClassProp( - getStylingContext(index + HEADER_OFFSET, getLView()), classIndex, input, - DEFAULT_TEMPLATE_DIRECTIVE_INDEX, forceOverride); + stylingContext, classIndex, input, DEFAULT_TEMPLATE_DIRECTIVE_INDEX, forceOverride); } @@ -287,9 +287,7 @@ export function ɵɵelementHostClassProp( classIndex: number, value: boolean | PlayerFactory, forceOverride?: boolean): void { const directiveStylingIndex = getActiveDirectiveStylingIndex(); const hostElementIndex = getSelectedIndex(); - - const lView = getLView(); - const stylingContext = getStylingContext(hostElementIndex + HEADER_OFFSET, lView); + const stylingContext = getStylingContext(hostElementIndex, getLView()); const input = (value instanceof BoundPlayerFactory) ? (value as BoundPlayerFactory) : @@ -330,8 +328,8 @@ export function ɵɵelementStylingMap( index: number, classes: {[key: string]: any} | string | NO_CHANGE | null, styles?: {[styleName: string]: any} | NO_CHANGE | null): void { const lView = getLView(); + const stylingContext = getStylingContext(index, lView); const tNode = getTNode(index, lView); - const stylingContext = getStylingContext(index + HEADER_OFFSET, lView); // inputs are only evaluated from a template binding into a directive, therefore, // there should not be a situation where a directive host bindings function @@ -384,10 +382,7 @@ export function ɵɵelementHostStylingMap( styles?: {[styleName: string]: any} | NO_CHANGE | null): void { const directiveStylingIndex = getActiveDirectiveStylingIndex(); const hostElementIndex = getSelectedIndex(); - - const lView = getLView(); - const stylingContext = getStylingContext(hostElementIndex + HEADER_OFFSET, lView); - + const stylingContext = getStylingContext(hostElementIndex, getLView()); const args: ParamsOf = [stylingContext, classes, styles, directiveStylingIndex]; enqueueHostInstruction(stylingContext, directiveStylingIndex, updateStylingMap, args); @@ -432,13 +427,22 @@ export function elementStylingApplyInternal(directiveStylingIndex: number, index // the styling apply code knows not to actually apply the values... const renderer = tNode.type === TNodeType.Element ? lView[RENDERER] : null; const isFirstRender = (lView[FLAGS] & LViewFlags.FirstLViewPass) !== 0; - const stylingContext = getStylingContext(index + HEADER_OFFSET, lView); + const stylingContext = getStylingContext(index, lView); const totalPlayersQueued = renderStyling( stylingContext, renderer, lView, isFirstRender, null, null, directiveStylingIndex); if (totalPlayersQueued > 0) { const rootContext = getRootContext(lView); scheduleTick(rootContext, RootContextFlags.FlushPlayers); } + + // because select(n) may not run between every instruction, the cached styling + // context may not get cleared between elements. The reason for this is because + // styling bindings (like `[style]` and `[class]`) are not recognized as property + // bindings by default so a select(n) instruction is not generated. To ensure the + // context is loaded correctly for the next element the cache below is pre-emptively + // cleared because there is no code in Angular that applies more styling code after a + // styling flush has occurred. Note that this will be fixed once FW-1254 lands. + setCachedStylingContext(null); } export function getActiveDirectiveStylingIndex() { @@ -450,3 +454,15 @@ export function getActiveDirectiveStylingIndex() { // sub-classed directive the inheritance depth is taken into account as well. return getActiveDirectiveId() + getActiveDirectiveSuperClassDepth(); } + +function getStylingContext(index: number, lView: LView) { + let context = getCachedStylingContext(); + if (!context) { + context = getStylingContextFromLView(index + HEADER_OFFSET, lView); + setCachedStylingContext(context); + } else if (ngDevMode) { + const actualContext = getStylingContextFromLView(index + HEADER_OFFSET, lView); + assertEqual(context, actualContext, 'The cached styling context is invalid'); + } + return context; +} diff --git a/packages/core/src/render3/players.ts b/packages/core/src/render3/players.ts index 50e2e07323..3c38289bd9 100644 --- a/packages/core/src/render3/players.ts +++ b/packages/core/src/render3/players.ts @@ -11,7 +11,7 @@ import {getLContext} from './context_discovery'; import {scheduleTick} from './instructions/shared'; import {ComponentInstance, DirectiveInstance, Player} from './interfaces/player'; import {RootContextFlags} from './interfaces/view'; -import {addPlayerInternal, getOrCreatePlayerContext, getPlayerContext, getPlayersInternal, getStylingContext, throwInvalidRefError} from './styling/util'; +import {addPlayerInternal, getOrCreatePlayerContext, getPlayerContext, getPlayersInternal, getStylingContextFromLView, throwInvalidRefError} from './styling/util'; import {getRootContext} from './util/view_traversal_utils'; @@ -61,7 +61,7 @@ export function getPlayers(ref: ComponentInstance | DirectiveInstance | HTMLElem return []; } - const stylingContext = getStylingContext(context.nodeIndex, context.lView); + const stylingContext = getStylingContextFromLView(context.nodeIndex, context.lView); const playerContext = stylingContext ? getPlayerContext(stylingContext) : null; return playerContext ? getPlayersInternal(playerContext) : []; } diff --git a/packages/core/src/render3/state.ts b/packages/core/src/render3/state.ts index 5bc7d315bc..8612cfa9d6 100644 --- a/packages/core/src/render3/state.ts +++ b/packages/core/src/render3/state.ts @@ -13,6 +13,7 @@ import {executeHooks} from './hooks'; import {ComponentDef, DirectiveDef} from './interfaces/definition'; import {TElementNode, TNode, TViewNode} from './interfaces/node'; import {BINDING_INDEX, CONTEXT, DECLARATION_VIEW, FLAGS, InitPhaseState, LView, LViewFlags, OpaqueViewState, TVIEW} from './interfaces/view'; +import {setCachedStylingContext} from './styling/state'; import {resetPreOrderHookFlags} from './util/view_utils'; @@ -456,6 +457,7 @@ export function leaveView(newView: LView): void { lView[BINDING_INDEX] = tView.bindingStartIndex; } } + setCachedStylingContext(null); enterView(newView, null); } @@ -482,6 +484,10 @@ export function getSelectedIndex() { */ export function setSelectedIndex(index: number) { _selectedIndex = index; + + // remove the styling context from the cache + // because we are now on a different element + setCachedStylingContext(null); } diff --git a/packages/core/src/render3/styling/state.ts b/packages/core/src/render3/styling/state.ts new file mode 100644 index 0000000000..c000295e37 --- /dev/null +++ b/packages/core/src/render3/styling/state.ts @@ -0,0 +1,30 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import {StylingContext} from '../interfaces/styling'; + +let stylingContext: StylingContext|null = null; + +/** + * Gets the most recent styling context value. + * + * Note that only one styling context is stored at a given time. + */ +export function getCachedStylingContext() { + return stylingContext; +} + +/** + * Sets the most recent styling context value. + * + * Note that only one styling context is stored at a given time. + * + * @param context The styling context value that will be stored + */ +export function setCachedStylingContext(context: StylingContext | null) { + stylingContext = context; +} diff --git a/packages/core/src/render3/styling/util.ts b/packages/core/src/render3/styling/util.ts index 50c52987eb..9ae4842a6d 100644 --- a/packages/core/src/render3/styling/util.ts +++ b/packages/core/src/render3/styling/util.ts @@ -124,7 +124,7 @@ export function allocStylingContext( * @param index Index of the style allocation. See: `elementStyling`. * @param viewData The view to search for the styling context */ -export function getStylingContext(index: number, viewData: LView): StylingContext { +export function getStylingContextFromLView(index: number, viewData: LView): StylingContext { let storageIndex = index; let slotValue: LContainer|LView|StylingContext|RElement = viewData[storageIndex]; let wrapper: LContainer|LView|StylingContext = viewData; @@ -252,7 +252,7 @@ export function getOrCreatePlayerContext(target: {}, context?: LContext | null): } const {lView, nodeIndex} = context; - const stylingContext = getStylingContext(nodeIndex, lView); + const stylingContext = getStylingContextFromLView(nodeIndex, lView); return getPlayerContext(stylingContext) || allocPlayerContext(stylingContext); } 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 98c70fb680..acb85251e0 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -408,7 +408,7 @@ "name": "getRootView" }, { - "name": "getStylingContext" + "name": "getStylingContextFromLView" }, { "name": "getTNode" @@ -614,6 +614,9 @@ { "name": "setBindingRoot" }, + { + "name": "setCachedStylingContext" + }, { "name": "setClass" }, 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 98e4f7685f..99e9f7e146 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -440,6 +440,9 @@ { "name": "setBindingRoot" }, + { + "name": "setCachedStylingContext" + }, { "name": "setClass" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 2673be4132..8b4cd066a4 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -641,6 +641,9 @@ { "name": "getBindingsEnabled" }, + { + "name": "getCachedStylingContext" + }, { "name": "getCheckNoChangesMode" }, @@ -824,6 +827,9 @@ { "name": "getStylingContext" }, + { + "name": "getStylingContextFromLView" + }, { "name": "getSymbolIterator" }, @@ -1178,6 +1184,9 @@ { "name": "setBindingRoot" }, + { + "name": "setCachedStylingContext" + }, { "name": "setCheckNoChangesMode" }, @@ -1265,6 +1274,9 @@ { "name": "stringify" }, + { + "name": "stylingContext" + }, { "name": "syncViewWithBlueprint" },