Skip to content

perf(ivy): add performance counters in ngDevMode #23385

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mhevery wants to merge 7 commits into from
Closed

perf(ivy): add performance counters in ngDevMode #23385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bb60345
perf(ivy): add performance counters in ngDevMode
mhevery Apr 14, 2018
13fb08e
fix(ivy): only generate TViews once per embedded template
kara Apr 26, 2018
a37bcef
fixup! fix(ivy): only generate TViews once per embedded template
kara Apr 26, 2018
a040063
fixup! perf(ivy): add performance counters in ngDevMode
kara Apr 27, 2018
89cde6a
fixup! fix(ivy): only generate TViews once per embedded template
kara Apr 27, 2018
95cf714
fixup! fix(ivy): only generate TViews once per embedded template
kara Apr 28, 2018
080d905
fixup! perf(ivy): add performance counters in ngDevMode
kara Apr 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions packages/core/src/render3/di.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,14 @@ import {EmbeddedViewRef as viewEngine_EmbeddedViewRef, ViewRef as viewEngine_Vie
import {Type} from '../type';

import {assertGreaterThan, assertLessThan, assertNotNull} from './assert';
import {addToViewTree, assertPreviousIsParent, createLContainer, createLNodeObject, getDirectiveInstance, getPreviousOrParentNode, getRenderer, isComponent, renderEmbeddedTemplate, resolveDirective} from './instructions';
import {addToViewTree, assertPreviousIsParent, createLContainer, createLNodeObject, createTView, getDirectiveInstance, getPreviousOrParentNode, getRenderer, isComponent, renderEmbeddedTemplate, resolveDirective} from './instructions';
import {LContainer} from './interfaces/container';
import {ComponentTemplate, DirectiveDef, DirectiveDefList, PipeDefList} from './interfaces/definition';
import {LInjector} from './interfaces/injector';
import {LContainerNode, LElementNode, LNode, LNodeType, LViewNode, TNodeFlags} from './interfaces/node';
import {QueryReadType} from './interfaces/query';
import {Renderer3} from './interfaces/renderer';
import {LView} from './interfaces/view';
import {LView, TView} from './interfaces/view';
import {assertNodeOfPossibleTypes, assertNodeType} from './node_assert';
import {insertView, removeView} from './node_manipulation';
import {notImplemented, stringify} from './util';
Expand Down Expand Up @@ -568,7 +569,6 @@ export function getOrCreateContainerRef(di: LInjector): viewEngine_ViewContainer
const vcRefHost = di.node;

ngDevMode && assertNodeOfPossibleTypes(vcRefHost, LNodeType.Container, LNodeType.Element);

const lContainer = createLContainer(vcRefHost.parent !, vcRefHost.view);
const lContainerNode: LContainerNode = createLNodeObject(
LNodeType.Container, vcRefHost.view, vcRefHost.parent !, undefined, lContainer, null);
Expand Down Expand Up @@ -695,29 +695,35 @@ class ViewContainerRef implements viewEngine_ViewContainerRef {
* @returns The TemplateRef instance to use
*/
export function getOrCreateTemplateRef<T>(di: LInjector): viewEngine_TemplateRef<T> {
ngDevMode && assertNodeType(di.node, LNodeType.Container);
const data = (di.node as LContainerNode).data;
const tView = di.node.view.tView;
return di.templateRef || (di.templateRef = new TemplateRef<any>(
getOrCreateElementRef(di), data.template !, getRenderer(),
tView.directiveRegistry, tView.pipeRegistry));
if (!di.templateRef) {
ngDevMode && assertNodeType(di.node, LNodeType.Container);
const hostNode = di.node as LContainerNode;
const hostTNode = hostNode.tNode !;
const hostTView = hostNode.view.tView;
if (!hostTNode.tViews) {
hostTNode.tViews = createTView(hostTView.directiveRegistry, hostTView.pipeRegistry);
}
ngDevMode && assertNotNull(hostTNode.tViews, 'TView must be allocated');
di.templateRef = new TemplateRef<any>(
getOrCreateElementRef(di), hostTNode.tViews as TView, hostNode.data.template !,
getRenderer(), hostTView.directiveRegistry, hostTView.pipeRegistry);
}
return di.templateRef;
}

class TemplateRef<T> implements viewEngine_TemplateRef<T> {
readonly elementRef: viewEngine_ElementRef;
private _template: ComponentTemplate<T>;

constructor(
elementRef: viewEngine_ElementRef, template: ComponentTemplate<T>,
private _renderer: Renderer3, private _directives: DirectiveDefList|null,
private _pipes: PipeDefList|null) {
elementRef: viewEngine_ElementRef, private _tView: TView,
private _template: ComponentTemplate<T>, private _renderer: Renderer3,
private _directives: DirectiveDefList|null, private _pipes: PipeDefList|null) {
this.elementRef = elementRef;
this._template = template;
}

createEmbeddedView(context: T): viewEngine_EmbeddedViewRef<T> {
const viewNode = renderEmbeddedTemplate(
null, this._template, context, this._renderer, this._directives, this._pipes);
null, this._tView, this._template, context, this._renderer, this._directives, this._pipes);
return addDestroyable(new EmbeddedViewRef(viewNode, this._template, context));
}
}
106 changes: 79 additions & 27 deletions packages/core/src/render3/instructions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import './ng_dev_mode';

import {assertEqual, assertLessThan, assertNotEqual, assertNotNull, assertNull, assertSame} from './assert';
import {LContainer, TContainer} from './interfaces/container';
import {LContainer} from './interfaces/container';
import {LInjector} from './interfaces/injector';
import {CssSelectorList, LProjection, NG_PROJECT_AS_ATTR_NAME} from './interfaces/projection';
import {LQueries} from './interfaces/query';
Expand Down Expand Up @@ -468,9 +468,20 @@ export function renderTemplate<T>(
return host;
}

/**
* Used for rendering embedded views (e.g. dynamically created views)
*
* Dynamically created views must store/retrieve their TViews differently from component views
* because their template functions are nested in the template functions of their hosts, creating
* closures. If their host template happens to be an embedded template in a loop (e.g. ngFor inside
* an ngFor), the nesting would mean we'd have multiple instances of the template function, so we
* can't store TViews in the template function itself (as we do for comps). Instead, we store the
* TView for dynamically created views on their host TNode, which only has one instance.
*/
export function renderEmbeddedTemplate<T>(
viewNode: LViewNode | null, template: ComponentTemplate<T>, context: T, renderer: Renderer3,
directives?: DirectiveDefList | null, pipes?: PipeDefList | null): LViewNode {
viewNode: LViewNode | null, tView: TView, template: ComponentTemplate<T>, context: T,
renderer: Renderer3, directives?: DirectiveDefList | null,
pipes?: PipeDefList | null): LViewNode {
const _isParent = isParent;
const _previousOrParentNode = previousOrParentNode;
let oldView: LView;
Expand All @@ -480,7 +491,6 @@ export function renderEmbeddedTemplate<T>(
previousOrParentNode = null !;

if (viewNode == null) {
const tView = getOrCreateTView(template, directives || null, pipes || null);
const lView = createLView(-1, renderer, tView, template, context, LViewFlags.CheckAlways);

viewNode = createLNode(null, LNodeType.View, null, lView);
Expand Down Expand Up @@ -566,22 +576,33 @@ export function elementStart(
assertEqual(
currentView.bindingStartIndex, -1, 'elements should be created before any bindings');

ngDevMode && ngDevMode.rendererCreateElement++;
const native: RElement = renderer.createElement(name);
const node: LElementNode = createLNode(index, LNodeType.Element, native !, null);

if (attrs) setUpAttributes(native, attrs);
appendChild(node.parent !, native, currentView);
createDirectivesAndLocals(index, name, attrs, localRefs, null);
createDirectivesAndLocals(index, name, attrs, localRefs, false);
return native;
}

/**
* Creates directive instances and populates local refs.
*
* @param index Index of the current node (to create TNode)
* @param name Tag name of the current node
* @param attrs Attrs of the current node
* @param localRefs Local refs of the current node
* @param inlineViews Whether or not this node will create inline views
*/
function createDirectivesAndLocals(
index: number, name: string | null, attrs: string[] | null | undefined,
localRefs: string[] | null | undefined, containerData: TView[] | null) {
localRefs: string[] | null | undefined, inlineViews: boolean) {
const node = previousOrParentNode;
if (firstTemplatePass) {
ngDevMode && ngDevMode.firstTemplatePass++;
ngDevMode && assertDataInRange(index - 1);
node.tNode = tData[index] = createTNode(name, attrs || null, containerData);
node.tNode = tData[index] = createTNode(name, attrs || null, inlineViews ? [] : null);
cacheMatchingDirectivesForNode(node.tNode, currentView.tView, localRefs || null);
} else {
instantiateDirectivesDirectly();
Expand Down Expand Up @@ -749,13 +770,21 @@ function saveResolvedLocalsInData(): void {
function getOrCreateTView(
template: ComponentTemplate<any>, directives: DirectiveDefListOrFactory | null,
pipes: PipeDefListOrFactory | null): TView {
// TODO(misko): reading `ngPrivateData` here is problematic for two reasons
// 1. It is a megamorphic call on each invocation.
// 2. For nested embedded views (ngFor inside ngFor) the template instance is per
// outer template invocation, which means that no such property will exist
// Correct solution is to only put `ngPrivateData` on the Component template
// and not on embedded templates.

return template.ngPrivateData ||
(template.ngPrivateData = createTView(directives, pipes) as never);
}

/** Creates a TView instance */
export function createTView(
defs: DirectiveDefListOrFactory | null, pipes: PipeDefListOrFactory | null): TView {
ngDevMode && ngDevMode.tView++;
return {
data: [],
directives: null,
Expand Down Expand Up @@ -784,6 +813,7 @@ function setUpAttributes(native: RElement, attrs: string[]): void {
const attrName = attrs[i];
if (attrName !== NG_PROJECT_AS_ATTR_NAME) {
const attrVal = attrs[i + 1];
ngDevMode && ngDevMode.rendererSetAttribute++;
isProc ? (renderer as ProceduralRenderer3).setAttribute(native, attrName, attrVal) :
native.setAttribute(attrName, attrVal);
}
Expand Down Expand Up @@ -867,6 +897,7 @@ export function listener(
// In order to match current behavior, native DOM event listeners must be added for all
// events (including outputs).
const cleanupFns = cleanup || (cleanup = currentView.cleanup = []);
ngDevMode && ngDevMode.rendererAddEventListener++;
if (isProceduralRenderer(renderer)) {
const wrappedListener = wrapListenerWithDirtyLogic(currentView, listenerFn);
const cleanupFn = renderer.listen(native, eventName, wrappedListener);
Expand Down Expand Up @@ -931,9 +962,11 @@ export function elementAttribute(
if (value !== NO_CHANGE) {
const element: LElementNode = data[index];
if (value == null) {
ngDevMode && ngDevMode.rendererRemoveAttribute++;
isProceduralRenderer(renderer) ? renderer.removeAttribute(element.native, name) :
element.native.removeAttribute(name);
} else {
ngDevMode && ngDevMode.rendererSetAttribute++;
const strValue = sanitizer == null ? stringify(value) : sanitizer(value);
isProceduralRenderer(renderer) ? renderer.setAttribute(element.native, name, strValue) :
element.native.setAttribute(name, strValue);
Expand Down Expand Up @@ -977,6 +1010,7 @@ export function elementProperty<T>(
// is risky, so sanitization can be done without further checks.
value = sanitizer != null ? (sanitizer(value) as any) : value;
const native = node.native;
ngDevMode && ngDevMode.rendererSetProperty++;
isProceduralRenderer(renderer) ? renderer.setProperty(native, propName, value) :
(native.setProperty ? native.setProperty(propName, value) :
(native as any)[propName] = value);
Expand All @@ -986,14 +1020,15 @@ export function elementProperty<T>(
/**
* Constructs a TNode object from the arguments.
*
* @param tagName
* @param attrs
* @param data
* @param tagName The tag name of the node
* @param attrs The attributes defined on this ndoe
* @param tViews Any TViews attached to this node
* @param localNames A list of local names and their matching indices
* @returns the TNode object
*/
function createTNode(
tagName: string | null, attrs: string[] | null, data: TContainer | null): TNode {
tagName: string | null, attrs: string[] | null, tViews: TView[] | null): TNode {
ngDevMode && ngDevMode.tNode++;
return {
flags: 0,
tagName: tagName,
Expand All @@ -1002,7 +1037,7 @@ function createTNode(
initialInputs: undefined,
inputs: undefined,
outputs: undefined,
data: data
tViews: tViews
};
}

Expand Down Expand Up @@ -1067,10 +1102,12 @@ export function elementClassNamed<T>(index: number, className: string, value: T
if (value !== NO_CHANGE) {
const lElement = data[index] as LElementNode;
if (value) {
ngDevMode && ngDevMode.rendererAddClass++;
isProceduralRenderer(renderer) ? renderer.addClass(lElement.native, className) :
lElement.native.classList.add(className);

} else {
ngDevMode && ngDevMode.rendererRemoveClass++;
isProceduralRenderer(renderer) ? renderer.removeClass(lElement.native, className) :
lElement.native.classList.remove(className);
}
Expand All @@ -1095,6 +1132,7 @@ export function elementClass<T>(index: number, value: T | NO_CHANGE): void {
// future
// we will add logic here which would work with the animation code.
const lElement: LElementNode = data[index];
ngDevMode && ngDevMode.rendererSetClassName++;
isProceduralRenderer(renderer) ? renderer.setProperty(lElement.native, 'className', value) :
lElement.native['className'] = stringify(value);
}
Expand All @@ -1121,13 +1159,15 @@ export function elementStyleNamed<T>(
if (value !== NO_CHANGE) {
const lElement: LElementNode = data[index];
if (value == null) {
ngDevMode && ngDevMode.rendererRemoveStyle++;
isProceduralRenderer(renderer) ?
renderer.removeStyle(lElement.native, styleName, RendererStyleFlags3.DashCase) :
lElement.native['style'].removeProperty(styleName);
} else {
let strValue =
typeof suffixOrSanitizer == 'function' ? suffixOrSanitizer(value) : stringify(value);
if (typeof suffixOrSanitizer == 'string') strValue = strValue + suffixOrSanitizer;
ngDevMode && ngDevMode.rendererSetStyle++;
isProceduralRenderer(renderer) ?
renderer.setStyle(lElement.native, styleName, strValue, RendererStyleFlags3.DashCase) :
lElement.native['style'].setProperty(styleName, strValue);
Expand Down Expand Up @@ -1155,14 +1195,20 @@ export function elementStyle<T>(
// we will add logic here which would work with the animation code.
const lElement = data[index] as LElementNode;
if (isProceduralRenderer(renderer)) {
ngDevMode && ngDevMode.rendererSetStyle++;
renderer.setProperty(lElement.native, 'style', value);
} else {
const style = lElement.native['style'];
for (let i = 0, keys = Object.keys(value); i < keys.length; i++) {
const styleName: string = keys[i];
const styleValue: any = (value as any)[styleName];
styleValue == null ? style.removeProperty(styleName) :
style.setProperty(styleName, styleValue);
if (styleValue == null) {
ngDevMode && ngDevMode.rendererRemoveStyle++;
style.removeProperty(styleName);
} else {
ngDevMode && ngDevMode.rendererSetStyle++;
style.setProperty(styleName, styleValue);
}
}
}
}
Expand All @@ -1184,6 +1230,7 @@ export function text(index: number, value?: any): void {
ngDevMode &&
assertEqual(
currentView.bindingStartIndex, -1, 'text nodes should be created before bindings');
ngDevMode && ngDevMode.rendererCreateTextNode++;
const textNode = createTextNode(value, renderer);
const node = createLNode(index, LNodeType.Element, textNode);
// Text nodes are self closing.
Expand All @@ -1203,6 +1250,7 @@ export function textBinding<T>(index: number, value: T | NO_CHANGE): void {
let existingNode = data[index] as LTextNode;
ngDevMode && assertNotNull(existingNode, 'LNode should exist');
ngDevMode && assertNotNull(existingNode.native, 'native element should exist');
ngDevMode && ngDevMode.rendererSetText++;
value !== NO_CHANGE &&
(isProceduralRenderer(renderer) ? renderer.setValue(existingNode.native, stringify(value)) :
existingNode.native.textContent = stringify(value));
Expand Down Expand Up @@ -1407,7 +1455,7 @@ export function createLContainer(
* @param localRefs A set of local reference bindings on the element.
*/
export function container(
index: number, template?: ComponentTemplate<any>, tagName?: string, attrs?: string[],
index: number, template?: ComponentTemplate<any>, tagName?: string | null, attrs?: string[],
localRefs?: string[] | null): void {
ngDevMode && assertEqual(
currentView.bindingStartIndex, -1,
Expand All @@ -1421,7 +1469,7 @@ export function container(
// Containers are added to the current view tree instead of their embedded views
// because views can be removed and re-inserted.
addToViewTree(currentView, node.data);
createDirectivesAndLocals(index, tagName || null, attrs, localRefs, []);
createDirectivesAndLocals(index, tagName || null, attrs, localRefs, template == null);

isParent = false;
ngDevMode && assertNodeType(previousOrParentNode, LNodeType.Container);
Expand Down Expand Up @@ -1486,9 +1534,12 @@ function refreshDynamicChildren() {
if (current.dynamicViewCount !== 0 && (current as LContainer).views) {
const container = current as LContainer;
for (let i = 0; i < container.views.length; i++) {
const view = container.views[i];
const lViewNode = container.views[i];
// The directives and pipes are not needed here as an existing view is only being refreshed.
renderEmbeddedTemplate(view, view.data.template !, view.data.context !, renderer);
const dynamicView = lViewNode.data;
ngDevMode && assertNotNull(dynamicView.tView, 'TView must be allocated');
renderEmbeddedTemplate(
lViewNode, dynamicView.tView, dynamicView.template !, dynamicView.context !, renderer);
}
}
}
Expand Down Expand Up @@ -1558,23 +1609,24 @@ export function embeddedViewStart(viewBlockId: number): RenderFlags {
/**
* Initialize the TView (e.g. static data) for the active embedded view.
*
* Each embedded view needs to set the global tData variable to the static data for
* that view. Otherwise, the view's static data for a particular node would overwrite
* the static data for a node in the view above it with the same index (since it's in the
* same template).
* Each embedded view block must create or retrieve its own TView. Otherwise, the embedded view's
* static data for a particular node would overwrite the static data for a node in the view above
* it with the same index (since it's in the same template).
*
* @param viewIndex The index of the TView in TContainer
* @param viewIndex The index of the TView in TNode.tViews
* @param parent The parent container in which to look for the view's static data
* @returns TView
*/
function getOrCreateEmbeddedTView(viewIndex: number, parent: LContainerNode): TView {
Copy link
Contributor

@vicb vicb Apr 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure to understand that part of the API docs:

  * Each embedded view needs to set the global tData variable to the static data for
  * that view. Otherwise, the view's static data for a particular node would overwrite
  * the static data for a node in the view above it with the same index (since it's in the
  * same template).

Is this still applicable ?

Copy link
Contributor

@kara kara Apr 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanics are correct, but tData doesn't exist anymore, so we should update to reflect that.

ngDevMode && assertNodeType(parent, LNodeType.Container);
const tContainer = (parent !.tNode as TContainerNode).data;
if (viewIndex >= tContainer.length || tContainer[viewIndex] == null) {
const containerTViews = (parent !.tNode as TContainerNode).tViews as TView[];
ngDevMode && assertNotNull(containerTViews, 'TView expected');
Copy link
Contributor

@vicb vicb Apr 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert is array ?

ngDevMode && assertEqual(Array.isArray(containerTViews), true, 'TViews should be in an array');
if (viewIndex >= containerTViews.length || containerTViews[viewIndex] == null) {
const tView = currentView.tView;
tContainer[viewIndex] = createTView(tView.directiveRegistry, tView.pipeRegistry);
containerTViews[viewIndex] = createTView(tView.directiveRegistry, tView.pipeRegistry);
}
return tContainer[viewIndex];
return containerTViews[viewIndex];
}

/** Marks the end of an embedded view. */
Expand Down
Loading