distinct perf updates#2049
Conversation
|
commit message:
I think the "no" was accidental. I think something like this is more clear:
|
|
Just curiosity - does micro perf test number shows expected gain? (I know micro perf hasn't run quite long time though..) |
Yes... ~9-10x faster in micro perf test than previous. |
yup, good catch |
src/operator/distinct.ts
Outdated
| * 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} [keySelector] optional function to select which value you want to check as distinct | ||
| * @param {function} [compare] optional comparison function called to test if an item is distinct from previous items in the source. |
src/operator/distinct.ts
Outdated
|
|
||
| /** | ||
| * 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. |
There was a problem hiding this comment.
@Blesh docs need to be updated to not describe the compare function and instead include the keySelector
|
@Blesh is going to update the jsdocs, then we'll merge. |
… 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
- 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
|
LGTM |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
4 participants
First commit
distinctto use a keySelector and NOT a comparer. Comparer check was to mimic RxJS 2 or 3, which was the current version when this operator was originally created.distinctKeyas it's redundant.Setimplementation ifSetdoes not exist globally.distinct()by 5x in first commit.distinct(keySelector)(versus olderdistinctKey) by 8x in first commitAdditional commit (do not flatten)
distinct()by another 2x (total of 9-10x), anddistinct(keySelector)slightly (total of 9x)Note
comparerfunction argument exists in Rx4, yes, but after discussion with @mattpodwysocki, we are removing it because it is of little use, and most use cases could be solved by the keySelector. We can add it at a later time if we choose with minimal breakage.