lib: replace _linklist with ES6 Sets

[Trott]

Hoping to get some early feedback on this.

Things remaining to be done:

• Run CI
• Benchmarks
• Check code coverage and write additional tests
• Check to see if this serendipitously fixes any outstanding bugs with timers

It's semver-major because _linklist is exposed and could be used in userland. In reality, this is unlikely but weirder things have happened. (Perhaps @ChALkeR can do one of their trademark greps through npm source to see if any examples come up.)

[thefourtheye]

cc @Fishrock123

[Trott]

CI: https://ci.nodejs.org/job/node-test-commit/645/

[Fishrock123]

Aren't sets a better replacement for Freelist or something?

There are some specific performance and memory related reasons for which we use linked lists. Timers is one of those, since the number of timers can be very large.

Also, linked list operations are very speedy.

Cc @bnoordhuis

[bnoordhuis]

I spotted three instances of for/of that prevent crankshaft optimization, although in two instances it doesn't really matter because of the try/catch statements in the same function.

As to using Sets vs. a linked list, I can see it going either way. Linked list insertion/deletion is theoretically a constant O(1) but because lib/_linklist.js is almost certainly megamorphic at run-time, real performance is much more variable.

Sets in V8 are basically hashmaps, with O(1) amortized performance. Amortized is worse than constant but because Set methods don't do property lookups, they don't turn megamorphic. this could turn megamorphic if people subclass Set but that doesn't affect it much, the Set object is just glue for the real hashmap.

[Trott]

Awesome, thanks @bnoordhuis and @Fishrock123. I'll keep experimenting and see if anything good comes of it.

[ChALkeR]

@Trott Atm, usage of _linklist core module by current versions of packages from npm is very limited (something around 5-10 packages). I will post a list a bit later. Also those modules could be fixed by using something like linkedlist or linked-list.

It looks to me that removing it is safe. Well, as long as it gives us an improvent.

[vkurchatkin]

@bnoordhuis wouldn't it be faster to have separate copies of all functions per each type then?

[bnoordhuis]

@vkurchatkin Yes, but that basically means open-coding the functions everywhere you use them. You could eval() code (or rather, new Function(...) - code generated from plain eval() is exempt from optimizations) but that is sure to raise some eyebrows.

[vkurchatkin]

Well, we can require _linklist and then erase it from cache. It seems like less of a hack

[bnoordhuis]

Oh, like that. Yes, that would amount to the same thing.

It wouldn't help much for objects that users can modify, though. E.g. for (var i = 0; i < 5; ++i) setTimeout(function(){}, i)['x'+i] = i; already makes it mostly megamorphic.

[Trott]

@bnoordhuis Do I need to worry about for-of vs. Crankshaft? Won't Crankshaft be replaced by TurboFan which can optimize for-of?

Disclaimer: I don't actually know what I'm talking about. I just read http://blog.chromium.org/2015/07/revving-up-javascript-performance-with.html

[bnoordhuis]

Won't Crankshaft be replaced by TurboFan which can optimize for-of?

Eventually, yes, but TF is not the default yet and I don't know when it will be.

[trevnorris]

Wouldn't it be easier to immediately resolve the time to its epoch offset (e.g. msecs += Date.now())? Then insert it ordered into an array? Could linear transverse if array is small and bisect if the array is large. Likely limit on elements checked would be 8-10. Then removing n timers off the array would be a single operation.

Anyway. Benchmarks have shown me that Maps and Sets are amazing for random access, but as soon as they need to be transversed you're in trouble. This implementation is clean, but also will suffer a performance hit because of needing to transverse the data.

[ChALkeR]

