util: add fast path to stripVTControlCharacters#61833
util: add fast path to stripVTControlCharacters#61833nodejs-github-bot merged 5 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61833 +/- ##
=======================================
Coverage 89.72% 89.73%
=======================================
Files 675 675
Lines 204797 204804 +7
Branches 39344 39353 +9
=======================================
+ Hits 183752 183773 +21
+ Misses 13324 13291 -33
- Partials 7721 7740 +19
🚀 New features to boost your workflow:
|
addaleax
left a comment
There was a problem hiding this comment.
Can you leave a comment in the source about why this short-circuiting is added? Because it still "looks" redundant when just reading the source.
lib/internal/util/inspect.js
Outdated
| if (!StringPrototypeIncludes(str, '\u001B') && | ||
| !StringPrototypeIncludes(str, '\u009B')) | ||
| return str; |
There was a problem hiding this comment.
You are doing two passes here, why not use a simple regex here that only checks these 2 characters in one pass?
| if (!StringPrototypeIncludes(str, '\u001B') && | |
| !StringPrototypeIncludes(str, '\u009B')) | |
| return str; | |
| if (!RegExpPrototypeTest(/[\u001B\u009B]/, str)) | |
| return str; |
Did you test the performance of this? I think this would be even faster
There was a problem hiding this comment.
It's unlikely that a Regex test will be faster.
Possibly this is better:
if (StringPrototypeIndexOf(str, '\u001B') === -1 &&
StringPrototypeIndexOf(str, '\u009B') === -1)
There was a problem hiding this comment.
Done — added a comment in bacd1a7 explaining why the short-circuit is there.
There was a problem hiding this comment.
@gurgunday Thanks for the suggestion! I benchmarked a single RegExpPrototypeTest(/[\u001B\u009B]/, str) call (one pass) vs two StringPrototypeIndexOf calls (two passes). Even with the extra pass, two indexOf calls are faster — especially for longer strings:
=== Short (~50 chars) ===
2x indexOf 17.6 ns/op
1x RegExp.test 29.5 ns/op
RegExp.test / indexOf = 1.67x
=== Long (~9991 chars) ===
2x indexOf 542.6 ns/op
1x RegExp.test 3991.4 ns/op
RegExp.test / indexOf = 7.36x
Single-char indexOf is a simpler operation than regex character class matching, so two indexOf passes still wins over one regex pass.
Switched to StringPrototypeIndexOf per @RafaelGSS's suggestion.
Benchmark code
const short = 'Hello, World! This is a test string without ANSI.';
const long = 'Long plain text without ANSI. '.repeat(333);
const re = /[\u001B\u009B]/;
const n = 2_000_000;
const shortArr = Array.from({length: 100}, (_, i) => short + String(i));
const longArr = Array.from({length: 100}, (_, i) => long + String(i));
function bench(label, fn) {
fn();
const runs = [];
for (let r = 0; r < 5; r++) {
const start = performance.now();
fn();
runs.push(performance.now() - start);
}
runs.sort((a, b) => a - b);
const median = runs[Math.floor(runs.length / 2)];
const perOp = (median / n * 1e6).toFixed(1);
console.log(label.padEnd(30) + median.toFixed(0).padStart(6) + ' ms (' + perOp + ' ns/op)');
return median;
}
console.log('=== Short (~' + shortArr[0].length + ' chars) ===');
const a = bench(' indexOf', () => { let r; for (let i = 0; i < n; i++) { const s = shortArr[i % 100]; r = s.indexOf('\u001B') === -1 && s.indexOf('\u009B') === -1; } return r; });
const b = bench(' RegExp.test', () => { let r; for (let i = 0; i < n; i++) { const s = shortArr[i % 100]; r = !re.test(s); } return r; });
console.log(' RegExp.test / indexOf = ' + (b / a).toFixed(2) + 'x');
console.log('\n=== Long (~' + longArr[0].length + ' chars) ===');
const c = bench(' indexOf', () => { let r; for (let i = 0; i < n; i++) { const s = longArr[i % 100]; r = s.indexOf('\u001B') === -1 && s.indexOf('\u009B') === -1; } return r; });
const d = bench(' RegExp.test', () => { let r; for (let i = 0; i < n; i++) { const s = longArr[i % 100]; r = !re.test(s); } return r; });
console.log(' RegExp.test / indexOf = ' + (d / c).toFixed(2) + 'x');
RafaelGSS
left a comment
There was a problem hiding this comment.
Please, share nodejs benchmark results. The one you attached in the PR description doesn't seem Node.js one.
lib/internal/util/inspect.js
Outdated
| if (!StringPrototypeIncludes(str, '\u001B') && | ||
| !StringPrototypeIncludes(str, '\u009B')) | ||
| return str; |
There was a problem hiding this comment.
It's unlikely that a Regex test will be faster.
Possibly this is better:
if (StringPrototypeIndexOf(str, '\u001B') === -1 &&
StringPrototypeIndexOf(str, '\u009B') === -1)
|
@RafaelGSS Switched to Node.js benchmark results using ~1.93x faster for short non-ANSI strings, ~6.91x for long non-ANSI strings. Negligible impact on ANSI input. |
|
Landed in febcecd |
PR-URL: #61833 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [node](https://nodejs.org) ([source](https://github.com/nodejs/node)) | minor | `25.7.0` → `25.8.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>nodejs/node (node)</summary> ### [`v25.8.0`](https://github.com/nodejs/node/releases/tag/v25.8.0): 2026-03-03, Version 25.8.0 (Current), @​richardlau [Compare Source](nodejs/node@v25.7.0...v25.8.0) ##### Notable Changes - \[[`e55eddea2a`](nodejs/node@e55eddea2a)] - **build, doc**: use new api doc tooling (flakey5) [#​57343](nodejs/node#57343) - \[[`4c181e2277`](nodejs/node@4c181e2277)] - **(SEMVER-MINOR)** **sqlite**: add limits property to DatabaseSync (Mert Can Altin) [#​61298](nodejs/node#61298) - \[[`46ee1eddd7`](nodejs/node@46ee1eddd7)] - **(SEMVER-MINOR)** **src**: add C++ support for diagnostics channels (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`9ddd1a9c27`](nodejs/node@9ddd1a9c27)] - **(SEMVER-MINOR)** **src,permission**: add --permission-audit (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`0d97ec4044`](nodejs/node@0d97ec4044)] - **(SEMVER-MINOR)** **test\_runner**: expose worker ID for concurrent test execution (Ali Hassan) [#​61394](nodejs/node#61394) ##### Commits - \[[`940b58c8c1`](nodejs/node@940b58c8c1)] - **buffer**: optimize buffer.concat performance (Mert Can Altin) [#​61721](nodejs/node#61721) - \[[`0589b0e5a1`](nodejs/node@0589b0e5a1)] - **build**: fix GN for new merve dep (Shelley Vohr) [#​61984](nodejs/node#61984) - \[[`f3d3968dcd`](nodejs/node@f3d3968dcd)] - ***Revert*** "**build**: add temporal test on GHA windows" (Antoine du Hamel) [#​61810](nodejs/node#61810) - \[[`e55eddea2a`](nodejs/node@e55eddea2a)] - **build, doc**: use new api doc tooling (flakey5) [#​57343](nodejs/node#57343) - \[[`b7715292f8`](nodejs/node@b7715292f8)] - **child\_process**: add tracing channel for spawn (Marco) [#​61836](nodejs/node#61836) - \[[`a32a598748`](nodejs/node@a32a598748)] - **crypto**: fix missing nullptr check on RSA\_new() (ndossche) [#​61888](nodejs/node#61888) - \[[`dc384f95b3`](nodejs/node@dc384f95b3)] - **crypto**: fix handling of null BUF\_MEM\* in ToV8Value() (Nora Dossche) [#​61885](nodejs/node#61885) - \[[`3337b095db`](nodejs/node@3337b095db)] - **crypto**: fix potential null pointer dereference when BIO\_meth\_new() fails (Nora Dossche) [#​61788](nodejs/node#61788) - \[[`51ded81139`](nodejs/node@51ded81139)] - **deps**: update undici to 7.22.0 (Node.js GitHub Bot) [#​62035](nodejs/node#62035) - \[[`8aa2fde931`](nodejs/node@8aa2fde931)] - **deps**: update minimatch to 10.2.4 (Node.js GitHub Bot) [#​62016](nodejs/node#62016) - \[[`57dc092eaf`](nodejs/node@57dc092eaf)] - **deps**: upgrade npm to 11.11.0 (npm team) [#​61994](nodejs/node#61994) - \[[`705bbd60a9`](nodejs/node@705bbd60a9)] - **deps**: update simdjson to 4.3.1 (Node.js GitHub Bot) [#​61930](nodejs/node#61930) - \[[`4d411d72e5`](nodejs/node@4d411d72e5)] - **deps**: update acorn-walk to 8.3.5 (Node.js GitHub Bot) [#​61928](nodejs/node#61928) - \[[`f53a32ab84`](nodejs/node@f53a32ab84)] - **deps**: update acorn to 8.16.0 (Node.js GitHub Bot) [#​61925](nodejs/node#61925) - \[[`9b483fbb27`](nodejs/node@9b483fbb27)] - **deps**: update minimatch to 10.2.2 (Node.js GitHub Bot) [#​61830](nodejs/node#61830) - \[[`4e54c103cb`](nodejs/node@4e54c103cb)] - **doc**: separate in-types and out-types in SQLite conversion docs (René) [#​62034](nodejs/node#62034) - \[[`ca78ebbeaa`](nodejs/node@ca78ebbeaa)] - **doc**: fix small logic error in DETECT\_MODULE\_SYNTAX (René) [#​62025](nodejs/node#62025) - \[[`e6b131f3fe`](nodejs/node@e6b131f3fe)] - **doc**: fix module.stripTypeScriptTypes indentation (René) [#​61992](nodejs/node#61992) - \[[`7508540e19`](nodejs/node@7508540e19)] - **doc**: update DEP0040 (punycode) to application type deprecation (Mike McCready) [#​61916](nodejs/node#61916) - \[[`33a364cb62`](nodejs/node@33a364cb62)] - **doc**: explicitly mention Slack handle (Rafael Gonzaga) [#​61986](nodejs/node#61986) - \[[`46a61922bd`](nodejs/node@46a61922bd)] - **doc**: support toolchain Visual Studio 2022 & 2026 + Windows 11 SDK (Mike McCready) [#​61864](nodejs/node#61864) - \[[`dc12a257aa`](nodejs/node@dc12a257aa)] - **doc**: rename invalid `function` parameter (René) [#​61942](nodejs/node#61942) - \[[`dafdc0a5b8`](nodejs/node@dafdc0a5b8)] - **http**: validate headers in writeEarlyHints (Richard Clarke) [#​61897](nodejs/node#61897) - \[[`3c94b56fa6`](nodejs/node@3c94b56fa6)] - **inspector**: unwrap internal/debugger/inspect imports (René) [#​61974](nodejs/node#61974) - \[[`8a24c17648`](nodejs/node@8a24c17648)] - **lib**: improve argument handling in Blob constructor (Ms2ger) [#​61980](nodejs/node#61980) - \[[`21d4baf256`](nodejs/node@21d4baf256)] - **meta**: bump github/codeql-action from 4.32.0 to 4.32.4 (dependabot\[bot]) [#​61911](nodejs/node#61911) - \[[`59a726a8e3`](nodejs/node@59a726a8e3)] - **meta**: bump step-security/harden-runner from 2.14.1 to 2.14.2 (dependabot\[bot]) [#​61909](nodejs/node#61909) - \[[`0072b7f991`](nodejs/node@0072b7f991)] - **meta**: bump actions/stale from 10.1.1 to 10.2.0 (dependabot\[bot]) [#​61908](nodejs/node#61908) - \[[`999bf22f47`](nodejs/node@999bf22f47)] - **repl**: keep reference count for `process.on('newListener')` (Anna Henningsen) [#​61895](nodejs/node#61895) - \[[`4c181e2277`](nodejs/node@4c181e2277)] - **(SEMVER-MINOR)** **sqlite**: add limits property to DatabaseSync (Mert Can Altin) [#​61298](nodejs/node#61298) - \[[`aee2a18257`](nodejs/node@aee2a18257)] - **src**: fix flags argument offset in JSUdpWrap (Weixie Cui) [#​61948](nodejs/node#61948) - \[[`46ee1eddd7`](nodejs/node@46ee1eddd7)] - **(SEMVER-MINOR)** **src**: add C++ support for diagnostics channels (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`9ddd1a9c27`](nodejs/node@9ddd1a9c27)] - **(SEMVER-MINOR)** **src,permission**: add --permission-audit (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`ea2df2a16f`](nodejs/node@ea2df2a16f)] - **stream**: fix pipeTo to defer writes per WHATWG spec (Matteo Collina) [#​61800](nodejs/node#61800) - \[[`aa0c7b09e0`](nodejs/node@aa0c7b09e0)] - **test**: remove unnecessary `process.exit` calls from test files (Antoine du Hamel) [#​62020](nodejs/node#62020) - \[[`ad96a6578f`](nodejs/node@ad96a6578f)] - **test**: skip `test-url` on `--shared-ada` builds (Antoine du Hamel) [#​62019](nodejs/node#62019) - \[[`7c72a31e4b`](nodejs/node@7c72a31e4b)] - **test**: skip strace test with shared openssl (Richard Lau) [#​61987](nodejs/node#61987) - \[[`604456c163`](nodejs/node@604456c163)] - **test**: avoid flaky debugger restart waits (Yuya Inoue) [#​61773](nodejs/node#61773) - \[[`4890d6bd43`](nodejs/node@4890d6bd43)] - **test\_runner**: run afterEach on runtime skip (Igor Shevelenkov) [#​61525](nodejs/node#61525) - \[[`fce2930110`](nodejs/node@fce2930110)] - **test\_runner**: expose expectFailure message (sangwook) [#​61563](nodejs/node#61563) - \[[`0d97ec4044`](nodejs/node@0d97ec4044)] - **(SEMVER-MINOR)** **test\_runner**: expose worker ID for concurrent test execution (Ali Hassan) [#​61394](nodejs/node#61394) - \[[`243e6b2009`](nodejs/node@243e6b2009)] - **test\_runner**: replace native methods with primordials (Ayoub Mabrouk) [#​61219](nodejs/node#61219) - \[[`bf1ed7e647`](nodejs/node@bf1ed7e647)] - **tls**: forward keepAlive, keepAliveInitialDelay, noDelay to socket (Sergey Zelenov) [#​62004](nodejs/node#62004) - \[[`0f15079d94`](nodejs/node@0f15079d94)] - **tools**: remove custom logic for skipping `test-strace-openat-openssl` (Antoine du Hamel) [#​62038](nodejs/node#62038) - \[[`54a055a59d`](nodejs/node@54a055a59d)] - **tools**: bump minimatch from 3.1.2 to 3.1.3 in `/tools/clang-format` (dependabot\[bot]) [#​61977](nodejs/node#61977) - \[[`a28744cb62`](nodejs/node@a28744cb62)] - **tools**: fix permissions for merve update script (Richard Lau) [#​62023](nodejs/node#62023) - \[[`31e7936354`](nodejs/node@31e7936354)] - **tools**: revert tools GHA workflow to ubuntu-latest (Richard Lau) [#​62024](nodejs/node#62024) - \[[`0a96a16e1f`](nodejs/node@0a96a16e1f)] - **tools**: bump minimatch from 3.1.2 to 3.1.3 in /tools/eslint (dependabot\[bot]) [#​61976](nodejs/node#61976) - \[[`f279233412`](nodejs/node@f279233412)] - **tools**: roll back to x86 runner on `scorecard.yml` (Antoine du Hamel) [#​61944](nodejs/node#61944) - \[[`192c0382f4`](nodejs/node@192c0382f4)] - **util**: add fast path to stripVTControlCharacters (Hiroki Osame) [#​61833](nodejs/node#61833) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My40OS4wIiwidXBkYXRlZEluVmVyIjoiNDMuNDkuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6Om1pbm9yIl19-->
Problem
stripVTControlCharactersalways runs a regex replacement, even when the input contains no ANSI escape codes. This is the common case — most strings passed through are already plain text (e.g. checking/sanitizing user input, processing log lines that are mostly text).Changes
Adds a
StringPrototypeIndexOfguard for ESC (\u001B) and CSI (\u009B) before the regex. Since all ANSI escape sequences start with one of these introducers, their absence means no ANSI codes exist and we can return the string immediately — skipping regex execution.Based on the same optimization proposed in chalk/strip-ansi#54.
Also adds a benchmark and test coverage for the fast path, 7-bit ESC sequences, and 8-bit CSI sequences.
Benchmarks
Node.js benchmark results using
benchmark/compare.js(30 runs, two separate builds):~1.93x faster for short non-ANSI strings, ~6.91x for long non-ANSI strings. Negligible impact on ANSI input.