Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

process: refactor nextTick for clarity #17738

Closed
wants to merge 2 commits into from
Closed

process: refactor nextTick for clarity #17738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fdf0e05
process: refactor nextTick for clarity
apapirovski Dec 18, 2017
1d84f1d
fixup: remove side-effects from constructor
apapirovski Dec 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 49 additions & 101 deletions lib/internal/process/next_tick.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,9 @@
'use strict';

// This value is used to prevent the nextTickQueue from becoming too
// large and cause the process to run out of memory. When this value
// is reached the nextTimeQueue array will be shortened (see tickDone
// for details).
const kMaxCallbacksPerLoop = 1e4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this expose infinite nextTick loop problems? and / or, isn't this a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This became irrelevant when @mscdex turned this into a linked list instead of an Array. I don't really see that it does anything right now other than make sure the indexes don't overflow and since I got rid of length & index, it's no longer necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, FWIW, our benchmarks are an indirect test for this and they still pass. (If one were to remove this without removing the index & length values then the integers overflow.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was to prevent infinite nextTick recursion... I'm not sure how a linkedlist helps that? Maybe I didn't look deep enough? Not sure.

Copy link
Member Author

@apapirovski apapirovski Dec 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't prevent infinite recursion. Nothing does, our documentation even calls it out as an issue.

When an Array used to be used for nextTick, we wouldn't clear it until 1e4 runs so it could grow to near Infinite size without this condition to remove stale tick objects.

Edit: This was confusingly worded. What I mean is: before this 1e4 limit was introduced, we never cleared the Array until the whole outer loop completed but this meant that _tickCallback could run long enough until the nextTickQueue array was so big that it hogged all available memory. The 1e4 limit was then introduced to occasionally empty the nextTickQueue so the process wouldn't run out of memory.

With a linked list this is no longer necessary as on each tock it adjusts the linked list and the GC can do its job whenever it wants/needs to. The only reason that code then remained was for the cases where an integer overflow could occur due to reaching too large of an index & length.

Copy link
Member Author

@apapirovski apapirovski Dec 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: The next tick queue is completely drained on each pass of the event loop before additional I/O is processed. As a result, recursively setting nextTick callbacks will block any I/O from happening, just like a while(true); loop.

https://nodejs.org/dist/latest-v9.x/docs/api/process.html#process_process_nexttick_callback_args

Just try const set = () => process.nextTick(set); set(); in repl and be prepared to force quit it. 😆

Copy link
Member Author

@apapirovski apapirovski Dec 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fishrock123 here's the commit that introduced this code apapirovski@5757642

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apapirovski That was a very helpful explanation… on some level it’s sad to understand code only once it’s going away, but I always wondered about this. :)


exports.setup = setupNextTick;
// Will be overwritten when setupNextTick() is called.
exports.nextTick = null;

class NextTickQueue {
constructor() {
this.head = null;
this.tail = null;
}

push(v) {
const entry = { data: v, next: null };
if (this.tail !== null)
this.tail.next = entry;
else
this.head = entry;
this.tail = entry;
}

shift() {
if (this.head === null)
return;
const ret = this.head.data;
if (this.head === this.tail)
this.head = this.tail = null;
else
this.head = this.head.next;
return ret;
}

clear() {
this.head = null;
this.tail = null;
}
}

