Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: increase robustness with primordials #41212

Merged
merged 1 commit into from
Sep 22, 2022

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Dec 16, 2021

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

@ljharb ljharb added the util Issues and PRs related to the built-in util module. label Dec 16, 2021
@ljharb ljharb requested a review from BridgeAR December 16, 2021 22:41
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 16, 2021
@aduh95
Copy link
Contributor

aduh95 commented Dec 17, 2021

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'
}

@ljharb ljharb marked this pull request as draft December 17, 2021 04:49
@ljharb ljharb marked this pull request as ready for review December 17, 2021 07:01
@ljharb ljharb marked this pull request as draft December 17, 2021 15:50
@ljharb ljharb marked this pull request as ready for review December 17, 2021 17:55
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.

The current implementation is much better performance wise and that was the reason for me implementing it like that. As such I would like to keep it as is.

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

@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.

@BridgeAR
Copy link
Member

Here is a short benchmark (the second one is with the regular expression):

image

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.

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

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.

@BridgeAR
Copy link
Member

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.

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

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.

@aduh95
Copy link
Contributor

aduh95 commented Dec 17, 2021

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 (***)

@ljharb
Copy link
Member Author

ljharb commented Dec 17, 2021

@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.

@ljharb
Copy link
Member Author

ljharb commented Dec 18, 2021

@BridgeAR now that this no longer includes the regex, would you be willing to rereview, or dismiss your block?

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Benchmark result looks fine to me.

@ljharb
Copy link
Member Author

ljharb commented Dec 22, 2021

@BridgeAR ping again?

@BridgeAR
Copy link
Member

I would like to have a conclusion about the primordials before changing things in that direction further.

@ljharb
Copy link
Member Author

ljharb commented Dec 22, 2021

@BridgeAR until there is such a decision, why not continue with the status quo? (see #41212 (comment) also)

@Trott
Copy link
Member

Trott commented Jan 12, 2022

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?

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2022
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2022
@nodejs-github-bot nodejs-github-bot merged commit 098eac7 into nodejs:main Sep 22, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 098eac7

@ljharb ljharb deleted the util-separator-regex branch September 22, 2022 14:53
@RafaelGSS
Copy link
Member

Hi @ljharb! This PR didn't land cleanly on v18.x. Could you please create a backport?

@RafaelGSS RafaelGSS added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 26, 2022
ljharb added a commit to ljharb/node that referenced this pull request Sep 26, 2022
@ljharb
Copy link
Member Author

ljharb commented Sep 26, 2022

Filed #44797

@RafaelGSS RafaelGSS added backport-open-v18.x Indicate that the PR has an open backport. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Sep 26, 2022
ljharb added a commit to ljharb/node that referenced this pull request Sep 26, 2022
ljharb added a commit to ljharb/node that referenced this pull request Sep 27, 2022
ljharb added a commit to ljharb/node that referenced this pull request Sep 29, 2022
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
PR-URL: #41212
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
PR-URL: #41212
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
PR-URL: #41212
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@juanarbol juanarbol mentioned this pull request Oct 4, 2022
danielleadams pushed a commit that referenced this pull request Oct 4, 2022
(backported from #41212)

Backport-PR-URL: #44797
PR-URL: #41212
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@danielleadams danielleadams added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-open-v18.x Indicate that the PR has an open backport. labels Oct 4, 2022
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
PR-URL: #41212
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 7, 2022
PR-URL: #41212
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 10, 2022
PR-URL: #41212
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 11, 2022
PR-URL: #41212
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
PR-URL: nodejs/node#41212
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
PR-URL: nodejs/node#41212
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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. backported-to-v18.x PRs backported to the v18.x-staging branch. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants