Skip to content

util(styleText): optimise + benchmark - #58063

Closed
AugustinMauroy wants to merge 3 commits into
nodejs:mainfrom
AugustinMauroy:pref(util)-optimise-styleText
Closed

util(styleText): optimise + benchmark#58063
AugustinMauroy wants to merge 3 commits into
nodejs:mainfrom
AugustinMauroy:pref(util)-optimise-styleText

Conversation

@AugustinMauroy

@AugustinMauroyAugustinMauroy commented Apr 28, 2025•
edited
Loading

Copy link
Copy Markdown
Member

I had modify logic behind. using a reducer, it's should be quicker. And I had updated benchmark

Plus I had updated test to use node:test in goal of having better DX when something fails.

@nodejs-github-bot

nodejs-github-bot commented Apr 28, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Apr 28, 2025
@codecov

codecovBot commented Apr 28, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.21%. Comparing base (6cd1c09) to head (a9d13f2).
⚠️ Report is 2518 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #58063 +/- ##
=======================================
Coverage 90.21% 90.21% =======================================
Files 630 630 Lines 186391 186448 +57 Branches 36610 36620 +10 =======================================
+ Hits 168146 168209 +63 + Misses 11066 11038 -28 - Partials 7179 7201 +22
Files with missing linesCoverage Δ
lib/util.js97.77% <100.00%> (+0.01%)⬆️

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

@lpinca

lpinca commented Apr 28, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Plus I had updated test to use node:test in goal of having better DX when something fails.

Please don't do that, we discussed it a lot. Those refactors are not useful.

@AugustinMauroy

AugustinMauroy commented Apr 28, 2025

Copy link
Copy Markdown
MemberAuthor

Please don't do that, we discussed it a lot. Those refactors are not useful.

oh okay, surprising because the use cases with the it allow you to find your way around the test and, in the event of a failure, search the codebase.
Do you have a link to the discussion?

@lpinca

lpinca commented Apr 28, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Start here #54796 and here #56027, follow the links there.

@AugustinMauroy
AugustinMauroyforce-pushed the pref(util)-optimise-styleText branch 2 times, most recently from 7e02fbd to 369b13fCompareApril 29, 2025 09:34
aduh95
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment threadtest/parallel/test-util-styletext.js Outdated
aduh95
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment threadtest/parallel/test-util-styletext.js
aduh95
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment threadtest/parallel/test-util-styletext.js
aduh95
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment threadtest/parallel/test-util-styletext.js Outdated
@AugustinMauroy
AugustinMauroyforce-pushed the pref(util)-optimise-styleText branch from cf38ccd to ae69a04CompareApril 29, 2025 09:47
aduh95
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment threadbenchmark/util/style-text.js Outdated
aduh95
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment threadbenchmark/util/style-text.js Outdated
aduh95
aduh95 reviewed Apr 29, 2025
View reviewed changes
Comment threadbenchmark/util/style-text.js Outdated
marco-ippolito
marco-ippolito reviewed Apr 29, 2025
View reviewed changes
Comment threadlib/util.js Outdated
@@ -129,8 +128,9 @@ function styleText(format, text, { validateStream = true, stream = process.stdou
throw new ERR_INVALID_ARG_TYPE('stream', ['ReadableStream', 'WritableStream', 'Stream'], stream);
}

// If the stream is falsy or should not be colorized, set skipColorize to true
skipColorize = !lazyUtilColors().shouldColorize(stream);
if (!lazyUtilColors().shouldColorize(stream)) {

@marco-ippolitomarco-ippolitoApr 29, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

early return will skip validation so its a necessary overhead
#56722 (comment)

RafaelGSS
RafaelGSS requested changes Apr 29, 2025
View reviewed changes

@RafaelGSSRafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As Marco mentioned, the "overhead" is necessary.

@AugustinMauroy

AugustinMauroy commented Apr 29, 2025

Copy link
Copy Markdown
MemberAuthor

Can I just keep benchmark change ?

@RafaelGSS

RafaelGSS commented Apr 29, 2025

Copy link
Copy Markdown
Member

Can I just keep benchmark change ?

Yes, but I'd do it as a separate benchmark, so we don't need to have benchmarks for:

noColors: true, format: 'italic',
noColors: true, format: 'underline',
noColors: true, format: 'read',

As they will all be the same. So, adding it as a separate benchmark makes more sense.

@AugustinMauroy@aduh95
util(styleText): optimise + benchmark
ef34ff0
Co-Authored-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@AugustinMauroy
AugustinMauroyforce-pushed the pref(util)-optimise-styleText branch from 029701a to ef34ff0CompareApril 30, 2025 10:14
@AugustinMauroyAugustinMauroy changed the title perf(util): optimise styleTextutil(styleText): optimise + benchmarkApr 30, 2025
RafaelGSS
RafaelGSS requested changes Apr 30, 2025
View reviewed changes
Comment threadbenchmark/util/style-text.js
validateStream: [1, 0],
n: [1e3],
});
withColor: {

@RafaelGSSRafaelGSSApr 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please, create a different file instead. style-text-nocolor.js

@AugustinMauroyAugustinMauroyApr 30, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

oh okay got it !

@github-actions

github-actionsBot commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added stale and removed stale labels Apr 20, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95aduh95aduh95 left review comments
@marco-ippolitomarco-ippolitomarco-ippolito left review comments
@RafaelGSSRafaelGSSRafaelGSS requested changes

Assignees

No one assigned

Labels

needs-ciPRs that need a full CI run.utilIssues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@AugustinMauroy@nodejs-github-bot@lpinca@RafaelGSS@aduh95@marco-ippolito