GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — util: improve inspect performance by BridgeAR · Pull Request #14881 · nodejs/node

Conversation

@BridgeAR

@BridgeAR BridgeAR commented Aug 17, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I reworked a couple of things in util inspect for performance. The code is fully covered.

EDIT: Updated the comment here and benchmarks.

 util/format.js type="no-replace" n=2000000                                1676.61 %        *** 6.038361e-60
 util/inspect-array.js type="denseArray_showHidden" len=100000 n=1000        87.89 %        *** 3.315801e-79
 util/inspect-array.js type="denseArray" len=100000 n=1000                   68.83 %        *** 5.648717e-52
 util/inspect-array.js type="mixedArray" len=100000 n=1000                   21.22 %        *** 2.438066e-52
 util/inspect-array.js type="sparseArray" len=100000 n=1000                   6.87 %        *** 9.551672e-50
 util/inspect-proxy.js n=1000000                                            418.34 %        *** 5.193464e-71
 util/inspect.js option="colors" method="Array" n=2000000                   121.92 %        *** 1.136217e-39
 util/inspect.js option="colors" method="Date" n=2000000                    294.42 %        *** 3.899375e-18
 util/inspect.js option="colors" method="Error" n=2000000                   507.31 %        *** 6.416333e-18
 util/inspect.js option="colors" method="Object_deep_ln" n=2000000          257.20 %        *** 8.804036e-23
 util/inspect.js option="colors" method="Object_empty" n=2000000            704.27 %        *** 8.422940e-46
 util/inspect.js option="colors" method="Object" n=2000000                  372.04 %        *** 3.430467e-19
 util/inspect.js option="colors" method="Set" n=2000000                     443.65 %        *** 7.503642e-30
 util/inspect.js option="colors" method="String_boxed" n=2000000            458.19 %        *** 4.199961e-23
 util/inspect.js option="colors" method="String_complex" n=2000000          875.46 %        *** 4.377511e-42
 util/inspect.js option="colors" method="String" n=5000000                 2209.03 %        *** 3.336942e-23
 util/inspect.js option="colors" method="TypedArray_extra" n=2000000        269.77 %        *** 6.353860e-22
 util/inspect.js option="colors" method="TypedArray" n=2000000              244.82 %        *** 1.157433e-47
 util/inspect.js option="none" method="Array" n=2000000                     298.39 %        *** 6.163207e-35
 util/inspect.js option="none" method="Date" n=2000000                      308.41 %        *** 3.828139e-26
 util/inspect.js option="none" method="Error" n=2000000                     499.46 %        *** 2.519987e-17
 util/inspect.js option="none" method="Object_deep_ln" n=2000000            470.60 %        *** 7.069515e-27
 util/inspect.js option="none" method="Object_empty" n=2000000              703.54 %        *** 5.821877e-41
 util/inspect.js option="none" method="Object" n=2000000                    678.48 %        *** 1.752143e-21
 util/inspect.js option="none" method="Set" n=2000000                       739.62 %        *** 5.402939e-18
 util/inspect.js option="none" method="String_boxed" n=2000000              483.94 %        *** 2.756513e-37
 util/inspect.js option="none" method="String_complex" n=2000000           1082.41 %        *** 3.900304e-64
 util/inspect.js option="none" method="String" n=5000000                   3954.65 %        *** 1.558337e-26
 util/inspect.js option="none" method="TypedArray_extra" n=2000000          410.59 %        *** 1.364578e-19
 util/inspect.js option="none" method="TypedArray" n=2000000                387.47 %        *** 1.789663e-23
 util/inspect.js option="showHidden" method="Array" n=2000000               272.92 %        *** 2.332172e-29
 util/inspect.js option="showHidden" method="Date" n=2000000                307.75 %        *** 4.581806e-16
 util/inspect.js option="showHidden" method="Error" n=2000000               254.46 %        *** 1.552997e-19
 util/inspect.js option="showHidden" method="Object_deep_ln" n=2000000      422.07 %        *** 4.184839e-46
 util/inspect.js option="showHidden" method="Object_empty" n=2000000        665.61 %        *** 2.095402e-24
 util/inspect.js option="showHidden" method="Object" n=2000000              626.02 %        *** 6.570478e-39
 util/inspect.js option="showHidden" method="Set" n=2000000                 815.83 %        *** 3.634105e-32
 util/inspect.js option="showHidden" method="String_boxed" n=2000000        680.82 %        *** 3.104074e-27
 util/inspect.js option="showHidden" method="String_complex" n=2000000     1058.45 %        *** 5.543800e-49
 util/inspect.js option="showHidden" method="String" n=5000000             3473.09 %        *** 6.496121e-29
 util/inspect.js option="showHidden" method="TypedArray_extra" n=2000000    345.93 %        *** 9.418444e-36
 util/inspect.js option="showHidden" method="TypedArray" n=2000000          316.96 %        *** 4.742310e-35

