From 3b626d20431e614cc9ad670d3982373d297ac4a9 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 21 Feb 2019 03:26:27 -0800 Subject: [PATCH 1/2] test: improve performance of test-crypto-timing-safe-equal-benchmarks Resetting require.cache() to `Object.create(null)` each time rather than deleting the specific key results in a 10x improvement in running time. Fixes: https://github.com/nodejs/node/issues/25984 Refs: https://github.com/nodejs/node/issues/26229 --- test/pummel/test-crypto-timing-safe-equal-benchmarks.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/pummel/test-crypto-timing-safe-equal-benchmarks.js b/test/pummel/test-crypto-timing-safe-equal-benchmarks.js index 0592b9a0d63674..3adc929b08ba2b 100644 --- a/test/pummel/test-crypto-timing-safe-equal-benchmarks.js +++ b/test/pummel/test-crypto-timing-safe-equal-benchmarks.js @@ -18,8 +18,12 @@ function runOneBenchmark(...args) { // Don't let the comparison function get cached. This avoid a timing // inconsistency due to V8 optimization where the function would take - // less time when called with a specific set of parameters. - delete require.cache[require.resolve(BENCHMARK_FUNC_PATH)]; + // less time when called with a specific set of parameters. This used to use + // `delete` instead of `Object.create(null)` but that resulted in many times + // worse performance, so compare the runtime for this test if you change it + // back or otherwise modify the `Object.create(null)` line below. + // Ref: https://github.com/nodejs/node/pull/26237 + require.cache = Object.create(null); return result; } From 69655d796517c0c747bb96fea7185cd9614d50c4 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 21 Feb 2019 14:50:42 -0800 Subject: [PATCH 2/2] squash! test: improve performance of test-crypto-timing-safe-equal-benchmarks Use eval() instead.... --- ...crypto-timing-safe-equal-benchmark-func.js | 17 ---------- ...est-crypto-timing-safe-equal-benchmarks.js | 31 +++++++++---------- 2 files changed, 15 insertions(+), 33 deletions(-) delete mode 100644 test/fixtures/crypto-timing-safe-equal-benchmark-func.js diff --git a/test/fixtures/crypto-timing-safe-equal-benchmark-func.js b/test/fixtures/crypto-timing-safe-equal-benchmark-func.js deleted file mode 100644 index 96470e3e4434a8..00000000000000 --- a/test/fixtures/crypto-timing-safe-equal-benchmark-func.js +++ /dev/null @@ -1,17 +0,0 @@ -'use strict'; - -const assert = require('assert'); -module.exports = (compareFunc, firstBufFill, secondBufFill, bufSize) => { - const firstBuffer = Buffer.alloc(bufSize, firstBufFill); - const secondBuffer = Buffer.alloc(bufSize, secondBufFill); - - const startTime = process.hrtime(); - const result = compareFunc(firstBuffer, secondBuffer); - const endTime = process.hrtime(startTime); - - // Ensure that the result of the function call gets used, so it doesn't - // get discarded due to engine optimizations. - assert.strictEqual(result, firstBufFill === secondBufFill); - - return endTime[0] * 1e9 + endTime[1]; -}; diff --git a/test/pummel/test-crypto-timing-safe-equal-benchmarks.js b/test/pummel/test-crypto-timing-safe-equal-benchmarks.js index 3adc929b08ba2b..b649b071e1e49d 100644 --- a/test/pummel/test-crypto-timing-safe-equal-benchmarks.js +++ b/test/pummel/test-crypto-timing-safe-equal-benchmarks.js @@ -6,25 +6,24 @@ if (!common.hasCrypto) if (!common.enoughTestMem) common.skip('memory-intensive test'); -const fixtures = require('../common/fixtures'); const assert = require('assert'); const crypto = require('crypto'); -const BENCHMARK_FUNC_PATH = - `${fixtures.fixturesDir}/crypto-timing-safe-equal-benchmark-func`; -function runOneBenchmark(...args) { - const benchmarkFunc = require(BENCHMARK_FUNC_PATH); - const result = benchmarkFunc(...args); - - // Don't let the comparison function get cached. This avoid a timing - // inconsistency due to V8 optimization where the function would take - // less time when called with a specific set of parameters. This used to use - // `delete` instead of `Object.create(null)` but that resulted in many times - // worse performance, so compare the runtime for this test if you change it - // back or otherwise modify the `Object.create(null)` line below. - // Ref: https://github.com/nodejs/node/pull/26237 - require.cache = Object.create(null); - return result; +function runOneBenchmark(compareFunc, firstBufFill, secondBufFill, bufSize) { + return eval(` + const firstBuffer = Buffer.alloc(bufSize, firstBufFill); + const secondBuffer = Buffer.alloc(bufSize, secondBufFill); + + const startTime = process.hrtime(); + const result = compareFunc(firstBuffer, secondBuffer); + const endTime = process.hrtime(startTime); + + // Ensure that the result of the function call gets used, so it doesn't + // get discarded due to engine optimizations. + assert.strictEqual(result, firstBufFill === secondBufFill); + + endTime[0] * 1e9 + endTime[1]; + `); } function getTValue(compareFunc) {