function setupNextTick() {
const async_wrap = process.binding('async_wrap');
const async_hooks = require('internal/async_hooks');
Expand All @@ -56,15 +18,47 @@ function setupNextTick() {
// Grab the constants necessary for working with internal arrays.
const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants;
const { async_id_symbol, trigger_async_id_symbol } = async_wrap;
const nextTickQueue = new NextTickQueue();
var microtasksScheduled = false;

// Used to run V8's micro task queue.
var _runMicrotasks = {};
// tickInfo is used so that the C++ code in src/node.cc can
// have easy access to our nextTick state, and avoid unnecessary
// calls into JS land.
// runMicrotasks is used to run V8's micro task queue.
const [
tickInfo,
runMicrotasks
] = process._setupNextTick(_tickCallback);

// *Must* match Environment::TickInfo::Fields in src/env.h.
var kIndex = 0;
var kLength = 1;
const kScheduled = 0;

const nextTickQueue = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, why make this an object literal instead of the class that existed before? I would think V8 still optimizes classes (using hidden classes) better than object literals.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, It does the same thing with both in terms of hidden classes. (That's part of the reason Object.create(null) is used in a lot of our code because it prevents creating a hidden class and just skips straight to dictionary object mode.)

The reasoning for the change is mostly that it's straight-up confusing the first time one looks at this code, NextTickQueue is declared outside of setup and it appears that multiple queues might be supported until one starts reading into the source.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the tickInfo[kScheduled] flag is flipped inside push and shift now, so declaring it outside of setup is impossible.

Copy link
Member

@AndreasMadsen AndreasMadsen Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Properly we could merge the code for the timers linked list and the nextTick linked list, but this is fine for now.

PS: new class NextTickQueue {} is a valid syntax, although I find it equally odd.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we wouldn't. nextTick has a special version that's optimized for its own use because it only needs a singly linked list, whereas timers definitely need doubly linked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have very different concepts of what is ideal :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using doubly linked list, despite it being completely unnecessary for nextTick, would come with a significant performance regression and memory consumption increase...

head: null,
tail: null,
push(data) {
const entry = { data, next: null };
if (this.tail !== null) {
this.tail.next = entry;
} else {
this.head = entry;
tickInfo[kScheduled] = 1;
}
this.tail = entry;
},
shift() {
if (this.head === null)
return;
const ret = this.head.data;
if (this.head === this.tail) {
this.head = this.tail = null;
tickInfo[kScheduled] = 0;
} else {
this.head = this.head.next;
}
return ret;
}
};

var microtasksScheduled = false;

process.nextTick = nextTick;
// Needs to be accessible from beyond this scope.
Expand All @@ -73,25 +67,6 @@ function setupNextTick() {
// Set the nextTick() function for internal usage.
exports.nextTick = internalNextTick;

// This tickInfo thing is used so that the C++ code in src/node.cc
// can have easy access to our nextTick state, and avoid unnecessary
// calls into JS land.
const tickInfo = process._setupNextTick(_tickCallback, _runMicrotasks);

_runMicrotasks = _runMicrotasks.runMicrotasks;

function tickDone() {
if (tickInfo[kLength] !== 0) {
if (tickInfo[kLength] <= tickInfo[kIndex]) {
nextTickQueue.clear();
tickInfo[kLength] = 0;
} else {
tickInfo[kLength] -= tickInfo[kIndex];
}
}
tickInfo[kIndex] = 0;
}

const microTasksTickObject = {
callback: runMicrotasksCallback,
args: undefined,
Expand All @@ -105,38 +80,27 @@ function setupNextTick() {
// For the moment all microtasks come from the void until the PromiseHook
// API is implemented.
nextTickQueue.push(microTasksTickObject);

tickInfo[kLength]++;
microtasksScheduled = true;
}

function runMicrotasksCallback() {
microtasksScheduled = false;
_runMicrotasks();
runMicrotasks();

if (tickInfo[kIndex] < tickInfo[kLength] ||
emitPendingUnhandledRejections()) {
if (nextTickQueue.head !== null || emitPendingUnhandledRejections())
scheduleMicrotasks();
}
}

function _tickCallback() {
let tock;
do {
while (tickInfo[kIndex] < tickInfo[kLength]) {
++tickInfo[kIndex];
const tock = nextTickQueue.shift();

// CHECK(Number.isSafeInteger(tock[async_id_symbol]))
// CHECK(tock[async_id_symbol] > 0)
// CHECK(Number.isSafeInteger(tock[trigger_async_id_symbol]))
// CHECK(tock[trigger_async_id_symbol] > 0)

while (tock = nextTickQueue.shift()) {
const asyncId = tock[async_id_symbol];
emitBefore(asyncId, tock[trigger_async_id_symbol]);
// emitDestroy() places the async_id_symbol into an asynchronous queue
// that calls the destroy callback in the future. It's called before
// calling tock.callback so destroy will be called even if the callback
// throws an exception that is handles by 'uncaughtException' or a
// throws an exception that is handled by 'uncaughtException' or a
// domain.
// TODO(trevnorris): This is a bit of a hack. It relies on the fact
// that nextTick() doesn't allow the event loop to proceed, but if
Expand All @@ -152,24 +116,21 @@ function setupNextTick() {
Reflect.apply(callback, undefined, tock.args);

emitAfter(asyncId);

if (kMaxCallbacksPerLoop < tickInfo[kIndex])
tickDone();
}
tickDone();
_runMicrotasks();
runMicrotasks();
emitPendingUnhandledRejections();
} while (tickInfo[kLength] !== 0);
} while (nextTickQueue.head !== null);
}

class TickObject {
constructor(callback, args, asyncId, triggerAsyncId) {
constructor(callback, args, triggerAsyncId) {
// this must be set to null first to avoid function tracking
// on the hidden class, revisit in V8 versions after 6.2
this.callback = null;
this.callback = callback;
this.args = args;

const asyncId = ++async_id_fields[kAsyncIdCounter];
this[async_id_symbol] = asyncId;
this[trigger_async_id_symbol] = triggerAsyncId;

Expand Down Expand Up @@ -203,13 +164,7 @@ function setupNextTick() {
args[i - 1] = arguments[i];
}

// In V8 6.2, moving tickInfo & async_id_fields[kAsyncIdCounter] into the
// TickObject incurs a significant performance penalty in the
// next-tick-breadth-args benchmark (revisit later)
++tickInfo[kLength];
nextTickQueue.push(new TickObject(callback,
args,
++async_id_fields[kAsyncIdCounter],
nextTickQueue.push(new TickObject(callback, args,
getDefaultTriggerAsyncId()));
}

Expand Down Expand Up @@ -238,13 +193,6 @@ function setupNextTick() {

if (triggerAsyncId === null)
triggerAsyncId = getDefaultTriggerAsyncId();
// In V8 6.2, moving tickInfo & async_id_fields[kAsyncIdCounter] into the
// TickObject incurs a significant performance penalty in the
// next-tick-breadth-args benchmark (revisit later)
++tickInfo[kLength];
nextTickQueue.push(new TickObject(callback,
args,
++async_id_fields[kAsyncIdCounter],
triggerAsyncId));
nextTickQueue.push(new TickObject(callback, args, triggerAsyncId));
}
}
14 changes: 3 additions & 11 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,24 +207,16 @@ inline Environment::TickInfo::TickInfo() {
fields_[i] = 0;
}

inline uint32_t* Environment::TickInfo::fields() {
inline uint8_t* Environment::TickInfo::fields() {
return fields_;
}

inline int Environment::TickInfo::fields_count() const {
return kFieldsCount;
}

inline uint32_t Environment::TickInfo::index() const {
return fields_[kIndex];
}

inline uint32_t Environment::TickInfo::length() const {
return fields_[kLength];
}

inline void Environment::TickInfo::set_index(uint32_t value) {
fields_[kIndex] = value;
inline uint8_t Environment::TickInfo::scheduled() const {
return fields_[kScheduled];
}

inline void Environment::AssignToContext(v8::Local<v8::Context> context,
Expand Down
11 changes: 4 additions & 7 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,23 +457,20 @@ class Environment {

class TickInfo {
public:
inline uint32_t* fields();
inline uint8_t* fields();
inline int fields_count() const;
inline uint32_t index() const;
inline uint32_t length() const;
inline void set_index(uint32_t value);
inline uint8_t scheduled() const;

private:
friend class Environment; // So we can call the constructor.
inline TickInfo();

enum Fields {
kIndex,
kLength,
kScheduled,
kFieldsCount
};

uint32_t fields_[kFieldsCount];
uint8_t fields_[kFieldsCount];

DISALLOW_COPY_AND_ASSIGN(TickInfo);
};
Expand Down
33 changes: 20 additions & 13 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ using v8::SealHandleScope;
using v8::String;
using v8::TryCatch;
using v8::Uint32Array;
using v8::Uint8Array;
using v8::Undefined;
using v8::V8;
using v8::Value;
Expand Down Expand Up @@ -867,25 +868,32 @@ void SetupNextTick(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

CHECK(args[0]->IsFunction());
CHECK(args[1]->IsObject());

env->set_tick_callback_function(args[0].As<Function>());

env->SetMethod(args[1].As<Object>(), "runMicrotasks", RunMicrotasks);

// Do a little housekeeping.
env->process_object()->Delete(
env->context(),
FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupNextTick")).FromJust();
FIXED_ONE_BYTE_STRING(env->isolate(), "_setupNextTick")).FromJust();

// Values use to cross communicate with processNextTick.
uint32_t* const fields = env->tick_info()->fields();
uint32_t const fields_count = env->tick_info()->fields_count();
uint8_t* const fields = env->tick_info()->fields();
uint8_t const fields_count = env->tick_info()->fields_count();

Local<ArrayBuffer> array_buffer =
ArrayBuffer::New(env->isolate(), fields, sizeof(*fields) * fields_count);

args.GetReturnValue().Set(Uint32Array::New(array_buffer, 0, fields_count));
v8::Local<v8::Function> run_microtasks_fn =
env->NewFunctionTemplate(RunMicrotasks)->GetFunction(env->context())
.ToLocalChecked();
run_microtasks_fn->SetName(
FIXED_ONE_BYTE_STRING(env->isolate(), "runMicrotasks"));

Local<Array> ret = Array::New(env->isolate(), 2);
ret->Set(env->context(), 0,
Uint8Array::New(array_buffer, 0, fields_count)).FromJust();
ret->Set(env->context(), 1, run_microtasks_fn).FromJust();

args.GetReturnValue().Set(ret);
}

void PromiseRejectCallback(PromiseRejectMessage message) {
Expand Down Expand Up @@ -1011,7 +1019,7 @@ void InternalCallbackScope::Close() {

Environment::TickInfo* tick_info = env_->tick_info();

if (tick_info->length() == 0) {
if (tick_info->scheduled() == 0) {
env_->isolate()->RunMicrotasks();
}

Expand All @@ -1022,10 +1030,7 @@ void InternalCallbackScope::Close() {
CHECK_EQ(env_->trigger_async_id(), 0);
}

Local<Object> process = env_->process_object();

if (tick_info->length() == 0) {
tick_info->set_index(0);
if (tick_info->scheduled() == 0) {
return;
}

Expand All @@ -1034,6 +1039,8 @@ void InternalCallbackScope::Close() {
CHECK_EQ(env_->trigger_async_id(), 0);
}

Local<Object> process = env_->process_object();

if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
failed_ = true;
}
Expand Down