util: improve util inspect performance

[BridgeAR]

This significantly improves the inspection performance for all array
types. From now on only the visible elements cause work instead of
having to process all array keys no matter how many entries are
visible. So the complexity for (typed) arrays is reduced from
O(n + m) to O(m), while n is the number of keys and m is the
number of visible entries. m is set to 100 by default
(maxArrayLength).

This also moves some code out of the main function to reduce the
overall function complexity.

It also adds a specific boxed primitives c++ function to reduce the
boundary crossing for the average use case (boxed primitives are rare).

I also reduced the runtime for the util benchmarks as they were significantly
to high and removed some obsolete options.

Intermediate benchmark results:

 util/inspect-array.js type='denseArray_showHidden' len=100 n=500 *** 16.58 % ±2.32% ±3.09% ±4.02% util/inspect-array.js type='denseArray_showHidden' len=100000 n=500 *** 35312.58 % ±352.42% ±474.97% ±630.57% util/inspect-array.js type='denseArray' len=100 n=500 *** 11.41 % ±2.29% ±3.04% ±3.96% util/inspect-array.js type='denseArray' len=100000 n=500 *** 19361.00 % ±243.13% ±327.68% ±435.02% util/inspect-array.js type='mixedArray' len=100 n=500 *** 16.69 % ±2.88% ±3.83% ±4.98% util/inspect-array.js type='mixedArray' len=100000 n=500 *** 1.64 % ±0.53% ±0.70% ±0.92% util/inspect-array.js type='sparseArray' len=100 n=500 *** 3.62 % ±1.40% ±1.87% ±2.45% util/inspect-array.js type='sparseArray' len=100000 n=500 0.31 % ±0.69% ±0.92% ±1.20% util/inspect-proxy.js n=20000 *** -5.93 % ±3.01% ±4.01% ±5.22% util/inspect.js option='none' method='Array' n=20000 *** 6.69 % ±0.92% ±1.23% ±1.60% util/inspect.js option='none' method='Date' n=20000 ** -5.01 % ±2.92% ±3.90% ±5.13% util/inspect.js option='none' method='Error' n=20000 *** -11.74 % ±3.11% ±4.16% ±5.44% util/inspect.js option='none' method='Number' n=20000 *** 19.64 % ±2.57% ±3.43% ±4.49% util/inspect.js option='none' method='Object_deep_ln' n=20000 0.61 % ±1.80% ±2.42% ±3.18% util/inspect.js option='none' method='Object_empty' n=20000 *** -9.55 % ±2.98% ±3.99% ±5.23% util/inspect.js option='none' method='Object' n=20000 -0.94 % ±1.80% ±2.39% ±3.12% util/inspect.js option='none' method='Set' n=20000 * -4.64 % ±3.54% ±4.72% ±6.15% util/inspect.js option='none' method='String_boxed' n=20000 *** -11.52 % ±1.96% ±2.61% ±3.40% util/inspect.js option='none' method='String_complex' n=20000 *** 7.97 % ±2.29% ±3.05% ±3.97% util/inspect.js option='none' method='String' n=20000 *** 15.03 % ±3.54% ±4.73% ±6.19% util/inspect.js option='none' method='TypedArray_extra' n=20000 *** 39.86 % ±2.39% ±3.19% ±4.16% util/inspect.js option='none' method='TypedArray' n=20000 *** 31.53 % ±3.66% ±4.91% ±6.47% util/inspect.js option='showHidden' method='Array' n=20000 *** 23.30 % ±3.02% ±4.07% ±5.39% util/inspect.js option='showHidden' method='Date' n=20000 ** -3.12 % ±1.92% ±2.56% ±3.34% util/inspect.js option='showHidden' method='Error' n=20000 -2.64 % ±3.82% ±5.12% ±6.74% util/inspect.js option='showHidden' method='Number' n=20000 *** 15.16 % ±2.83% ±3.77% ±4.90% util/inspect.js option='showHidden' method='Object_deep_ln' n=20000 -0.62 % ±1.68% ±2.26% ±2.98% util/inspect.js option='showHidden' method='Object_empty' n=20000 *** -10.84 % ±2.09% ±2.80% ±3.66% util/inspect.js option='showHidden' method='Object' n=20000 0.08 % ±1.86% ±2.48% ±3.23% util/inspect.js option='showHidden' method='Set' n=20000 -2.69 % ±3.61% ±4.84% ±6.36% util/inspect.js option='showHidden' method='String_boxed' n=20000 *** -10.19 % ±1.70% ±2.26% ±2.95% util/inspect.js option='showHidden' method='String_complex' n=20000 *** 6.34 % ±2.57% ±3.42% ±4.45% util/inspect.js option='showHidden' method='String' n=20000 *** 9.59 % ±2.45% ±3.27% ±4.26% util/inspect.js option='showHidden' method='TypedArray_extra' n=20000 *** 60.41 % ±2.94% ±3.91% ±5.08% util/inspect.js option='showHidden' method='TypedArray' n=20000 *** 75.31 % ±1.55% ±2.07% ±2.71% 

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

