Implment queues using DLLs #1205

megawac · 2016-06-30T17:48:55Z

A linked list is a more optimal method to keep track of a stack of tasks as we can end up mutating the tasks very frequently. The previous solution could have been further optimized to avoid several splices (i.e. doing 1 splice instead of n in several scenarios), though a linked list solution should generally be more performant.

/cc @aearly

Fixes #756

megawac · 2016-06-30T17:53:03Z

also /cc @mikestopcontinues @bbrowning @zeluis9
If any of you have a second can you verify this solves your performance issues?

megawac · 2016-06-30T18:05:15Z

lib/priorityQueue.js

        arrayEach(data, function(task) {
            var item = {
                data: task,
                priority: priority,
                callback: typeof callback === 'function' ? callback : noop
            };

-            q.tasks.splice(_binarySearch(q.tasks, item, _compareTasks) + 1, 0, item);
-
-            setImmediate(q.process);


note this change, moved process outside of the loop. I think this is the correct behaviour

mikestopcontinues · 2016-06-30T18:57:55Z

I'll be happy to, but it might have to wait until the weekend. Deadlines! :)

aearly · 2016-06-30T21:08:24Z

lib/internal/DoublyLinkedList.js

+
+DLL.prototype.insertAfter = function(node, newNode) {
+    newNode.prev = node;
+    newNode.next = node.next;


Will this add prev and next properties to items people push to queues? Perhaps the list should store {prev, next, data} objects, where data points to the original object.

no, push wraps the item in an internal object already, {data, callback} for queue and {data, callback, priority} for priority queue

aearly · 2016-06-30T21:10:27Z

Also, what do our benchmarks look like with these changes? (Just realized I haven't tried to run those since modularization.)

megawac · 2016-06-30T22:15:44Z

./perf/benchmark.js -g queue
Latest tag is  v2.0.0-rc.6
Comparing v2.0.0-rc.6 with current on Node v6.2.0
--------------------------------------
queue(1000) v2.0.0-rc.6 x 298 ops/sec ±5.46% (28 runs sampled), 3.35ms per run
queue(1000) current x 316 ops/sec ±5.05% (29 runs sampled), 3.16ms per run
current is faster
--------------------------------------
queue(30000) v2.0.0-rc.6 x 8.06 ops/sec ±8.54% (15 runs sampled), 124ms per run
queue(30000) current x 8.95 ops/sec ±4.06% (17 runs sampled), 112ms per run
current is faster
--------------------------------------
queue(100000) v2.0.0-rc.6 x 0.14 ops/sec ±0.00% (1 run sampled), 7.12e+3ms per run
queue(100000) current x 2.54 ops/sec ±9.01% (6 runs sampled), 394ms per run
current is faster
--------------------------------------
queue(200000) v2.0.0-rc.6 x 0.04 ops/sec ±0.00% (1 run sampled), 2.70e+4ms per run
queue(200000) current x 1.22 ops/sec ±31.59% (3 runs sampled), 817ms per run
current is faster
--------------------------------------
current faster overall (1330ms total vs. 34200ms total)
current won more benchmarks (4 vs. 0)

1330ms total vs. 34200ms total (it will sometimes tie on the smaller suites but generally time is 1.3s for this pr, 36s for rc-6)

aearly · 2016-06-30T22:25:21Z

Damn, thats impressive. Looks like with small queues it will be slower, but at those sizes, the list overhead is negligible anyway.

aearly · 2016-06-30T22:28:16Z

Also, I realize this is a subtle breaking change -- the tasks array was exposed (but undocumented). Some people might be relying on that array for custom splicing.

megawac · 2016-06-30T22:38:42Z

Ye, that's why I changed the variable name and want to get this in v2.
Anyone relying on tasks being an array will get a reference error
On Jun 30, 2016 6:28 PM, "Alex Early" notifications@github.com wrote:

Also, I realize this is a subtle breaking change -- the tasks array was
exposed (but undocumented). Some people might be relying on that array for
custom splicing.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1205 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ADUIEBRoEA9dwMuPc09M2l7oZ5mKrOR2ks5qREMMgaJpZM4JCaXj
.

megawac · 2016-06-30T22:42:54Z

Also based on CS intuition-fu I'd expect this to be faster in all cases
(except some edge priority queue ones) =d
On Jun 30, 2016 6:38 PM, "Graeme Yeates" megawac@gmail.com wrote:

Ye, that's why I changed the variable name and want to get this in v2.
Anyone relying on tasks being an array will get a reference error
On Jun 30, 2016 6:28 PM, "Alex Early" notifications@github.com wrote:

Also, I realize this is a subtle breaking change -- the tasks array was
exposed (but undocumented). Some people might be relying on that array for
custom splicing.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1205 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ADUIEBRoEA9dwMuPc09M2l7oZ5mKrOR2ks5qREMMgaJpZM4JCaXj
.

mikestopcontinues · 2016-07-02T23:00:03Z

For the record, this totally fixes my issue. Thanks @megawac. You rock!

megawac · 2016-07-03T00:36:38Z

good to hear 🎉 @mikestopcontinues thanks for getting back

Implment queues using DLLs

592f95c

megawac added feature performance queue labels Jun 30, 2016

Minor cleanup

81e002d

megawac reviewed Jun 30, 2016
View reviewed changes

aearly reviewed Jun 30, 2016
View reviewed changes

aearly added this to the 2.0 milestone Jun 30, 2016

aearly added the breaking-change label Jun 30, 2016

aearly merged commit 9527b32 into master Jul 2, 2016

megawac deleted the dll branch July 3, 2016 00:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implment queues using DLLs #1205

Implment queues using DLLs #1205

megawac commented Jun 30, 2016

megawac commented Jun 30, 2016

megawac Jun 30, 2016

mikestopcontinues commented Jun 30, 2016

aearly Jun 30, 2016

megawac Jun 30, 2016 •

edited

aearly commented Jun 30, 2016

megawac commented Jun 30, 2016 •

edited

aearly commented Jun 30, 2016

aearly commented Jun 30, 2016

megawac commented Jun 30, 2016

megawac commented Jun 30, 2016 •

edited

mikestopcontinues commented Jul 2, 2016

megawac commented Jul 3, 2016

Implment queues using DLLs #1205

Implment queues using DLLs #1205

Conversation

megawac commented Jun 30, 2016

megawac commented Jun 30, 2016

megawac Jun 30, 2016

Choose a reason for hiding this comment

mikestopcontinues commented Jun 30, 2016

aearly Jun 30, 2016

Choose a reason for hiding this comment

megawac Jun 30, 2016 • edited

Choose a reason for hiding this comment

aearly commented Jun 30, 2016

megawac commented Jun 30, 2016 • edited

aearly commented Jun 30, 2016

aearly commented Jun 30, 2016

megawac commented Jun 30, 2016

megawac commented Jun 30, 2016 • edited

mikestopcontinues commented Jul 2, 2016

megawac commented Jul 3, 2016

megawac Jun 30, 2016 •

edited

megawac commented Jun 30, 2016 •

edited

megawac commented Jun 30, 2016 •

edited