From 69dd197a1d85b8dd8509242f99608d0d98c3a609 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 2 May 2018 15:39:08 +0200 Subject: [PATCH 1/2] lib: expose FixedQueue internally and fix nextTick bug A bug was introduced together with the FixedQueue implementation for process.nextTick which meant that the queue wouldn't necessarily fully clear on each run through. Fix it and abstract the data structure into an internal module that can later be used elsewhere. --- lib/internal/fixed_queue.js | 112 ++++++++++++++++ lib/internal/process/next_tick.js | 124 ++---------------- node.gyp | 1 + .../test-next-tick-fixed-queue-regression.js | 18 +++ 4 files changed, 139 insertions(+), 116 deletions(-) create mode 100644 lib/internal/fixed_queue.js create mode 100644 test/parallel/test-next-tick-fixed-queue-regression.js diff --git a/lib/internal/fixed_queue.js b/lib/internal/fixed_queue.js new file mode 100644 index 00000000000000..d56b667a434b58 --- /dev/null +++ b/lib/internal/fixed_queue.js @@ -0,0 +1,112 @@ +'use strict'; + +// Currently optimal queue size, tested on V8 6.0 - 6.6. Must be power of two. +const kSize = 2048; +const kMask = kSize - 1; + +// The FixedQueue is implemented as a singly-linked list of fixed-size +// circular buffers. It looks something like this: +// +// head tail +// | | +// v v +// +-----------+ <-----\ +-----------+ <------\ +-----------+ +// | [null] | \----- | next | \------- | next | +// +-----------+ +-----------+ +-----------+ +// | item | <-- bottom | item | <-- bottom | [empty] | +// | item | | item | | [empty] | +// | item | | item | | [empty] | +// | item | | item | | [empty] | +// | item | | item | bottom --> | item | +// | item | | item | | item | +// | ... | | ... | | ... | +// | item | | item | | item | +// | item | | item | | item | +// | [empty] | <-- top | item | | item | +// | [empty] | | item | | item | +// | [empty] | | item | | item | +// +-----------+ +-----------+ <-- top top --> +-----------+ +// +// Or, if there is only one circular buffer, it looks something +// like either of these: +// +// head tail head tail +// | | | | +// v v v v +// +-----------+ +-----------+ +// | [null] | | [null] | +// +-----------+ +-----------+ +// | [empty] | | item | +// | [empty] | | item | +// | item | <-- bottom top --> | [empty] | +// | item | | [empty] | +// | [empty] | <-- top bottom --> | item | +// | [empty] | | item | +// +-----------+ +-----------+ +// +// Adding a value means moving `top` forward by one, removing means +// moving `bottom` forward by one. +// +// We let `bottom` and `top` wrap around, so when `top` is conceptually +// pointing to the end of the list, that means that the actual value is `0`. +// +// In particular, when `top === bottom`, this can mean *either* that the +// current queue is empty or that it is full. We can differentiate by +// checking whether an entry in the queue is empty (a.k.a. `=== undefined`). + +const FixedCircularBuffer = class FixedCircularBuffer { + constructor() { + this.bottom = 0; + this.top = 0; + this.list = new Array(kSize); + this.next = null; + } + + isEmpty() { + return this.top === this.bottom && this.list[this.top] === undefined; + } + + push(data) { + this.list[this.top] = data; + this.top = (this.top + 1) & kMask; + } + + shift() { + const nextItem = this.list[this.bottom]; + if (nextItem === undefined) + return null; + this.list[this.bottom] = undefined; + this.bottom = (this.bottom + 1) & kMask; + return nextItem; + } +}; + +module.exports = class FixedQueue { + constructor() { + this.tail = this.head = new FixedCircularBuffer(); + } + + isEmpty() { + return this.head.isEmpty(); + } + + push(data) { + let { head } = this; + if (head.bottom === head.top && head.list[head.top] !== undefined) { + // Head is full: Creates a new queue, sets the old queue's `.next` to it, + // and sets it as the new main queue. + this.head = head = head.next = new FixedCircularBuffer(); + } + head.push(data); + } + + shift() { + const { tail } = this; + const next = tail.shift(); + if (tail.top === tail.bottom && tail.next !== null) { + // If there is another queue, it forms the new tail. + this.tail = tail.next; + } + return next; + } +}; diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index 7a5d2f88a6d17e..dbe0ce8cdbdacf 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -16,6 +16,7 @@ function setupNextTick() { } = require('internal/async_hooks'); const promises = require('internal/process/promises'); const { ERR_INVALID_CALLBACK } = require('internal/errors').codes; + const FixedQueue = require('internal/fixed_queue'); const { emitPromiseRejectionWarnings } = promises; // tickInfo is used so that the C++ code in src/node.cc can @@ -31,119 +32,7 @@ function setupNextTick() { const kHasScheduled = 0; const kHasPromiseRejections = 1; - // Queue size for each tick array. Must be a power of two. - const kQueueSize = 2048; - const kQueueMask = kQueueSize - 1; - - // The next tick queue is implemented as a singly-linked list of fixed-size - // circular buffers. It looks something like this: - // - // head tail - // | | - // v v - // +-----------+ <-----\ +-----------+ <------\ +-----------+ - // | [null] | \----- | next | \------- | next | - // +-----------+ +-----------+ +-----------+ - // | tick | <-- bottom | tick | <-- bottom | [empty] | - // | tick | | tick | | [empty] | - // | tick | | tick | | [empty] | - // | tick | | tick | | [empty] | - // | tick | | tick | bottom --> | tick | - // | tick | | tick | | tick | - // | ... | | ... | | ... | - // | tick | | tick | | tick | - // | tick | | tick | | tick | - // | [empty] | <-- top | tick | | tick | - // | [empty] | | tick | | tick | - // | [empty] | | tick | | tick | - // +-----------+ +-----------+ <-- top top --> +-----------+ - // - // Or, if there is only one fixed-size queue, it looks something - // like either of these: - // - // head tail head tail - // | | | | - // v v v v - // +-----------+ +-----------+ - // | [null] | | [null] | - // +-----------+ +-----------+ - // | [empty] | | tick | - // | [empty] | | tick | - // | tick | <-- bottom top --> | [empty] | - // | tick | | [empty] | - // | [empty] | <-- top bottom --> | tick | - // | [empty] | | tick | - // +-----------+ +-----------+ - // - // Adding a value means moving `top` forward by one, removing means - // moving `bottom` forward by one. - // - // We let `bottom` and `top` wrap around, so when `top` is conceptually - // pointing to the end of the list, that means that the actual value is `0`. - // - // In particular, when `top === bottom`, this can mean *either* that the - // current queue is empty or that it is full. We can differentiate by - // checking whether an entry in the queue is empty (a.k.a. `=== undefined`). - - class FixedQueue { - constructor() { - this.bottom = 0; - this.top = 0; - this.list = new Array(kQueueSize); - this.next = null; - } - - push(data) { - this.list[this.top] = data; - this.top = (this.top + 1) & kQueueMask; - } - - shift() { - const nextItem = this.list[this.bottom]; - if (nextItem === undefined) - return null; - this.list[this.bottom] = undefined; - this.bottom = (this.bottom + 1) & kQueueMask; - return nextItem; - } - } - - var head = new FixedQueue(); - var tail = head; - - function push(data) { - if (head.bottom === head.top) { - // Either empty or full: - if (head.list[head.top] !== undefined) { - // It's full: Creates a new queue, sets the old queue's `.next` to it, - // and sets it as the new main queue. - head = head.next = new FixedQueue(); - } else { - // If the head is empty, that means that it was the only fixed-sized - // queue in existence. - DCHECK_EQ(head.next, null); - // This is the first tick object in existence, so we need to inform - // the C++ side that we do want to run `_tickCallback()`. - tickInfo[kHasScheduled] = 1; - } - } - head.push(data); - } - - function shift() { - const next = tail.shift(); - if (tail.top === tail.bottom) { // -> .shift() emptied the current queue. - if (tail.next !== null) { - // If there is another queue, it forms the new tail. - tail = tail.next; - } else { - // We've just run out of items. Let the native side know that it - // doesn't need to bother calling into JS to run the queue. - tickInfo[kHasScheduled] = 0; - } - } - return next; - } + const queue = new FixedQueue(); process.nextTick = nextTick; // Needs to be accessible from beyond this scope. @@ -152,7 +41,7 @@ function setupNextTick() { function _tickCallback() { let tock; do { - while (tock = shift()) { + while (tock = queue.shift()) { const asyncId = tock[async_id_symbol]; emitBefore(asyncId, tock[trigger_async_id_symbol]); // emitDestroy() places the async_id_symbol into an asynchronous queue @@ -175,8 +64,9 @@ function setupNextTick() { emitAfter(asyncId); } + tickInfo[kHasScheduled] = 0; runMicrotasks(); - } while (head.top !== head.bottom || emitPromiseRejectionWarnings()); + } while (!queue.isEmpty() || emitPromiseRejectionWarnings()); tickInfo[kHasPromiseRejections] = 0; } @@ -222,6 +112,8 @@ function setupNextTick() { args[i - 1] = arguments[i]; } - push(new TickObject(callback, args, getDefaultTriggerAsyncId())); + if (queue.isEmpty()) + tickInfo[kHasScheduled] = 1; + queue.push(new TickObject(callback, args, getDefaultTriggerAsyncId())); } } diff --git a/node.gyp b/node.gyp index 78a15bd7b838a7..3e513a4a71ceb8 100644 --- a/node.gyp +++ b/node.gyp @@ -101,6 +101,7 @@ 'lib/internal/constants.js', 'lib/internal/encoding.js', 'lib/internal/errors.js', + 'lib/internal/fixed_queue.js', 'lib/internal/freelist.js', 'lib/internal/fs.js', 'lib/internal/http.js', diff --git a/test/parallel/test-next-tick-fixed-queue-regression.js b/test/parallel/test-next-tick-fixed-queue-regression.js new file mode 100644 index 00000000000000..1fe82d02b10907 --- /dev/null +++ b/test/parallel/test-next-tick-fixed-queue-regression.js @@ -0,0 +1,18 @@ +'use strict'; + +const common = require('../common'); + +// This tests a highly specific regression tied to the FixedQueue size, which +// was introduced in Node.js 9.7.0: https://github.com/nodejs/node/pull/18617 +// More specifically, a nextTick list could potentially end up not fully +// clearing in one run through if exactly 2048 ticks were added after +// microtasks were executed within the nextTick loop. + +process.nextTick(() => { + Promise.resolve(1).then(() => { + for (let i = 0; i < 2047; i++) + process.nextTick(common.mustCall()); + const immediate = setImmediate(common.mustNotCall()); + process.nextTick(common.mustCall(() => clearImmediate(immediate))); + }); +}); From a6a26a1bdf54c1b895217c3a3c0d7a3a1d72eaa5 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 3 May 2018 09:10:26 +0200 Subject: [PATCH 2/2] fixup: improve perf slightly and add tests --- lib/internal/fixed_queue.js | 33 +++++++++++++++--------------- test/parallel/test-fixed-queue.js | 34 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 16 deletions(-) create mode 100644 test/parallel/test-fixed-queue.js diff --git a/lib/internal/fixed_queue.js b/lib/internal/fixed_queue.js index d56b667a434b58..7571a8f67e36d2 100644 --- a/lib/internal/fixed_queue.js +++ b/lib/internal/fixed_queue.js @@ -24,8 +24,8 @@ const kMask = kSize - 1; // | item | | item | | item | // | [empty] | <-- top | item | | item | // | [empty] | | item | | item | -// | [empty] | | item | | item | -// +-----------+ +-----------+ <-- top top --> +-----------+ +// | [empty] | | [empty] | <-- top top --> | [empty] | +// +-----------+ +-----------+ +-----------+ // // Or, if there is only one circular buffer, it looks something // like either of these: @@ -45,14 +45,12 @@ const kMask = kSize - 1; // +-----------+ +-----------+ // // Adding a value means moving `top` forward by one, removing means -// moving `bottom` forward by one. -// -// We let `bottom` and `top` wrap around, so when `top` is conceptually -// pointing to the end of the list, that means that the actual value is `0`. +// moving `bottom` forward by one. After reaching the end, the queue +// wraps around. // -// In particular, when `top === bottom`, this can mean *either* that the -// current queue is empty or that it is full. We can differentiate by -// checking whether an entry in the queue is empty (a.k.a. `=== undefined`). +// When `top === bottom` the current queue is empty and when +// `top + 1 === bottom` it's full. This wastes a single space of storage +// but allows much quicker checks. const FixedCircularBuffer = class FixedCircularBuffer { constructor() { @@ -63,7 +61,11 @@ const FixedCircularBuffer = class FixedCircularBuffer { } isEmpty() { - return this.top === this.bottom && this.list[this.top] === undefined; + return this.top === this.bottom; + } + + isFull() { + return ((this.top + 1) & kMask) === this.bottom; } push(data) { @@ -83,7 +85,7 @@ const FixedCircularBuffer = class FixedCircularBuffer { module.exports = class FixedQueue { constructor() { - this.tail = this.head = new FixedCircularBuffer(); + this.head = this.tail = new FixedCircularBuffer(); } isEmpty() { @@ -91,19 +93,18 @@ module.exports = class FixedQueue { } push(data) { - let { head } = this; - if (head.bottom === head.top && head.list[head.top] !== undefined) { + if (this.head.isFull()) { // Head is full: Creates a new queue, sets the old queue's `.next` to it, // and sets it as the new main queue. - this.head = head = head.next = new FixedCircularBuffer(); + this.head = this.head.next = new FixedCircularBuffer(); } - head.push(data); + this.head.push(data); } shift() { const { tail } = this; const next = tail.shift(); - if (tail.top === tail.bottom && tail.next !== null) { + if (tail.isEmpty() && tail.next !== null) { // If there is another queue, it forms the new tail. this.tail = tail.next; } diff --git a/test/parallel/test-fixed-queue.js b/test/parallel/test-fixed-queue.js new file mode 100644 index 00000000000000..a50be1309a5ea8 --- /dev/null +++ b/test/parallel/test-fixed-queue.js @@ -0,0 +1,34 @@ +// Flags: --expose-internals +'use strict'; + +require('../common'); + +const assert = require('assert'); +const FixedQueue = require('internal/fixed_queue'); + +{ + const queue = new FixedQueue(); + assert.strictEqual(queue.head, queue.tail); + assert(queue.isEmpty()); + queue.push('a'); + assert(!queue.isEmpty()); + assert.strictEqual(queue.shift(), 'a'); + assert.strictEqual(queue.shift(), null); +} + +{ + const queue = new FixedQueue(); + for (let i = 0; i < 2047; i++) + queue.push('a'); + assert(queue.head.isFull()); + queue.push('a'); + assert(!queue.head.isFull()); + + assert.notStrictEqual(queue.head, queue.tail); + for (let i = 0; i < 2047; i++) + assert.strictEqual(queue.shift(), 'a'); + assert.strictEqual(queue.head, queue.tail); + assert(!queue.isEmpty()); + assert.strictEqual(queue.shift(), 'a'); + assert(queue.isEmpty()); +}