From b3bcc7a127fd40018b99f2c41e23578bb42f813b Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Wed, 29 Aug 2018 23:31:58 -0700 Subject: [PATCH 1/5] [BUGFIX] ES6 classes on/removeListener and observes/removeObserver interop v2 This is a rework of #16874 which flattens and caches the state of event listeners more efficiently. Rather than rebuild the result of a `matchListeners` query each time, including deduping, we flatten the listeners down the hierarchy of metas the first time an event match is requested. This still defers the majority of the work early on (adding listeners is cheap) but also prevents us from having to do the work again later. --- packages/@ember/-internals/meta/lib/meta.ts | 313 +++++++++++++----- .../@ember/-internals/meta/tests/meta_test.js | 55 ++- .../@ember/-internals/metal/lib/events.ts | 8 +- .../system/object/es-compatibility-test.js | 82 +++++ 4 files changed, 379 insertions(+), 79 deletions(-) diff --git a/packages/@ember/-internals/meta/lib/meta.ts b/packages/@ember/-internals/meta/lib/meta.ts index 7e930f82bd9..d62bcd02319 100644 --- a/packages/@ember/-internals/meta/lib/meta.ts +++ b/packages/@ember/-internals/meta/lib/meta.ts @@ -12,6 +12,11 @@ export interface MetaCounters { deleteCalls: number; metaCalls: number; metaInstantiated: number; + addToListenersCalls: number; + removeFromListenersCalls: number; + removeAllListenersCalls: number; + listenersInherited: number; + reopensAfterFlatten: number; } let counters: MetaCounters | undefined; @@ -23,6 +28,11 @@ if (DEBUG) { deleteCalls: 0, metaCalls: 0, metaInstantiated: 0, + addToListenersCalls: 0, + removeFromListenersCalls: 0, + removeAllListenersCalls: 0, + listenersInherited: 0, + reopensAfterFlatten: 0, }; } @@ -41,6 +51,38 @@ const enum MetaFlags { INITIALIZING = 1 << 3, } +const enum ListenerKind { + ADD = 0, + ONCE = 1, + REMOVE = 2, + REMOVE_ALL = 3, +} + +interface RemoveAllListener { + event: string; + target: null; + method: null; + kind: ListenerKind.REMOVE_ALL; +} + +interface StringListener { + event: string; + target: null; + method: string; + kind: ListenerKind.ADD | ListenerKind.ONCE | ListenerKind.REMOVE; +} + +interface FunctionListener { + event: string; + target: object | null; + method: Function; + kind: ListenerKind.ADD | ListenerKind.ONCE | ListenerKind.REMOVE; +} + +type Listener = RemoveAllListener | StringListener | FunctionListener; + +let currentListenerVersion = 1; + export class Meta { _descriptors: any | undefined; _watching: any | undefined; @@ -54,8 +96,11 @@ export class Meta { source: object; proto: object | undefined; _parent: Meta | undefined | null; - _listeners: any | undefined; - _listenersFinalized: boolean; + + _listeners: Listener[] | undefined; + _listenersVersion = 1; + _inheritedEnd = 0; + _flattenedVersion = 0; // DEBUG _values: any | undefined; @@ -85,7 +130,6 @@ export class Meta { this.proto = obj.constructor === undefined ? undefined : obj.constructor.prototype; this._listeners = undefined; - this._listenersFinalized = false; } get parent() { @@ -449,82 +493,199 @@ export class Meta { method: Function | string, once: boolean ) { - if (this._listeners === undefined) { - this._listeners = []; + if (DEBUG) { + counters!.addToListenersCalls++; } - this._listeners.push(eventName, target, method, once); + + this.pushListener(eventName, target, method, once ? ListenerKind.ONCE : ListenerKind.ADD); } - _finalizeListeners() { - if (this._listenersFinalized) { - return; + removeFromListeners(eventName: string, target: object | null, method: Function | string): void { + if (DEBUG) { + counters!.removeFromListenersCalls++; } - if (this._listeners === undefined) { - this._listeners = []; + + this.pushListener(eventName, target, method, ListenerKind.REMOVE); + } + + removeAllListeners(event: string) { + if (DEBUG) { + counters!.removeAllListenersCalls++; } - let pointer = this.parent; - while (pointer !== null) { - let listeners = pointer._listeners; - if (listeners !== undefined) { - this._listeners = this._listeners.concat(listeners); + + let listeners = this.writableListeners(); + let inheritedEnd = this._inheritedEnd; + // remove all listeners of event name + // adjusting the inheritedEnd if listener is below it + for (let i = listeners.length - 1; i >= 0; i--) { + let listener = listeners[i]; + if (listener.event === event) { + listeners.splice(i, 1); + if (i < inheritedEnd) { + inheritedEnd--; + } } - if (pointer._listenersFinalized) { - break; + } + this._inheritedEnd = inheritedEnd; + // we put remove alls at start because rare and easy to check there + listeners.splice(inheritedEnd, 0, { + event, + target: null, + method: null, + kind: ListenerKind.REMOVE_ALL, + }); + } + + private pushListener( + event: string, + target: object | null, + method: Function | string, + kind: ListenerKind.ADD | ListenerKind.ONCE | ListenerKind.REMOVE + ): void { + let listeners = this.writableListeners(); + + let i = indexOfListener(listeners, event, target, method!); + + // remove if found listener was inherited + if (i !== -1 && i < this._inheritedEnd) { + listeners.splice(i, 1); + this._inheritedEnd--; + i = -1; + } + + // if not found, push + if (i === -1) { + listeners.push({ + event, + target, + method, + kind, + } as Listener); + } else { + // update own listener + listeners[i].kind = kind; + } + } + + /** + Flattening is based on a global revision counter. If the revision has + bumped it means that somewhere in a class inheritance chain something has + changed, so we need to reflatten everything. This can only happen if: + + 1. A meta has been flattened (listener has been called) + 2. The meta is a prototype meta with children who have inherited its + listeners + 3. A new listener is subsequently added to the meta (e.g. via `.reopen()`) + + This is a very rare occurence, so while the counter is global it shouldn't + be updated very often in practice. + */ + private _shouldFlatten() { + return this._flattenedVersion < currentListenerVersion; + } + + private _isFlattened() { + // A meta is flattened _only_ if the saved version is equal to the current + // version. Otherwise, it will flatten again the next time + // `flattenedListeners` is called, so there is no reason to bump the global + // version again. + return this._flattenedVersion === currentListenerVersion; + } + + private _setFlattened() { + this._flattenedVersion = currentListenerVersion; + } + + private writableListeners(): Listener[] { + let listeners = this._listeners; + + if (listeners === undefined) { + listeners = this._listeners = [] as Listener[]; + } + + // Check if the meta is owned by a prototype. If so, our listeners are + // inheritable so check the meta has been flattened. If it has, children + // have inherited its listeners, so bump the global version counter to + // invalidate. + if (this.source === this.proto && this._isFlattened()) { + if (DEBUG) { + counters!.reopensAfterFlatten++; } - pointer = pointer.parent; + + currentListenerVersion++; } - this._listenersFinalized = true; + + return listeners; } - removeFromListeners(eventName: string, target: any, method: Function | string): void { - let pointer: Meta | null = this; - while (pointer !== null) { - let listeners = pointer._listeners; - if (listeners !== undefined) { - for (let index = listeners.length - 4; index >= 0; index -= 4) { - if ( - listeners[index] === eventName && - (!method || (listeners[index + 1] === target && listeners[index + 2] === method)) - ) { - if (pointer === this) { - listeners.splice(index, 4); // we are modifying our own list, so we edit directly - } else { - // we are trying to remove an inherited listener, so we do - // just-in-time copying to detach our own listeners from - // our inheritance chain. - this._finalizeListeners(); - return this.removeFromListeners(eventName, target, method); + private flattenedListeners(): Listener[] | undefined { + if (this._shouldFlatten()) { + let parent = this.parent; + + if (parent !== null) { + // compute + let parentListeners = parent.flattenedListeners(); + + if (parentListeners !== undefined) { + let listeners = this._listeners; + + if (listeners === undefined) { + listeners = this._listeners = [] as Listener[]; + } + + if (this._inheritedEnd > 0) { + listeners.splice(0, this._inheritedEnd); + this._inheritedEnd = 0; + } + + for (let i = 0; i < parentListeners.length; i++) { + let listener = parentListeners[i]; + let index = indexOfListener( + listeners, + listener.event, + listener.target, + listener.method + ); + + if (index === -1) { + if (DEBUG) { + counters!.listenersInherited++; + } + + listeners.unshift(listener); + this._inheritedEnd++; } } } } - if (pointer._listenersFinalized) { - break; - } - pointer = pointer.parent; + + this._setFlattened(); } + + return this._listeners; } - matchingListeners(eventName: string) { - let pointer: Meta | null = this; - // fix type - let result: any[] | undefined; - while (pointer !== null) { - let listeners = pointer._listeners; - if (listeners !== undefined) { - for (let index = 0; index < listeners.length; index += 4) { - if (listeners[index] === eventName) { - result = result || []; - pushUniqueListener(result, listeners, index); - } + matchingListeners(eventName: string): (string | boolean | object | null)[] | undefined | void { + let listeners = this.flattenedListeners(); + + if (listeners !== undefined) { + let result = []; + + for (let index = 0; index < listeners.length; index++) { + let listener = listeners[index]; + + // REMOVE and REMOVE_ALL listeners are placeholders that tell us not to + // inherit, so they never match. Only ADD and ONCE can match. + if ( + listener.event === eventName && + (listener.kind === ListenerKind.ADD || listener.kind === ListenerKind.ONCE) + ) { + result.push(listener.target!, listener.method, listener.kind === ListenerKind.ONCE); } } - if (pointer._listenersFinalized) { - break; - } - pointer = pointer.parent; + + return result.length === 0 ? undefined : result; } - return result; } } @@ -774,24 +935,22 @@ export function isDescriptor(possibleDesc: any | undefined | null): boolean { export { counters }; -/* - When we render a rich template hierarchy, the set of events that - *might* happen tends to be much larger than the set of events that - actually happen. This implies that we should make listener creation & - destruction cheap, even at the cost of making event dispatch more - expensive. - - Thus we store a new listener with a single push and no new - allocations, without even bothering to do deduplication -- we can - save that for dispatch time, if an event actually happens. - */ -function pushUniqueListener(destination: any[], source: any[], index: number) { - let target = source[index + 1]; - let method = source[index + 2]; - for (let destinationIndex = 0; destinationIndex < destination.length; destinationIndex += 3) { - if (destination[destinationIndex] === target && destination[destinationIndex + 1] === method) { - return; +function indexOfListener( + listeners: Listener[], + event: string, + target: object | null, + method: Function | string | null +) { + for (let i = listeners.length - 1; i >= 0; i--) { + let listener = listeners[i]; + + if ( + listener.event === event && + ((listener.target === target && listener.method === method) || + listener.kind === ListenerKind.REMOVE_ALL) + ) { + return i; } } - destination.push(target, method, source[index + 3]); + return -1; } diff --git a/packages/@ember/-internals/meta/tests/meta_test.js b/packages/@ember/-internals/meta/tests/meta_test.js index b05c181fa69..717318bcb59 100644 --- a/packages/@ember/-internals/meta/tests/meta_test.js +++ b/packages/@ember/-internals/meta/tests/meta_test.js @@ -1,5 +1,5 @@ import { AbstractTestCase, moduleFor } from 'internal-test-helpers'; -import { meta } from '..'; +import { meta, counters } from '..'; moduleFor( 'Ember.meta', @@ -76,6 +76,59 @@ moduleFor( assert.equal(matching[0], t); } + ['@test meta.listeners reopen after flatten'](assert) { + // Ensure counter is zeroed + counters.reopensAfterFlatten = 0; + + class Class1 {} + let class1Meta = meta(Class1.prototype); + class1Meta.addToListeners('hello', null, 'm', 0); + + let instance1 = new Class1(); + let m1 = meta(instance1); + + class Class2 {} + let class2Meta = meta(Class2.prototype); + class2Meta.addToListeners('hello', null, 'm', 0); + + let instance2 = new Class2(); + let m2 = meta(instance2); + + m1.matchingListeners('hello'); + m2.matchingListeners('hello'); + + assert.equal(counters.reopensAfterFlatten, 0, 'no reopen calls yet'); + + m1.addToListeners('world', null, 'm', 0); + m2.addToListeners('world', null, 'm', 0); + m1.matchingListeners('world'); + m2.matchingListeners('world'); + + assert.equal( + counters.reopensAfterFlatten, + 0, + 'no reopen calls after mutating leaf listeners' + ); + + class1Meta.removeFromListeners('hello', null, 'm'); + class2Meta.removeFromListeners('hello', null, 'm'); + m1.matchingListeners('hello'); + m2.matchingListeners('hello'); + + assert.equal(counters.reopensAfterFlatten, 1, 'one reopen call after mutating parents'); + + class1Meta.addToListeners('hello', null, 'm', 0); + m1.matchingListeners('hello'); + class2Meta.addToListeners('hello', null, 'm', 0); + m2.matchingListeners('hello'); + + assert.equal( + counters.reopensAfterFlatten, + 2, + 'one reopen call after mutating parents and flattening out of order' + ); + } + ['@test meta.writeWatching issues useful error after destroy']() { let target = { toString() { diff --git a/packages/@ember/-internals/metal/lib/events.ts b/packages/@ember/-internals/metal/lib/events.ts index b32038a2b4b..6b5ec777b79 100644 --- a/packages/@ember/-internals/metal/lib/events.ts +++ b/packages/@ember/-internals/metal/lib/events.ts @@ -80,7 +80,13 @@ export function removeListener( target = null; } - metaFor(obj).removeFromListeners(eventName, target, method!); + let m = metaFor(obj); + + if (method === undefined) { + m.removeAllListeners(eventName); + } else { + m.removeFromListeners(eventName, target, method); + } } /** diff --git a/packages/@ember/-internals/runtime/tests/system/object/es-compatibility-test.js b/packages/@ember/-internals/runtime/tests/system/object/es-compatibility-test.js index bc4a925d212..db9255f8b55 100644 --- a/packages/@ember/-internals/runtime/tests/system/object/es-compatibility-test.js +++ b/packages/@ember/-internals/runtime/tests/system/object/es-compatibility-test.js @@ -3,8 +3,12 @@ import { Mixin, defineProperty, computed, + observer, + on, addObserver, + removeObserver, addListener, + removeListener, sendEvent, } from '@ember/-internals/metal'; import { moduleFor, AbstractTestCase } from 'internal-test-helpers'; @@ -278,6 +282,84 @@ moduleFor( SubEmberObject.metaForProperty('foo'); } + '@test observes / removeObserver on / removeListener interop'(assert) { + let fooDidChangeBase = 0; + let fooDidChangeA = 0; + let fooDidChangeB = 0; + let someEventBase = 0; + let someEventA = 0; + let someEventB = 0; + class A extends EmberObject.extend({ + fooDidChange: observer('foo', function() { + fooDidChangeBase++; + }), + + onSomeEvent: on('someEvent', function() { + someEventBase++; + }), + }) { + init() { + super.init(); + this.foo = 'bar'; + } + + fooDidChange() { + super.fooDidChange(); + fooDidChangeA++; + } + + onSomeEvent() { + super.onSomeEvent(); + someEventA++; + } + } + + class B extends A { + fooDidChange() { + super.fooDidChange(); + fooDidChangeB++; + } + + onSomeEvent() { + super.onSomeEvent(); + someEventB++; + } + } + + removeObserver(B.prototype, 'foo', null, 'fooDidChange'); + removeListener(B.prototype, 'someEvent', null, 'onSomeEvent'); + + assert.equal(fooDidChangeBase, 0); + assert.equal(fooDidChangeA, 0); + assert.equal(fooDidChangeB, 0); + + assert.equal(someEventBase, 0); + assert.equal(someEventA, 0); + assert.equal(someEventB, 0); + + let a = A.create(); + a.set('foo', 'something'); + assert.equal(fooDidChangeBase, 1); + assert.equal(fooDidChangeA, 1); + assert.equal(fooDidChangeB, 0); + + sendEvent(a, 'someEvent'); + assert.equal(someEventBase, 1); + assert.equal(someEventA, 1); + assert.equal(someEventB, 0); + + let b = B.create(); + b.set('foo', 'something'); + assert.equal(fooDidChangeBase, 1); + assert.equal(fooDidChangeA, 1); + assert.equal(fooDidChangeB, 0); + + sendEvent(b, 'someEvent'); + assert.equal(someEventBase, 1); + assert.equal(someEventA, 1); + assert.equal(someEventB, 0); + } + '@test super and _super interop between old and new methods'(assert) { let calls = []; let changes = []; From 7164bd6c69f4c9b3ce33d129aac564dc1ec95920 Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Tue, 25 Sep 2018 16:14:16 -0700 Subject: [PATCH 2/5] completely remove function listeners --- packages/@ember/-internals/meta/lib/meta.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/@ember/-internals/meta/lib/meta.ts b/packages/@ember/-internals/meta/lib/meta.ts index d62bcd02319..a5274f8ab1a 100644 --- a/packages/@ember/-internals/meta/lib/meta.ts +++ b/packages/@ember/-internals/meta/lib/meta.ts @@ -562,8 +562,20 @@ export class Meta { kind, } as Listener); } else { - // update own listener - listeners[i].kind = kind; + let listener = listeners[i]; + // If the listener is our own function listener and we are trying to + // remove it, we want to splice it out entirely so we don't hold onto a + // reference. + if ( + typeof method === 'function' && + kind === ListenerKind.REMOVE && + listener.kind !== ListenerKind.REMOVE + ) { + listeners.splice(i, 1); + } else { + // update own listener + listener.kind = kind; + } } } From da28ceadc37c0d3f7ad4ba150defd70285233e9c Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Wed, 10 Oct 2018 17:17:21 -0700 Subject: [PATCH 3/5] fix flattening, add deprecations --- packages/@ember/-internals/meta/lib/meta.ts | 51 +++++++++++++++++++-- 1 file changed, 47 insertions(+), 4 deletions(-) diff --git a/packages/@ember/-internals/meta/lib/meta.ts b/packages/@ember/-internals/meta/lib/meta.ts index a5274f8ab1a..079756accd2 100644 --- a/packages/@ember/-internals/meta/lib/meta.ts +++ b/packages/@ember/-internals/meta/lib/meta.ts @@ -1,5 +1,5 @@ import { lookupDescriptor, symbol, toString } from '@ember/-internals/utils'; -import { assert } from '@ember/debug'; +import { assert, deprecate } from '@ember/debug'; import { DEBUG } from '@glimmer/env'; import { Tag } from '@glimmer/reference'; @@ -509,6 +509,16 @@ export class Meta { } removeAllListeners(event: string) { + deprecate( + 'The remove all functionality of removeListener and removeObserver has been deprecated. Remove each listener/observer individually instead.', + false, + { + id: 'events.remove-all-listeners', + until: '3.9.0', + url: 'https://emberjs.com/deprecations/v3.x#toc_events-remove-all-listeners', + } + ); + if (DEBUG) { counters!.removeAllListenersCalls++; } @@ -553,8 +563,34 @@ export class Meta { i = -1; } - // if not found, push + // if not found, push. Note that we must always push if a listener is not + // found, even in the case of a function listener remove, because we may be + // attempting to add or remove listeners _before_ flattening has occured. if (i === -1) { + deprecate( + 'Adding function listeners to prototypes has been deprecated. Convert the listener to a string listener, or add it to the instance instead.', + !(this.isPrototypeMeta(this.source) && typeof method === 'function'), + { + id: 'events.inherited-function-listeners', + until: '3.9.0', + url: 'https://emberjs.com/deprecations/v3.x#toc_events-inherited-function-listeners', + } + ); + + deprecate( + 'You attempted to remove a function listener which did not exist on the instance, which means it was an inherited prototype listener, or you attempted to remove it before it was added. Prototype function listeners have been deprecated, and attempting to remove a non-existent function listener this will error in the future.', + !( + !this.isPrototypeMeta(this.source) && + typeof method === 'function' && + kind === ListenerKind.REMOVE + ), + { + id: 'events.inherited-function-listeners', + until: '3.9.0', + url: 'https://emberjs.com/deprecations/v3.x#toc_events-inherited-function-listeners', + } + ); + listeners.push({ event, target, @@ -567,9 +603,9 @@ export class Meta { // remove it, we want to splice it out entirely so we don't hold onto a // reference. if ( - typeof method === 'function' && kind === ListenerKind.REMOVE && - listener.kind !== ListenerKind.REMOVE + listener.kind !== ListenerKind.REMOVE && + typeof method === 'function' ) { listeners.splice(i, 1); } else { @@ -631,6 +667,13 @@ export class Meta { } private flattenedListeners(): Listener[] | undefined { + // If this instance doesn't have any of its own listeners (writableListeners + // has never been called) then we don't need to do any flattening, return + // the parent's listeners instead. + if (this._listeners === undefined) { + return this.parent !== null ? this.parent.flattenedListeners() : undefined; + } + if (this._shouldFlatten()) { let parent = this.parent; From ef21456efee841d38d703f5b3ea7ef9698e1e021 Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Mon, 15 Oct 2018 09:18:57 -0700 Subject: [PATCH 4/5] fix tests, sort macro --- .../tests/integration/helpers/unbound-test.js | 16 +++---- .../-internals/metal/tests/alias_test.js | 5 +- .../-internals/metal/tests/events_test.js | 46 +++++++++++-------- .../-internals/metal/tests/observer_test.js | 4 ++ .../metal/tests/watching/is_watching_test.js | 19 ++++---- .../lib/computed/reduce_computed_macros.js | 12 ++++- 6 files changed, 60 insertions(+), 42 deletions(-) diff --git a/packages/@ember/-internals/glimmer/tests/integration/helpers/unbound-test.js b/packages/@ember/-internals/glimmer/tests/integration/helpers/unbound-test.js index 35f856c747f..da7e1478c63 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/helpers/unbound-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/helpers/unbound-test.js @@ -362,13 +362,13 @@ moduleFor( ['@test should be able to render an unbound helper invocation for helpers with dependent keys']() { this.registerHelper('capitalizeName', { destroy() { - this.removeObserver('value.firstName'); + this.removeObserver('value.firstName', this, this.recompute); this._super(...arguments); }, compute([value]) { if (this.get('value')) { - this.removeObserver('value.firstName'); + this.removeObserver('value.firstName', this, this.recompute); } this.set('value', value); this.addObserver('value.firstName', this, this.recompute); @@ -382,8 +382,8 @@ moduleFor( this._super(...arguments); }, teardown() { - this.removeObserver('value.firstName'); - this.removeObserver('value.lastName'); + this.removeObserver('value.firstName', this, this.recompute); + this.removeObserver('value.lastName', this, this.recompute); }, compute([value]) { if (this.get('value')) { @@ -478,13 +478,13 @@ moduleFor( ['@test should be able to render an unbound helper invocation with bound hash options']() { this.registerHelper('capitalizeName', { destroy() { - this.removeObserver('value.firstName'); + this.removeObserver('value.firstName', this, this.recompute); this._super(...arguments); }, compute([value]) { if (this.get('value')) { - this.removeObserver('value.firstName'); + this.removeObserver('value.firstName', this, this.recompute); } this.set('value', value); this.addObserver('value.firstName', this, this.recompute); @@ -498,8 +498,8 @@ moduleFor( this._super(...arguments); }, teardown() { - this.removeObserver('value.firstName'); - this.removeObserver('value.lastName'); + this.removeObserver('value.firstName', this, this.recompute); + this.removeObserver('value.lastName', this, this.recompute); }, compute([value]) { if (this.get('value')) { diff --git a/packages/@ember/-internals/metal/tests/alias_test.js b/packages/@ember/-internals/metal/tests/alias_test.js index 9dbb186ea48..0546b99ea32 100644 --- a/packages/@ember/-internals/metal/tests/alias_test.js +++ b/packages/@ember/-internals/metal/tests/alias_test.js @@ -61,13 +61,14 @@ moduleFor( redefining the alias on the instance to another property dependent on same key does not call the observer twice`](assert) { let obj1 = Object.create(null); + obj1.incrementCount = incrementCount; meta(obj1).proto = obj1; defineProperty(obj1, 'foo', null, null); defineProperty(obj1, 'bar', alias('foo')); defineProperty(obj1, 'baz', alias('foo')); - addObserver(obj1, 'baz', incrementCount); + addObserver(obj1, 'baz', null, 'incrementCount'); let obj2 = Object.create(obj1); defineProperty(obj2, 'baz', alias('bar')); // override baz @@ -75,7 +76,7 @@ moduleFor( set(obj2, 'foo', 'FOO'); assert.equal(count, 1); - removeObserver(obj2, 'baz', incrementCount); + removeObserver(obj2, 'baz', null, 'incrementCount'); set(obj2, 'foo', 'OOF'); assert.equal(count, 1); diff --git a/packages/@ember/-internals/metal/tests/events_test.js b/packages/@ember/-internals/metal/tests/events_test.js index 47553698171..8437b54942f 100644 --- a/packages/@ember/-internals/metal/tests/events_test.js +++ b/packages/@ember/-internals/metal/tests/events_test.js @@ -26,13 +26,15 @@ moduleFor( } ['@test listeners should be inherited'](assert) { - let obj = {}; let count = 0; - let F = function() { - count++; + + let obj = { + func() { + count++; + }, }; - addListener(obj, 'event!', F); + addListener(obj, 'event!', null, 'func'); let obj2 = Object.create(obj); @@ -41,7 +43,7 @@ moduleFor( sendEvent(obj2, 'event!'); assert.equal(count, 1, 'received event'); - removeListener(obj2, 'event!', F); + removeListener(obj2, 'event!', null, 'func'); count = 0; sendEvent(obj2, 'event!'); @@ -52,13 +54,15 @@ moduleFor( } ['@test adding a listener more than once should only invoke once'](assert) { - let obj = {}; let count = 0; - function F() { - count++; - } - addListener(obj, 'event!', F); - addListener(obj, 'event!', F); + let obj = { + func() { + count++; + }, + }; + + addListener(obj, 'event!', null, 'func'); + addListener(obj, 'event!', null, 'func'); sendEvent(obj, 'event!'); assert.equal(count, 1, 'should only invoke once'); @@ -135,19 +139,21 @@ moduleFor( } ['@test calling removeListener without method should remove all listeners'](assert) { - let obj = {}; - function F() {} - function F2() {} + expectDeprecation(() => { + let obj = {}; + function F() {} + function F2() {} - assert.equal(hasListeners(obj, 'event!'), false, 'no listeners at first'); + assert.equal(hasListeners(obj, 'event!'), false, 'no listeners at first'); - addListener(obj, 'event!', F); - addListener(obj, 'event!', F2); + addListener(obj, 'event!', F); + addListener(obj, 'event!', F2); - assert.equal(hasListeners(obj, 'event!'), true, 'has listeners'); - removeListener(obj, 'event!'); + assert.equal(hasListeners(obj, 'event!'), true, 'has listeners'); + removeListener(obj, 'event!'); - assert.equal(hasListeners(obj, 'event!'), false, 'has no more listeners'); + assert.equal(hasListeners(obj, 'event!'), false, 'has no more listeners'); + }, /The remove all functionality of removeListener and removeObserver has been deprecated/); } ['@test a listener can be added as part of a mixin'](assert) { diff --git a/packages/@ember/-internals/metal/tests/observer_test.js b/packages/@ember/-internals/metal/tests/observer_test.js index 8cc6f176044..17e83ee48b2 100644 --- a/packages/@ember/-internals/metal/tests/observer_test.js +++ b/packages/@ember/-internals/metal/tests/observer_test.js @@ -914,6 +914,7 @@ moduleFor( let yetAnotherBeer = new Beer(); addObserver(yetAnotherBeer, 'type', K); set(yetAnotherBeer, 'type', 'ale'); + addObserver(beer, 'type', K); removeObserver(beer, 'type', K); assert.deepEqual( Object.keys(yetAnotherBeer), @@ -923,6 +924,7 @@ moduleFor( let itsMyLastBeer = new Beer(); set(itsMyLastBeer, 'type', 'ale'); + addObserver(beer, 'type', K); removeObserver(beer, 'type', K); assert.deepEqual(Object.keys(itsMyLastBeer), ['type'], 'set -> removeObserver'); } @@ -945,6 +947,7 @@ moduleFor( let yetAnotherBeer = new Beer(); addObserver(yetAnotherBeer, 'type', K); set(yetAnotherBeer, 'type', 'ale'); + addObserver(beer, 'type', K); removeObserver(beer, 'type', K); assert.deepEqual( Object.keys(yetAnotherBeer), @@ -954,6 +957,7 @@ moduleFor( let itsMyLastBeer = new Beer(); set(itsMyLastBeer, 'type', 'ale'); + addObserver(beer, 'type', K); removeObserver(beer, 'type', K); assert.deepEqual(Object.keys(itsMyLastBeer), ['type'], 'set -> removeObserver'); } diff --git a/packages/@ember/-internals/metal/tests/watching/is_watching_test.js b/packages/@ember/-internals/metal/tests/watching/is_watching_test.js index faa6c4ed338..77321839554 100644 --- a/packages/@ember/-internals/metal/tests/watching/is_watching_test.js +++ b/packages/@ember/-internals/metal/tests/watching/is_watching_test.js @@ -12,12 +12,11 @@ import { moduleFor, AbstractTestCase } from 'internal-test-helpers'; function testObserver(assert, setup, teardown, key = 'key') { let obj = {}; - function fn() {} assert.equal(isWatching(obj, key), false, 'precond - isWatching is false by default'); - setup(obj, key, fn); + setup(obj, key, 'fn'); assert.equal(isWatching(obj, key), true, 'isWatching is true when observers are added'); - teardown(obj, key, fn); + teardown(obj, key, 'fn'); assert.equal(isWatching(obj, key), false, 'isWatching is false after observers are removed'); } @@ -29,7 +28,7 @@ moduleFor( assert, (obj, key, fn) => { Mixin.create({ - didChange: observer(key, fn), + [fn]: observer(key, function() {}), }).apply(obj); }, (obj, key, fn) => removeObserver(obj, key, obj, fn) @@ -62,10 +61,10 @@ moduleFor( testObserver( assert, (obj, key, fn) => { - defineProperty(obj, 'computed', computed(fn).property(key)); - get(obj, 'computed'); + defineProperty(obj, fn, computed(function() {}).property(key)); + get(obj, fn); }, - obj => defineProperty(obj, 'computed', null) + (obj, key, fn) => defineProperty(obj, fn, null) ); } @@ -73,10 +72,10 @@ moduleFor( testObserver( assert, (obj, key, fn) => { - defineProperty(obj, 'computed', computed(fn).property(key + '.bar')); - get(obj, 'computed'); + defineProperty(obj, fn, computed(function() {}).property(key + '.bar')); + get(obj, fn); }, - obj => defineProperty(obj, 'computed', null) + (obj, key, fn) => defineProperty(obj, fn, null) ); } diff --git a/packages/@ember/object/lib/computed/reduce_computed_macros.js b/packages/@ember/object/lib/computed/reduce_computed_macros.js index db4368c614e..e41dd98aa9b 100644 --- a/packages/@ember/object/lib/computed/reduce_computed_macros.js +++ b/packages/@ember/object/lib/computed/reduce_computed_macros.js @@ -832,10 +832,17 @@ function propertySort(itemsKey, sortPropertiesKey) { let activeObserversMap = cp._activeObserverMap || (cp._activeObserverMap = new WeakMap()); let activeObservers = activeObserversMap.get(this); - function sortPropertyDidChange() { - this.notifyPropertyChange(key); + let sortPropertyDidChangeMap = + cp._sortPropertyDidChangeMap || (cp._sortPropertyDidChangeMap = new WeakMap()); + + if (!sortPropertyDidChangeMap.has(this)) { + sortPropertyDidChangeMap.set(this, function() { + this.notifyPropertyChange(key); + }); } + let sortPropertyDidChange = sortPropertyDidChangeMap.get(this); + if (activeObservers !== undefined) { activeObservers.forEach(path => removeObserver(this, path, sortPropertyDidChange)); } @@ -871,6 +878,7 @@ function propertySort(itemsKey, sortPropertiesKey) { ); cp._activeObserverMap = undefined; + cp._sortPropertyDidChangeMap = undefined; return cp; } From 506b8841888190de9cc35814d16a8d3098cfb20e Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Wed, 17 Oct 2018 12:41:23 -0700 Subject: [PATCH 5/5] add more counters, fix performance issues --- packages/@ember/-internals/meta/lib/meta.ts | 155 +++++++++-------- .../-internals/meta/tests/listeners_test.js | 159 ++++++++++++++++++ .../@ember/-internals/meta/tests/meta_test.js | 98 +---------- 3 files changed, 244 insertions(+), 168 deletions(-) create mode 100644 packages/@ember/-internals/meta/tests/listeners_test.js diff --git a/packages/@ember/-internals/meta/lib/meta.ts b/packages/@ember/-internals/meta/lib/meta.ts index 079756accd2..2ea31b0db30 100644 --- a/packages/@ember/-internals/meta/lib/meta.ts +++ b/packages/@ember/-internals/meta/lib/meta.ts @@ -12,10 +12,14 @@ export interface MetaCounters { deleteCalls: number; metaCalls: number; metaInstantiated: number; + matchingListenersCalls: number; addToListenersCalls: number; removeFromListenersCalls: number; removeAllListenersCalls: number; listenersInherited: number; + listenersFlattened: number; + parentListenersUsed: number; + flattenedListenersCalls: number; reopensAfterFlatten: number; } @@ -28,10 +32,14 @@ if (DEBUG) { deleteCalls: 0, metaCalls: 0, metaInstantiated: 0, + matchingListenersCalls: 0, addToListenersCalls: 0, removeFromListenersCalls: 0, removeAllListenersCalls: 0, listenersInherited: 0, + listenersFlattened: 0, + parentListenersUsed: 0, + flattenedListenersCalls: 0, reopensAfterFlatten: 0, }; } @@ -99,7 +107,7 @@ export class Meta { _listeners: Listener[] | undefined; _listenersVersion = 1; - _inheritedEnd = 0; + _inheritedEnd = -1; _flattenedVersion = 0; // DEBUG @@ -615,6 +623,32 @@ export class Meta { } } + private writableListeners(): Listener[] { + // Check if we need to invalidate and reflatten. We need to do this if we + // have already flattened (flattened version is the current version) and + // we are either writing to a prototype meta OR we have never inherited, and + // may have cached the parent's listeners. + if ( + this._flattenedVersion === currentListenerVersion && + (this.source === this.proto || this._inheritedEnd === -1) + ) { + if (DEBUG) { + counters!.reopensAfterFlatten++; + } + + currentListenerVersion++; + } + + // Inherited end has not been set, then we have never created our own + // listeners, but may have cached the parent's + if (this._inheritedEnd === -1) { + this._inheritedEnd = 0; + this._listeners = []; + } + + return this._listeners!; + } + /** Flattening is based on a global revision counter. If the revision has bumped it means that somewhere in a class inheritance chain something has @@ -628,53 +662,16 @@ export class Meta { This is a very rare occurence, so while the counter is global it shouldn't be updated very often in practice. */ - private _shouldFlatten() { - return this._flattenedVersion < currentListenerVersion; - } - - private _isFlattened() { - // A meta is flattened _only_ if the saved version is equal to the current - // version. Otherwise, it will flatten again the next time - // `flattenedListeners` is called, so there is no reason to bump the global - // version again. - return this._flattenedVersion === currentListenerVersion; - } - - private _setFlattened() { - this._flattenedVersion = currentListenerVersion; - } - - private writableListeners(): Listener[] { - let listeners = this._listeners; - - if (listeners === undefined) { - listeners = this._listeners = [] as Listener[]; + private flattenedListeners(): Listener[] | undefined { + if (DEBUG) { + counters!.flattenedListenersCalls++; } - // Check if the meta is owned by a prototype. If so, our listeners are - // inheritable so check the meta has been flattened. If it has, children - // have inherited its listeners, so bump the global version counter to - // invalidate. - if (this.source === this.proto && this._isFlattened()) { + if (this._flattenedVersion < currentListenerVersion) { if (DEBUG) { - counters!.reopensAfterFlatten++; + counters!.listenersFlattened++; } - currentListenerVersion++; - } - - return listeners; - } - - private flattenedListeners(): Listener[] | undefined { - // If this instance doesn't have any of its own listeners (writableListeners - // has never been called) then we don't need to do any flattening, return - // the parent's listeners instead. - if (this._listeners === undefined) { - return this.parent !== null ? this.parent.flattenedListeners() : undefined; - } - - if (this._shouldFlatten()) { let parent = this.parent; if (parent !== null) { @@ -682,39 +679,46 @@ export class Meta { let parentListeners = parent.flattenedListeners(); if (parentListeners !== undefined) { - let listeners = this._listeners; + if (this._listeners === undefined) { + // If this instance doesn't have any of its own listeners (writableListeners + // has never been called) then we don't need to do any flattening, return + // the parent's listeners instead. + if (DEBUG) { + counters!.parentListenersUsed++; + } - if (listeners === undefined) { - listeners = this._listeners = [] as Listener[]; - } + this._listeners = parentListeners; + } else { + let listeners = this._listeners; - if (this._inheritedEnd > 0) { - listeners.splice(0, this._inheritedEnd); - this._inheritedEnd = 0; - } + if (this._inheritedEnd > 0) { + listeners.splice(0, this._inheritedEnd); + this._inheritedEnd = 0; + } - for (let i = 0; i < parentListeners.length; i++) { - let listener = parentListeners[i]; - let index = indexOfListener( - listeners, - listener.event, - listener.target, - listener.method - ); - - if (index === -1) { - if (DEBUG) { - counters!.listenersInherited++; + for (let i = 0; i < parentListeners.length; i++) { + let listener = parentListeners[i]; + let index = indexOfListener( + listeners, + listener.event, + listener.target, + listener.method + ); + + if (index === -1) { + if (DEBUG) { + counters!.listenersInherited++; + } + + listeners.unshift(listener); + this._inheritedEnd++; } - - listeners.unshift(listener); - this._inheritedEnd++; } } } } - this._setFlattened(); + this._flattenedVersion = currentListenerVersion; } return this._listeners; @@ -722,10 +726,13 @@ export class Meta { matchingListeners(eventName: string): (string | boolean | object | null)[] | undefined | void { let listeners = this.flattenedListeners(); + let result; - if (listeners !== undefined) { - let result = []; + if (DEBUG) { + counters!.matchingListenersCalls++; + } + if (listeners !== undefined) { for (let index = 0; index < listeners.length; index++) { let listener = listeners[index]; @@ -735,12 +742,18 @@ export class Meta { listener.event === eventName && (listener.kind === ListenerKind.ADD || listener.kind === ListenerKind.ONCE) ) { + if (result === undefined) { + // we create this array only after we've found a listener that + // matches to avoid allocations when no matches are found. + result = [] as any[]; + } + result.push(listener.target!, listener.method, listener.kind === ListenerKind.ONCE); } } - - return result.length === 0 ? undefined : result; } + + return result; } } diff --git a/packages/@ember/-internals/meta/tests/listeners_test.js b/packages/@ember/-internals/meta/tests/listeners_test.js new file mode 100644 index 00000000000..bb1bdb0e2b0 --- /dev/null +++ b/packages/@ember/-internals/meta/tests/listeners_test.js @@ -0,0 +1,159 @@ +import { AbstractTestCase, moduleFor } from 'internal-test-helpers'; +import { meta, counters } from '..'; + +moduleFor( + 'Ember.meta listeners', + class extends AbstractTestCase { + ['@test basics'](assert) { + let t = {}; + let m = meta({}); + m.addToListeners('hello', t, 'm', 0); + let matching = m.matchingListeners('hello'); + assert.equal(matching.length, 3); + assert.equal(matching[0], t); + m.removeFromListeners('hello', t, 'm'); + matching = m.matchingListeners('hello'); + assert.equal(matching, undefined); + } + + ['@test inheritance'](assert) { + let target = {}; + let parent = {}; + let parentMeta = meta(parent); + parentMeta.addToListeners('hello', target, 'm', 0); + + let child = Object.create(parent); + let m = meta(child); + + let matching = m.matchingListeners('hello'); + assert.equal(matching.length, 3); + assert.equal(matching[0], target); + assert.equal(matching[1], 'm'); + assert.equal(matching[2], 0); + m.removeFromListeners('hello', target, 'm'); + matching = m.matchingListeners('hello'); + assert.equal(matching, undefined); + matching = parentMeta.matchingListeners('hello'); + assert.equal(matching.length, 3); + } + + ['@test deduplication'](assert) { + let t = {}; + let m = meta({}); + m.addToListeners('hello', t, 'm', 0); + m.addToListeners('hello', t, 'm', 0); + let matching = m.matchingListeners('hello'); + assert.equal(matching.length, 3); + assert.equal(matching[0], t); + } + + ['@test parent caching'](assert) { + counters.flattenedListenersCalls = 0; + counters.parentListenersUsed = 0; + + class Class {} + let classMeta = meta(Class.prototype); + classMeta.addToListeners('hello', null, 'm', 0); + + let instance = new Class(); + let m = meta(instance); + + let matching = m.matchingListeners('hello'); + + assert.equal(matching.length, 3); + assert.equal(counters.flattenedListenersCalls, 2); + assert.equal(counters.parentListenersUsed, 1); + + matching = m.matchingListeners('hello'); + + assert.equal(matching.length, 3); + assert.equal(counters.flattenedListenersCalls, 3); + assert.equal(counters.parentListenersUsed, 1); + } + + ['@test parent cache invalidation'](assert) { + counters.flattenedListenersCalls = 0; + counters.parentListenersUsed = 0; + counters.listenersInherited = 0; + + class Class {} + let classMeta = meta(Class.prototype); + classMeta.addToListeners('hello', null, 'm', 0); + + let instance = new Class(); + let m = meta(instance); + + let matching = m.matchingListeners('hello'); + + assert.equal(matching.length, 3); + assert.equal(counters.flattenedListenersCalls, 2); + assert.equal(counters.parentListenersUsed, 1); + assert.equal(counters.listenersInherited, 0); + + m.addToListeners('hello', null, 'm2'); + + matching = m.matchingListeners('hello'); + + assert.equal(matching.length, 6); + assert.equal(counters.flattenedListenersCalls, 4); + assert.equal(counters.parentListenersUsed, 1); + assert.equal(counters.listenersInherited, 1); + } + + ['@test reopen after flatten'](assert) { + // Ensure counter is zeroed + counters.reopensAfterFlatten = 0; + + class Class1 {} + let class1Meta = meta(Class1.prototype); + class1Meta.addToListeners('hello', null, 'm', 0); + + let instance1 = new Class1(); + let m1 = meta(instance1); + + class Class2 {} + let class2Meta = meta(Class2.prototype); + class2Meta.addToListeners('hello', null, 'm', 0); + + let instance2 = new Class2(); + let m2 = meta(instance2); + + m1.matchingListeners('hello'); + m2.matchingListeners('hello'); + + assert.equal(counters.reopensAfterFlatten, 0, 'no reopen calls yet'); + + m1.addToListeners('world', null, 'm', 0); + m2.addToListeners('world', null, 'm', 0); + m1.matchingListeners('world'); + m2.matchingListeners('world'); + + assert.equal(counters.reopensAfterFlatten, 1, 'reopen calls after invalidating parent cache'); + + m1.addToListeners('world', null, 'm', 0); + m2.addToListeners('world', null, 'm', 0); + m1.matchingListeners('world'); + m2.matchingListeners('world'); + + assert.equal(counters.reopensAfterFlatten, 1, 'no reopen calls after mutating leaf nodes'); + + class1Meta.removeFromListeners('hello', null, 'm'); + class2Meta.removeFromListeners('hello', null, 'm'); + m1.matchingListeners('hello'); + m2.matchingListeners('hello'); + + assert.equal(counters.reopensAfterFlatten, 2, 'one reopen call after mutating parents'); + + class1Meta.addToListeners('hello', null, 'm', 0); + m1.matchingListeners('hello'); + class2Meta.addToListeners('hello', null, 'm', 0); + m2.matchingListeners('hello'); + + assert.equal( + counters.reopensAfterFlatten, + 3, + 'one reopen call after mutating parents and flattening out of order' + ); + } + } +); diff --git a/packages/@ember/-internals/meta/tests/meta_test.js b/packages/@ember/-internals/meta/tests/meta_test.js index 717318bcb59..8ba91bcdf9d 100644 --- a/packages/@ember/-internals/meta/tests/meta_test.js +++ b/packages/@ember/-internals/meta/tests/meta_test.js @@ -1,5 +1,5 @@ import { AbstractTestCase, moduleFor } from 'internal-test-helpers'; -import { meta, counters } from '..'; +import { meta } from '..'; moduleFor( 'Ember.meta', @@ -33,102 +33,6 @@ moduleFor( } } - ['@test meta.listeners basics'](assert) { - let t = {}; - let m = meta({}); - m.addToListeners('hello', t, 'm', 0); - let matching = m.matchingListeners('hello'); - assert.equal(matching.length, 3); - assert.equal(matching[0], t); - m.removeFromListeners('hello', t, 'm'); - matching = m.matchingListeners('hello'); - assert.equal(matching, undefined); - } - - ['@test meta.listeners inheritance'](assert) { - let target = {}; - let parent = {}; - let parentMeta = meta(parent); - parentMeta.addToListeners('hello', target, 'm', 0); - - let child = Object.create(parent); - let m = meta(child); - - let matching = m.matchingListeners('hello'); - assert.equal(matching.length, 3); - assert.equal(matching[0], target); - assert.equal(matching[1], 'm'); - assert.equal(matching[2], 0); - m.removeFromListeners('hello', target, 'm'); - matching = m.matchingListeners('hello'); - assert.equal(matching, undefined); - matching = parentMeta.matchingListeners('hello'); - assert.equal(matching.length, 3); - } - - ['@test meta.listeners deduplication'](assert) { - let t = {}; - let m = meta({}); - m.addToListeners('hello', t, 'm', 0); - m.addToListeners('hello', t, 'm', 0); - let matching = m.matchingListeners('hello'); - assert.equal(matching.length, 3); - assert.equal(matching[0], t); - } - - ['@test meta.listeners reopen after flatten'](assert) { - // Ensure counter is zeroed - counters.reopensAfterFlatten = 0; - - class Class1 {} - let class1Meta = meta(Class1.prototype); - class1Meta.addToListeners('hello', null, 'm', 0); - - let instance1 = new Class1(); - let m1 = meta(instance1); - - class Class2 {} - let class2Meta = meta(Class2.prototype); - class2Meta.addToListeners('hello', null, 'm', 0); - - let instance2 = new Class2(); - let m2 = meta(instance2); - - m1.matchingListeners('hello'); - m2.matchingListeners('hello'); - - assert.equal(counters.reopensAfterFlatten, 0, 'no reopen calls yet'); - - m1.addToListeners('world', null, 'm', 0); - m2.addToListeners('world', null, 'm', 0); - m1.matchingListeners('world'); - m2.matchingListeners('world'); - - assert.equal( - counters.reopensAfterFlatten, - 0, - 'no reopen calls after mutating leaf listeners' - ); - - class1Meta.removeFromListeners('hello', null, 'm'); - class2Meta.removeFromListeners('hello', null, 'm'); - m1.matchingListeners('hello'); - m2.matchingListeners('hello'); - - assert.equal(counters.reopensAfterFlatten, 1, 'one reopen call after mutating parents'); - - class1Meta.addToListeners('hello', null, 'm', 0); - m1.matchingListeners('hello'); - class2Meta.addToListeners('hello', null, 'm', 0); - m2.matchingListeners('hello'); - - assert.equal( - counters.reopensAfterFlatten, - 2, - 'one reopen call after mutating parents and flattening out of order' - ); - } - ['@test meta.writeWatching issues useful error after destroy']() { let target = { toString() {