Skip to content

Commit

Permalink
refactor(core): explore approach to avoid storing LView in __ngContext__
Browse files Browse the repository at this point in the history
**Note:** this is mostly an exploratory PR to check whether the approach actually works and to start a discussion. There's more work to be done to make it mergeable (e.g. it introduces new circular dependencies, there are no unit tests etc.).

Explores adding a unique ID to each `LView` which is stored in `__ngContext__`, rather than the LView itself. This will avoid worsening memory leaks where a detached element is retained along with its `LView`.

Also reworks `LContext` to store the ID of its `LView`, rather than the `LView` itself, because the `LContext` can also be stored in `__ngContext__`.
  • Loading branch information
crisbeto committed Mar 21, 2021
1 parent 5eb7f34 commit d44a212
Show file tree
Hide file tree
Showing 25 changed files with 299 additions and 212 deletions.
31 changes: 31 additions & 0 deletions goldens/circular-deps/packages.json
Expand Up @@ -108,6 +108,33 @@
"packages/compiler/src/render3/view/styling_builder.ts",
"packages/compiler/src/render3/view/template.ts"
],
[
"packages/core/src/change_detection/change_detection.ts",
"packages/core/src/change_detection/change_detector_ref.ts",
"packages/core/src/render3/view_ref.ts",
"packages/core/src/render3/collect_native_nodes.ts",
"packages/core/src/render3/node_manipulation.ts",
"packages/core/src/render3/context_discovery.ts",
"packages/core/src/render3/instructions/shared.ts",
"packages/core/src/error_handler.ts",
"packages/core/src/errors.ts",
"packages/core/src/view/types.ts",
"packages/core/src/linker/component_factory.ts"
],
[
"packages/core/src/change_detection/change_detection.ts",
"packages/core/src/change_detection/change_detector_ref.ts",
"packages/core/src/render3/view_ref.ts",
"packages/core/src/render3/collect_native_nodes.ts",
"packages/core/src/render3/node_manipulation.ts",
"packages/core/src/render3/util/view_traversal_utils.ts",
"packages/core/src/render3/context_discovery.ts",
"packages/core/src/render3/instructions/shared.ts",
"packages/core/src/error_handler.ts",
"packages/core/src/errors.ts",
"packages/core/src/view/types.ts",
"packages/core/src/linker/component_factory.ts"
],
[
"packages/core/src/change_detection/change_detection.ts",
"packages/core/src/change_detection/change_detector_ref.ts",
Expand Down Expand Up @@ -209,6 +236,10 @@
"packages/core/src/metadata/ng_module.ts",
"packages/core/src/render3/jit/module.ts"
],
[
"packages/core/src/render3/context_discovery.ts",
"packages/core/src/render3/instructions/shared.ts"
],
[
"packages/core/src/render3/interfaces/container.ts",
"packages/core/src/render3/interfaces/node.ts",
Expand Down
12 changes: 7 additions & 5 deletions packages/core/src/debug/debug_node.ts
Expand Up @@ -8,6 +8,7 @@

import {Injector} from '../di/injector';
import {assertTNodeForLView} from '../render3/assert';
import {getLViewById} from '../render3/instructions/shared';
import {CONTAINER_HEADER_OFFSET, LContainer, NATIVE} from '../render3/interfaces/container';
import {TElementNode, TNode, TNodeFlags, TNodeType} from '../render3/interfaces/node';
import {isComponentHost, isLContainer} from '../render3/interfaces/type_checks';
Expand Down Expand Up @@ -263,7 +264,7 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme
get name(): string {
try {
const context = loadLContext(this.nativeNode)!;
const lView = context.lView;
const lView = getLViewById(context.lViewId)!; // TODO: use assertion function
const tData = lView[TVIEW].data;
const tNode = tData[context.nodeIndex] as TNode;
return tNode.value!;
Expand All @@ -290,7 +291,7 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme
return {};
}

const lView = context.lView;
const lView = getLViewById(context.lViewId)!; // TODO: use assertion function
const tData = lView[TVIEW].data;
const tNode = tData[context.nodeIndex] as TNode;

Expand All @@ -316,7 +317,7 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme
return {};
}

const lView = context.lView;
const lView = getLViewById(context.lViewId)!; // TODO: use assertion function
const tNodeAttrs = (lView[TVIEW].data[context.nodeIndex] as TNode).attrs;
const lowercaseTNodeAttrs: string[] = [];

Expand Down Expand Up @@ -503,9 +504,10 @@ function _queryAllR3(
matches: DebugElement[]|DebugNode[], elementsOnly: boolean) {
const context = loadLContext(parentElement.nativeNode, false);
if (context !== null) {
const parentTNode = context.lView[TVIEW].data[context.nodeIndex] as TNode;
const lView = getLViewById(context.lViewId)!; // TODO: use assertion function
const parentTNode = lView[TVIEW].data[context.nodeIndex] as TNode;
_queryNodeChildrenR3(
parentTNode, context.lView, predicate, matches, elementsOnly, parentElement.nativeNode);
parentTNode, lView, predicate, matches, elementsOnly, parentElement.nativeNode);
} else {
// If the context is null, then `parentElement` was either created with Renderer2 or native DOM
// APIs.
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/linker/view_container_ref.ts
Expand Up @@ -9,15 +9,15 @@
import {Injector} from '../di/injector';
import {assertNodeInjector} from '../render3/assert';
import {getParentInjectorLocation, NodeInjector} from '../render3/di';
import {addToViewTree, createLContainer} from '../render3/instructions/shared';
import {addToViewTree, createLContainer, destroyLView} from '../render3/instructions/shared';
import {CONTAINER_HEADER_OFFSET, LContainer, NATIVE, VIEW_REFS} from '../render3/interfaces/container';
import {NodeInjectorOffset} from '../render3/interfaces/injector';
import {TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TNodeType} from '../render3/interfaces/node';
import {RComment, RElement} from '../render3/interfaces/renderer_dom';
import {isLContainer} from '../render3/interfaces/type_checks';
import {LView, PARENT, RENDERER, T_HOST, TVIEW} from '../render3/interfaces/view';
import {assertTNodeType} from '../render3/node_assert';
import {addViewToContainer, destroyLView, detachView, getBeforeNodeForView, insertView, nativeInsertBefore, nativeNextSibling, nativeParentNode} from '../render3/node_manipulation';
import {addViewToContainer, detachView, getBeforeNodeForView, insertView, nativeInsertBefore, nativeNextSibling, nativeParentNode} from '../render3/node_manipulation';
import {getCurrentTNode, getLView} from '../render3/state';
import {getParentInjectorIndex, getParentInjectorView, hasParentInjector} from '../render3/util/injector_utils';
import {getNativeByTNode, unwrapRNode, viewAttachedToContainer} from '../render3/util/view_utils';
Expand Down
33 changes: 19 additions & 14 deletions packages/core/src/render3/context_discovery.ts
Expand Up @@ -8,12 +8,12 @@
import '../util/ng_dev_mode';

import {assertDefined, assertDomNode} from '../util/assert';

import {EMPTY_ARRAY} from '../util/empty';
import {getLViewById} from './instructions/shared';
import {LContext} from './interfaces/context';
import {TNode, TNodeFlags} from './interfaces/node';
import {RElement, RNode} from './interfaces/renderer_dom';
import {CONTEXT, HEADER_OFFSET, HOST, LView, TVIEW} from './interfaces/view';
import {CONTEXT, HEADER_OFFSET, HOST, ID, LView, TVIEW} from './interfaces/view';
import {getComponentLViewByIndex, unwrapRNode} from './util/view_utils';


Expand Down Expand Up @@ -109,7 +109,7 @@ export function getLContext(target: any): LContext|null {
if (Array.isArray(parentContext)) {
lView = parentContext as LView;
} else {
lView = parentContext.lView;
lView = getLViewById(parentContext.lViewId)!; // TODO: use assertion function
}

// the edge of the app was also reached here through another means
Expand Down Expand Up @@ -137,7 +137,7 @@ export function getLContext(target: any): LContext|null {
*/
function createLContext(lView: LView, nodeIndex: number, native: RNode): LContext {
return {
lView,
lViewId: lView[ID],
nodeIndex,
native,
component: undefined,
Expand All @@ -153,19 +153,20 @@ function createLContext(lView: LView, nodeIndex: number, native: RNode): LContex
* @returns The component's view
*/
export function getComponentViewByInstance(componentInstance: {}): LView {
let lView = readPatchedData(componentInstance);
let patchedData = readPatchedData(componentInstance);
let view: LView;

if (Array.isArray(lView)) {
const nodeIndex = findViaComponent(lView, componentInstance);
view = getComponentLViewByIndex(nodeIndex, lView);
const context = createLContext(lView, nodeIndex, view[HOST] as RElement);
if (Array.isArray(patchedData)) {
const nodeIndex = findViaComponent(patchedData, componentInstance);
view = getComponentLViewByIndex(nodeIndex, patchedData);
const context = createLContext(patchedData, nodeIndex, view[HOST] as RElement);
context.component = componentInstance;
attachPatchData(componentInstance, context);
attachPatchData(context.native, context);
} else {
const context = lView as any as LContext;
view = getComponentLViewByIndex(context.nodeIndex, context.lView);
const context = patchedData as any as LContext;
const lView = getLViewById(context.lViewId)!; // TODO: use assertion function
view = getComponentLViewByIndex(context.nodeIndex, lView);
}
return view;
}
Expand All @@ -181,7 +182,7 @@ const MONKEY_PATCH_KEY_NAME = '__ngContext__';
*/
export function attachPatchData(target: any, data: LView|LContext) {
ngDevMode && assertDefined(target, 'Target expected');
target[MONKEY_PATCH_KEY_NAME] = data;
target[MONKEY_PATCH_KEY_NAME] = Array.isArray(data) ? data[ID] : data;
}

/**
Expand All @@ -190,13 +191,17 @@ export function attachPatchData(target: any, data: LView|LContext) {
*/
export function readPatchedData(target: any): LView|LContext|null {
ngDevMode && assertDefined(target, 'Target expected');
return target[MONKEY_PATCH_KEY_NAME] || null;
const data = target[MONKEY_PATCH_KEY_NAME];
if (typeof data === 'number') {
return getLViewById(data);
}
return data || null;
}

export function readPatchedLView(target: any): LView|null {
const value = readPatchedData(target);
if (value) {
return Array.isArray(value) ? value : (value as LContext).lView;
return Array.isArray(value) ? value : getLViewById(value.lViewId);
}
return null;
}
Expand Down
31 changes: 29 additions & 2 deletions packages/core/src/render3/instructions/shared.ts
Expand Up @@ -33,9 +33,9 @@ import {isProceduralRenderer, Renderer3, RendererFactory3} from '../interfaces/r
import {RComment, RElement, RNode, RText} from '../interfaces/renderer_dom';
import {SanitizerFn} from '../interfaces/sanitization';
import {isComponentDef, isComponentHost, isContentQueryHost, isRootView} from '../interfaces/type_checks';
import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, HostBindingOpCodes, InitPhaseState, INJECTOR, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, T_HOST, TData, TRANSPLANTED_VIEWS_TO_REFRESH, TVIEW, TView, TViewType} from '../interfaces/view';
import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, HostBindingOpCodes, ID, InitPhaseState, INJECTOR, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, T_HOST, TData, TRANSPLANTED_VIEWS_TO_REFRESH, TVIEW, TView, TViewType} from '../interfaces/view';
import {assertPureTNodeType, assertTNodeType} from '../node_assert';
import {updateTextNode} from '../node_manipulation';
import {applyView, destroyViewTree, updateTextNode, WalkTNodeTreeAction} from '../node_manipulation';
import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher';
import {enterView, getBindingsEnabled, getCurrentDirectiveIndex, getCurrentParentTNode, getCurrentTNode, getCurrentTNodePlaceholderOk, getSelectedIndex, isCurrentTNodeParent, isInCheckNoChangesMode, isInI18nBlock, leaveView, setBindingIndex, setBindingRootForHostBindings, setCurrentDirectiveIndex, setCurrentQueryIndex, setCurrentTNode, setIsInCheckNoChangesMode, setSelectedIndex} from '../state';
import {NO_CHANGE} from '../tokens';
Expand Down Expand Up @@ -123,6 +123,8 @@ function renderChildComponents(hostLView: LView, components: number[]): void {
}
}

const ACTIVE_LVIEWS: (LView|null)[] = [];

export function createLView<T>(
parentLView: LView|null, tView: TView, context: T|null, flags: LViewFlags, host: RElement|null,
tHostNode: TNode|null, rendererFactory: RendererFactory3|null, renderer: Renderer3|null,
Expand All @@ -142,6 +144,7 @@ export function createLView<T>(
lView[SANITIZER] = sanitizer || parentLView && parentLView[SANITIZER] || null!;
lView[INJECTOR as any] = injector || parentLView && parentLView[INJECTOR] || null;
lView[T_HOST] = tHostNode;
lView[ID] = ACTIVE_LVIEWS.push(lView) - 1;
ngDevMode &&
assertEqual(
tView.type == TViewType.Embedded ? parentLView !== null : true, true,
Expand All @@ -152,6 +155,30 @@ export function createLView<T>(
return lView;
}

export function getLViewById(id: number): LView|null {
return ACTIVE_LVIEWS[id] || null;
}


/**
* A standalone function which destroys an LView,
* conducting clean up (e.g. removing listeners, calling onDestroys).
*
* @param tView The `TView' of the `LView` to be destroyed
* @param lView The view to be destroyed.
*/
export function destroyLView(tView: TView, lView: LView) {
if (!(lView[FLAGS] & LViewFlags.Destroyed)) {
const renderer = lView[RENDERER];
if (isProceduralRenderer(renderer) && renderer.destroyNode) {
ACTIVE_LVIEWS[lView[ID]] = null;
applyView(tView, lView, renderer, WalkTNodeTreeAction.Destroy, null, null);
}

destroyViewTree(lView);
}
}

/**
* Create and stores the TNode, and hooks it up to the tree.
*
Expand Down
5 changes: 2 additions & 3 deletions packages/core/src/render3/interfaces/context.ts
Expand Up @@ -8,7 +8,6 @@


import {RNode} from './renderer_dom';
import {LView} from './view';


/**
Expand All @@ -23,9 +22,9 @@ import {LView} from './view';
*/
export interface LContext {
/**
* The component's parent view data.
* ID of the component's parent view data.
*/
lView: LView;
lViewId: number;

/**
* The index instance of the node.
Expand Down
8 changes: 6 additions & 2 deletions packages/core/src/render3/interfaces/view.ts
Expand Up @@ -47,14 +47,15 @@ export const DECLARATION_COMPONENT_VIEW = 16;
export const DECLARATION_LCONTAINER = 17;
export const PREORDER_HOOK_FLAGS = 18;
export const QUERIES = 19;
export const ID = 20;
/**
* Size of LView's header. Necessary to adjust for it when setting slots.
*
* IMPORTANT: `HEADER_OFFSET` should only be referred to the in the `ɵɵ*` instructions to translate
* instruction index into `LView` index. All other indexes should be in the `LView` index space and
* there should be no need to refer to `HEADER_OFFSET` anywhere else.
*/
export const HEADER_OFFSET = 20;
export const HEADER_OFFSET = 21;


// This interface replaces the real LView interface if it is an arg or a
Expand Down Expand Up @@ -326,6 +327,9 @@ export interface LView extends Array<any> {
* are not `Dirty`/`CheckAlways`.
*/
[TRANSPLANTED_VIEWS_TO_REFRESH]: number;

/** Unique ID of the view. */
[ID]: number;
}

/** Flags associated with an LView (saved in LView[FLAGS]) */
Expand Down Expand Up @@ -1157,4 +1161,4 @@ export interface NodeInjectorDebug {
* Location of the parent `TNode`.
*/
parentInjectorIndex: number;
}
}
26 changes: 4 additions & 22 deletions packages/core/src/render3/node_manipulation.ts
Expand Up @@ -33,7 +33,7 @@ import {getNativeByTNode, unwrapRNode, updateTransplantedViewCount} from './util

const unusedValueToPlacateAjd = unused1 + unused2 + unused3 + unused4 + unused5;

const enum WalkTNodeTreeAction {
export const enum WalkTNodeTreeAction {
/** node create in the native environment. Run on initial creation. */
Create = 0,

Expand Down Expand Up @@ -380,24 +380,6 @@ export function detachView(lContainer: LContainer, removeIndex: number): LView|u
return viewToDetach;
}

/**
* A standalone function which destroys an LView,
* conducting clean up (e.g. removing listeners, calling onDestroys).
*
* @param tView The `TView' of the `LView` to be destroyed
* @param lView The view to be destroyed.
*/
export function destroyLView(tView: TView, lView: LView) {
if (!(lView[FLAGS] & LViewFlags.Destroyed)) {
const renderer = lView[RENDERER];
if (isProceduralRenderer(renderer) && renderer.destroyNode) {
applyView(tView, lView, renderer, WalkTNodeTreeAction.Destroy, null, null);
}

destroyViewTree(lView);
}
}

/**
* Calls onDestroys hooks for all directives and pipes in a given view and then removes all
* listeners. Listeners are removed as the last step so events delivered in the onDestroys hooks
Expand Down Expand Up @@ -901,13 +883,13 @@ function applyNodes(
* @param parentRElement parent DOM element for insertion (Removal does not need it).
* @param beforeNode Before which node the insertions should happen.
*/
function applyView(
export function applyView(
tView: TView, lView: LView, renderer: Renderer3, action: WalkTNodeTreeAction.Destroy,
parentRElement: null, beforeNode: null): void;
function applyView(
export function applyView(
tView: TView, lView: LView, renderer: Renderer3, action: WalkTNodeTreeAction,
parentRElement: RElement|null, beforeNode: RNode|null): void;
function applyView(
export function applyView(
tView: TView, lView: LView, renderer: Renderer3, action: WalkTNodeTreeAction,
parentRElement: RElement|null, beforeNode: RNode|null): void {
applyNodes(renderer, action, tView.firstChild, lView, parentRElement, beforeNode, false);
Expand Down

0 comments on commit d44a212

Please sign in to comment.