Skip to content

Conversation

@gurgunday
Copy link
Member

@gurgunday gurgunday commented Oct 26, 2025

After:

console/log.js
console/log.js variant="plain" n=2000000: 10,760,828.681086333
console/log.js variant="format" n=2000000: 3,104,163.7037100764
console/log.js variant="object" n=2000000: 1,304,646.6897291192
console/log.js variant="group" n=2000000: 5,147,729.648404505
console/log.js variant="info" n=2000000: 10,760,667.090984877
console/log.js variant="warn" n=2000000: 10,730,321.278856428
console/log.js variant="error" n=2000000: 10,739,136.76713795

Before:

console/log.js
console/log.js variant="plain" n=2000000: 6,362,790.401730679
console/log.js variant="format" n=2000000: 3,067,171.4347502897
console/log.js variant="object" n=2000000: 1,260,652.1490095456
console/log.js variant="group" n=2000000: 2,917,442.398797694
console/log.js variant="info" n=2000000: 6,465,884.779549699
console/log.js variant="warn" n=2000000: 6,477,797.477083796
console/log.js variant="error" n=2000000: 6,345,277.494387991

This peaked my interest in the flame graph:

  • Adds a fast path for when we log one string, which avoids ReflectApply and options validation. inspect has a similar fast path, but it's too late for console.log.
  • Replaces a SafeWeakMap that acts as if it's present in every instance with a symbol. It should be faster since we avoid method calls and || on every invocation.

P.S. Couldn't find any benchmarks for console.log. And inspect benchmarks won't pick this up...

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added console Issues and PRs related to the console subsystem. needs-ci PRs that need a full CI run. labels Oct 26, 2025
@gurgunday gurgunday added the performance Issues and PRs related to the performance of Node.js. label Oct 26, 2025
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

It would also be possible to speed up other arguments by calling inspect directly for these. I guess that would also improve their performance a bit.

About the map vs symbol property: I doubt that it changes the performance, while it does make the property visible.

@codecov
Copy link

codecov bot commented Oct 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.56%. Comparing base (fb84f35) to head (03fec6a).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60422      +/-   ##
==========================================
- Coverage   88.57%   88.56%   -0.02%     
==========================================
  Files         704      704              
  Lines      207778   207798      +20     
  Branches    40030    40027       -3     
==========================================
- Hits       184049   184040       -9     
- Misses      15768    15799      +31     
+ Partials     7961     7959       -2     
Files with missing lines Coverage Δ
lib/internal/console/constructor.js 98.19% <100.00%> (+0.05%) ⬆️

... and 36 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.

@gurgunday
Copy link
Member Author

gurgunday commented Oct 27, 2025

@BridgeAR I'm getting a measurable, consistent difference:

With the WeakMap change:

console/log.js
console/log.js variant="plain" n=2000000: 10,883,048.308746813
console/log.js variant="format" n=2000000: 3,149,799.334788839
console/log.js variant="object" n=2000000: 1,264,378.6850980576
console/log.js variant="group" n=2000000: 5,179,205.360995469
console/log.js variant="info" n=2000000: 10,917,626.507997163
console/log.js variant="warn" n=2000000: 10,770,223.543582655
console/log.js variant="error" n=2000000: 10,905,653.149791192

Without it:

console/log.js
console/log.js variant="plain" n=2000000: 10,081,197.747104334
console/log.js variant="format" n=2000000: 3,018,537.215869073
console/log.js variant="object" n=2000000: 1,261,919.5199784338
console/log.js variant="group" n=2000000: 4,423,174.704583957
console/log.js variant="info" n=2000000: 10,469,331.438858952
console/log.js variant="warn" n=2000000: 10,366,440.721452443
console/log.js variant="error" n=2000000: 10,422,488.603660118

I'll let you decide, but it seems to be there

@aduh95
Copy link
Contributor

aduh95 commented Oct 27, 2025

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1749/

                                          confidence improvement accuracy (*)   (**)  (***)
console/log.js variant='error' n=2000000         ***     73.77 %       ±2.08% ±2.78% ±3.66%
console/log.js variant='format' n=2000000                 0.78 %       ±0.89% ±1.18% ±1.54%
console/log.js variant='group' n=2000000         ***    103.48 %       ±2.58% ±3.47% ±4.58%
console/log.js variant='info' n=2000000          ***     74.01 %       ±1.35% ±1.80% ±2.35%
console/log.js variant='object' n=2000000                -0.06 %       ±1.06% ±1.42% ±1.85%
console/log.js variant='plain' n=2000000         ***     73.56 %       ±2.21% ±2.96% ±3.90%
console/log.js variant='warn' n=2000000          ***     74.45 %       ±1.45% ±1.94% ±2.54%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 7 comparisons, you can thus
expect the following amount of false-positive results:
  0.35 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.07 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 27, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 27, 2025
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. console Issues and PRs related to the console subsystem. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants