From 89612b2914fe0f48123ceaa66e3e63b5f7975873 Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Wed, 26 Oct 2016 15:42:35 -0700 Subject: [PATCH] feat(distinct): remove `distinctKey`, `distinct` signature change and perf improvements (#2049) * feat(distinct): remove `distinctKey`, `distinct` signature change and perf improvements - Adds a limited Set ponyfill for runtimes that do not support `Set` - `distinct` now supports an optional keySelector argument that the user can use to select the value to check distinct on - `distinctKey` is removed as it is redundant - `distinct` no longer supports a comparer function argument, as there is little to no use case for such an argument that could not be covered by the keySelector - updates tests to remove tests that do not make sense and test new functionality BREAKING CHANGE: `distinctKey` has been removed. Use `distinct` BREAKING CHANGE: `distinct` operator has changed, first argument is an optional `keySelector`. The custom `compare` function is no longer supported. resolves #2009 * perf(distinct): increase `distinct()` perf by improving deopts - moves keySelector call to a different function with a try catch to improve V8 optimization - distinct calls with no keySelector passed now take a fully optimized path, doubling speed again related #2009 * docs(distinct): update distinct docs to fit new API --- spec/operators/distinct-spec.ts | 50 ++----- spec/operators/distinctKey-spec.ts | 226 ----------------------------- spec/util/Set-spec.ts | 60 ++++++++ src/Rx.ts | 1 - src/operator/distinct.ts | 72 +++++---- src/operator/distinctKey.ts | 29 ---- src/util/Set.ts | 40 +++++ 7 files changed, 156 insertions(+), 322 deletions(-) delete mode 100644 spec/operators/distinctKey-spec.ts create mode 100644 spec/util/Set-spec.ts delete mode 100644 src/operator/distinctKey.ts create mode 100644 src/util/Set.ts diff --git a/spec/operators/distinct-spec.ts b/spec/operators/distinct-spec.ts index 5191a4ffc0..cda0bbe65e 100644 --- a/spec/operators/distinct-spec.ts +++ b/spec/operators/distinct-spec.ts @@ -138,46 +138,29 @@ describe('Observable.prototype.distinct', () => { expectSubscriptions(e1.subscriptions).toBe(e1subs); }); - it('should emit once if comparer returns true always regardless of source emits', () => { - const e1 = hot('--a--b--c--d--e--f--|'); - const e1subs = '^ !'; - const expected = '--a-----------------|'; - - expectObservable((e1).distinct(() => true)).toBe(expected); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should emit all if comparer returns false always regardless of source emits', () => { - const e1 = hot('--a--a--a--a--a--a--|'); + it('should distinguish values by key', () => { + const values = {a: 1, b: 2, c: 3, d: 4, e: 5, f: 6}; + const e1 = hot('--a--b--c--d--e--f--|', values); const e1subs = '^ !'; - const expected = '--a--a--a--a--a--a--|'; + const expected = '--a--b--c-----------|'; + const selector = (value: number) => value % 3; - expectObservable((e1).distinct(() => false)).toBe(expected); + expectObservable((e1).distinct(selector)).toBe(expected, values); expectSubscriptions(e1.subscriptions).toBe(e1subs); }); - it('should distinguish values by selector', () => { - const e1 = hot('--a--b--c--d--e--f--|', {a: 1, b: 2, c: 3, d: 4, e: 5, f: 6}); - const e1subs = '^ !'; - const expected = '--a-----c-----e-----|'; - const selector = (x: number, y: number) => y % 2 === 0; - - expectObservable((e1).distinct(selector)).toBe(expected, {a: 1, c: 3, e: 5}); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should raises error when comparer throws', () => { + it('should raises error when selector throws', () => { const e1 = hot('--a--b--c--d--e--f--|'); const e1subs = '^ ! '; const expected = '--a--b--c--# '; - const selector = (x: string, y: string) => { - if (y === 'd') { - throw 'error'; + const selector = (value: string) => { + if (value === 'd') { + throw new Error('d is for dumb'); } - return x === y; + return value; }; - expectObservable((e1).distinct(selector)).toBe(expected); + expectObservable((e1).distinct(selector)).toBe(expected, undefined, new Error('d is for dumb')); expectSubscriptions(e1.subscriptions).toBe(e1subs); }); @@ -187,9 +170,8 @@ describe('Observable.prototype.distinct', () => { const e2 = hot('-----------x--------|'); const e2subs = '^ !'; const expected = '--a--b--------a--b--|'; - const selector = (x: string, y: string) => x === y; - expectObservable((e1).distinct(selector, e2)).toBe(expected); + expectObservable((e1).distinct(null, e2)).toBe(expected); expectSubscriptions(e1.subscriptions).toBe(e1subs); expectSubscriptions(e2.subscriptions).toBe(e2subs); }); @@ -200,9 +182,8 @@ describe('Observable.prototype.distinct', () => { const e2 = hot('-----------x-#'); const e2subs = '^ !'; const expected = '--a--b-------#'; - const selector = (x: string, y: string) => x === y; - expectObservable((e1).distinct(selector, e2)).toBe(expected); + expectObservable((e1).distinct(null, e2)).toBe(expected); expectSubscriptions(e1.subscriptions).toBe(e1subs); expectSubscriptions(e2.subscriptions).toBe(e2subs); }); @@ -214,9 +195,8 @@ describe('Observable.prototype.distinct', () => { const e2subs = '^ ! '; const unsub = ' ! '; const expected = '--a--b------'; - const selector = (x: string, y: string) => x === y; - expectObservable((e1).distinct(selector, e2), unsub).toBe(expected); + expectObservable((e1).distinct(null, e2), unsub).toBe(expected); expectSubscriptions(e1.subscriptions).toBe(e1subs); expectSubscriptions(e2.subscriptions).toBe(e2subs); }); diff --git a/spec/operators/distinctKey-spec.ts b/spec/operators/distinctKey-spec.ts deleted file mode 100644 index 064a0a38fa..0000000000 --- a/spec/operators/distinctKey-spec.ts +++ /dev/null @@ -1,226 +0,0 @@ -import * as Rx from '../../dist/cjs/Rx'; -declare const {hot, cold, asDiagram, expectObservable, expectSubscriptions}; - -const Observable = Rx.Observable; - -/** @test {distinctKey} */ -describe('Observable.prototype.distinctKey', () => { - asDiagram('distinctKey(\'k\')')('should distinguish between values', () => { - const values = {a: {k: 1}, b: {k: 2}, c: {k: 3}}; - const e1 = hot('-a--b-b----a-c-|', values); - const expected = '-a--b--------c-|'; - - const result = (e1).distinctKey('k'); - - expectObservable(result).toBe(expected, values); - }); - - it('should distinguish between values', () => { - const values = {a: {val: 1}, b: {val: 2}}; - const e1 = hot('--a--a--a--b--b--a--|', values); - const e1subs = '^ !'; - const expected = '--a--------b--------|'; - - expectObservable((e1).distinctKey('val')).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should distinguish between values and does not completes', () => { - const values = {a: {val: 1}, b: {val: 2}}; - const e1 = hot('--a--a--a--b--b--a-', values); - const e1subs = '^ '; - const expected = '--a--------b-------'; - - expectObservable((e1).distinctKey('val')).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should distinguish between values with key', () => { - const values = {a: {val: 1}, b: {valOther: 1}, c: {valOther: 3}, d: {val: 1}, e: {val: 5}}; - const e1 = hot('--a--b--c--d--e--|', values); - const e1subs = '^ !'; - const expected = '--a--b--------e--|'; - - expectObservable((e1).distinctKey('val')).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should not compare if source does not have element with key', () => { - const values = {a: {valOther: 1}, b: {valOther: 1}, c: {valOther: 3}, d: {valOther: 1}, e: {valOther: 5}}; - const e1 = hot('--a--b--c--d--e--|', values); - const e1subs = '^ !'; - const expected = '--a--------------|'; - - expectObservable((e1).distinctKey('val')).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should not completes if source never completes', () => { - const e1 = cold('-'); - const e1subs = '^'; - const expected = '-'; - - expectObservable((e1).distinctKey('val')).toBe(expected); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should not completes if source does not completes', () => { - const e1 = hot('-'); - const e1subs = '^'; - const expected = '-'; - - expectObservable((e1).distinctKey('val')).toBe(expected); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should complete if source is empty', () => { - const e1 = cold('|'); - const e1subs = '(^!)'; - const expected = '|'; - - expectObservable((e1).distinctKey('val')).toBe(expected); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should complete if source does not emit', () => { - const e1 = hot('------|'); - const e1subs = '^ !'; - const expected = '------|'; - - expectObservable((e1).distinctKey('val')).toBe(expected); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should emit if source emits single element only', () => { - const values = {a: {val: 1}}; - const e1 = hot('--a--|', values); - const e1subs = '^ !'; - const expected = '--a--|'; - - expectObservable((e1).distinctKey('val')).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should emit if source is scalar', () => { - const values = {a: {val: 1}}; - const e1 = Observable.of(values.a); - const expected = '(a|)'; - - expectObservable((e1).distinctKey('val')).toBe(expected, values); - }); - - it('should raises error if source raises error', () => { - const values = {a: {val: 1}}; - const e1 = hot('--a--a--#', values); - const e1subs = '^ !'; - const expected = '--a-----#'; - - expectObservable((e1).distinctKey('val')).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should raises error if source throws', () => { - const e1 = cold('#'); - const e1subs = '(^!)'; - const expected = '#'; - - expectObservable((e1).distinctKey('val')).toBe(expected); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should not omit if source elements are all different', () => { - const values = {a: {val: 1}, b: {val: 2}, c: {val: 3}, d: {val: 4}, e: {val: 5}}; - const e1 = hot('--a--b--c--d--e--|', values); - const e1subs = '^ !'; - const expected = '--a--b--c--d--e--|'; - - expectObservable((e1).distinctKey('val')).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should allow unsubscribing early and explicitly', () => { - const values = {a: {val: 1}, b: {val: 2}, c: {val: 3}, d: {val: 4}, e: {val: 5}}; - const e1 = hot('--a--b--b--d--a--e--|', values); - const e1subs = '^ ! '; - const expected = '--a--b----- '; - const unsub = ' ! '; - - const result = (e1).distinctKey('val'); - - expectObservable(result, unsub).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should not break unsubscription chains when unsubscribed explicitly', () => { - const values = {a: {val: 1}, b: {val: 2}, c: {val: 3}, d: {val: 4}, e: {val: 5}}; - const e1 = hot('--a--b--b--d--a--e--|', values); - const e1subs = '^ ! '; - const expected = '--a--b----- '; - const unsub = ' ! '; - - const result = (e1) - .mergeMap((x: any) => Observable.of(x)) - .distinctKey('val') - .mergeMap((x: any) => Observable.of(x)); - - expectObservable(result, unsub).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should emit once if source elements are all same', () => { - const values = {a: {val: 1}}; - const e1 = hot('--a--a--a--a--a--a--|', values); - const e1subs = '^ !'; - const expected = '--a-----------------|'; - - expectObservable((e1).distinctKey('val')).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should emit once if comparer returns true always regardless of source emits', () => { - const values = {a: {val: 1}, b: {val: 2}, c: {val: 3}, d: {val: 4}, e: {val: 5}}; - const e1 = hot('--a--b--c--d--e--|', values); - const e1subs = '^ !'; - const expected = '--a--------------|'; - - expectObservable((e1).distinctKey('val', () => true)).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should emit all if comparer returns false always regardless of source emits', () => { - const values = {a: {val: 1}}; - const e1 = hot('--a--a--a--a--a--a--|', values); - const e1subs = '^ !'; - const expected = '--a--a--a--a--a--a--|'; - - expectObservable((e1).distinctKey('val', () => false)).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should distinguish values by selector', () => { - const values = {a: {val: 1}, b: {val: 2}, c: {val: 3}, d: {val: 4}, e: {val: 5}}; - const e1 = hot('--a--b--c--d--e--|', values); - const e1subs = '^ !'; - const expected = '--a-----c-----e--|'; - const selector = (x: number, y: number) => y % 2 === 0; - - expectObservable((e1).distinctKey('val', selector)).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); - - it('should raises error when comparer throws', () => { - const values = {a: {val: 1}, b: {val: 2}, c: {val: 3}, d: {val: 4}, e: {val: 5}}; - const e1 = hot('--a--b--c--d--e--|', values); - const e1subs = '^ ! '; - const expected = '--a--b--c--# '; - const selector = (x: number, y: number) => { - if (y === 4) { - throw 'error'; - } - return x === y; - }; - - expectObservable((e1).distinctKey('val', selector)).toBe(expected, values); - expectSubscriptions(e1.subscriptions).toBe(e1subs); - }); -}); \ No newline at end of file diff --git a/spec/util/Set-spec.ts b/spec/util/Set-spec.ts new file mode 100644 index 0000000000..814342db86 --- /dev/null +++ b/spec/util/Set-spec.ts @@ -0,0 +1,60 @@ +import {expect} from 'chai'; +import { Set as TestSet, minimalSetImpl } from '../../dist/cjs/util/Set'; + +describe('Set', () => { + if (typeof Set === 'function') { + it('should use Set if Set exists', () => { + expect(TestSet).to.equal(Set); + }); + } +}); + +describe('minimalSetImpl()', () => { + it('should provide the minimal Set we require', () => { + const MinSet = minimalSetImpl(); + const test = new MinSet(); + + expect(MinSet.prototype.add).to.be.a('function'); + expect(MinSet.prototype.has).to.be.a('function'); + expect(MinSet.prototype.clear).to.be.a('function'); + expect(test.size).to.be.a('number'); + }); + + describe('returned MinimalSet', () => { + it('should implement add, has, size and clear', () => { + const MinSet = minimalSetImpl(); + const test = new MinSet(); + + expect(test.size).to.equal(0); + + test.add('Laverne'); + expect(test.size).to.equal(1); + expect(test.has('Laverne')).to.be.true; + expect(test.has('Shirley')).to.be.false; + + test.add('Shirley'); + expect(test.size).to.equal(2); + expect(test.has('Laverne')).to.be.true; + expect(test.has('Shirley')).to.be.true; + + const squiggy = { name: 'Andrew Squiggman' }; + const identicalSquiggy = { name: 'Andrew Squiggman' }; // lol, imposter! + + test.add(squiggy); + expect(test.size).to.equal(3); + expect(test.has(identicalSquiggy)).to.be.false; + expect(test.has(squiggy)).to.be.true; + + test.clear(); + expect(test.size).to.equal(0); + expect(test.has('Laverne')).to.be.false; + expect(test.has('Shirley')).to.be.false; + expect(test.has(squiggy)).to.be.false; + expect(test.has(identicalSquiggy)).to.be.false; + + test.add('Fonzi'); + expect(test.size).to.equal(1); + expect(test.has('Fonzi')).to.be.true; + }); + }); +}); \ No newline at end of file diff --git a/src/Rx.ts b/src/Rx.ts index 5f52c96d28..b04c99c505 100644 --- a/src/Rx.ts +++ b/src/Rx.ts @@ -59,7 +59,6 @@ import './add/operator/defaultIfEmpty'; import './add/operator/delay'; import './add/operator/delayWhen'; import './add/operator/distinct'; -import './add/operator/distinctKey'; import './add/operator/distinctUntilChanged'; import './add/operator/distinctUntilKeyChanged'; import './add/operator/do'; diff --git a/src/operator/distinct.ts b/src/operator/distinct.ts index 68d140695e..9cc1fcd8e7 100644 --- a/src/operator/distinct.ts +++ b/src/operator/distinct.ts @@ -5,29 +5,36 @@ import { TeardownLogic } from '../Subscription'; import { OuterSubscriber } from '../OuterSubscriber'; import { InnerSubscriber } from '../InnerSubscriber'; import { subscribeToResult } from '../util/subscribeToResult'; +import { ISet, Set } from '../util/Set'; /** * Returns an Observable that emits all items emitted by the source Observable that are distinct by comparison from previous items. - * If a comparator function is provided, then it will be called for each item to test for whether or not that value should be emitted. - * If a comparator function is not provided, an equality check is used by default. - * As the internal HashSet of this operator grows larger and larger, care should be taken in the domain of inputs this operator may see. - * An optional parameter is also provided such that an Observable can be provided to queue the internal HashSet to flush the values it holds. - * @param {function} [compare] optional comparison function called to test if an item is distinct from previous items in the source. + * If a keySelector function is provided, then it will project each value from the source observable into a new value that it will + * check for equality with previously projected values. If a keySelector function is not provided, it will use each value from the + * source observable directly with an equality check against previous values. + * In JavaScript runtimes that support `Set`, this operator will use a `Set` to improve performance of the distinct value checking. + * In other runtimes, this operator will use a minimal implementation of `Set` that relies on an `Array` and `indexOf` under the + * hood, so performance will degrade as more values are checked for distinction. Even in newer browsers, a long-running `distinct` + * use might result in memory leaks. To help alleviate this in some scenarios, an optional `flushes` parameter is also provided so + * that the internal `Set` can be "flushed", basically clearing it of values. + * @param {function} [keySelector] optional function to select which value you want to check as distinct. * @param {Observable} [flushes] optional Observable for flushing the internal HashSet of the operator. * @return {Observable} an Observable that emits items from the source Observable with distinct values. * @method distinct * @owner Observable */ -export function distinct(this: Observable, compare?: (x: T, y: T) => boolean, flushes?: Observable): Observable { - return this.lift(new DistinctOperator(compare, flushes)); +export function distinct(this: Observable, + keySelector?: (value: T) => K, + flushes?: Observable): Observable { + return this.lift(new DistinctOperator(keySelector, flushes)); } -class DistinctOperator implements Operator { - constructor(private compare: (x: T, y: T) => boolean, private flushes: Observable) { +class DistinctOperator implements Operator { + constructor(private keySelector: (value: T) => K, private flushes: Observable) { } call(subscriber: Subscriber, source: any): TeardownLogic { - return source._subscribe(new DistinctSubscriber(subscriber, this.compare, this.flushes)); + return source._subscribe(new DistinctSubscriber(subscriber, this.keySelector, this.flushes)); } } @@ -36,14 +43,11 @@ class DistinctOperator implements Operator { * @ignore * @extends {Ignored} */ -export class DistinctSubscriber extends OuterSubscriber { - private values: Array = []; +export class DistinctSubscriber extends OuterSubscriber { + private values: ISet = new Set(); - constructor(destination: Subscriber, compare: (x: T, y: T) => boolean, flushes: Observable) { + constructor(destination: Subscriber, private keySelector: (value: T) => K, flushes: Observable) { super(destination); - if (typeof compare === 'function') { - this.compare = compare; - } if (flushes) { this.add(subscribeToResult(this, flushes)); @@ -53,7 +57,7 @@ export class DistinctSubscriber extends OuterSubscriber { notifyNext(outerValue: T, innerValue: T, outerIndex: number, innerIndex: number, innerSub: InnerSubscriber): void { - this.values.length = 0; + this.values.clear(); } notifyError(error: any, innerSub: InnerSubscriber): void { @@ -61,25 +65,31 @@ export class DistinctSubscriber extends OuterSubscriber { } protected _next(value: T): void { - let found = false; - const values = this.values; - const len = values.length; + if (this.keySelector) { + this._useKeySelector(value); + } else { + this._finalizeNext(value, value); + } + } + + private _useKeySelector(value: T): void { + let key: K; + const { destination } = this; try { - for (let i = 0; i < len; i++) { - if (this.compare(values[i], value)) { - found = true; - return; - } - } + key = this.keySelector(value); } catch (err) { - this.destination.error(err); + destination.error(err); return; } - this.values.push(value); - this.destination.next(value); + this._finalizeNext(key, value); } - private compare(x: T, y: T): boolean { - return x === y; + private _finalizeNext(key: K|T, value: T) { + const { values } = this; + if (!values.has(key)) { + values.add(key); + this.destination.next(value); + } } + } diff --git a/src/operator/distinctKey.ts b/src/operator/distinctKey.ts deleted file mode 100644 index 4b8f69f48d..0000000000 --- a/src/operator/distinctKey.ts +++ /dev/null @@ -1,29 +0,0 @@ -import { distinct } from './distinct'; -import { Observable } from '../Observable'; - -/** - * Returns an Observable that emits all items emitted by the source Observable that are distinct by comparison from previous items, - * using a property accessed by using the key provided to check if the two items are distinct. - * If a comparator function is provided, then it will be called for each item to test for whether or not that value should be emitted. - * If a comparator function is not provided, an equality check is used by default. - * As the internal HashSet of this operator grows larger and larger, care should be taken in the domain of inputs this operator may see. - * An optional parameter is also provided such that an Observable can be provided to queue the internal HashSet to flush the values it holds. - * @param {string} key string key for object property lookup on each item. - * @param {function} [compare] optional comparison function called to test if an item is distinct from previous items in the source. - * @param {Observable} [flushes] optional Observable for flushing the internal HashSet of the operator. - * @return {Observable} an Observable that emits items from the source Observable with distinct values. - * @method distinctKey - * @owner Observable - */ -/* tslint:disable:max-line-length */ -export function distinctKey(this: Observable, key: string): Observable; -export function distinctKey(this: Observable, key: string, compare: (x: K, y: K) => boolean, flushes?: Observable): Observable; -/* tslint:disable:max-line-length */ -export function distinctKey(this: Observable, key: string, compare?: (x: T, y: T) => boolean, flushes?: Observable): Observable { - return distinct.call(this, function(x: T, y: T) { - if (compare) { - return compare(x[key], y[key]); - } - return x[key] === y[key]; - }, flushes); -} diff --git a/src/util/Set.ts b/src/util/Set.ts new file mode 100644 index 0000000000..825d6ef2af --- /dev/null +++ b/src/util/Set.ts @@ -0,0 +1,40 @@ +import { root } from './root'; + +export interface ISetCtor { + new(): ISet; +} + +export interface ISet { + add(value: T): void; + has(value: T): boolean; + size: number; + clear(): void; +} + +export function minimalSetImpl(): ISetCtor { + // THIS IS NOT a full impl of Set, this is just the minimum + // bits of functionality we need for this library. + return class MinimalSet implements ISet { + private _values: T[] = []; + + add(value: T): void { + if (!this.has(value)) { + this._values.push(value); + } + } + + has(value: T): boolean { + return this._values.indexOf(value) !== -1; + } + + get size(): number { + return this._values.length; + } + + clear(): void { + this._values.length = 0; + } + }; +} + +export const Set: ISetCtor = root.Set || minimalSetImpl(); \ No newline at end of file