The main optimizations are

  • Removed visibleKeys
  • Removed proxy cache
  • Removed Object.assign
  • No key concatenating anymore
  • No key recalculating anymore
  • Improved indentation logic
  • Improved string escape logic
  • Added many fast paths
  • Optimized code branches a lot
  • Optimized (boxed) primitive handling
  • Inline code if possible
  • Only check extra keys if necessary

Besides that there were lots of smaller optimizations, the
code got a bit cleaned up and a few more tests got in.

Also two bug fixes came.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

util, benchmark

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@BridgeAR BridgeAR added benchmark Issues and PRs related to Node.js benchmarks and benchmarking infrastructure. blocked PRs that are blocked by other issues or PRs. performance Issues and PRs related to the performance of Node.js. util Issues and PRs related to the built-in util module. labels Aug 17, 2017
@BridgeAR BridgeAR mentioned this pull request Aug 17, 2017
util: use constructor name #14886
Closed
4 tasks done
andrasq
andrasq reviewed Aug 17, 2017
View reviewed changes
Comment thread lib/util.js Outdated

@andrasq andrasq Aug 17, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

a long series of if - else if - else if statements can also be formatted as a switch (true) { ... } (note that switch does a strict comparison, ie matches not truthy but true). Strictly a layout issue, otherwise completely equivalent, both O(n) in the number of tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@BridgeAR BridgeAR Aug 17, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I you prefer that I can change it, otherwise I would just keep it as is.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
addaleax
addaleax reviewed Aug 17, 2017
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is great work, thank you!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/util.js Outdated

@addaleax addaleax Aug 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

grammar nit: lazy → lazily

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@BridgeAR BridgeAR Aug 17, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/util.js Outdated

@addaleax addaleax Aug 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

ctx.seen = [...ctx.seenSet]?

Also, I, personally, would be fine with having seen just be a getter/setter pair.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@BridgeAR BridgeAR Aug 17, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@BridgeAR BridgeAR Aug 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ref #14881 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/util.js Outdated

@addaleax addaleax Aug 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I feel like this could just be turned into a Set as well

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@BridgeAR BridgeAR Aug 17, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I tried that and it was actually a bit slower in my benchmarks. When 6.1 lands I might have another look at it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/util.js Outdated

@addaleax addaleax Aug 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This really sounds like it would return a boolean ;)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@BridgeAR BridgeAR Aug 17, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/util.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/util.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/util.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Aug 17, 2017

Copy link
Copy Markdown
Member

Also, sorry for the merge conflict, I fixed Evan’s nit while landing my PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Aug 17, 2017

Copy link
Copy Markdown
Member Author

@addaleax I addressed your comments and don't worry about the conflicts, I was aware of those changes and I'll rebase after #14880 lands :)
Those are minor changes anyway.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
BridgeAR
BridgeAR commented Aug 18, 2017
View reviewed changes
Comment thread lib/util.js Outdated

@BridgeAR BridgeAR Aug 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I reverted my change to a Set here: the seen part will only contain up to ctx.depth + 1 elements and in the regular case that is three. Using a Set for such a low amount of elements seems unnecessary, as it increases the code complexity due to having to handle both, an array and a set in case customInspect is set.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@BridgeAR
BridgeAR force-pushed the improve-inspect branch from 33adc94 to 6b87703 Compare August 21, 2017 04:02
@BridgeAR

