Is there a reason to move this into the constructor? I find moving side-effects into constructors to be surprising about 99 % of the time…

Probably not. Was trying to consolidate between the two nextTick functions but you're right, this likely doesn't belong here.

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

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.

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.)

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.

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.

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. 😆

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

@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. :)

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.

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.

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

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.

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.

We have very different concepts of what is ideal :)

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