From f4cdd35b3c5c6aaf0ba9a6ea30a687bc6215eb70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mi=C5=A1ko=20Hevery?= Date: Fri, 8 Nov 2019 15:13:22 -0800 Subject: [PATCH] perf(ivy): Improve performance of transplanted views (#33702) PR Close #33702 --- .../src/render3/instructions/container.ts | 12 ++- .../src/render3/instructions/embedded_view.ts | 9 +- .../src/render3/instructions/lview_debug.ts | 11 +- .../core/src/render3/instructions/shared.ts | 102 +++++++++++------- .../core/src/render3/interfaces/container.ts | 44 +++++++- packages/core/src/render3/interfaces/view.ts | 84 ++++++++++++++- .../core/src/render3/node_manipulation.ts | 21 +++- .../src/render3/view_engine_compatibility.ts | 5 +- .../test/acceptance/change_detection_spec.ts | 92 +++++++++------- .../cyclic_import/bundle.golden_symbols.json | 6 ++ .../hello_world/bundle.golden_symbols.json | 6 ++ .../bundling/todo/bundle.golden_symbols.json | 9 ++ 12 files changed, 307 insertions(+), 94 deletions(-) diff --git a/packages/core/src/render3/instructions/container.ts b/packages/core/src/render3/instructions/container.ts index 40e6a50bf6..2ab64e639f 100644 --- a/packages/core/src/render3/instructions/container.ts +++ b/packages/core/src/render3/instructions/container.ts @@ -9,7 +9,7 @@ import {assertDataInRange, assertEqual} from '../../util/assert'; import {assertHasParent} from '../assert'; import {attachPatchData} from '../context_discovery'; import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags, registerPostOrderHooks} from '../hooks'; -import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer} from '../interfaces/container'; +import {ACTIVE_INDEX, ActiveIndexFlag, CONTAINER_HEADER_OFFSET, LContainer} from '../interfaces/container'; import {ComponentTemplate} from '../interfaces/definition'; import {LocalRefExtractor, TAttributes, TContainerNode, TNode, TNodeType, TViewNode} from '../interfaces/node'; import {isDirectiveHost} from '../interfaces/type_checks'; @@ -160,7 +160,7 @@ export function ɵɵcontainerRefreshEnd(): void { ngDevMode && assertNodeType(previousOrParentTNode, TNodeType.Container); const lContainer: LContainer = getLView()[previousOrParentTNode.index]; - const nextIndex = lContainer[ACTIVE_INDEX]; + const nextIndex = getLContainerActiveIndex(lContainer); // remove extra views at the end of the container while (nextIndex < lContainer.length - CONTAINER_HEADER_OFFSET) { @@ -194,3 +194,11 @@ function containerInternal( ngDevMode && assertNodeType(getPreviousOrParentTNode(), TNodeType.Container); return tNode; } + +export function getLContainerActiveIndex(lContainer: LContainer) { + return lContainer[ACTIVE_INDEX] >> ActiveIndexFlag.SHIFT; +} + +export function setLContainerActiveIndex(lContainer: LContainer, index: number) { + lContainer[ACTIVE_INDEX] = index << ActiveIndexFlag.SHIFT; +} diff --git a/packages/core/src/render3/instructions/embedded_view.ts b/packages/core/src/render3/instructions/embedded_view.ts index c94a38e752..270aa8d256 100644 --- a/packages/core/src/render3/instructions/embedded_view.ts +++ b/packages/core/src/render3/instructions/embedded_view.ts @@ -8,7 +8,7 @@ import {assertDefined, assertEqual} from '../../util/assert'; import {assertLContainerOrUndefined} from '../assert'; -import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer} from '../interfaces/container'; +import {ACTIVE_INDEX, ActiveIndexFlag, CONTAINER_HEADER_OFFSET, LContainer} from '../interfaces/container'; import {RenderFlags} from '../interfaces/definition'; import {TContainerNode, TNodeType} from '../interfaces/node'; import {CONTEXT, LView, LViewFlags, PARENT, TVIEW, TView, TViewType, T_HOST} from '../interfaces/view'; @@ -16,6 +16,7 @@ import {assertNodeType} from '../node_assert'; import {insertView, removeView} from '../node_manipulation'; import {enterView, getIsParent, getLView, getPreviousOrParentTNode, leaveViewProcessExit, setIsParent, setPreviousOrParentTNode} from '../state'; import {isCreationMode} from '../util/view_utils'; +import {getLContainerActiveIndex} from './container'; import {assignTViewNodeToLView, createLView, createTView, refreshView, renderView} from './shared'; @@ -38,7 +39,7 @@ export function ɵɵembeddedViewStart(viewBlockId: number, decls: number, vars: const lContainer = lView[containerTNode.index] as LContainer; ngDevMode && assertNodeType(containerTNode, TNodeType.Container); - let viewToRender = scanForView(lContainer, lContainer[ACTIVE_INDEX] !, viewBlockId); + let viewToRender = scanForView(lContainer, getLContainerActiveIndex(lContainer), viewBlockId); if (viewToRender) { setIsParent(); @@ -57,9 +58,9 @@ export function ɵɵembeddedViewStart(viewBlockId: number, decls: number, vars: if (lContainer) { if (isCreationMode(viewToRender)) { // it is a new view, insert it into collection of views for a given container - insertView(viewToRender, lContainer, lContainer[ACTIVE_INDEX] !); + insertView(viewToRender, lContainer, getLContainerActiveIndex(lContainer)); } - lContainer[ACTIVE_INDEX] !++; + lContainer[ACTIVE_INDEX] += ActiveIndexFlag.INCREMENT; } return isCreationMode(viewToRender) ? RenderFlags.Create | RenderFlags.Update : RenderFlags.Update; diff --git a/packages/core/src/render3/instructions/lview_debug.ts b/packages/core/src/render3/instructions/lview_debug.ts index 3f3e420f3c..676f24ec71 100644 --- a/packages/core/src/render3/instructions/lview_debug.ts +++ b/packages/core/src/render3/instructions/lview_debug.ts @@ -7,11 +7,11 @@ */ import {AttributeMarker, ComponentTemplate} from '..'; -import {SchemaMetadata, Type} from '../../core'; +import {SchemaMetadata} from '../../core'; import {assertDefined} from '../../util/assert'; import {createNamedArrayType} from '../../util/named_array_type'; import {initNgDevMode} from '../../util/ng_dev_mode'; -import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer, MOVED_VIEWS, NATIVE} from '../interfaces/container'; +import {ACTIVE_INDEX, ActiveIndexFlag, CONTAINER_HEADER_OFFSET, LContainer, MOVED_VIEWS, NATIVE} from '../interfaces/container'; import {DirectiveDefList, PipeDefList, ViewQueriesFunction} from '../interfaces/definition'; import {COMMENT_MARKER, ELEMENT_MARKER, I18nMutateOpCode, I18nMutateOpCodes, I18nUpdateOpCode, I18nUpdateOpCodes, TIcu} from '../interfaces/i18n'; import {PropertyAliases, TConstants, TContainerNode, TElementNode, TNode as ITNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TViewNode} from '../interfaces/node'; @@ -24,6 +24,7 @@ import {DebugNodeStyling, NodeStylingDebug} from '../styling/styling_debug'; import {attachDebugObject} from '../util/debug_utils'; import {isStylingContext} from '../util/styling_utils'; import {getTNode, unwrapRNode} from '../util/view_utils'; +import {getLContainerActiveIndex} from './container'; const NG_DEV_MODE = ((typeof ngDevMode === 'undefined' || !!ngDevMode) && initNgDevMode()); @@ -433,7 +434,11 @@ export function buildDebugNode(tNode: TNode, lView: LView, nodeIndex: number): D export class LContainerDebug { constructor(private readonly _raw_lContainer: LContainer) {} - get activeIndex(): number { return this._raw_lContainer[ACTIVE_INDEX]; } + get activeIndex(): number { return getLContainerActiveIndex(this._raw_lContainer); } + get hasTransplantedViews(): boolean { + return (this._raw_lContainer[ACTIVE_INDEX] & ActiveIndexFlag.HAS_TRANSPLANTED_VIEWS) === + ActiveIndexFlag.HAS_TRANSPLANTED_VIEWS; + } get views(): LViewDebug[] { return this._raw_lContainer.slice(CONTAINER_HEADER_OFFSET) .map(toDebug as(l: LView) => LViewDebug); diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 286ddb4ddd..58c1e03ec6 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -15,20 +15,20 @@ import {assertDataInRange, assertDefined, assertDomNode, assertEqual, assertGrea import {createNamedArrayType} from '../../util/named_array_type'; import {initNgDevMode} from '../../util/ng_dev_mode'; import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../../util/ng_reflect'; -import {assertFirstCreatePass, assertLView} from '../assert'; +import {assertFirstCreatePass, assertLContainer, assertLView} from '../assert'; import {attachPatchData} from '../context_discovery'; import {getFactoryDef} from '../definition'; import {diPublicInInjector, getNodeInjectable, getOrCreateNodeInjectorForNode} from '../di'; import {throwMultipleComponentError} from '../errors'; import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags, registerPreOrderHooks} from '../hooks'; -import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer, MOVED_VIEWS} from '../interfaces/container'; +import {ACTIVE_INDEX, ActiveIndexFlag, CONTAINER_HEADER_OFFSET, LContainer, MOVED_VIEWS} 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, TConstants, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TProjectionNode, TViewNode} from '../interfaces/node'; 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, TViewType, T_HOST} from '../interfaces/view'; +import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, ExpandoInstructions, FLAGS, HEADER_OFFSET, HOST, INJECTOR, InitPhaseState, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, TData, TVIEW, TView, TViewType, T_HOST} from '../interfaces/view'; import {assertNodeOfPossibleTypes} from '../node_assert'; import {isNodeMatchingSelectorList} from '../node_selector_matcher'; import {ActiveElementFlags, enterView, executeElementExitFn, getBindingsEnabled, getCheckNoChangesMode, getIsParent, getPreviousOrParentTNode, getSelectedIndex, hasActiveElementFlag, incrementActiveDirectiveId, leaveView, leaveViewProcessExit, setActiveHostElement, setBindingIndex, setBindingRoot, setCheckNoChangesMode, setCurrentDirectiveDef, setCurrentQueryIndex, setPreviousOrParentTNode, setSelectedIndex} from '../state'; @@ -37,7 +37,7 @@ import {NO_CHANGE} from '../tokens'; import {isAnimationProp} from '../util/attrs_utils'; import {INTERPOLATION_DELIMITER, renderStringify, stringifyForError} from '../util/misc_utils'; import {getInitialStylingValue} from '../util/styling_utils'; -import {findComponentView, getLViewParent} from '../util/view_traversal_utils'; +import {getLViewParent} from '../util/view_traversal_utils'; import {getComponentLViewByIndex, getNativeByIndex, getNativeByTNode, getTNode, isCreationMode, readPatchedLView, resetPreOrderHookFlags, unwrapRNode, viewAttachedToChangeDetector} from '../util/view_utils'; import {selectIndexInternal} from './advance'; @@ -173,6 +173,9 @@ export function createLView( lView[SANITIZER] = sanitizer || parentLView && parentLView[SANITIZER] || null !; lView[INJECTOR as any] = injector || parentLView && parentLView[INJECTOR] || null; lView[T_HOST] = tHostNode; + lView[DECLARATION_COMPONENT_VIEW] = tView.type == TViewType.Embedded ? + (parentLView === null ? null : parentLView ![DECLARATION_COMPONENT_VIEW]) : + lView; ngDevMode && attachLViewDebug(lView); return lView; } @@ -637,6 +640,7 @@ export function createTView( schemas, // schemas: SchemaMetadata[]|null, consts) : // consts: TConstants|null { + type: type, id: viewIndex, blueprint: blueprint, template: templateFn, @@ -1469,15 +1473,15 @@ export function createLContainer( ngDevMode && !isProceduralRenderer(currentView[RENDERER]) && assertDomNode(native); // https://jsperf.com/array-literal-vs-new-array-really const lContainer: LContainer = new (ngDevMode ? LContainerArray : Array)( - hostNative, // host native - true, // Boolean `true` in this position signifies that this is an `LContainer` - -1, // active index - currentView, // parent - null, // next - null, // queries - tNode, // t_host - native, // native, - null, // view refs + hostNative, // host native + true, // Boolean `true` in this position signifies that this is an `LContainer` + ActiveIndexFlag.DYNAMIC_EMBEDDED_VIEWS_ONLY << ActiveIndexFlag.SHIFT, // active index + currentView, // parent + null, // next + null, // queries + tNode, // t_host + native, // native, + null, // view refs ); ngDevMode && attachLContainerDebug(lContainer); return lContainer; @@ -1493,38 +1497,21 @@ function refreshDynamicEmbeddedViews(lView: LView) { while (viewOrContainer !== null) { // Note: viewOrContainer can be an LView or an LContainer instance, but here we are only // interested in LContainer - if (isLContainer(viewOrContainer) && viewOrContainer[ACTIVE_INDEX] === -1) { + let activeIndexFlag: ActiveIndexFlag; + if (isLContainer(viewOrContainer) && + (activeIndexFlag = viewOrContainer[ACTIVE_INDEX]) >> ActiveIndexFlag.SHIFT === + ActiveIndexFlag.DYNAMIC_EMBEDDED_VIEWS_ONLY) { for (let i = CONTAINER_HEADER_OFFSET; i < viewOrContainer.length; i++) { const embeddedLView = viewOrContainer[i]; const embeddedTView = embeddedLView[TVIEW]; ngDevMode && assertDefined(embeddedTView, 'TView must be allocated'); refreshView(embeddedLView, embeddedTView, embeddedTView.template, embeddedLView[CONTEXT] !); } - const movedViews = viewOrContainer[MOVED_VIEWS]; - if (movedViews !== null) { + if ((activeIndexFlag & ActiveIndexFlag.HAS_TRANSPLANTED_VIEWS) !== 0) { // We should only CD moved views if the component where they were inserted does not match - // the component where they were declared. Moved views also contains intra component moves, - // which we don't care about. - // TODO(misko): this is not the most efficient way to do this as we have to do a lot of - // searches. Will refactor for performance later. - const declaredComponentLView = findComponentView(lView); - for (let i = 0; i < movedViews.length; i++) { - const movedLView = movedViews[i] !; - let parentLView = movedLView[PARENT]; - while (isLContainer(parentLView)) { - parentLView = parentLView[PARENT]; - } - const insertedComponentLView = findComponentView(parentLView !); - const insertionIsOnPush = - (insertedComponentLView[FLAGS] & LViewFlags.CheckAlways) !== LViewFlags.CheckAlways; - if (insertionIsOnPush && insertedComponentLView !== declaredComponentLView) { - // Here we know that the template has been transplanted across components - // (not just moved within a component) - const movedTView = movedLView[TVIEW]; - ngDevMode && assertDefined(movedTView, 'TView must be allocated'); - refreshView(movedLView, movedTView, movedTView.template, movedLView[CONTEXT] !); - } - } + // the component where they were declared and insertion is on-push. Moved views also + // contains intra component moves, or check-always which need to be skipped. + refreshTransplantedViews(viewOrContainer, lView[DECLARATION_COMPONENT_VIEW] !); } } viewOrContainer = viewOrContainer[NEXT]; @@ -1532,6 +1519,45 @@ function refreshDynamicEmbeddedViews(lView: LView) { } +/** + * Refresh transplanted LViews. + * + * See: `ActiveIndexFlag.HAS_TRANSPLANTED_VIEWS` and `LView[DECLARATION_COMPONENT_VIEW]` for + * explanation of transplanted views. + * + * @param lContainer The `LContainer` which has transplanted views. + * @param declaredComponentLView The `lContainer` parent component `LView`. + */ +function refreshTransplantedViews(lContainer: LContainer, declaredComponentLView: LView) { + const movedViews = lContainer[MOVED_VIEWS] !; + ngDevMode && assertDefined(movedViews, 'Transplanted View flags set but missing MOVED_VIEWS'); + for (let i = 0; i < movedViews.length; i++) { + const movedLView = movedViews[i] !; + const insertionLContainer = movedLView[PARENT] as LContainer; + ngDevMode && assertLContainer(insertionLContainer); + const insertedComponentLView = insertionLContainer[PARENT][DECLARATION_COMPONENT_VIEW] !; + ngDevMode && assertDefined(insertedComponentLView, 'Missing LView'); + // Check if we have a transplanted view by compering declaration and insertion location. + if (insertedComponentLView !== declaredComponentLView) { + // Yes the `LView` is transplanted. + // Here we would like to know if the component is `OnPush`. We don't have + // explicit `OnPush` flag instead we set `CheckAlways` to false (which is `OnPush`) + // Not to be confused with `ManualOnPush` which is used with wether a DOM event + // should automatically mark a view as dirty. + const insertionComponentIsOnPush = + (insertedComponentLView[FLAGS] & LViewFlags.CheckAlways) === 0; + if (insertionComponentIsOnPush) { + // Here we know that the template has been transplanted across components and is + // on-push (not just moved within a component). If the insertion is marked dirty, then + // there is no need to CD here as we will do it again later when we get to insertion + // point. + const movedTView = movedLView[TVIEW]; + ngDevMode && assertDefined(movedTView, 'TView must be allocated'); + refreshView(movedLView, movedTView, movedTView.template, movedLView[CONTEXT] !); + } + } + } +} ///////////// diff --git a/packages/core/src/render3/interfaces/container.ts b/packages/core/src/render3/interfaces/container.ts index eb33ae6b27..44e292daf2 100644 --- a/packages/core/src/render3/interfaces/container.ts +++ b/packages/core/src/render3/interfaces/container.ts @@ -46,6 +46,41 @@ export const VIEW_REFS = 8; */ export const CONTAINER_HEADER_OFFSET = 9; + +/** + * Used to track: + * - Inline embedded views (see: `ɵɵembeddedViewStart`) + * - Transplanted `LView`s (see: `LView[DECLARATION_COMPONENT_VIEW])` + */ +export const enum ActiveIndexFlag { + /** + * Flag which signifies that the `LContainer` does not have any inline embedded views. + */ + DYNAMIC_EMBEDDED_VIEWS_ONLY = -1, + + /** + * Flag to signify that this `LContainer` may have transplanted views which need to be change + * detected. (see: `LView[DECLARATION_COMPONENT_VIEW])`. + * + * This flag once set is never unset for the `LContainer`. This means that when unset we can skip + * a lot of work in `refreshDynamicEmbeddedViews`. But when set we still need to verify + * that the `MOVED_VIEWS` are transplanted and on-push. + */ + HAS_TRANSPLANTED_VIEWS = 1, + + /** + * Number of bits to shift inline embedded views counter to make space for other flags. + */ + SHIFT = 1, + + + /** + * When incrementing the active index for inline embedded views, the amount to increment to leave + * space for other flags. + */ + INCREMENT = 1 << SHIFT, +} + /** * The state associated with a container. * @@ -75,8 +110,15 @@ export interface LContainer extends Array { * In the case the LContainer is created for a ViewContainerRef, * it is set to null to identify this scenario, as indices are "absolute" in that case, * i.e. provided directly by the user of the ViewContainerRef API. + * + * This is used by `ɵɵembeddedViewStart` to track which `LView` is currently active. + * Because `ɵɵembeddedViewStart` is not generated by the compiler this feature is essentially + * unused. + * + * The lowest bit signals that this `LContainer` has transplanted views which need to be change + * detected as part of the declaration CD. (See `LView[DECLARATION_COMPONENT_VIEW]`) */ - [ACTIVE_INDEX]: number; + [ACTIVE_INDEX]: ActiveIndexFlag; /** * Access to the parent view is necessary so we can propagate back diff --git a/packages/core/src/render3/interfaces/view.ts b/packages/core/src/render3/interfaces/view.ts index 997631eb52..1c040242ee 100644 --- a/packages/core/src/render3/interfaces/view.ts +++ b/packages/core/src/render3/interfaces/view.ts @@ -41,10 +41,11 @@ export const SANITIZER = 12; export const CHILD_HEAD = 13; export const CHILD_TAIL = 14; export const DECLARATION_VIEW = 15; -export const DECLARATION_LCONTAINER = 16; -export const PREORDER_HOOK_FLAGS = 17; +export const DECLARATION_COMPONENT_VIEW = 16; +export const DECLARATION_LCONTAINER = 17; +export const PREORDER_HOOK_FLAGS = 18; /** Size of LView's header. Necessary to adjust for it when setting slots. */ -export const HEADER_OFFSET = 18; +export const HEADER_OFFSET = 19; // This interface replaces the real LView interface if it is an arg or a @@ -194,6 +195,78 @@ export interface LView extends Array { */ [DECLARATION_VIEW]: LView|null; + + /** + * Points to the declaration component view, used to track transplanted `LView`s. + * + * See: `DECLARATION_VIEW` which points to the actual `LView` where it was declared, whereas + * `DECLARATION_COMPONENT_VIEW` points to the component which may not be same as + * `DECLARATION_VIEW`. + * + * Example: + * ``` + * <#VIEW #myComp> + *
+ * ... + *
+ * + * ``` + * In the above case `DECLARATION_VIEW` for `myTmpl` points to the `LView` of `ngIf` whereas + * `DECLARATION_COMPONENT_VIEW` points to `LView` of the `myComp` which owns the template. + * + * The reason for this is that all embedded views are always check-always whereas the component + * view can be check-always or on-push. When we have a transplanted view it is important to + * determine if we have transplanted a view from check-always declaration to on-push insertion + * point. In such a case the transplanted view needs to be added to the `LContainer` in the + * declared `LView` and CD during the declared view CD (in addition to the CD at the insertion + * point.) (Any transplanted views which are intra Component are of no interest because the CD + * strategy of declaration and insertion will always be the same, because it is the same + * component.) + * + * Queries already track moved views in `LView[DECLARATION_LCONTAINER]` and + * `LContainer[MOVED_VIEWS]`. However the queries also track `LView`s which moved within the same + * component `LView`. Transplanted views are a subset of moved views, and we use + * `DECLARATION_COMPONENT_VIEW` to differentiate them. As in this example. + * + * Example showing intra component `LView` movement. + * ``` + * <#VIEW #myComp> + *
+ * Content to render when condition is true. + * Content to render when condition is false. + * + * ``` + * The `thenBlock` and `elseBlock` is moved but not transplanted. + * + * Example showing inter component `LView` movement (transplanted view). + * ``` + * <#VIEW #myComp> + * ... + * + * + * ``` + * In the above example `myTmpl` is passed into a different component. If `insertion-component` + * instantiates `myTmpl` and `insertion-component` is on-push then the `LContainer` needs to be + * marked as containing transplanted views and those views need to be CD as part of the + * declaration CD. + * + * + * When change detection runs, it iterates over `[MOVED_VIEWS]` and CDs any child `LView`s where + * the `DECLARATION_COMPONENT_VIEW` of the current component and the child `LView` does not match + * (it has been transplanted across components.) + * + * Note: `[DECLARATION_COMPONENT_VIEW]` points to itself if the LView is a component view (the + * simplest / most common case). + * + * see also: + * - https://hackmd.io/@mhevery/rJUJsvv9H write up of the problem + * - `LContainer[ACTIVE_INDEX]` for flag which marks which `LContainer` has transplanted views. + * - `LContainer[TRANSPLANT_HEAD]` and `LContainer[TRANSPLANT_TAIL]` storage for transplanted + * - `LView[DECLARATION_LCONTAINER]` similar problem for queries + * - `LContainer[MOVED_VIEWS]` similar problem for queries + */ + [DECLARATION_COMPONENT_VIEW]: LView|null; + /** * A declaration point of embedded views (ones instantiated based on the content of a * ), null for other types of views. @@ -346,6 +419,11 @@ export const enum TViewType { * Stored on the `ComponentDef.tView`. */ export interface TView { + /** + * Type of `TView` (`Root`|`Component`|`Embedded`). + */ + type: TViewType; + /** * ID for inline views to determine whether a view is the same as the previous view * in a certain position. If it's not, we know the new view needs to be inserted diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 1b41b9f9b1..c987928317 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -11,14 +11,14 @@ import {addToArray, removeFromArray} from '../util/array_utils'; import {assertDefined, assertDomNode, assertEqual} from '../util/assert'; import {assertLContainer, assertLView, assertTNodeForLView} from './assert'; import {attachPatchData} from './context_discovery'; -import {CONTAINER_HEADER_OFFSET, LContainer, MOVED_VIEWS, NATIVE, unusedValueExportToPlacateAjd as unused1} from './interfaces/container'; +import {ACTIVE_INDEX, ActiveIndexFlag, CONTAINER_HEADER_OFFSET, LContainer, MOVED_VIEWS, NATIVE, unusedValueExportToPlacateAjd as unused1} from './interfaces/container'; import {ComponentDef} from './interfaces/definition'; import {NodeInjectorFactory} from './interfaces/injector'; import {TElementNode, TNode, TNodeFlags, TNodeType, TProjectionNode, TViewNode, unusedValueExportToPlacateAjd as unused2} from './interfaces/node'; import {unusedValueExportToPlacateAjd as unused3} from './interfaces/projection'; import {ProceduralRenderer3, RElement, RNode, RText, Renderer3, isProceduralRenderer, unusedValueExportToPlacateAjd as unused4} from './interfaces/renderer'; import {isLContainer, isLView, isRootView} from './interfaces/type_checks'; -import {CHILD_HEAD, CLEANUP, DECLARATION_LCONTAINER, FLAGS, HOST, HookData, LView, LViewFlags, NEXT, PARENT, QUERIES, RENDERER, TVIEW, T_HOST, unusedValueExportToPlacateAjd as unused5} from './interfaces/view'; +import {CHILD_HEAD, CLEANUP, DECLARATION_COMPONENT_VIEW, DECLARATION_LCONTAINER, FLAGS, HOST, HookData, LView, LViewFlags, NEXT, PARENT, QUERIES, RENDERER, TVIEW, T_HOST, unusedValueExportToPlacateAjd as unused5} from './interfaces/view'; import {assertNodeOfPossibleTypes, assertNodeType} from './node_assert'; import {findComponentView, getLViewParent} from './util/view_traversal_utils'; import {getNativeByTNode, unwrapRNode} from './util/view_utils'; @@ -259,6 +259,23 @@ function trackMovedView(declarationContainer: LContainer, lView: LView) { ngDevMode && assertDefined(lView, 'LView required'); ngDevMode && assertLContainer(declarationContainer); const movedViews = declarationContainer[MOVED_VIEWS]; + const insertedLContainer = lView[PARENT] as LContainer; + ngDevMode && assertLContainer(insertedLContainer); + const insertedComponentLView = insertedLContainer[PARENT] ![DECLARATION_COMPONENT_VIEW] !; + ngDevMode && assertDefined(insertedComponentLView, 'Missing insertedComponentLView'); + const insertedComponentIsOnPush = + (insertedComponentLView[FLAGS] & LViewFlags.CheckAlways) !== LViewFlags.CheckAlways; + if (insertedComponentIsOnPush) { + const declaredComponentLView = lView[DECLARATION_COMPONENT_VIEW]; + ngDevMode && assertDefined(declaredComponentLView, 'Missing declaredComponentLView'); + if (declaredComponentLView !== insertedComponentLView) { + // At this point the declaration-component is not same as insertion-component and we are in + // on-push mode, this means that this is a transplanted view. Mark the declared lView as + // having + // transplanted views so that those views can participate in CD. + declarationContainer[ACTIVE_INDEX] |= ActiveIndexFlag.HAS_TRANSPLANTED_VIEWS; + } + } if (movedViews === null) { declarationContainer[MOVED_VIEWS] = [lView]; } else { diff --git a/packages/core/src/render3/view_engine_compatibility.ts b/packages/core/src/render3/view_engine_compatibility.ts index f877ac13d0..a2522e837f 100644 --- a/packages/core/src/render3/view_engine_compatibility.ts +++ b/packages/core/src/render3/view_engine_compatibility.ts @@ -20,8 +20,9 @@ import {assertDefined, assertGreaterThan, assertLessThan} from '../util/assert'; import {assertLContainer} from './assert'; import {NodeInjector, getParentInjectorLocation} from './di'; +import {setLContainerActiveIndex} from './instructions/container'; import {addToViewTree, createLContainer, createLView, renderView} from './instructions/shared'; -import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer, VIEW_REFS} from './interfaces/container'; +import {ActiveIndexFlag, CONTAINER_HEADER_OFFSET, LContainer, VIEW_REFS} from './interfaces/container'; import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeType, TViewNode} from './interfaces/node'; import {RComment, RElement, isProceduralRenderer} from './interfaces/renderer'; import {isComponentHost, isLContainer, isLView, isRootView} from './interfaces/type_checks'; @@ -326,7 +327,7 @@ export function createContainerRef( if (isLContainer(slotValue)) { // If the host is a container, we don't need to create a new LContainer lContainer = slotValue; - lContainer[ACTIVE_INDEX] = -1; + setLContainerActiveIndex(lContainer, ActiveIndexFlag.DYNAMIC_EMBEDDED_VIEWS_ONLY); } else { let commentNode: RComment; // If the host is an element container, the native host element is guaranteed to be a diff --git a/packages/core/test/acceptance/change_detection_spec.ts b/packages/core/test/acceptance/change_detection_spec.ts index a9548bd91f..b1046c9517 100644 --- a/packages/core/test/acceptance/change_detection_spec.ts +++ b/packages/core/test/acceptance/change_detection_spec.ts @@ -9,12 +9,10 @@ import {CommonModule} from '@angular/common'; import {ApplicationRef, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentFactoryResolver, ComponentRef, Directive, DoCheck, EmbeddedViewRef, ErrorHandler, Input, NgModule, OnInit, QueryList, TemplateRef, Type, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; -import {ivyEnabled} from '@angular/core/src/ivy_switch'; -import {markDirty} from '@angular/core/src/render3/index'; -import {CONTEXT} from '@angular/core/src/render3/interfaces/view'; -import {getCheckNoChangesMode, instructionState} from '@angular/core/src/render3/state'; +import {AfterContentChecked, AfterViewChecked} from '@angular/core/src/core'; import {ComponentFixture, TestBed} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; +import {ivyEnabled} from '@angular/private/testing'; import {BehaviorSubject} from 'rxjs'; describe('change detection', () => { @@ -1108,16 +1106,22 @@ describe('change detection', () => { changeDetection: ChangeDetectionStrategy.OnPush, template: ` InsertComp({{greeting}}) - - +
+ + + +
` }) - class InsertComp { + class InsertComp implements DoCheck, + AfterViewChecked { get template(): TemplateRef { return declareComp.myTmpl; } greeting: string = 'Hello'; constructor(public changeDetectorRef: ChangeDetectorRef) { insertComp = this; } + ngDoCheck(): void { logValue = 'Insert'; } + ngAfterViewChecked(): void { logValue = null; } } @Component({ @@ -1125,30 +1129,24 @@ describe('change detection', () => { template: ` DeclareComp({{name}}) - {{greeting}} {{nameWithLog}}! + {{greeting}} {{logName()}}! ` }) - class DeclareComp { + class DeclareComp implements DoCheck, + AfterViewChecked { @ViewChild('myTmpl') myTmpl !: TemplateRef; name: string = 'world'; - get nameWithLog() { - if (ivyEnabled && !getCheckNoChangesMode()) { - const lFrame = instructionState.lFrame; - const parentChangeDetectionLView = lFrame.parent.lView; - if (parentChangeDetectionLView[CONTEXT] === declareComp) { - log.push('Declare'); - } else if (parentChangeDetectionLView[CONTEXT] === insertComp) { - log.push('Insert'); - } else { - log.push( - 'UNEXPECTED VIEW: ' + parentChangeDetectionLView ![CONTEXT] !.constructor.name); - } - } + constructor() { declareComp = this; } + ngDoCheck(): void { logValue = 'Declare'; } + logName() { + // This will log when the embedded view gets CD. The `logValue` will show if the CD was from + // `Insert` or from `Declare` component. + log.push(logValue !); return this.name; } - constructor() { declareComp = this; } + ngAfterViewChecked(): void { logValue = null; } } @Component({ @@ -1164,6 +1162,7 @@ describe('change detection', () => { } let log !: string[]; + let logValue !: string | null; let fixture !: ComponentFixture; let appComp !: AppComp; let insertComp !: InsertComp; @@ -1179,31 +1178,46 @@ describe('change detection', () => { }); it('should CD with declaration', () => { - fixture.detectChanges(); - ivyEnabled && expect(log).toEqual(['Insert']); + // NOTE: The CD of VE and Ivy is different and is captured in the assertions: + // `expect(log).toEqual(ivyEnabled ? [...] : [...])` + // + // The reason for this difference is in the algorithm which VE and Ivy use to deal with + // transplanted views: + // - VE: always runs CD at insertion point. If the insertion component is `OnPush` and the + // transplanted view is `CheckAlways` then the insertion component will be changed to + // `CheckAlways` (defeating the benefit of `OnPush`) + // - Ivy: Runs the CD at both the declaration as well as insertion point. The benefit of this + // approach is that each side (declaration/insertion) gets to keep its own semantics (either + // `OnPush` or `CheckAlways`). The implication is that: + // 1. The two semantics are slightly different. + // 2. Ivy will CD the transplanted view twice under some circumstances. (When both insertion + // and declaration are both dirty.) + + fixture.detectChanges(false); + expect(log).toEqual(['Insert']); log.length = 0; expect(trim(fixture.nativeElement.textContent)) .toEqual('DeclareComp(world) InsertComp(Hello) Hello world!'); declareComp.name = 'Angular'; - fixture.detectChanges(); - ivyEnabled && expect(log).toEqual(['Declare']); + fixture.detectChanges(false); + expect(log).toEqual(ivyEnabled ? ['Declare'] : ['Insert']); log.length = 0; // Expect transplanted LView to be CD because the declaration is CD. expect(trim(fixture.nativeElement.textContent)) .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello Angular!'); insertComp.greeting = 'Hi'; - fixture.detectChanges(); - ivyEnabled && expect(log).toEqual(['Declare']); + fixture.detectChanges(false); + expect(log).toEqual(ivyEnabled ? ['Declare'] : ['Insert']); log.length = 0; // expect no change because it is on push. expect(trim(fixture.nativeElement.textContent)) .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello Angular!'); insertComp.changeDetectorRef.markForCheck(); - fixture.detectChanges(); - ivyEnabled && expect(log).toEqual(['Declare', 'Insert']); + fixture.detectChanges(false); + expect(log).toEqual(ivyEnabled ? ['Declare', 'Insert'] : ['Insert']); log.length = 0; expect(trim(fixture.nativeElement.textContent)) .toEqual('DeclareComp(Angular) InsertComp(Hi) Hi Angular!'); @@ -1211,15 +1225,15 @@ describe('change detection', () => { // Destroy insertion should also destroy declaration appComp.showInsert = false; insertComp.changeDetectorRef.markForCheck(); - fixture.detectChanges(); - ivyEnabled && expect(log).toEqual([]); // No update in declaration + fixture.detectChanges(false); + expect(log).toEqual([]); log.length = 0; expect(trim(fixture.nativeElement.textContent)).toEqual('DeclareComp(Angular)'); // Restore both appComp.showInsert = true; - fixture.detectChanges(); - ivyEnabled && expect(log).toEqual(['Insert']); + fixture.detectChanges(false); + expect(log).toEqual(['Insert']); log.length = 0; expect(trim(fixture.nativeElement.textContent)) .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello Angular!'); @@ -1228,8 +1242,8 @@ describe('change detection', () => { appComp.showDeclare = false; insertComp.greeting = 'Hello'; insertComp.changeDetectorRef.markForCheck(); - fixture.detectChanges(); - ivyEnabled && expect(log).toEqual(['Insert']); + fixture.detectChanges(false); + expect(log).toEqual(['Insert']); log.length = 0; expect(trim(fixture.nativeElement.textContent)).toEqual('InsertComp(Hello) Hello Angular!'); }); 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 e7a7a8772c..f2190522b2 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -23,6 +23,9 @@ { "name": "ChangeDetectionStrategy" }, + { + "name": "DECLARATION_COMPONENT_VIEW" + }, { "name": "DECLARATION_VIEW" }, @@ -557,6 +560,9 @@ { "name": "refreshDynamicEmbeddedViews" }, + { + "name": "refreshTransplantedViews" + }, { "name": "refreshView" }, 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 0980b28f7d..4364d40a7b 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -23,6 +23,9 @@ { "name": "ChangeDetectionStrategy" }, + { + "name": "DECLARATION_COMPONENT_VIEW" + }, { "name": "DECLARATION_VIEW" }, @@ -413,6 +416,9 @@ { "name": "refreshDynamicEmbeddedViews" }, + { + "name": "refreshTransplantedViews" + }, { "name": "refreshView" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index de1cb1a2db..b6d69ec252 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -29,6 +29,9 @@ { "name": "ChangeDetectionStrategy" }, + { + "name": "DECLARATION_COMPONENT_VIEW" + }, { "name": "DECLARATION_LCONTAINER" }, @@ -1097,6 +1100,9 @@ { "name": "refreshDynamicEmbeddedViews" }, + { + "name": "refreshTransplantedViews" + }, { "name": "refreshView" }, @@ -1244,6 +1250,9 @@ { "name": "setIsNotParent" }, + { + "name": "setLContainerActiveIndex" + }, { "name": "setMapAsDirty" },