Updated list is here! Expression: require(._linklist.
Based on 100% of current versions of npm packages, but it could contain both false negatives and false positives.

ape-algorithm-0.0.8.tgz/linklist.js:2://var L = require('_linklist');
biojs-vis-blast-0.1.5.tgz/node/lib/timers.js:23:var L = require('_linklist');
biojs-vis-blast-0.1.5.tgz/node/test/simple/test-timers-linked-list.js:24:var L = require('_linklist');
candle-0.4.0.tgz/timers.js:23:var L = require('_linklist');
chromify-0.0.2.tgz/builtins/timers.js:23:var L = require('_linklist');
commonjs-everywhere-0.9.7.tgz/node/lib/timers.js:23:var L = require('_linklist');
flush-all-0.1.1.tgz/node-v0.13/lib/timers.js:25:var L = require('_linklist');
flush-all-0.1.1.tgz/node-v0.13/test/simple/test-timers-linked-list.js:24:var L = require('_linklist');
jsg-0.0.3.tgz/testdata/node_core_modules/timers.js:23:var L = require('_linklist');
micropromise-0.4.10.tgz/promise-bench/benchmark/lib/timers-ctx.js:23:var L = require('_linklist');
node-cjs-deps-0.2.2.tgz/nodeCjsDeps/Global.js:19:       _linklist = require('_linklist'),
node-core-lib-0.11.11.tgz/timers.js:23:var L = require('_linklist');
node-core-test-simple-0.11.11.tgz/test-timers-linked-list.js:24:var L = require('_linklist');
node-natives-0.10.25.tgz/timers.js:23:var L = require('_linklist');
perf-time-0.1.0.tgz/bench/settimeout/timers2.js:23:var L = require('_linklist');
perf-time-0.1.0.tgz/bench/settimeout/timers3.js:23:var L = require('_linklist');
perf-time-0.1.0.tgz/bench/settimeout/timers.js:23:var L = require('_linklist');
perf-timers-0.1.0.tgz/bench/settimeout/timers2.js:23:var L = require('_linklist');
perf-timers-0.1.0.tgz/bench/settimeout/timers3.js:23:var L = require('_linklist');
perf-timers-0.1.0.tgz/bench/settimeout/timers.js:23:var L = require('_linklist');
pezhu-0.0.0.tgz/Downloads/node-v0.9.11/lib/timers.js:23:var L = require('_linklist');
pezhu-0.0.0.tgz/Downloads/node-v0.9.11/test/simple/test-timers-linked-list.js:24:var L = require('_linklist');
portable-js-0.0.3.tgz/misc/io/timers.js:4:const L = require('_linklist');
portable-js-0.0.3.tgz/misc/node/timers.js:25:var L = require('_linklist');
promise-bench-0.0.2.tgz/benchmark/lib/timers-ctx.js:23:var L = require('_linklist');
wtfnode-0.2.1.tgz/index.js:28:    var L = require('_linklist');

That's actually 17 packages, but we could even file issues to each of them in advance.

[Trott]

@trevnorris Sure enough, a single ordered array is more efficient than a bunch of sets. The benchmarks in benchmark/misc/timers.js are getting comparable performance from this and 4.1.1. Here's the results I got just now with 4.1.1:

misc/timers.js thousands=500 type=depth: 0.71460
misc/timers.js thousands=500 type=breadth: 1991.22620

And with this array-based implementation:

misc/timers.js thousands=500 type=depth: 0.71673
misc/timers.js thousands=500 type=breadth: 2036.47098

I suspect this is not worth doing if the only result is identical performance. Still tinkering, though...

[Trott]

Additional changes seem to have made the benchmarks difference more substantial.

Node 4.1.1:

$ node benchmark/misc/timers.js type=breadth
misc/timers.js thousands=500 type=breadth: 1936.78221
$ node benchmark/misc/timers.js type=breadth
misc/timers.js thousands=500 type=breadth: 1889.21099
$

Current master branch:

$ ./node benchmark/misc/timers.js type=breadth
misc/timers.js thousands=500 type=breadth: 1934.28420
$ ./node benchmark/misc/timers.js type=breadth
misc/timers.js thousands=500 type=breadth: 1979.13838
$

This implementation:

$ ./node benchmark/misc/timers.js type=breadth
misc/timers.js thousands=500 type=breadth: 2109.95938
$ ./node benchmark/misc/timers.js type=breadth
misc/timers.js thousands=500 type=breadth: 2282.01890
$

Small sample size here, of course. Maybe I'll try to do 100 or 1000 runs and get a median or something...

[Trott]

Annnnnd....if I push the number of timers from 500,000 to 5,000,000, then current node starts out-performing this version. Which makes a lot of sense. Was just kind of thinking some magical use-native-constructs optimizations would trump it somehow.

Anyway, I'm going to close this for now and will re-open if something amazing and unexpected happens.

[ChALkeR]

Linking this with #3078 — _linklist deprecation moved there.