BridgeAR commented Aug 21, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

I rebased and added a new array fast path. So PTAL

Just for fun - this is how the profiling looks like with Array(1e5).fill(1)

 [Summary]:
   ticks  total  nonlib   name
    261    0.6%    0.6%  JavaScript
  39502   94.0%   94.3%  C++
   1797    4.3%    4.3%  GC
    129    0.3%          Shared libraries
   2147    5.1%          Unaccounted

 [C++ entry points]:
   ticks    cpp   total   name
  37497   99.2%   89.2%  v8::internal::Runtime_ObjectKeys(int, v8::internal::Object**, v8::internal::Isolate*)
     75    0.2%    0.2%  v8::internal::Builtin_ObjectGetOwnPropertyDescriptor(int, v8::internal::Object**, v8::internal::Isolate*)

0.6 % JavaScript 🤣
refack reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Aug 21, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/9767/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack
refack approved these changes Aug 21, 2017
View reviewed changes

@refack refack left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

1 nit otherwise LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/util.js Outdated

@refack refack Aug 17, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Not worth it.

  1. changes util.inspect.length
  2. drives my IDE crazy

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax reacted with thumbs down emoji
All reactions
  • 👎 1 reaction

@BridgeAR BridgeAR Aug 21, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Well, I do not have a strong opinion.
I went ahead and changed it back to a similar way as before to make you happy ;-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@BridgeAR

BridgeAR commented Aug 21, 2017

Copy link
Copy Markdown
Member Author

So I pushed a bit more code and updated the benchmarks. The result is pretty decent now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Aug 21, 2017

Copy link
Copy Markdown
Member Author

Ok, the new benchmarks are outdated again as I was able to improve some more things that had significant impact on showHidden. So just think of most showHidden results +100%.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Aug 23, 2017
@BridgeAR
BridgeAR force-pushed the improve-inspect branch from 30d4685 to 2716b99 Compare August 24, 2017 00:07
@BridgeAR BridgeAR removed blocked PRs that are blocked by other issues or PRs. wip Issues and PRs that are still a work in progress. labels Aug 24, 2017
@BridgeAR

BridgeAR commented Aug 24, 2017

Copy link
Copy Markdown
Member Author

So after looking at it again I added some final changes. The benchmarks are updated in the main part. As the code still changed significantly, please take a close look again. @addaleax @refack @andrasq

And I have an issue with one test running this locally but I can not explain why the test is failing. I get the following output. @bnoordhuis you wrote that test, would you be so kind and have a look at it? Because I do not see the connection of the error to my changes...

	async-hooks doctool inspector known_issues message parallel pseudo-tty sequential \
	addons addons-napi
[02:05|%  91|+ 1642|-   0]: release stack_overflow_async length differs.       
expect=4
actual=2
patterns:
pattern = ^RangeError\:\ Maximum\ call\ stack\ size\ exceeded$
pattern = ^\ \ \ \ at\ f\ \(.*test.*message.*stack\_overflow\_async\.js\:.*\)$
pattern = ^\ \ \ \ at\ f\ \(.*test.*message.*stack\_overflow\_async\.js\:7\:.*\)$
pattern = ^\ \ \ \ at\ f\ \(.*test.*message.*stack\_overflow\_async\.js\:7\:.*\)$
outlines:
outline = (node:21361) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): RangeError: Maximum call stack size exceeded
outline = (node:21361) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
=== release stack_overflow_async ===                    
Path: message/stack_overflow_async
(node:21361) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): RangeError: Maximum call stack size exceeded
(node:21361) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
Command: out/Release/node --stack_trace_limit=3 /home/ruben/repos/node/node/test/message/stack_overflow_async.js
[02:46|% 100|+ 1785|-   1]: Done
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Aug 24, 2017

Copy link
Copy Markdown
Member

