node
node copied to clipboard
nodejs
util: increase robustness with primordials
I noticed in #41003 that the loop-slice logic in addNumericSeparator could be done with a single regex replace.
I did not change the implementation of addNumericSeparatorEnd because the logic is slightly different, but I can try to do that if desired.
cc @nodejs/util
Seemingly related CI failure:
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected
+ '123_456_789.123_456_789_.12_345_678'
- '123_456_789.123_456_78'
^
at Object.<anonymous> (/home/runner/work/node/node/test/parallel/test-util-inspect.js:3169:10)
at Module._compile (node:internal/modules/cjs/loader:1097:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1149:10)
at Module.load (node:internal/modules/cjs/loader:975:32)
at Function.Module._load (node:internal/modules/cjs/loader:822:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
at node:internal/main/run_main_module:17:47 {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: '123_456_789.123_456_789_.12_345_678',
expected: '123_456_789.123_456_78',
operator: 'strictEqual'
}
@BridgeAR are there tests that preserve the performance?
I'm surprised that a single regex replace would be slower than multiple loops over the string, but i'm also confused why performance is particularly important on a debugging method.
Here is a short benchmark (the second one is with the regular expression):
util.inspect is not only used for debugging. It is in fact used in a lot of production code as it allows to serialize lots of things. It has been a performance bottlenecks in real applications years ago before optimizing the performance. Now it is mostly fine and we should try to keep that.
Alrighty. I'll update this PR to revert the regex refactor.
It seems like if the performance here is important, there should be benchmarks which would fail on a PR that makes it slower - then it wouldn't fall on one person to be paying attention to ensure things aren't slowed down.
We discussed such performance tests before but they would have to verify too many cases and take too long to be run regularly. We do have benchmarks in place to run though.
I've updated the PR to revert the regex change, and only leave the primordial changes.
In the event the TSC makes a decision that changes the status quo such that these changes are no longer desirable, I will be happy to volunteer to make a PR to remove these, but I hope that until that decision, the status quo means they should land.
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1063/
confidence improvement accuracy (*) (**) (***)
util/inspect-array.js type='denseArray' len=100000 n=500 -3.07 % ±5.15% ±6.86% ±8.93%
util/inspect-array.js type='denseArray' len=100 n=500 0.65 % ±5.80% ±7.71% ±10.04%
util/inspect-array.js type='denseArray_showHidden' len=100000 n=500 0.19 % ±4.16% ±5.53% ±7.21%
util/inspect-array.js type='denseArray_showHidden' len=100 n=500 * -5.42 % ±4.92% ±6.56% ±8.58%
util/inspect-array.js type='mixedArray' len=100000 n=500 -1.08 % ±3.11% ±4.14% ±5.40%
util/inspect-array.js type='mixedArray' len=100 n=500 -2.01 % ±4.93% ±6.57% ±8.56%
util/inspect-array.js type='sparseArray' len=100000 n=500 -1.11 % ±3.55% ±4.73% ±6.15%
util/inspect-array.js type='sparseArray' len=100 n=500 1.30 % ±7.91% ±10.53% ±13.70%
util/inspect.js option='colors' method='Array' n=20000 1.82 % ±3.37% ±4.49% ±5.87%
util/inspect.js option='colors' method='Date' n=20000 -2.46 % ±4.87% ±6.48% ±8.45%
util/inspect.js option='colors' method='Error' n=20000 0.17 % ±4.82% ±6.42% ±8.37%
util/inspect.js option='colors' method='Number' n=20000 -3.12 % ±6.75% ±9.01% ±11.78%
util/inspect.js option='colors' method='Object_deep_ln' n=20000 0.19 % ±3.43% ±4.60% ±6.04%
util/inspect.js option='colors' method='Object_empty' n=20000 1.95 % ±6.60% ±8.79% ±11.43%
util/inspect.js option='colors' method='Object' n=20000 -1.22 % ±2.99% ±3.98% ±5.19%
util/inspect.js option='colors' method='Set' n=20000 -4.74 % ±4.83% ±6.43% ±8.37%
util/inspect.js option='colors' method='String_boxed' n=20000 1.84 % ±4.83% ±6.43% ±8.40%
util/inspect.js option='colors' method='String_complex' n=20000 -1.52 % ±6.22% ±8.28% ±10.78%
util/inspect.js option='colors' method='String' n=20000 -2.28 % ±5.09% ±6.78% ±8.83%
util/inspect.js option='colors' method='TypedArray_extra' n=20000 0.29 % ±2.50% ±3.33% ±4.33%
util/inspect.js option='colors' method='TypedArray' n=20000 1.21 % ±2.83% ±3.76% ±4.90%
util/inspect.js option='none' method='Array' n=20000 5.01 % ±8.91% ±11.90% ±15.56%
util/inspect.js option='none' method='Date' n=20000 -4.29 % ±6.08% ±8.11% ±10.59%
util/inspect.js option='none' method='Error' n=20000 0.77 % ±4.53% ±6.03% ±7.85%
util/inspect.js option='none' method='Number' n=20000 2.36 % ±5.67% ±7.54% ±9.82%
util/inspect.js option='none' method='Object_deep_ln' n=20000 -0.76 % ±2.48% ±3.30% ±4.29%
util/inspect.js option='none' method='Object_empty' n=20000 -4.22 % ±4.94% ±6.57% ±8.56%
util/inspect.js option='none' method='Object' n=20000 0.24 % ±3.81% ±5.08% ±6.62%
util/inspect.js option='none' method='Set' n=20000 -1.59 % ±5.16% ±6.87% ±8.95%
util/inspect.js option='none' method='String_boxed' n=20000 -2.31 % ±5.86% ±7.79% ±10.15%
util/inspect.js option='none' method='String_complex' n=20000 -1.21 % ±5.83% ±7.77% ±10.14%
util/inspect.js option='none' method='String' n=20000 -0.67 % ±6.39% ±8.51% ±11.07%
util/inspect.js option='none' method='TypedArray_extra' n=20000 3.68 % ±4.67% ±6.22% ±8.13%
util/inspect.js option='none' method='TypedArray' n=20000 -0.30 % ±3.36% ±4.48% ±5.87%
util/inspect.js option='showHidden' method='Array' n=20000 -0.38 % ±1.23% ±1.64% ±2.13%
util/inspect.js option='showHidden' method='Date' n=20000 -0.53 % ±6.14% ±8.16% ±10.62%
util/inspect.js option='showHidden' method='Error' n=20000 -2.77 % ±2.80% ±3.73% ±4.85%
util/inspect.js option='showHidden' method='Number' n=20000 4.34 % ±5.14% ±6.87% ±9.00%
util/inspect.js option='showHidden' method='Object_deep_ln' n=20000 0.57 % ±2.89% ±3.85% ±5.01%
util/inspect.js option='showHidden' method='Object_empty' n=20000 * -4.74 % ±4.53% ±6.04% ±7.89%
util/inspect.js option='showHidden' method='Object' n=20000 * -8.57 % ±7.39% ±9.88% ±12.96%
util/inspect.js option='showHidden' method='Set' n=20000 -3.68 % ±5.02% ±6.68% ±8.69%
util/inspect.js option='showHidden' method='String_boxed' n=20000 0.86 % ±4.94% ±6.57% ±8.56%
util/inspect.js option='showHidden' method='String_complex' n=20000 -3.38 % ±5.51% ±7.34% ±9.57%
util/inspect.js option='showHidden' method='String' n=20000 -0.24 % ±5.41% ±7.21% ±9.39%
util/inspect.js option='showHidden' method='TypedArray_extra' n=20000 0.80 % ±4.36% ±5.85% ±7.71%
util/inspect.js option='showHidden' method='TypedArray' n=20000 -1.36 % ±2.46% ±3.27% ±4.26%
util/inspect-proxy.js isProxy=0 showProxy=0 n=100000 -1.26 % ±5.92% ±7.92% ±10.38%
util/inspect-proxy.js isProxy=0 showProxy=1 n=100000 -1.64 % ±2.16% ±2.88% ±3.77%
util/inspect-proxy.js isProxy=1 showProxy=0 n=100000 -0.05 % ±2.28% ±3.05% ±4.00%
util/inspect-proxy.js isProxy=1 showProxy=1 n=100000 * -3.57 % ±2.94% ±3.91% ±5.09%
Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 51 comparisons, you can thus
expect the following amount of false-positive results:
2.55 false positives, when considering a 5% risk acceptance (*, **, ***),
0.51 false positives, when considering a 1% risk acceptance (**, ***),
0.05 false positives, when considering a 0.1% risk acceptance (***)
@aduh95 I'm not sure how to read it, but if it includes my latest commit then it wouldn't show the impact of the regex implementation, unfortunately.
@BridgeAR now that this no longer includes the regex, would you be willing to rereview, or dismiss your block?
I would like to have a conclusion about the primordials before changing things in that direction further.
@BridgeAR until there is such a decision, why not continue with the status quo? (see https://github.com/nodejs/node/pull/41212#issuecomment-997061916 also)
Given that this does not significantly expand primordials usage, and that @ljharb is volunteering to undo the primordials later if that's the decision, and that there's no reason to believe that we will imminently arrive at a decision on primordials (although we are making progress, just slow progress), I think we should land this. @BridgeAR What do you think?
Now that the primordials vote has made it clear that we will not be removing them, and considering this has been here awhile with no response, can it be landed?
Now that the primordials vote has made it clear that we will not be removing them, and considering this has been here awhile with no response, can it be landed?
/ping @BridgeAR
Any update here? It's been 7 months. I've freshly rebased this and assuming tests pass, I'd love it to land.
Based on the lack of a response and the resolution of the primordials discussion elsewhere, I'm going to clear @BridgeAR's objection. If you still oppose this, @BridgeAR, please put it back. Thanks.
Hi @ljharb! This PR didn't land cleanly on v18.x. Could you please create a backport?
