From 4e547f406054105599a25f9ea7c52210fc7d9398 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 30 Nov 2019 10:56:11 +0100 Subject: [PATCH 1/4] benchmark: add more util inspect and format benchmarks This adds a couple of benchmarks to check different options and code paths. --- benchmark/util/format.js | 2 ++ benchmark/util/inspect-proxy.js | 18 +++++++++++++----- test/benchmark/test-benchmark-util.js | 4 +++- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/benchmark/util/format.js b/benchmark/util/format.js index 44e950a6ca6522..976e0f4e655486 100644 --- a/benchmark/util/format.js +++ b/benchmark/util/format.js @@ -13,6 +13,8 @@ const inputs = { 'no-replace-2': ['foobar', 'yeah', 'mensch', 5], 'only-objects': [{ msg: 'This is an error' }, { msg: 'This is an error' }], 'many-%': ['replace%%%%s%%%%many%s%s%s', 'percent'], + 'object-to-string': ['foo %s bar', { toString() { return 'bla'; } }], + 'object-%s': ['foo %s bar', { a: true, b: false }], }; const bench = common.createBenchmark(main, { diff --git a/benchmark/util/inspect-proxy.js b/benchmark/util/inspect-proxy.js index dde86941ff42a3..fd89d568aba35f 100644 --- a/benchmark/util/inspect-proxy.js +++ b/benchmark/util/inspect-proxy.js @@ -3,13 +3,21 @@ const util = require('util'); const common = require('../common.js'); -const bench = common.createBenchmark(main, { n: [2e4] }); +const bench = common.createBenchmark(main, { + n: [2e4], + showProxy: [0, 1], + isProxy: [0, 1] +}); -function main({ n }) { - const proxyA = new Proxy({}, { get: () => {} }); - const proxyB = new Proxy(() => {}, {}); +function main({ n, showProxy, isProxy }) { + let proxyA = {}; + let proxyB = () => {}; + if (isProxy) { + proxyA = new Proxy(proxyA, { get: () => {} }); + proxyB = new Proxy(proxyB, {}); + } bench.start(); for (let i = 0; i < n; i += 1) - util.inspect({ a: proxyA, b: proxyB }, { showProxy: true }); + util.inspect({ a: proxyA, b: proxyB }, { showProxy }); bench.end(n); } diff --git a/test/benchmark/test-benchmark-util.js b/test/benchmark/test-benchmark-util.js index 97b02bbdeed5cd..b66d4fdb9b4cf6 100644 --- a/test/benchmark/test-benchmark-util.js +++ b/test/benchmark/test-benchmark-util.js @@ -14,5 +14,7 @@ runBenchmark('util', 'size=1', 'type=', 'len=1', - 'version=native'], + 'version=native', + 'isProxy=1', + 'showProxy=1'], { NODEJS_BENCHMARK_ZERO_ALLOWED: 1 }); From 1effffd96528899878db4247ee3a520a240e59e6 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 30 Nov 2019 11:04:34 +0100 Subject: [PATCH 2/4] util: improve performance inspecting proxies This makes sure we do not retrieve the handler in case it's not required. This improves the performance a tiny bit for these cases. --- lib/internal/util/inspect.js | 4 ++-- src/node_util.cc | 20 +++++++++++++------- test/parallel/test-util-inspect-proxy.js | 5 ++++- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js index dde2066294590b..ea459c033a132b 100644 --- a/lib/internal/util/inspect.js +++ b/lib/internal/util/inspect.js @@ -570,12 +570,12 @@ function formatValue(ctx, value, recurseTimes, typedArray) { const context = value; // Always check for proxies to prevent side effects and to prevent triggering // any proxy handlers. - const proxy = getProxyDetails(value); + const proxy = getProxyDetails(value, ctx.showProxy); if (proxy !== undefined) { if (ctx.showProxy) { return formatProxy(ctx, proxy, recurseTimes); } - value = proxy[0]; + value = proxy; } // Provide a hook for user-specified inspect functions. diff --git a/src/node_util.cc b/src/node_util.cc index 9c24985a47a507..375d0d02164d7f 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -93,13 +93,19 @@ static void GetProxyDetails(const FunctionCallbackInfo& args) { Local proxy = args[0].As(); - Local ret[] = { - proxy->GetTarget(), - proxy->GetHandler() - }; - - args.GetReturnValue().Set( - Array::New(args.GetIsolate(), ret, arraysize(ret))); + if (args[1]->IsTrue()) { + Local ret[] = { + proxy->GetTarget(), + proxy->GetHandler() + }; + + args.GetReturnValue().Set( + Array::New(args.GetIsolate(), ret, arraysize(ret))); + } else { + Local ret = proxy->GetTarget(); + + args.GetReturnValue().Set(ret); + } } static void PreviewEntries(const FunctionCallbackInfo& args) { diff --git a/test/parallel/test-util-inspect-proxy.js b/test/parallel/test-util-inspect-proxy.js index da0512eda1a8b2..21cc1a4658f9cd 100644 --- a/test/parallel/test-util-inspect-proxy.js +++ b/test/parallel/test-util-inspect-proxy.js @@ -43,10 +43,13 @@ util.inspect(proxyObj, opts); // getProxyDetails is an internal method, not intended for public use. // This is here to test that the internals are working correctly. -const details = processUtil.getProxyDetails(proxyObj); +let details = processUtil.getProxyDetails(proxyObj, true); assert.strictEqual(target, details[0]); assert.strictEqual(handler, details[1]); +details = processUtil.getProxyDetails(proxyObj, false); +assert.strictEqual(target, details); + assert.strictEqual( util.inspect(proxyObj, opts), 'Proxy [\n' + From fcf8b19c540678da3de7b78e9e347f6c70d22a24 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 1 Dec 2019 01:08:37 +0100 Subject: [PATCH 3/4] util: never trigger any proxy traps using `format()` --- lib/internal/util/inspect.js | 7 +++++++ test/parallel/test-util-inspect-proxy.js | 3 +++ 2 files changed, 10 insertions(+) diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js index ea459c033a132b..c987b0c6f7e326 100644 --- a/lib/internal/util/inspect.js +++ b/lib/internal/util/inspect.js @@ -1575,6 +1575,13 @@ function reduceToSingleString( } function hasBuiltInToString(value) { + // Prevent triggering proxy traps. + const getFullProxy = false; + const proxyTarget = getProxyDetails(value, getFullProxy); + if (proxyTarget !== undefined) { + value = proxyTarget; + } + // Count objects that have no `toString` function as built-in. if (typeof value.toString !== 'function') { return true; diff --git a/test/parallel/test-util-inspect-proxy.js b/test/parallel/test-util-inspect-proxy.js index 21cc1a4658f9cd..b8611a5f8cd8c6 100644 --- a/test/parallel/test-util-inspect-proxy.js +++ b/test/parallel/test-util-inspect-proxy.js @@ -41,6 +41,9 @@ proxyObj = new Proxy(target, handler); // Inspecting the proxy should not actually walk it's properties util.inspect(proxyObj, opts); +// Make sure inspecting object does not trigger any proxy traps. +util.format('%s', proxyObj); + // getProxyDetails is an internal method, not intended for public use. // This is here to test that the internals are working correctly. let details = processUtil.getProxyDetails(proxyObj, true); From 0f86ca4172909991409bf675a5a763be32569f8a Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 3 Dec 2019 10:20:37 +0100 Subject: [PATCH 4/4] fixup: util: improve performance inspecting proxies --- benchmark/util/inspect-proxy.js | 2 +- lib/internal/util/inspect.js | 2 +- src/node_util.cc | 2 ++ test/parallel/test-util-inspect-proxy.js | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/benchmark/util/inspect-proxy.js b/benchmark/util/inspect-proxy.js index fd89d568aba35f..02379cdc770f3b 100644 --- a/benchmark/util/inspect-proxy.js +++ b/benchmark/util/inspect-proxy.js @@ -4,7 +4,7 @@ const util = require('util'); const common = require('../common.js'); const bench = common.createBenchmark(main, { - n: [2e4], + n: [1e5], showProxy: [0, 1], isProxy: [0, 1] }); diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js index c987b0c6f7e326..7e4e7643291aee 100644 --- a/lib/internal/util/inspect.js +++ b/lib/internal/util/inspect.js @@ -570,7 +570,7 @@ function formatValue(ctx, value, recurseTimes, typedArray) { const context = value; // Always check for proxies to prevent side effects and to prevent triggering // any proxy handlers. - const proxy = getProxyDetails(value, ctx.showProxy); + const proxy = getProxyDetails(value, !!ctx.showProxy); if (proxy !== undefined) { if (ctx.showProxy) { return formatProxy(ctx, proxy, recurseTimes); diff --git a/src/node_util.cc b/src/node_util.cc index 375d0d02164d7f..07a7b69dbd9db4 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -91,6 +91,8 @@ static void GetProxyDetails(const FunctionCallbackInfo& args) { if (!args[0]->IsProxy()) return; + CHECK(args[1]->IsBoolean()); + Local proxy = args[0].As(); if (args[1]->IsTrue()) { diff --git a/test/parallel/test-util-inspect-proxy.js b/test/parallel/test-util-inspect-proxy.js index b8611a5f8cd8c6..505503d5790759 100644 --- a/test/parallel/test-util-inspect-proxy.js +++ b/test/parallel/test-util-inspect-proxy.js @@ -111,7 +111,7 @@ const expected6 = 'Proxy [\n' + ' ]\n' + ']'; assert.strictEqual( - util.inspect(proxy1, { showProxy: true, depth: null }), + util.inspect(proxy1, { showProxy: 1, depth: null }), expected1); assert.strictEqual(util.inspect(proxy2, opts), expected2); assert.strictEqual(util.inspect(proxy3, opts), expected3);