Skip to content

Commit

Permalink
refactor(ivy): remove ngPrivateData megamorphic prop access (angular#…
Browse files Browse the repository at this point in the history
  • Loading branch information
mhevery authored and jasonaden committed May 20, 2019
1 parent 28ae22e commit 6454f76
Showing 10 changed files with 50 additions and 86 deletions.
7 changes: 2 additions & 5 deletions packages/core/src/render3/component.ts
Original file line number Diff line number Diff line change
@@ -174,11 +174,8 @@ export function createRootComponentView(
const tView = rootView[TVIEW];
const tNode: TElementNode = createNodeAtIndex(0, TNodeType.Element, rNode, null, null);
const componentView = createLView(
rootView, getOrCreateTView(
def.template, def.consts, def.vars, def.directiveDefs, def.pipeDefs,
def.viewQuery, def.schemas),
null, def.onPush ? LViewFlags.Dirty : LViewFlags.CheckAlways, rootView[HEADER_OFFSET], tNode,
rendererFactory, renderer, sanitizer);
rootView, getOrCreateTView(def), null, def.onPush ? LViewFlags.Dirty : LViewFlags.CheckAlways,
rootView[HEADER_OFFSET], tNode, rendererFactory, renderer, sanitizer);

if (tView.firstTemplatePass) {
diPublicInInjector(getOrCreateNodeInjectorForNode(tNode, rootView), rootView, def.type);
1 change: 1 addition & 0 deletions packages/core/src/render3/definition.ts
Original file line number Diff line number Diff line change
@@ -285,6 +285,7 @@ export function ΔdefineComponent<T>(componentDefinition: {
_: null as never,
setInput: null,
schemas: componentDefinition.schemas || null,
tView: null,
};
def._ = noSideEffects(() => {
const directiveTypes = componentDefinition.directives !;
30 changes: 6 additions & 24 deletions packages/core/src/render3/instructions/shared.ts
Original file line number Diff line number Diff line change
@@ -548,29 +548,13 @@ function saveResolvedLocalsInData(
* Gets TView from a template function or creates a new TView
* if it doesn't already exist.
*
* @param templateFn The template from which to get static data
* @param consts The number of nodes, local refs, and pipes in this view
* @param vars The number of bindings and pure function bindings in this view
* @param directives Directive defs that should be saved on TView
* @param pipes Pipe defs that should be saved on TView
* @param viewQuery View query that should be saved on TView
* @param schemas Schemas that should be saved on TView
* @param def ComponentDef
* @returns TView
*/
export function getOrCreateTView(
templateFn: ComponentTemplate<any>, consts: number, vars: number,
directives: DirectiveDefListOrFactory | null, pipes: PipeDefListOrFactory | null,
viewQuery: ViewQueriesFunction<any>| null, schemas: SchemaMetadata[] | 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 templateFn.ngPrivateData ||
(templateFn.ngPrivateData = createTView(
-1, templateFn, consts, vars, directives, pipes, viewQuery, schemas) as never);
export function getOrCreateTView(def: ComponentDef<any>): TView {
return def.tView || (def.tView = createTView(
-1, def.template, def.consts, def.vars, def.directiveDefs, def.pipeDefs,
def.viewQuery, def.schemas));
}

/**
@@ -1262,9 +1246,7 @@ function addComponentLogic<T>(
lView: LView, previousOrParentTNode: TNode, def: ComponentDef<T>): void {
const native = getNativeByTNode(previousOrParentTNode, lView);

const tView = getOrCreateTView(
def.template, def.consts, def.vars, def.directiveDefs, def.pipeDefs, def.viewQuery,
def.schemas);
const tView = getOrCreateTView(def);

// Only component views should be added to the view tree directly. Embedded views are
// accessed through their containers because they may be removed / re-added later.
9 changes: 8 additions & 1 deletion packages/core/src/render3/interfaces/definition.ts
Original file line number Diff line number Diff line change
@@ -10,6 +10,7 @@ import {SchemaMetadata, ViewEncapsulation} from '../../core';
import {ProcessProvidersFunction} from '../../di/interface/provider';
import {Type} from '../../interface/type';
import {CssSelectorList} from './projection';
import {TView} from './view';


/**
@@ -19,7 +20,7 @@ export type ComponentTemplate<T> = {
// Note: the ctx parameter is typed as T|U, as using only U would prevent a template with
// e.g. ctx: {} from being assigned to ComponentTemplate<any> as TypeScript won't infer U = any
// in that scenario. By including T this incompatibility is resolved.
<U extends T>(rf: RenderFlags, ctx: T | U): void; ngPrivateData?: never;
<U extends T>(rf: RenderFlags, ctx: T | U): void;
};

/**
@@ -302,6 +303,12 @@ export interface ComponentDef<T> extends DirectiveDef<T> {
*/
schemas: SchemaMetadata[]|null;

/**
* Ivy runtime uses this place to store the computed tView for the component. This gets filled on
* the first run of component.
*/
tView: TView|null;

/**
* Used to store the result of `noSideEffects` function so that it is not removed by closure
* compiler. The property should never be read.
2 changes: 1 addition & 1 deletion packages/core/src/render3/interfaces/view.ts
Original file line number Diff line number Diff line change
@@ -327,7 +327,7 @@ export interface ExpandoInstructions extends Array<number|HostBindingsFunction<a
* The static data for an LView (shared between all templates of a
* given type).
*
* Stored on the template function as ngPrivateData.
* Stored on the `ComponentDef.tView`.
*/
export interface TView {
/**
2 changes: 1 addition & 1 deletion packages/core/src/render3/jit/module.ts
Original file line number Diff line number Diff line change
@@ -361,7 +361,7 @@ export function patchComponentDefWithScope<C>(
// may face a problem where previously compiled defs available to a given Component/Directive
// are cached in TView and may become stale (in case any of these defs gets recompiled). In
// order to avoid this problem, we force fresh TView to be created.
componentDef.template.ngPrivateData = undefined;
componentDef.tView = null;
}

/**
17 changes: 17 additions & 0 deletions packages/core/test/acceptance/integration_spec.ts
Original file line number Diff line number Diff line change
@@ -7,6 +7,7 @@
*/
import {CommonModule} from '@angular/common';
import {Component, ContentChild, Directive, ElementRef, EventEmitter, HostBinding, HostListener, Input, OnInit, Output, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core';
import {ngDevModeResetPerfCounters} from '@angular/core/src/util/ng_dev_mode';
import {TestBed} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
import {expect} from '@angular/platform-browser/testing/src/matchers';
@@ -29,23 +30,39 @@ describe('acceptance integration tests', () => {
});

it('should render and update basic "Hello, World" template', () => {
ngDevModeResetPerfCounters();
@Component({template: '<h1>Hello, {{name}}!</h1>'})
class App {
name = '';
}

onlyInIvy('perf counters').expectPerfCounters({
tView: 0,
tNode: 0,
});

TestBed.configureTestingModule({declarations: [App]});
const fixture = TestBed.createComponent(App);

fixture.componentInstance.name = 'World';
fixture.detectChanges();

expect(fixture.nativeElement.innerHTML).toEqual('<h1>Hello, World!</h1>');
onlyInIvy('perf counters').expectPerfCounters({
tView: 2, // Host view + App
tNode: 4, // Host Node + App Node + <span> + #text
});

fixture.componentInstance.name = 'New World';
fixture.detectChanges();

expect(fixture.nativeElement.innerHTML).toEqual('<h1>Hello, New World!</h1>');
// Assert that the tView/tNode count does not increase (they are correctly cached)
onlyInIvy('perf counters').expectPerfCounters({
tView: 2,
tNode: 4,
});

});
});

2 changes: 1 addition & 1 deletion packages/core/test/linker/ng_module_integration_spec.ts
Original file line number Diff line number Diff line change
@@ -142,7 +142,7 @@ function declareTests(config?: {useJit: boolean}) {
// may face a problem where previously compiled defs available to a given
// Component/Directive are cached in TView and may become stale (in case any of these defs
// gets recompiled). In order to avoid this problem, we force fresh TView to be created.
componentDef.template.ngPrivateData = null;
componentDef.TView = null;
}

const ngModule = createModule(moduleType, injector);
50 changes: 0 additions & 50 deletions packages/core/test/render3/integration_spec.ts
Original file line number Diff line number Diff line change
@@ -196,56 +196,6 @@ describe('render3 integration test', () => {

});

describe('template data', () => {

it('should re-use template data and node data', () => {
/**
* % if (condition) {
* <div></div>
* % }
*/
function Template(rf: RenderFlags, ctx: any) {
if (rf & RenderFlags.Create) {
Δcontainer(0);
}
if (rf & RenderFlags.Update) {
ΔcontainerRefreshStart(0);
{
if (ctx.condition) {
let rf1 = ΔembeddedViewStart(0, 1, 0);
if (rf1 & RenderFlags.Create) {
Δelement(0, 'div');
}
ΔembeddedViewEnd();
}
}
ΔcontainerRefreshEnd();
}
}

expect((Template as any).ngPrivateData).toBeUndefined();

renderToHtml(Template, {condition: true}, 1);

const oldTemplateData = (Template as any).ngPrivateData;
const oldContainerData = (oldTemplateData as any).data[HEADER_OFFSET];
const oldElementData = oldContainerData.tViews[0][HEADER_OFFSET];
expect(oldContainerData).not.toBeNull();
expect(oldElementData).not.toBeNull();

renderToHtml(Template, {condition: false}, 1);
renderToHtml(Template, {condition: true}, 1);

const newTemplateData = (Template as any).ngPrivateData;
const newContainerData = (oldTemplateData as any).data[HEADER_OFFSET];
const newElementData = oldContainerData.tViews[0][HEADER_OFFSET];
expect(newTemplateData === oldTemplateData).toBe(true);
expect(newContainerData === oldContainerData).toBe(true);
expect(newElementData === oldElementData).toBe(true);
});

});

describe('component styles', () => {
it('should pass in the component styles directly into the underlying renderer', () => {
class StyledComp {
16 changes: 13 additions & 3 deletions packages/core/test/render3/render_util.ts
Original file line number Diff line number Diff line change
@@ -28,7 +28,7 @@ import {CreateComponentOptions} from '../../src/render3/component';
import {getDirectivesAtNodeIndex, getLContext, isComponentInstance} from '../../src/render3/context_discovery';
import {extractDirectiveDef, extractPipeDef} from '../../src/render3/definition';
import {NG_ELEMENT_ID} from '../../src/render3/fields';
import {ComponentTemplate, ComponentType, DirectiveDef, DirectiveType, RenderFlags, renderComponent as _renderComponent, tick, ΔProvidersFeature, ΔdefineComponent, ΔdefineDirective} from '../../src/render3/index';
import {ComponentDef, ComponentTemplate, ComponentType, DirectiveDef, DirectiveType, RenderFlags, renderComponent as _renderComponent, tick, ΔProvidersFeature, ΔdefineComponent, ΔdefineDirective} from '../../src/render3/index';
import {DirectiveDefList, DirectiveDefListOrFactory, DirectiveTypesOrFactory, HostBindingsFunction, PipeDef, PipeDefList, PipeDefListOrFactory, PipeTypesOrFactory} from '../../src/render3/interfaces/definition';
import {PlayerHandler} from '../../src/render3/interfaces/player';
import {ProceduralRenderer3, RComment, RElement, RNode, RText, Renderer3, RendererFactory3, RendererStyleFlags3, domRendererFactory3} from '../../src/render3/interfaces/renderer';
@@ -258,8 +258,18 @@ export function renderTemplate<T>(
LViewFlags.CheckAlways | LViewFlags.IsRoot, null, null, providedRendererFactory, renderer);
enterView(hostLView, null); // SUSPECT! why do we need to enter the View?

const componentTView =
getOrCreateTView(templateFn, consts, vars, directives || null, pipes || null, null, null);
const def: ComponentDef<any> = ΔdefineComponent({
factory: () => null,
selectors: [],
type: Object,
template: templateFn,
consts: consts,
vars: vars,
});
def.directiveDefs = directives || null;
def.pipeDefs = pipes || null;

const componentTView = getOrCreateTView(def);
const hostTNode = createNodeAtIndex(0, TNodeType.Element, hostNode, null, null);
componentView = createLView(
hostLView, componentTView, context, LViewFlags.CheckAlways, hostNode, hostTNode,

0 comments on commit 6454f76

Please sign in to comment.