From 4191344cb4c7b8c38b444cdbb02e5921e3b83311 Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Mon, 8 Apr 2019 14:29:48 +0200 Subject: [PATCH] perf(ivy): coalesce handlers for events with the same name on the same element (#29786) PR Close #29786 --- .../core/src/render3/instructions/listener.ts | 120 +++++++++++++---- .../core/test/acceptance/listener_spec.ts | 123 ++++++++++++++++++ .../bundling/todo/bundle.golden_symbols.json | 6 + 3 files changed, 223 insertions(+), 26 deletions(-) create mode 100644 packages/core/test/acceptance/listener_spec.ts diff --git a/packages/core/src/render3/instructions/listener.ts b/packages/core/src/render3/instructions/listener.ts index fbc15856aa..4cd9fe1a3c 100644 --- a/packages/core/src/render3/instructions/listener.ts +++ b/packages/core/src/render3/instructions/listener.ts @@ -11,7 +11,7 @@ import {assertDataInRange} from '../../util/assert'; import {isObservable} from '../../util/lang'; import {PropertyAliasValue, TNode, TNodeFlags, TNodeType} from '../interfaces/node'; import {GlobalTargetResolver, RElement, Renderer3, isProceduralRenderer} from '../interfaces/renderer'; -import {FLAGS, LView, LViewFlags, RENDERER, TVIEW} from '../interfaces/view'; +import {CLEANUP, FLAGS, LView, LViewFlags, RENDERER, TVIEW} from '../interfaces/view'; import {assertNodeOfPossibleTypes} from '../node_assert'; import {getLView, getPreviousOrParentTNode} from '../state'; import {getComponentViewByIndex, getNativeByTNode, unwrapRNode} from '../util/view_utils'; @@ -64,6 +64,36 @@ export function ΔcomponentHostSyntheticListener( listenerInternal(eventName, listenerFn, useCapture, eventTargetResolver, loadComponentRenderer); } +/** + * A utility function that checks if a given element has already an event handler registered for an + * event with a specified name. The TView.cleanup data structure is used to find out which events + * are registered for a given element. + */ +function findExistingListener( + lView: LView, eventName: string, tNodeIdx: number): ((e?: any) => any)|null { + const tView = lView[TVIEW]; + const tCleanup = tView.cleanup; + if (tCleanup != null) { + for (let i = 0; i < tCleanup.length - 1; i += 2) { + if (tCleanup[i] === eventName && tCleanup[i + 1] === tNodeIdx) { + // We have found a matching event name on the same node but it might not have been + // registered yet, so we must explicitly verify entries in the LView cleanup data + // structures. + const lCleanup = lView[CLEANUP] !; + const listenerIdxInLCleanup = tCleanup[i + 2]; + return lCleanup.length > listenerIdxInLCleanup ? lCleanup[listenerIdxInLCleanup] : null; + } + // TView.cleanup can have a mix of 4-elements entries (for event handler cleanups) or + // 2-element entries (for directive and queries destroy hooks). As such we can encounter + // blocks of 4 or 2 items in the tView.cleanup and this is why we iterate over 2 elements + // first and jump another 2 elements if we detect listeners cleanup (4 elements). Also check + // documentation of TView.cleanup for more details of this data structure layout. + i += 2; + } + } + return null; +} + function listenerInternal( eventName: string, listenerFn: (e?: any) => any, useCapture = false, eventTargetResolver?: GlobalTargetResolver, @@ -82,32 +112,56 @@ function listenerInternal( const native = getNativeByTNode(tNode, lView) as RElement; const resolved = eventTargetResolver ? eventTargetResolver(native) : {} as any; const target = resolved.target || native; - ngDevMode && ngDevMode.rendererAddEventListener++; const renderer = loadRendererFn ? loadRendererFn(tNode, lView) : lView[RENDERER]; const lCleanup = getCleanup(lView); const lCleanupIndex = lCleanup.length; - let useCaptureOrSubIdx: boolean|number = useCapture; + const idxOrTargetGetter = eventTargetResolver ? + (_lView: LView) => eventTargetResolver(unwrapRNode(_lView[tNode.index])).target : + tNode.index; // In order to match current behavior, native DOM event listeners must be added for all // events (including outputs). if (isProceduralRenderer(renderer)) { - // The first argument of `listen` function in Procedural Renderer is: - // - either a target name (as a string) in case of global target (window, document, body) - // - or element reference (in all other cases) - listenerFn = wrapListener(tNode, lView, listenerFn, false /** preventDefault */); - const cleanupFn = renderer.listen(resolved.name || target, eventName, listenerFn); - lCleanup.push(listenerFn, cleanupFn); - useCaptureOrSubIdx = lCleanupIndex + 1; + // There might be cases where multiple directives on the same element try to register an event + // handler function for the same event. In this situation we want to avoid registration of + // several native listeners as each registration would be intercepted by NgZone and + // trigger change detection. This would mean that a single user action would result in several + // change detections being invoked. To avoid this situation we want to have only one call to + // native handler registration (for the same element and same type of event). + // + // In order to have just one native event handler in presence of multiple handler functions, + // we just register a first handler function as a native event listener and then chain + // (coalesce) other handler functions on top of the first native handler function. + // + // Please note that the coalescing described here doesn't happen for events specifying an + // alternative target (ex. (document:click)) - this is to keep backward compatibility with the + // view engine. + const existingListener = + eventTargetResolver ? null : findExistingListener(lView, eventName, tNode.index); + if (existingListener !== null) { + // Attach a new listener at the head of the coalesced listeners list. + (listenerFn).__ngNextListenerFn__ = (existingListener).__ngNextListenerFn__; + (existingListener).__ngNextListenerFn__ = listenerFn; + } else { + // The first argument of `listen` function in Procedural Renderer is: + // - either a target name (as a string) in case of global target (window, document, body) + // - or element reference (in all other cases) + listenerFn = wrapListener(tNode, lView, listenerFn, false /** preventDefault */); + const cleanupFn = renderer.listen(resolved.name || target, eventName, listenerFn); + ngDevMode && ngDevMode.rendererAddEventListener++; + + lCleanup.push(listenerFn, cleanupFn); + tCleanup && tCleanup.push(eventName, idxOrTargetGetter, lCleanupIndex, lCleanupIndex + 1); + } + } else { listenerFn = wrapListener(tNode, lView, listenerFn, true /** preventDefault */); target.addEventListener(eventName, listenerFn, useCapture); - lCleanup.push(listenerFn); - } + ngDevMode && ngDevMode.rendererAddEventListener++; - const idxOrTargetGetter = eventTargetResolver ? - (_lView: LView) => eventTargetResolver(unwrapRNode(_lView[tNode.index])).target : - tNode.index; - tCleanup && tCleanup.push(eventName, idxOrTargetGetter, lCleanupIndex, useCaptureOrSubIdx); + lCleanup.push(listenerFn); + tCleanup && tCleanup.push(eventName, idxOrTargetGetter, lCleanupIndex, useCapture); + } } // subscribe to directive outputs @@ -144,6 +198,15 @@ function listenerInternal( } } +function executeListenerWithErrorHandling(lView: LView, listenerFn: (e?: any) => any, e: any): any { + try { + return listenerFn(e); + } catch (error) { + handleError(lView, error); + return false; + } +} + /** * Wraps an event listener with a function that marks ancestors dirty and prevents default behavior, * if applicable. @@ -170,16 +233,21 @@ function wrapListener( markViewDirty(startView); } - try { - const result = listenerFn(e); - if (wrapWithPreventDefault && result === false) { - e.preventDefault(); - // Necessary for legacy browsers that don't support preventDefault (e.g. IE) - e.returnValue = false; - } - return result; - } catch (error) { - handleError(lView, error); + let result = executeListenerWithErrorHandling(lView, listenerFn, e); + // A just-invoked listener function might have coalesced listeners so we need to check for + // their presence and invoke as needed. + let nextListenerFn = (wrapListenerIn_markDirtyAndPreventDefault).__ngNextListenerFn__; + while (nextListenerFn) { + result = executeListenerWithErrorHandling(lView, nextListenerFn, e); + nextListenerFn = (nextListenerFn).__ngNextListenerFn__; } + + if (wrapWithPreventDefault && result === false) { + e.preventDefault(); + // Necessary for legacy browsers that don't support preventDefault (e.g. IE) + e.returnValue = false; + } + + return result; }; } diff --git a/packages/core/test/acceptance/listener_spec.ts b/packages/core/test/acceptance/listener_spec.ts new file mode 100644 index 0000000000..4ba782fcb2 --- /dev/null +++ b/packages/core/test/acceptance/listener_spec.ts @@ -0,0 +1,123 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Component, Directive, ErrorHandler, HostListener} from '@angular/core'; +import {TestBed} from '@angular/core/testing'; +import {By} from '@angular/platform-browser'; +import {onlyInIvy} from '@angular/private/testing'; + +function getNoOfNativeListeners(): number { + return ngDevMode ? ngDevMode.rendererAddEventListener : 0; +} + +describe('event listeners', () => { + + describe('coalescing', () => { + + @Component({ + selector: 'with-clicks-cmpt', + template: `` + }) + class WithClicksCmpt { + counter = 0; + count() { this.counter++; } + } + + @Directive({selector: '[md-button]'}) + class MdButton { + counter = 0; + @HostListener('click') + count() { this.counter++; } + } + + @Directive({selector: '[likes-clicks]'}) + class LikesClicks { + counter = 0; + @HostListener('click') + count() { this.counter++; } + } + + onlyInIvy('ngDevMode.rendererAddEventListener counters are only available in ivy') + .it('should coalesce multiple event listeners for the same event on the same element', + () => { + + @Component({ + selector: 'test-cmpt', + template: + `` + }) + class TestCmpt { + } + + TestBed.configureTestingModule( + {declarations: [TestCmpt, WithClicksCmpt, LikesClicks, MdButton]}); + const noOfEventListenersRegisteredSoFar = getNoOfNativeListeners(); + const fixture = TestBed.createComponent(TestCmpt); + fixture.detectChanges(); + const buttonDebugEls = fixture.debugElement.queryAll(By.css('button')); + const withClicksEls = fixture.debugElement.queryAll(By.css('with-clicks-cmpt')); + + // We want to assert that only one native event handler was registered but still all + // directives are notified when an event fires. This assertion can only be verified in + // the ngDevMode (but the coalescing always happens!). + ngDevMode && + expect(getNoOfNativeListeners()).toBe(noOfEventListenersRegisteredSoFar + 2); + + buttonDebugEls[0].nativeElement.click(); + expect(withClicksEls[0].injector.get(WithClicksCmpt).counter).toBe(1); + expect(buttonDebugEls[0].injector.get(LikesClicks).counter).toBe(1); + expect(buttonDebugEls[0].injector.get(MdButton).counter).toBe(1); + expect(withClicksEls[1].injector.get(WithClicksCmpt).counter).toBe(0); + expect(buttonDebugEls[1].injector.get(LikesClicks).counter).toBe(0); + expect(buttonDebugEls[1].injector.get(MdButton).counter).toBe(0); + + buttonDebugEls[1].nativeElement.click(); + expect(withClicksEls[0].injector.get(WithClicksCmpt).counter).toBe(1); + expect(buttonDebugEls[0].injector.get(LikesClicks).counter).toBe(1); + expect(buttonDebugEls[0].injector.get(MdButton).counter).toBe(1); + expect(withClicksEls[1].injector.get(WithClicksCmpt).counter).toBe(1); + expect(buttonDebugEls[1].injector.get(LikesClicks).counter).toBe(1); + expect(buttonDebugEls[1].injector.get(MdButton).counter).toBe(1); + }); + + + it('should try to execute remaining coalesced listeners if one of the listeners throws', () => { + + @Directive({selector: '[throws-on-clicks]'}) + class ThrowsOnClicks { + @HostListener('click') + dontCount() { throw new Error('I was clicked and I don\'t like it!'); } + } + + @Component( + {selector: 'test-cmpt', template: `