Skip to content

Commit

Permalink
fixup! perf(core): avoid storing LView in __ngContext__
Browse files Browse the repository at this point in the history
  • Loading branch information
@crisbeto
crisbeto committed Apr 22, 2021
1 parent 1fa1ae7 commit 39abf88
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 48 deletions.
26 changes: 12 additions & 14 deletions packages/core/src/debug/debug_node.ts
View file
Expand Up @@ -7,7 +7,7 @@
*/

import {Injector} from '../di/injector';
import {assertLView, assertTNodeForLView} from '../render3/assert';
import {assertTNodeForLView} from '../render3/assert';
import {getLContext} from '../render3/context_discovery';
import {CONTAINER_HEADER_OFFSET, LContainer, NATIVE} from '../render3/interfaces/container';
import {TElementNode, TNode, TNodeFlags, TNodeType} from '../render3/interfaces/node';
Expand Down Expand Up @@ -264,7 +264,6 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme
get name(): string {
const context = getLContext(this.nativeNode)!;
const lView = context ? context.lView : null;
ngDevMode && assertLView(lView);

if (lView !== null) {
const tData = lView[TVIEW].data;
Expand All @@ -288,13 +287,13 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme
* - attribute bindings (e.g. `[attr.role]="menu"`)
*/
get properties(): {[key: string]: any;} {
const context = getLContext(this.nativeNode);
if (context === null) {
const context = getLContext(this.nativeNode)!;
const lView = context ? context.lView : null;

if (lView === null) {
return {};
}

const lView = context.lView!;
ngDevMode && assertLView(lView);
const tData = lView[TVIEW].data;
const tNode = tData[context.nodeIndex] as TNode;

Expand All @@ -315,13 +314,13 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme
return attributes;
}

const context = getLContext(element);
if (context === null) {
const context = getLContext(element)!;
const lView = context ? context.lView : null;

if (lView === null) {
return {};
}

const lView = context.lView!;
ngDevMode && assertLView(lView);
const tNodeAttrs = (lView[TVIEW].data[context.nodeIndex] as TNode).attrs;
const lowercaseTNodeAttrs: string[] = [];

Expand Down Expand Up @@ -506,10 +505,9 @@ function _queryAllR3(
function _queryAllR3(
parentElement: DebugElement, predicate: Predicate<DebugElement>|Predicate<DebugNode>,
matches: DebugElement[]|DebugNode[], elementsOnly: boolean) {
const context = getLContext(parentElement.nativeNode);
if (context !== null) {
const lView = context.lView!;
ngDevMode && assertLView(lView);
const context = getLContext(parentElement.nativeNode)!;
const lView = context ? context.lView : null;
if (lView !== null) {
const parentTNode = lView[TVIEW].data[context.nodeIndex] as TNode;
_queryNodeChildrenR3(
parentTNode, lView, predicate, matches, elementsOnly, parentElement.nativeNode);
Expand Down
8 changes: 1 addition & 7 deletions packages/core/src/render3/context_discovery.ts
View file
Expand Up @@ -108,13 +108,7 @@ export function getLContext(target: any): LContext|null {
while (parent = parent.parentNode) {
const parentContext = readPatchedData(parent);
if (parentContext) {
let lView: LView|null;
if (Array.isArray(parentContext)) {
lView = parentContext as LView;
} else {
lView = parentContext.lView;
ngDevMode && assertLView(lView);
}
const lView = Array.isArray(parentContext) ? parentContext as LView : parentContext.lView;

// the edge of the app was also reached here through another means
// (maybe because the DOM was changed manually).
Expand Down
43 changes: 18 additions & 25 deletions packages/core/src/render3/util/discovery_utils.ts
View file
Expand Up @@ -11,7 +11,7 @@ import {Injector} from '../../di/injector';
import {ViewEncapsulation} from '../../metadata/view';
import {assertEqual} from '../../util/assert';
import {assertLView} from '../assert';
import {discoverLocalRefs, getComponentAtNodeIndex, getDirectivesAtNodeIndex, getLContext} from '../context_discovery';
import {discoverLocalRefs, getComponentAtNodeIndex, getDirectivesAtNodeIndex, getLContext, readPatchedLView} from '../context_discovery';
import {getComponentDef, getDirectiveDef} from '../definition';
import {NodeInjector} from '../di';
import {buildDebugNode} from '../instructions/lview_debug';
Expand Down Expand Up @@ -81,13 +81,9 @@ export function getComponent<T>(element: Element): T|null {
*/
export function getContext<T>(element: Element): T|null {
assertDomElement(element);
const context = getLContext(element);
if (context === null) {
return null;
}
const lView = context.lView!;
ngDevMode && assertLView(lView);
return lView[CONTEXT] as T;
const context = getLContext(element)!;
const lView = context ? context.lView : null;
return lView === null ? null : lView[CONTEXT] as T;
}

/**
Expand All @@ -106,12 +102,11 @@ export function getContext<T>(element: Element): T|null {
* @globalApi ng
*/
export function getOwningComponent<T>(elementOrDir: Element|{}): T|null {
const context = getLContext(elementOrDir);
if (context === null) return null;
const context = getLContext(elementOrDir)!;
let lView = context ? context.lView : null;
if (lView === null) return null;

let parent: LView|null;
let lView = context.lView!;
ngDevMode && assertLView(lView);
while (lView[TVIEW].type === TViewType.Embedded && (parent = getLViewParent(lView)!)) {
lView = parent;
}
Expand All @@ -130,7 +125,8 @@ export function getOwningComponent<T>(elementOrDir: Element|{}): T|null {
* @globalApi ng
*/
export function getRootComponents(elementOrDir: Element|{}): {}[] {
return [...getRootContext(elementOrDir).components];
const lView = readPatchedLView(elementOrDir);
return lView ? [...getRootContext(lView).components] : [];
}

/**
Expand All @@ -144,11 +140,10 @@ export function getRootComponents(elementOrDir: Element|{}): {}[] {
* @globalApi ng
*/
export function getInjector(elementOrDir: Element|{}): Injector {
const context = getLContext(elementOrDir);
if (context === null) return Injector.NULL;
const context = getLContext(elementOrDir)!;
const lView = context ? context.lView : null;
if (lView === null) return Injector.NULL;

const lView = context.lView!;
ngDevMode && assertLView(lView);
const tNode = lView[TVIEW].data[context.nodeIndex] as TElementNode;
return new NodeInjector(tNode, lView);
}
Expand All @@ -159,10 +154,9 @@ export function getInjector(elementOrDir: Element|{}): Injector {
* @param element Element for which the injection tokens should be retrieved.
*/
export function getInjectionTokens(element: Element): any[] {
const context = getLContext(element);
if (context === null) return [];
const lView = context.lView!;
ngDevMode && assertLView(lView);
const context = getLContext(element)!;
const lView = context ? context.lView : null;
if (lView === null) return [];
const tView = lView[TVIEW];
const tNode = tView.data[context.nodeIndex] as TNode;
const providerTokens: any[] = [];
Expand Down Expand Up @@ -211,13 +205,12 @@ export function getDirectives(node: Node): {}[] {
return [];
}

const context = getLContext(node);
if (context === null) {
const context = getLContext(node)!;
const lView = context ? context.lView : null;
if (lView === null) {
return [];
}

const lView = context.lView!;
ngDevMode && assertLView(lView);
const tView = lView[TVIEW];
const nodeIndex = context.nodeIndex;
if (!tView?.data[nodeIndex]) {
Expand Down
71 changes: 69 additions & 2 deletions packages/core/test/acceptance/discover_utils_spec.ts
View file
Expand Up @@ -69,16 +69,19 @@ onlyInIvy('Ivy-specific utilities').describe('discovery utils', () => {
@Component({
selector: 'my-app',
template: `
<span (click)="log($event)">{{text}}</span>
<span (click)="log($event)" *ngIf="spanVisible">{{text}}</span>
<div dirA #div #foo="dirA"></div>
<child></child>
<child dirA #child></child>
<child dirA *ngIf="true"></child>
<child dirA *ngIf="conditionalChildVisible"></child>
<ng-container><p></p></ng-container>
<b *ngIf="visible">Bold</b>
`
})
class MyApp {
text: string = 'INIT';
spanVisible = true;
conditionalChildVisible = true;
@Input('a') b = 2;
@Output('c') d = new EventEmitter();
constructor() {
Expand All @@ -105,6 +108,15 @@ onlyInIvy('Ivy-specific utilities').describe('discovery utils', () => {
expect(getComponent<Child>(child[0])).toEqual(childComponent[0]);
expect(getComponent<Child>(child[1])).toEqual(childComponent[1]);
});
it('should not throw when called on a destroyed node', () => {
expect(getComponent(span[0])).toEqual(null);
expect(getComponent<Child>(child[2])).toEqual(childComponent[2]);
fixture.componentInstance.spanVisible = false;
fixture.componentInstance.conditionalChildVisible = false;
fixture.detectChanges();
expect(getComponent(span[0])).toEqual(null);
expect(getComponent<Child>(child[2])).toEqual(childComponent[2]);
});
});

describe('getComponentLView', () => {
Expand All @@ -131,6 +143,12 @@ onlyInIvy('Ivy-specific utilities').describe('discovery utils', () => {
expect(getContext<{$implicit: boolean}>(child[2])!.$implicit).toEqual(true);
expect(getContext<Child>(p[0])).toEqual(childComponent[0]);
});
it('should return null for destroyed node', () => {
expect(getContext(span[0])).toBeTruthy();
fixture.componentInstance.spanVisible = false;
fixture.detectChanges();
expect(getContext(span[0])).toBeNull();
});
});

describe('getHostElement', () => {
Expand All @@ -146,6 +164,12 @@ onlyInIvy('Ivy-specific utilities').describe('discovery utils', () => {
it('should throw on unknown target', () => {
expect(() => getHostElement({})).toThrowError(); //
});
it('should return element for destroyed node', () => {
expect(getHostElement(span[0])).toEqual(span[0]);
fixture.componentInstance.spanVisible = false;
fixture.detectChanges();
expect(getHostElement(span[0])).toEqual(span[0]);
});
});

describe('getInjector', () => {
Expand All @@ -163,6 +187,12 @@ onlyInIvy('Ivy-specific utilities').describe('discovery utils', () => {
expect(getInjector(dirA[0]).get(String)).toEqual('Module');
expect(getInjector(dirA[1]).get(String)).toEqual('Child');
});
it('should retrieve injector from destroyed node', () => {
expect(getInjector(span[0])).toBeTruthy();
fixture.componentInstance.spanVisible = false;
fixture.detectChanges();
expect(getInjector(span[0])).toBeTruthy();
});
});

describe('getDirectives', () => {
Expand All @@ -175,6 +205,12 @@ onlyInIvy('Ivy-specific utilities').describe('discovery utils', () => {
expect(getDirectives(div[0])).toEqual([dirA[0]]);
expect(getDirectives(child[1])).toEqual([dirA[1]]);
});
it('should return empty array for destroyed node', () => {
expect(getDirectives(span[0])).toEqual([]);
fixture.componentInstance.spanVisible = false;
fixture.detectChanges();
expect(getDirectives(span[0])).toEqual([]);
});
});

describe('getOwningComponent', () => {
Expand Down Expand Up @@ -202,6 +238,12 @@ onlyInIvy('Ivy-specific utilities').describe('discovery utils', () => {
expect(getOwningComponent<MyApp>(dirA[0])).toEqual(myApp);
expect(getOwningComponent<MyApp>(dirA[1])).toEqual(myApp);
});
it('should return null for destroyed node', () => {
expect(getOwningComponent<MyApp>(span[0])).toEqual(myApp);
fixture.componentInstance.spanVisible = false;
fixture.detectChanges();
expect(getOwningComponent<MyApp>(span[0])).toEqual(null);
});
});

describe('getLocalRefs', () => {
Expand All @@ -219,6 +261,13 @@ onlyInIvy('Ivy-specific utilities').describe('discovery utils', () => {
expect(getLocalRefs(child[1])).toEqual({child: childComponent[1]});
expect(getLocalRefs(dirA[1])).toEqual({child: childComponent[1]});
});

it('should retrieve from a destroyed node', () => {
expect(getLocalRefs(span[0])).toEqual({});
fixture.componentInstance.spanVisible = false;
fixture.detectChanges();
expect(getLocalRefs(span[0])).toEqual({});
});
});

describe('getRootComponents', () => {
Expand All @@ -234,6 +283,12 @@ onlyInIvy('Ivy-specific utilities').describe('discovery utils', () => {
expect(getRootComponents(div[0])).toEqual(rootComponents);
expect(getRootComponents(p[0])).toEqual(rootComponents);
});
it('should return an empty array for a destroyed node', () => {
expect(getRootComponents(span[0])).toEqual([myApp]);
fixture.componentInstance.spanVisible = false;
fixture.detectChanges();
expect(getRootComponents(span[0])).toEqual([]);
});
});

describe('getListeners', () => {
Expand All @@ -251,6 +306,12 @@ onlyInIvy('Ivy-specific utilities').describe('discovery utils', () => {
listeners[0].callback('CLICKED');
expect(log).toEqual(['CLICKED']);
});
it('should return no listeners for destroyed node', () => {
expect(getListeners(span[0]).length).toEqual(1);
fixture.componentInstance.spanVisible = false;
fixture.detectChanges();
expect(getListeners(span[0]).length).toEqual(0);
});
});

describe('getInjectionTokens', () => {
Expand All @@ -259,6 +320,12 @@ onlyInIvy('Ivy-specific utilities').describe('discovery utils', () => {
expect(getInjectionTokens(child[0])).toEqual([String, Child]);
expect(getInjectionTokens(child[1])).toEqual([String, Child, DirectiveA]);
});
it('should retrieve tokens from destroyed node', () => {
expect(getInjectionTokens(span[0])).toEqual([]);
fixture.componentInstance.spanVisible = false;
fixture.detectChanges();
expect(getInjectionTokens(span[0])).toEqual([]);
});
});

describe('markDirty', () => {
Expand Down

0 comments on commit 39abf88

Please sign in to comment.