inspect: fix pop undefined when array prototype pop deleted

[yashLadha]

If one explicitly deletes the array prototype in the repl instance then
the method to check for pop seen in the context starts giving error.
Reason being primordials is not used and userland has modified the
prototype.

Fixed by using array pop method primordial.

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

[yashLadha]

Benchmark run: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/863/

[aduh95]

There are a few perf regressions, but that might be an acceptable trade-off.

                                                                                                confidence improvement accuracy (*)   (**)   (***)
 util/format.js type='object-%s' n=100000                                                               **     -3.04 %       ±1.81% ±2.41%  ±3.14%
 util/format.js type='only-objects' n=100000                                                           ***     -6.02 %       ±2.08% ±2.77%  ±3.61%
 util/inspect.js option='colors' method='Object' n=20000                                                **     -4.03 %       ±2.72% ±3.62%  ±4.71%
 util/inspect.js option='colors' method='String_boxed' n=20000                                          **     -5.31 %       ±3.09% ±4.11%  ±5.35%
 util/inspect.js option='showHidden' method='Set' n=20000                                               **     -7.76 %       ±4.54% ±6.04%  ±7.87%
 util/inspect-proxy.js isProxy=0 showProxy=0 n=100000                                                   **     -2.86 %       ±1.99% ±2.64%  ±3.45%
 util/inspect-proxy.js isProxy=1 showProxy=0 n=100000                                                  ***     -4.76 %       ±1.41% ±1.87%  ±2.44%

[Trott]

Can we add a test for this? I think the perf hit might be worth it for the robustness in this case, although I wonder if @nodejs/util would agree or not.

[ExE-Boss]

Note that this also uses %Array.prototype.includes% and %Array.prototype.push%:

node/lib/internal/util/inspect.js

Line 771 in b489173

if (ctx.seen.includes(value)) {

node/lib/internal/util/inspect.js

Line 991 in b489173

ctx.seen.push(value);

[yashLadha]

@ExE-Boss Those changes are not in the scope of the fix but I can raise another PR to move them to primordials.

[yashLadha]

@ExE-Boss Updated PR to include other primordials as well.

[aduh95]

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/905/ (queued, will 404 until it starts)

[ljharb]

Any reason not to rebase and land this?

[DerekNonGeneric]

Any reason not to rebase and land this?

I believe @Trott had requested a test for this in #36742 (comment), so that might be the only thing lacking.

[yashLadha]

Sure will add UTs

[BridgeAR]

util.inspect is quite performance sensitive. It is a sync API that many people use in their production, no matter if it's mainly meant for debugging. As such taking a performance hit does not seem right to me.