#219 - Improve Map performance

[neenhouse]

Addresses #219

Only has test plan. Implementation still pending.

[CLAassistant]

All committers have signed the CLA.

[neenhouse]

First commit only adds a performance unit test that consistently fails IE11 locally (per contributing.md)

[JakeChampion]

@neenhouse Thanks for working on this, I've triggered a run on CircleCI for this.

In order for us to be able to accept your contribution though, we require you to have read and accepted our contributor license agreement.

[neenhouse]

Test fails in CI in expected way 👍

[neenhouse]

Took a first pass at the algorithm. Two notes:

• I avoided climbing the ladder of abstraction for first pass, so some code (normalize key + retrieve from hash table) is not DRY yet.
• When looking up functions and objects by key, algo currently assigns an internal id property to the object __meta__.id so it can identify the object in the internal hash table. There are alternatives to this approach that don't involve modifying the key function or object, but they won't be as performant.

I'd like to run a few more tests to compare performance between master + branch. Also want to try at least one algo that does not do the key modification approach and instead uses smaller hash tables that are iterated on to handle those lookups as edge cases.

[neenhouse]

Running local Karma unit test in Chrome 76.0.3809.100 (Official Build) (64-bit) comparing native implementation of Map vs polyfill-library:

• Native Chrome: 86ms, 107ms, 110ms, 94ms, 96ms, 81ms, 107ms, 100ms
• Polyfill Library (branch): 127ms, 116ms, 124ms, 116ms, 128ms, 116ms, 99ms, 121ms
• Polyfill Library (master): fails consistently (after 1000ms when Map size reaches around 6000 / 10000 records)

So reasonably close performance wise to the native implementation in Chrome.

[JakeChampion]

Looking great @neenhouse!

[neenhouse]

@JakeChampion Think this is ready for 1st review. I improved the marking of objects/functions by introducing randomness to avoid collisions if lib is loaded multiple times and abuse. Using reference comparison based algos even in smaller hash tables eventually leads to the same O(n) perf problem if a particular key type is over-used, so I thought this would be an acceptable trade off (thought also since primitives are usually used as keys in most UIs I've worked on, maybe a moot issue?).

[zloirock]

• Metadata should be unobservable (Object.assign, Object.getOwnPropertySymbol, Reflect.ownKeys, etc. on keys).
• It should work with frozen keys.
• .size and .delete also could be O(1).

[neenhouse]

@zloirock Thanks for the feedback. Frozen object keys forced me to climb the ladder of abstraction by 1 rung and were a bit tricky. I don't see a way to avoid O(n) computation time when dealing with those since I'm depending on being able to mark an object key with some kind of meta data to map it back to the internal state of the Map. Do you have any suggestions for dealing with perf in those cases? Appreciate any additional feedback.

@JakeChampion - I saw for size you already were precomputing a _size property and thought it would be a good idea (as @zloirock suggested) to convert the size getter to implement that instead of performing a O(n) iteration. I couldn't spot any logical fallacies for doing that. Is there anything I'm missing there? (reference: https://github.com/Financial-Times/polyfill-library/pull/293/files#diff-aaebc3d5cda7f687fb935dad9a4a98eaR502)

[zloirock]

@neenhouse see core-js.

[neenhouse]

Thanks for the feedback @zloirock

[neenhouse]

☝️ Last CI failure looks like browser stack flake:

[craigkovatch]

Will this also fix the perf issues in Set? I'm guessing they have the same root cause.

[neenhouse]

Hi @craigkovatch, this only improves Map, but Set has the same underlying issue with O(n) perf. I would imagine the right path would to be either to:

• Abstract a private class to use between Map and Set that implement similar, optimized operations.
• Re-implement the same optimizations used in Map in Set.

[craigkovatch]

Any update on this? We would really like to be able to use Set/Map