@BridgeAR That’s a really new test, and I’m pretty sure it’s not related to your PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR BridgeAR mentioned this pull request Aug 24, 2017
console: implement minimal console.group() #14910
Closed
4 tasks done
@BridgeAR

BridgeAR commented Aug 24, 2017

Copy link
Copy Markdown
Member Author

CI https://ci.nodejs.org/job/node-test-pull-request/9817
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Aug 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@addaleax the weird part is that is it happening only with this PR. On master it does not fail. It is reproducible for me on my local machine and also the CI fails exactly at this test. [EDIT] And the test passes if I call console.log() on top of the function call. Only calling console.log fails. [/EDIT]
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Aug 29, 2017

Copy link
Copy Markdown
Member Author

@bnoordhuis it would be nice if you could have a look at this PR because it somehow breaks this test even though there should not be any connection. The only way for me to fix the test is by calling process.stdout.write("") (or console.log()) somewhere before the test starts.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell requested a review from bnoordhuis August 29, 2017 04:45
@BridgeAR BridgeAR mentioned this pull request Aug 31, 2017
test: remove faulty test case #15110
Closed
4 tasks done
@BridgeAR

BridgeAR commented Sep 5, 2017

Copy link
Copy Markdown
Member Author

@nodejs/collaborators PTAL
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Sep 5, 2017
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM with both CI and CITGM green. Those type of large scale perf improvements can lead to broken production code.

This should be semver-minor at least, and it should bake for LTS for some month before being backported.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
@BridgeAR @addaleax
test: fix actual and expected order
50505ac 
In addition use the newer common.expectsError version.

PR-URL: nodejs/node#14881
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@jasnell jasnell added the backport-requested-v8.x label Sep 20, 2017
@jasnell

jasnell commented Sep 20, 2017

Copy link
Copy Markdown
Member

This needs a backport pr to land in v8.x
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell pushed a commit that referenced this pull request Sep 20, 2017
@BridgeAR @jasnell
util: add fast internal array join method
5394458 
PR-URL: #14881
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
@BridgeAR @jasnell
test: fix actual and expected order
757c342 
In addition use the newer common.expectsError version.

PR-URL: #14881
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
This was referenced Sep 20, 2017
benchmark: var to const #13757
Closed
Node.js 8.6.0 Release Proposal #15509
Merged
BridgeAR added a commit to BridgeAR/node that referenced this pull request Sep 20, 2017
@BridgeAR
benchmark: improve and add more inspect benchmarks
5f23de8 
PR-URL: nodejs#14881
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Sep 20, 2017
@BridgeAR
util: refactor inspect for performance and more
4356088 
The main optimizations are
- Removed visibleKeys
- Removed proxy cache
- Removed Object.assign
- No key concatenating anymore
- No key recalculating anymore
- Improved indentation logic
- Improved string escape logic
- Added many fast paths
- Optimized code branches a lot
- Optimized (boxed) primitive handling
- Inline code if possible
- Only check extra keys if necessary
- Guard against unnecessary more expensive calls

This also fixes a bug with special array number keys as e.g. "00".

Besides that there were lots of smaller optimizations, the
code got a bit cleaned up and a few more tests got in.

PR-URL: nodejs#14881
Fixes: nodejs#15288
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Sep 20, 2017
@BridgeAR
util: fix out of bounds indices in util.inspect
cc57533 
This fixes a issue brought up in nodejs#15288.

PR-URL: nodejs#14881
Refs: nodejs#15288
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Sep 20, 2017
@BridgeAR
test: improve util inspect tests
3b4f62a 
Remove unnecessary code parts and outdated code

PR-URL: nodejs#14881
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Sep 20, 2017
[v8.x backport] util: improve inspect performance #15512
Closed
4 tasks done
jasnell pushed a commit that referenced this pull request Sep 21, 2017
@BridgeAR @jasnell
benchmark: improve and add more inspect benchmarks
0f8f37e 
PR-URL: #14881
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 21, 2017
@BridgeAR @jasnell
util: refactor inspect for performance and more
9e8b1b3 
The main optimizations are
- Removed visibleKeys
- Removed proxy cache
- Removed Object.assign
- No key concatenating anymore
- No key recalculating anymore
- Improved indentation logic
- Improved string escape logic
- Added many fast paths
- Optimized code branches a lot
- Optimized (boxed) primitive handling
- Inline code if possible
- Only check extra keys if necessary
- Guard against unnecessary more expensive calls

