classnames icon indicating copy to clipboard operation
classnames copied to clipboard

Use Set for dedupe

Open newyork-anthonyng opened this issue 7 years ago • 3 comments

See #173 for benchmark numbers.

newyork-anthonyng avatar Sep 07 '18 22:09 newyork-anthonyng

Sorry for the delayed reply @newyork-anthonyng, and thanks for the PR

I'm concerned about dropping support for IE11. I know it's a pain but there's still too much usage for that to be safe in a library that's this widely used.

However IE11 support might be good enough aside from the missing Set.values method. I think we could work around that by detecting Set.prototype.values and falling back to using forEach to turn the result into a string.

What do you think of that?

JedWatson avatar Mar 07 '19 15:03 JedWatson

@JedWatson Good callout.

I updated and removed the Set.prototype.values and Array.from calls because of the lack of IE11 support. Let me know if there's any other changes.

newyork-anthonyng avatar Mar 09 '19 16:03 newyork-anthonyng

Are we able to merge this once the conflict has been resolved?

tom-sherman avatar Apr 22 '20 11:04 tom-sherman

Running the benchmarks in browser and node

Node

* local/dedupe#strings x 1,823,058 ops/sec ±0.61% (95 runs sampled)
*   npm/dedupe#strings x 2,021,462 ops/sec ±0.96% (94 runs sampled)

* local/dedupe#object x 3,403,641 ops/sec ±0.50% (95 runs sampled)
*   npm/dedupe#object x 3,255,057 ops/sec ±2.78% (82 runs sampled)

* local/dedupe#strings, object x 1,912,406 ops/sec ±0.85% (91 runs sampled)
*   npm/dedupe#strings, object x 1,770,558 ops/sec ±1.53% (84 runs sampled)

* local/dedupe#mix x 1,318,324 ops/sec ±1.07% (93 runs sampled)
*   npm/dedupe#mix x 625,322 ops/sec ±2.56% (81 runs sampled)

* local/dedupe#arrays x 869,689 ops/sec ±0.72% (93 runs sampled)
*   npm/dedupe#arrays x 854,968 ops/sec ±1.79% (85 runs sampled)

For node, I can see some benefit for local/dedupe#mix, but everything else is within a margin of error.

Mozilla/5.0 (X11; Linux x86_64; rv:105.0) Gecko/20100101 Firefox/105.0

* local/dedupe#strings x 1,136,879 ops/sec ±0.53% (65 runs sampled)
* npm/dedupe#strings x 2,187,764 ops/sec ±0.50% (63 runs sampled)

* local/dedupe#object x 1,418,991 ops/sec ±0.97% (62 runs sampled)
* npm/dedupe#object x 3,763,277 ops/sec ±0.55% (63 runs sampled)

* local/dedupe#strings, object x 1,010,576 ops/sec ±1.02% (61 runs sampled)
* npm/dedupe#strings, object x 1,836,281 ops/sec ±1.31% (62 runs sampled)

* local/dedupe#mix x 783,200 ops/sec ±1.48% (62 runs sampled)
* npm/dedupe#mix x 604,930 ops/sec ±28.67% (41 runs sampled)
* 
* local/dedupe#arrays x 488,734 ops/sec ±6.45% (60 runs sampled)
* npm/dedupe#arrays x 772,377 ops/sec ±0.54% (64 runs sampled)

For my browser, I am seeing significant regressions in the ops/sec for each category except local/dedupe#mix. I don't think we will accept this change as written; not without a significant positive benefit.

dcousens avatar Sep 26 '22 23:09 dcousens