timers: check for undefined instead of nullish value

[gurgunday]

Now that the Priority Queue heap is a PACKED array, we can safely check if the value is undefined instead of nullish here

Relevant code:

node/lib/internal/priority_queue.js

Lines 11 to 32 in 32851f3

	#heap = [undefined, undefined];
	#setPosition;
	#size = 0;
	constructor(comparator, setPosition) {
	if (comparator !== undefined)
	this.#compare = comparator;
	if (setPosition !== undefined)
	this.#setPosition = setPosition;
	}
	insert(value) {
	const heap = this.#heap;
	const pos = ++this.#size;
	heap[pos] = value;
	this.percolateUp(pos);
	}
	peek() {
	return this.#heap[1];
	}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.52%. Comparing base (32851f3) to head (0c18b90).
⚠️ Report is 188 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60124      +/-   ##
==========================================
- Coverage   88.53%   88.52%   -0.02%     
==========================================
  Files         703      703              
  Lines      207833   207833              
  Branches    40014    40011       -3     
==========================================
- Hits       184011   183981      -30     
- Misses      15821    15859      +38     
+ Partials     8001     7993       -8     

Files with missing lines	Coverage Δ
lib/internal/timers.js	99.58% <100.00%> (ø)

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aduh95]

But why?

[gurgunday]

The only values in the queue are undefined and TimersList, checking explicitly against undefined communicates this more clearly - I myself thought null could somehow be in the queue when I first read this part

We do the same when shifting the FixedQueue at task_queues, where only null and TickObject are in the list :

node/lib/internal/process/task_queues.js

Line 75 in b757a8f

while ((tock = queue.shift()) !== null) {

I'm trying to align the various queue codes and their usage a little bit, but I can close the PR if you think nullish check is still fine

[aduh95]

I don't really see how this communicate any intent any clearer: checking for === undefined doesn't communicate that it never takes the value null, it only means that if it takes the value null it would not be treated same as undefined. My preference is to keep the current syntax, and always treat nullish values the same unless there's a good reason to do so. (I'm not blocking, count me as -0.5)

[gurgunday]

Well, I see your reasoning, and honestly this is such a small change that I wouldn't press on it

However, we're making the check stricter, which is faster, albeit pretty insignificantly (the bytecode for a nullish check, TestUndetectable, checks a few more things than a direct TestUndefined)

function a(b) {
  return b === undefined;
}
a(5);

[generated bytecode for function: a (0x36cd517d9c39 <SharedFunctionInfo a>)]
Bytecode length: 4
Parameter count 2
Register count 0
Frame size 0
   18 S> 0x343d26581e90 @    0 : 0b 03             Ldar a0
         0x343d26581e92 @    2 : 1f                TestUndefined
   41 S> 0x343d26581e93 @    3 : ae                Return
Constant pool (size = 0)
Handler Table (size = 0)
Source Position Table (size = 6)
0x343d26581e99 <Other heap object (TRUSTED_BYTE_ARRAY_TYPE)>

function a(b) {
  return b == null;
}
a(5);

[generated bytecode for function: a (0x37cbf4859c39 <SharedFunctionInfo a>)]
Bytecode length: 4
Parameter count 2
Register count 0
Frame size 0
   18 S> 0x2768f5841e90 @    0 : 0b 03             Ldar a0
         0x2768f5841e92 @    2 : 1d                TestUndetectable
   40 S> 0x2768f5841e93 @    3 : ae                Return
Constant pool (size = 0)
Handler Table (size = 0)
Source Position Table (size = 6)
0x2768f5841e99 <Other heap object (TRUSTED_BYTE_ARRAY_TYPE)>

I won't leave the PR hanging for long if we don't want to do it though

[aduh95]

If you're able to run a benchmark on that specific part of the code that shows a non-zero perf diff, I would be convinced. But until then, my preference is to keep the current syntax.
BTW, if using to == null does result in a noticeable perf regression, we should probably add a lint rule and forbid it in the whole code base, otherwise it's not unlikely it will be reverted in a future change.

[gurgunday]

It's definitely not noticeable