util: improve util.inspect performance

[BridgeAR]

Fixes#14487

A hole in a sparse array is now in itself a O(1) operation instead of O(n) where n is the number of coherent elements of the hole.

In addition a hasOwnProperty check was replaced with a cheaper one.

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

[tniessen]

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

[mscdex]

Couldn't these previous two lines be merged into if (++visibleLength === ctx.maxArrayLength) ?

[BridgeAR]

Done

[TimothyGu]

Can you add a comment on why this is necessary, and different from i <= 0? (I understand it because I saw an earlier version of the PR, but comments are more for future than now anyway.)

[BridgeAR]

Added

[BridgeAR]

Comments addressed

[aqrln]

Thanks for addressing the issue! I made some benchmarks back when it was opened, so feel free to include them in your pull request if you wish: https://gist.github.com/aqrln/195743d38f632b5b9a8d5944f30cb6af

Speaking of which, I observe a performance regression with dense arrays:

 improvement confidence p.value
util/inspect-array.js len=100000 n=1000 -35.07 % *** 1.862234e-35
util/inspect-sparse-array.js len=100000 n=1000 2662.66 % *** 1.516381e-41

Can you reproduce it or is it just me (the machine was a bit loaded with other stuff, so the result might be unreliable)?

[aqrln]

It may be worth to eventually make visibleKeys an ES2015 Set instead of emulating it via an object, but I don't know how it will affect performance with the current V8 (if it will at all).

[BridgeAR]

When I looked at the code there were a couple more optimizations possible but I tried to be more surgical here. E.g. for arrays there is no need for Object.keys when using for in and ctx.seen should be a Set instead of an Array.

[BridgeAR]

@aqrln thanks for the benchmarks. I saw some minor drop as well and changed it now. With for in there was also a minor regression (a edge case) and I added a test for that.

Current benchmark:

 util/inspect-array.js type="denseArray" len=100000 n=100 1.09 % 1.853220e-01
util/inspect-array.js type="mixedArray" len=100000 n=100 2596.93 % *** 8.927819e-18
util/inspect-array.js type="sparseArray" len=100000 n=100 3455.27 % *** 1.563057e-17

[aqrln]

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

[BridgeAR]

PTAL

[refack]

Landed in 95bbb68
Extra sanity test of master: https://ci.nodejs.org/job/node-test-commit-linuxone/7938/ ✔️

[MylesBorins]

I'm not sure if we want to backport this to v6.x, but I've put it under baking

[BridgeAR]

@MylesBorins I think it should be backported - especially as there are other commits that should also be backported at some point and they depend on this. But when this is backported #14880 has to be backported as well due to the minor regression introduced here.

[BridgeAR]

I think enough time has passed to backport this. From this change itself there was no further complain about any regression. What do you think @MylesBorins ?