[nodejs-github-bot]

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/660/pipeline

[BridgeAR]

CI https://ci.nodejs.org/job/node-test-pull-request/16729/
CI https://ci.nodejs.org/job/node-test-pull-request/16730/
Benchmark https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/228/

[mscdex]

Would it be faster to do this in javascript somehow? For example: typeof arg === 'object' && arg !== null && arg.constructor === String ?

[BridgeAR]

I did not test it but I would not want to use it as it's not a safe check. Someone could easily manipulate the constructor.

consts=newString('test')s.constructor=Number

[mscdex]

Alright, how about:

if(typeofarg==='object'&&arg!==null&&Object.prototype.toString.call(arg)==='[object String]')

? Object.prototype.toString could even be cached at startup in case someone overrides it later on.

The only issue would be symbols and big integers as they don't have constructors that return object types, but at least maybe we could speed up the other type checks if this js method ends up being faster or avoid crossing the C++<->js boundary entirely if we place the symbol and bigint checks last.

[BridgeAR]

That is also not a safe check:

consta=newString('test')a[Symbol.toStringTag]='Foo'console.log(Object.prototype.toString.call(a))// '[object Foo]'

I'll move the bigint check in the C++ function down and I'll see if I can work around the performance penalty. Do you expect a lot of people using boxed primitives? Because for all other cases there are now less calls to C++ than before.

[BridgeAR]

Seems like the main regression came due to moving the functionality to an extra function. It's probably not inlined anymore as other code is hotter and the main function is already big. I moved it back in to address the performance regression.

[jdalton]

@BridgeAR 👆 Ideally this helper should have a JS form (that calls it) exposed on util.types like the other isNumberObject, etc.

[BridgeAR]

That is automatically the case. But good that you bring it up! I'll move this to an individual commit that also adds documentation for the function.

[refack]

I -1 on this.
IMHO util.inspect is a cold path, and I would much rather stability and readability. Again IMHO this is not a place that is worth the churn:

[mscdex]

I'm not sure I'd call util.inspect a "cold path". I could see end users using it for logging purposes for example.

[refack]

I'm not sure I'd call util.inspect a "cold path". I could see end users using it for logging purposes for example.

Ok. But then I'd assume log appending will generate I/O that will shadow the CPU work by inspect. In anyway I'm not convinced even a 100% improvement here is worth the churn, and possible instability.
If we can find a real world workload that this change improves significantly, I'd be happy to change my mind.

[jdalton]

@refack

This is goodness for util.inspect which is used by the REPL and many console methods. And the helpers introduced in this PR can in turn be goodness for util.types(in a follow-up). @BridgeAR is heavily involved in making improvements to util.inspect. I'm in favor of more of these incremental improvements. Long term I'd like to see inspect moved to its own file or folder of files (it's got enough parts).

[refack]

This is goodness for util.inspect which is used by the REPL and many console methods. And the helpers introduced in this PR can in turn be goodness for util.types (in a follow-up).

Then this needs reframing. Call it util: refactor `util.inspect` and let's judge this on merits of readability and reusability.

[jdalton]

@refack The PR here doesn't appear to make things any harder to reason about, simplifies an expensive set of operations, and opens the possibility of carrying over useful helpers to util.types.

Would it help if this PR moved/saved any of the source refactoring and cleanup bits for another PR?

[BridgeAR]

@refack I actually tried to reduce the overall complexity while significantly improving the performance. util.inspect is used a ton and we ran into multiple issues because there is a performance overhead when inspecting bigger objects. Here are some issues and PRs that dealt with util.inspect overhead #19994#19405#17791#6334.

@jdalton

Long term I'd like to see inspect moved to its own file or folder of files

Definitely! I want to do that as well.

[BridgeAR]

I just pushed two new commits: one to document the new function and one to address the boxed regression.

CI https://ci.nodejs.org/job/node-test-pull-request/16759/
CI https://ci.nodejs.org/job/node-test-pull-request/16771/

[BridgeAR]

@nodejs/util PTAL

[targos]

Should this be backported to v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.