From 86b13ccf80010474c98bb72341d638e320d1fe7e Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Tue, 5 Jun 2018 15:28:15 -0700 Subject: [PATCH] refactor(ivy): move static parts of LView.cleanup to TView (#24301) PR Close #24301 --- packages/core/src/render3/instructions.ts | 76 +++++--- packages/core/src/render3/interfaces/view.ts | 43 ++-- .../core/src/render3/node_manipulation.ts | 42 ++-- packages/core/src/render3/query.ts | 4 +- packages/core/src/render3/view_ref.ts | 6 +- .../bundling/todo/bundle.golden_symbols.json | 9 + packages/core/test/render3/listeners_spec.ts | 183 +++++++++++++++++- packages/core/test/render3/render_util.ts | 5 +- 8 files changed, 302 insertions(+), 66 deletions(-) diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index d485c11202..a004f10d81 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -154,30 +154,14 @@ let data: any[]; */ let directives: any[]|null; -/** - * When a view is destroyed, listeners need to be released and outputs need to be - * unsubscribed. This cleanup array stores both listener data (in chunks of 4) - * and output data (in chunks of 2) for a particular view. Combining the arrays - * saves on memory (70 bytes per array) and on a few bytes of code size (for two - * separate for loops). - * - * If it's a listener being stored: - * 1st index is: event name to remove - * 2nd index is: native element - * 3rd index is: listener function - * 4th index is: useCapture boolean - * - * If it's an output subscription: - * 1st index is: unsubscribe function - * 2nd index is: context for function - */ -let cleanup: any[]|null; - -export function getCleanup(): any[] { +function getCleanup(view: LView): any[] { // top level variables should not be exported for performance reasons (PERF_NOTES.md) - return cleanup || (cleanup = currentView.cleanup = []); + return view.cleanupInstances || (view.cleanupInstances = []); } +function getTViewCleanup(view: LView): any[] { + return view.tView.cleanup || (view.tView.cleanup = []); +} /** * In this mode, any changes in bindings will throw an ExpressionChangedAfterChecked error. * @@ -213,7 +197,6 @@ export function enterView(newView: LView, host: LElementNode | LViewNode | null) creationMode = newView && (newView.flags & LViewFlags.CreationMode) === LViewFlags.CreationMode; firstTemplatePass = newView && newView.tView.firstTemplatePass; - cleanup = newView && newView.cleanup; renderer = newView && newView.renderer; if (host != null) { @@ -312,7 +295,7 @@ export function createLView( data: [], directives: null, tView: tView, - cleanup: null, + cleanupInstances: null, renderer: renderer, tail: null, next: null, @@ -818,6 +801,7 @@ export function createTView( viewCheckHooks: null, destroyHooks: null, pipeDestroyHooks: null, + cleanup: null, hostBindings: null, components: null, directiveRegistry: typeof directives === 'function' ? directives() : directives, @@ -915,19 +899,23 @@ export function listener( ngDevMode && assertPreviousIsParent(); const node = previousOrParentNode; const native = node.native as RElement; + ngDevMode && ngDevMode.rendererAddEventListener++; // In order to match current behavior, native DOM event listeners must be added for all // events (including outputs). - const cleanupFns = getCleanup(); - ngDevMode && ngDevMode.rendererAddEventListener++; if (isProceduralRenderer(renderer)) { const wrappedListener = wrapListenerWithDirtyLogic(currentView, listenerFn); const cleanupFn = renderer.listen(native, eventName, wrappedListener); - cleanupFns.push(cleanupFn, null); + storeCleanupFn(currentView, cleanupFn); } else { const wrappedListener = wrapListenerWithDirtyAndDefault(currentView, listenerFn); native.addEventListener(eventName, wrappedListener, useCapture); - cleanupFns.push(eventName, native, wrappedListener, useCapture); + const cleanupInstances = getCleanup(currentView); + cleanupInstances.push(wrappedListener); + if (firstTemplatePass) { + getTViewCleanup(currentView) + .push(eventName, node.tNode.index, cleanupInstances !.length - 1, useCapture); + } } let tNode: TNode|null = node.tNode; @@ -952,7 +940,39 @@ function createOutput(outputs: PropertyAliasValue, listener: Function): void { for (let i = 0; i < outputs.length; i += 2) { ngDevMode && assertDataInRange(outputs[i] as number, directives !); const subscription = directives ![outputs[i] as number][outputs[i + 1]].subscribe(listener); - getCleanup().push(subscription.unsubscribe, subscription); + storeCleanupWithContext(currentView, subscription, subscription.unsubscribe); + } +} + +/** + * Saves context for this cleanup function in LView.cleanupInstances. + * + * On the first template pass, saves in TView: + * - Cleanup function + * - Index of context we just saved in LView.cleanupInstances + */ +export function storeCleanupWithContext( + view: LView = currentView, context: any, cleanupFn: Function): void { + getCleanup(view).push(context); + + if (view.tView.firstTemplatePass) { + getTViewCleanup(view).push(cleanupFn, view.cleanupInstances !.length - 1); + } +} + +/** + * Saves the cleanup function itself in LView.cleanupInstances. + * + * This is necessary for functions that are wrapped with their contexts, like in renderer2 + * listeners. + * + * On the first template pass, the index of the cleanup function is saved in TView. + */ +export function storeCleanupFn(view: LView, cleanupFn: Function): void { + getCleanup(view).push(cleanupFn); + + if (view.tView.firstTemplatePass) { + getTViewCleanup(view).push(view.cleanupInstances !.length - 1, null); } } diff --git a/packages/core/src/render3/interfaces/view.ts b/packages/core/src/render3/interfaces/view.ts index fa672663fc..c069e6f0ed 100644 --- a/packages/core/src/render3/interfaces/view.ts +++ b/packages/core/src/render3/interfaces/view.ts @@ -63,22 +63,14 @@ export interface LView { /** * When a view is destroyed, listeners need to be released and outputs need to be - * unsubscribed. This cleanup array stores both listener data (in chunks of 4) - * and output data (in chunks of 2) for a particular view. Combining the arrays - * saves on memory (70 bytes per array) and on a few bytes of code size (for two - * separate for loops). + * unsubscribed. This context array stores both listener functions wrapped with + * their context and output subscription instances for a particular view. * - * If it's a listener being stored: - * 1st index is: event name to remove - * 2nd index is: native element - * 3rd index is: listener function - * 4th index is: useCapture boolean - * - * If it's an output subscription: - * 1st index is: unsubscribe function - * 2nd index is: context for function + * These change per LView instance, so they cannot be stored on TView. Instead, + * TView.cleanup saves an index to the necessary context in this array. */ - cleanup: any[]|null; + // TODO: collapse into data[] + cleanupInstances: any[]|null; /** * The last LView or LContainer beneath this LView in the hierarchy. @@ -368,6 +360,29 @@ export interface TView { */ pipeDestroyHooks: HookData|null; + /** + * When a view is destroyed, listeners need to be released and outputs need to be + * unsubscribed. This cleanup array stores both listener data (in chunks of 4) + * and output data (in chunks of 2) for a particular view. Combining the arrays + * saves on memory (70 bytes per array) and on a few bytes of code size (for two + * separate for loops). + * + * If it's a native DOM listener being stored: + * 1st index is: event name to remove + * 2nd index is: index of native element in LView.data[] + * 3rd index is: index of wrapped listener function in LView.cleanupInstances[] + * 4th index is: useCapture boolean + * + * If it's a renderer2 style listener or ViewRef destroy hook being stored: + * 1st index is: index of the cleanup function in LView.cleanupInstances[] + * 2nd index is: null + * + * If it's an output subscription or query list destroy hook: + * 1st index is: output unsubscribe function / query list destroy function + * 2nd index is: index of function context in LView.cleanupInstances[] + */ + cleanup: any[]|null; + /** * A list of directive and element indices for child components that will need to be * refreshed when the current view has finished its check. diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 257c6e97b8..db4eb95d50 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -297,7 +297,7 @@ export function destroyViewTree(rootView: LView): void { } else if (viewOrContainer.next) { // Only move to the side and clean if operating below rootView - // otherwise we would start cleaning up sibling views of the rootView. - cleanUpView(viewOrContainer as LView); + cleanUpView(viewOrContainer); next = viewOrContainer.next; } @@ -306,10 +306,10 @@ export function destroyViewTree(rootView: LView): void { // with a root view that doesn't have children. We didn't descend into child views // so no need to go back up the views tree. while (viewOrContainer && !viewOrContainer !.next && viewOrContainer !== rootView) { - cleanUpView(viewOrContainer as LView); + cleanUpView(viewOrContainer); viewOrContainer = getParentState(viewOrContainer, rootView); } - cleanUpView(viewOrContainer as LView || rootView); + cleanUpView(viewOrContainer || rootView); next = viewOrContainer && viewOrContainer.next; } @@ -472,30 +472,42 @@ export function getParentState(state: LViewOrLContainer, rootView: LView): LView * * @param view The LView to clean up */ -function cleanUpView(view: LView): void { - removeListeners(view); - executeOnDestroys(view); - executePipeOnDestroys(view); - // For component views only, the local renderer is destroyed as clean up time. - if (view.tView && view.tView.id === -1 && isProceduralRenderer(view.renderer)) { - ngDevMode && ngDevMode.rendererDestroy++; - view.renderer.destroy(); +function cleanUpView(viewOrContainer: LViewOrLContainer): void { + if ((viewOrContainer as LView).tView) { + const view = viewOrContainer as LView; + removeListeners(view); + executeOnDestroys(view); + executePipeOnDestroys(view); + // For component views only, the local renderer is destroyed as clean up time. + if (view.tView.id === -1 && isProceduralRenderer(view.renderer)) { + ngDevMode && ngDevMode.rendererDestroy++; + view.renderer.destroy(); + } } } /** Removes listeners and unsubscribes from output subscriptions */ function removeListeners(view: LView): void { - const cleanup = view.cleanup !; + const cleanup = view.tView.cleanup !; if (cleanup != null) { for (let i = 0; i < cleanup.length - 1; i += 2) { if (typeof cleanup[i] === 'string') { - cleanup ![i + 1].removeEventListener(cleanup[i], cleanup[i + 2], cleanup[i + 3]); + // This is a listener with the native renderer + const native = view.data[cleanup[i + 1]].native; + const listener = view.cleanupInstances ![cleanup[i + 2]]; + native.removeEventListener(cleanup[i], listener, cleanup[i + 3]); i += 2; + } else if (typeof cleanup[i] === 'number') { + // This is a listener with renderer2 (cleanup fn can be found by index) + const cleanupFn = view.cleanupInstances ![cleanup[i]]; + cleanupFn(); } else { - cleanup[i].call(cleanup[i + 1]); + // This is a cleanup function that is grouped with the index of its context + const context = view.cleanupInstances ![cleanup[i + 1]]; + cleanup[i].call(context); } } - view.cleanup = null; + view.cleanupInstances = null; } } diff --git a/packages/core/src/render3/query.ts b/packages/core/src/render3/query.ts index 7e3f029848..fd6a667a01 100644 --- a/packages/core/src/render3/query.ts +++ b/packages/core/src/render3/query.ts @@ -17,7 +17,7 @@ import {getSymbolIterator} from '../util'; import {assertEqual, assertNotNull} from './assert'; import {ReadFromInjectorFn, getOrCreateNodeInjectorForNode} from './di'; -import {assertPreviousIsParent, getCleanup, getCurrentQueries, store} from './instructions'; +import {assertPreviousIsParent, getCurrentQueries, store, storeCleanupWithContext} from './instructions'; import {DirectiveDef, unusedValueExportToPlacateAjd as unused1} from './interfaces/definition'; import {LInjector, unusedValueExportToPlacateAjd as unused2} from './interfaces/injector'; import {LContainerNode, LElementNode, LNode, TNode, TNodeFlags, unusedValueExportToPlacateAjd as unused3} from './interfaces/node'; @@ -416,7 +416,7 @@ export function query( const queryList = new QueryList(); const queries = getCurrentQueries(LQueries_); queries.track(queryList, predicate, descend, read); - getCleanup().push(queryList.destroy, queryList); + storeCleanupWithContext(undefined, queryList, queryList.destroy); if (memoryIndex != null) { store(memoryIndex, queryList); } diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index 4d912ffc55..060f2c997b 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -9,7 +9,7 @@ import {ViewContainerRef as viewEngine_ViewContainerRef} from '../linker/view_container_ref'; import {EmbeddedViewRef as viewEngine_EmbeddedViewRef} from '../linker/view_ref'; -import {checkNoChanges, detectChanges, markViewDirty} from './instructions'; +import {checkNoChanges, detectChanges, markViewDirty, storeCleanupFn} from './instructions'; import {ComponentTemplate} from './interfaces/definition'; import {LViewNode} from './interfaces/node'; import {LView, LViewFlags} from './interfaces/view'; @@ -33,9 +33,7 @@ export class ViewRef implements viewEngine_EmbeddedViewRef { destroy(): void { destroyLView(this._view); } - onDestroy(callback: Function) { - (this._view.cleanup || (this._view.cleanup = [])).push(callback, null); - } + onDestroy(callback: Function) { storeCleanupFn(this._view, callback); } /** * Marks a view and all of its ancestors dirty. diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 0d19433be3..64f8a6e020 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -443,6 +443,9 @@ { "name": "getSymbolIterator" }, + { + "name": "getTViewCleanup" + }, { "name": "getTypeNameForDebugging" }, @@ -626,6 +629,12 @@ { "name": "setUpAttributes" }, + { + "name": "storeCleanupFn" + }, + { + "name": "storeCleanupWithContext" + }, { "name": "stringify" }, diff --git a/packages/core/test/render3/listeners_spec.ts b/packages/core/test/render3/listeners_spec.ts index d3a6cc9c00..f9aa7042ec 100644 --- a/packages/core/test/render3/listeners_spec.ts +++ b/packages/core/test/render3/listeners_spec.ts @@ -10,7 +10,7 @@ import {defineComponent, defineDirective} from '../../src/render3/index'; import {container, containerRefreshEnd, containerRefreshStart, elementEnd, elementStart, embeddedViewEnd, embeddedViewStart, listener, text} from '../../src/render3/instructions'; import {RenderFlags} from '../../src/render3/interfaces/definition'; import {getRendererFactory2} from './imported_renderer2'; -import {containerEl, renderComponent, renderToHtml} from './render_util'; +import {ComponentFixture, containerEl, renderComponent, renderToHtml} from './render_util'; describe('event listeners', () => { @@ -228,6 +228,187 @@ describe('event listeners', () => { expect(comp.counter).toEqual(2); }); + it('should destroy listeners in views with renderer2', () => { + + /** + * % if (ctx.showing) { + * + * % } + */ + class AppComp { + counter = 0; + showing = true; + + onClick() { this.counter++; } + + static ngComponentDef = defineComponent({ + type: AppComp, + selectors: [['app-comp']], + factory: () => new AppComp(), + template: function(rf: RenderFlags, ctx: any) { + if (rf & RenderFlags.Create) { + container(0); + } + if (rf & RenderFlags.Update) { + containerRefreshStart(0); + { + if (ctx.showing) { + if (embeddedViewStart(0)) { + elementStart(0, 'button'); + { + listener('click', function() { return ctx.onClick(); }); + text(1, 'Click me'); + } + elementEnd(); + } + embeddedViewEnd(); + } + } + containerRefreshEnd(); + } + } + }); + } + + const fixture = new ComponentFixture(AppComp, {rendererFactory: getRendererFactory2(document)}); + const comp = fixture.component; + const button = fixture.hostElement.querySelector('button') !; + + button.click(); + expect(comp.counter).toEqual(1); + + button.click(); + expect(comp.counter).toEqual(2); + + // the listener should be removed when the view is removed + comp.showing = false; + fixture.update(); + button.click(); + expect(comp.counter).toEqual(2); + }); + + it('should destroy listeners in for loops', () => { + + /** + * % for (let i = 0; i < ctx.buttons; i++) { + * + * % } + */ + class AppComp { + buttons = 2; + counters = [0, 0]; + + onClick(index: number) { this.counters[index]++; } + + static ngComponentDef = defineComponent({ + type: AppComp, + selectors: [['app-comp']], + factory: () => new AppComp(), + template: function(rf: RenderFlags, ctx: any) { + if (rf & RenderFlags.Create) { + container(0); + } + if (rf & RenderFlags.Update) { + containerRefreshStart(0); + { + for (let i = 0; i < ctx.buttons; i++) { + if (embeddedViewStart(0)) { + elementStart(0, 'button'); + { + listener('click', function() { return ctx.onClick(i); }); + text(1, 'Click me'); + } + elementEnd(); + } + embeddedViewEnd(); + } + } + containerRefreshEnd(); + } + } + }); + } + + const fixture = new ComponentFixture(AppComp); + const comp = fixture.component; + const buttons = fixture.hostElement.querySelectorAll('button') !; + + buttons[0].click(); + expect(comp.counters).toEqual([1, 0]); + + buttons[1].click(); + expect(comp.counters).toEqual([1, 1]); + + // the listener should be removed when the view is removed + comp.buttons = 0; + fixture.update(); + + buttons[0].click(); + buttons[1].click(); + expect(comp.counters).toEqual([1, 1]); + }); + + it('should destroy listeners in for loops with renderer2', () => { + + /** + * % for (let i = 0; i < ctx.buttons; i++) { + * + * % } + */ + class AppComp { + buttons = 2; + counters = [0, 0]; + + onClick(index: number) { this.counters[index]++; } + + static ngComponentDef = defineComponent({ + type: AppComp, + selectors: [['app-comp']], + factory: () => new AppComp(), + template: function(rf: RenderFlags, ctx: any) { + if (rf & RenderFlags.Create) { + container(0); + } + if (rf & RenderFlags.Update) { + containerRefreshStart(0); + { + for (let i = 0; i < ctx.buttons; i++) { + if (embeddedViewStart(1)) { + elementStart(0, 'button'); + { + listener('click', function() { return ctx.onClick(i); }); + text(1, 'Click me'); + } + elementEnd(); + } + embeddedViewEnd(); + } + } + containerRefreshEnd(); + } + } + }); + } + + const fixture = new ComponentFixture(AppComp, {rendererFactory: getRendererFactory2(document)}); + const comp = fixture.component; + const buttons = fixture.hostElement.querySelectorAll('button') !; + + buttons[0].click(); + expect(comp.counters).toEqual([1, 0]); + + buttons[1].click(); + expect(comp.counters).toEqual([1, 1]); + + // the listener should be removed when the view is removed + comp.buttons = 0; + fixture.update(); + + buttons[0].click(); + buttons[1].click(); + expect(comp.counters).toEqual([1, 1]); + }); + it('should support host listeners', () => { let events: string[] = []; diff --git a/packages/core/test/render3/render_util.ts b/packages/core/test/render3/render_util.ts index 2486905eae..cb4bf297ad 100644 --- a/packages/core/test/render3/render_util.ts +++ b/packages/core/test/render3/render_util.ts @@ -103,7 +103,7 @@ export class ComponentFixture extends BaseFixture { constructor( private componentType: ComponentType, - opts: {injector?: Injector, sanitizer?: Sanitizer} = {}) { + opts: {injector?: Injector, sanitizer?: Sanitizer, rendererFactory?: RendererFactory3} = {}) { super(); this.requestAnimationFrame = function(fn: () => void) { requestAnimationFrame.queue.push(fn); @@ -119,7 +119,8 @@ export class ComponentFixture extends BaseFixture { host: this.hostElement, scheduler: this.requestAnimationFrame, injector: opts.injector, - sanitizer: opts.sanitizer + sanitizer: opts.sanitizer, + rendererFactory: opts.rendererFactory || domRendererFactory3 }); }