util: fix inspect performance bug

[BridgeAR]

In case an object contained a circular reference Object.keys was
called even though it was not necessary at all. This caused a
significant overhead for objects that contained a lot of such entries.

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

[BridgeAR]

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

[vsemozhetbyt]

.includes() unless it has a performance penalty?

[BridgeAR]

It had a performance penalty the last time I checked.

[apapirovski]

indexOf should be much faster than anything else.

[vsemozhetbyt]

Maybe it is a case for @nodejs/v8 performance group to look into then.

[mcollina]

LGTM

I would love to have/see a benchmark for this.

[BridgeAR]

@mcollina I tested a extreme case:

'use strict';

const util = require('util');

let i = 0;
const obj = {};

while (i++ < 1500) {
  obj[i] = {};
}

const circular = {};

for (const k of Object.keys(obj)) {
  circular[k] = obj[k];
  obj[k].circular = circular;
  obj[k].obj = obj;
}

console.time('run');
util.inspect(obj);
console.timeEnd('run');

// Before the patch:
// run: 165971.450ms

// After the patch:
// run: 7377.811ms

[richardlau]

I don't believe this qualifies for fast tracking based on the guidance given in https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#waiting-for-approvals.

[BridgeAR]

Landed in f413f56