This also fixes a bug with special array number keys as e.g. "00".

Besides that there were lots of smaller optimizations, the
code got a bit cleaned up and a few more tests got in.

PR-URL: #14881
Fixes: #15288
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 21, 2017
@BridgeAR @jasnell
util: fix out of bounds indices in util.inspect
5b47f84 
This fixes a issue brought up in #15288.

PR-URL: #14881
Refs: #15288
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 21, 2017
@BridgeAR @jasnell
test: improve util inspect tests
93c08b0 
Remove unnecessary code parts and outdated code

PR-URL: #14881
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@jasnell jasnell added backported-to-v8.x and removed backport-requested-v8.x labels Sep 21, 2017
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR Stephen Belanger
benchmark: improve and add more inspect benchmarks
014a444 
PR-URL: nodejs/node#14881
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR Stephen Belanger
util: add fast internal array join method
d11e175 
PR-URL: nodejs/node#14881
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR Stephen Belanger
util: refactor inspect for performance and more
5e9592a 
The main optimizations are
- Removed visibleKeys
- Removed proxy cache
- Removed Object.assign
- No key concatenating anymore
- No key recalculating anymore
- Improved indentation logic
- Improved string escape logic
- Added many fast paths
- Optimized code branches a lot
- Optimized (boxed) primitive handling
- Inline code if possible
- Only check extra keys if necessary
- Guard against unnecessary more expensive calls

This also fixes a bug with special array number keys as e.g. "00".

Besides that there were lots of smaller optimizations, the
code got a bit cleaned up and a few more tests got in.

PR-URL: nodejs/node#14881
Fixes: nodejs/node#15288
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR Stephen Belanger
util: fix out of bounds indices in util.inspect
4b14c98 
This fixes a issue brought up in #15288.

PR-URL: nodejs/node#14881
Refs: nodejs/node#15288
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR Stephen Belanger
test: improve util inspect tests
9e5bdbc 
Remove unnecessary code parts and outdated code

PR-URL: nodejs/node#14881
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR Stephen Belanger
test: fix actual and expected order
26fb44e 
In addition use the newer common.expectsError version.

PR-URL: nodejs/node#14881
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@MylesBorins MylesBorins added baking-for-lts PRs whose changes need more time before an LTS backport. lts-watch-v6.x labels Oct 17, 2017
@MylesBorins

MylesBorins commented Oct 17, 2017

Copy link
Copy Markdown
Contributor

should this land in LTS? If so it will need to bake a bit longer.

Please change labels as appropriate
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR BridgeAR mentioned this pull request Dec 20, 2017
doc: warn about using util.inspect/util.format in prod #17791
Closed
3 tasks done
@MylesBorins MylesBorins removed the baking-for-lts PRs whose changes need more time before an LTS backport. label Aug 17, 2018
@BridgeAR
BridgeAR deleted the improve-inspect branch April 1, 2019 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax left review comments
@benjamingr benjamingr benjamingr approved these changes
@tniessen tniessen tniessen left review comments
@mcollina mcollina mcollina approved these changes
@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis
+5 more reviewers
@mscdex mscdex mscdex left review comments
@ronkorving ronkorving ronkorving left review comments
@TimothyGu TimothyGu TimothyGu left review comments
@andrasq andrasq andrasq left review comments
@refack refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

benchmark Issues and PRs related to Node.js benchmarks and benchmarking infrastructure. performance Issues and PRs related to the performance of Node.js. util Issues 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.

Uh oh!

There was an error while loading. Please reload this page.

13 participants

@BridgeAR @addaleax @mscdex @TimothyGu @bmeurer @jasnell @MylesBorins @mcollina @refack @ronkorving @benjamingr @tniessen @andrasq
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.