test: skip less scenarios for test-linux-perf.js

[refack]

Refs: nodejs/build#1774

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

[mmarchini]

These changes should suffice, thank you for looking into this!

09:00:06 not ok 1 v8-updates/test-linux-perf
09:00:06   ---
09:00:06   duration_ms: 0.816
09:00:06   severity: fail
09:00:06   exitcode: 1
09:00:06   stack: |-
09:00:06     /home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/v8-updates/test-linux-perf.js:52
09:00:06       throw new Error(`Failed to execute 'perf': ${perf.stderr}`);
09:00:06       ^
09:00:06     
09:00:06     Error: Failed to execute 'perf': 
09:00:06     
09:00:06     #
09:00:06     # Fatal error in , line 0
09:00:06     # Check failed: !obj->IsCode().
09:00:06     #
09:00:06     #
09:00:06     #
09:00:06     #FailureMessage Object: 0x7ffd12ff5640[ perf record: Woken up 1 times to write data ]
09:00:06     [ perf record: Captured and wrote 0.003 MB perf.data (2 samples) ]
09:00:06     
09:00:06         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/v8-updates/test-linux-perf.js:52:9)
09:00:06         at Module._compile (internal/modules/cjs/loader.js:759:30)
09:00:06         at Object.Module._extensions..js (internal/modules/cjs/loader.js:770:10)
09:00:06         at Module.load (internal/modules/cjs/loader.js:628:32)
09:00:06         at Function.Module._load (internal/modules/cjs/loader.js:555:12)
09:00:06         at Function.Module.runMain (internal/modules/cjs/loader.js:826:10)
09:00:06         at internal/main/run_main_module.js:17:11

The failing test is caused by #27346, which will be fixed once we pick v8/v8@d915b8d (I requested a upstream backport to 7.4, waiting for response. If backport upstream is not possible, I'll cherry-pick and open a PR). Not sure if we should wait until the issue is fixed on master before landing this PR, since it's exposing a legitimate failure.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/23631/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2357/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/23632/
Windows-CI resume: https://ci.nodejs.org/job/node-test-commit-windows-fanned/27819/

[refack]

@mmarchini do you know what this means?

12:51:53 not ok 2 v8-updates/test-linux-perf
12:51:53   ---
12:51:53   duration_ms: 3.121
12:51:53   severity: fail
12:51:53   exitcode: 1
12:51:53   stack: |-
12:51:53     assert.js:362
12:51:53         throw err;
12:51:53         ^
12:51:53     
12:51:53     AssertionError [ERR_ASSERTION]: Couldn't find interpreted functionOne()
12:51:53         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-v8-linux/nodes/benchmark-ubuntu1604-intel-64/v8test/v8test/test/v8-updates/test-linux-perf.js:67:8)
12:51:53         at Module._compile (internal/modules/cjs/loader.js:777:30)
12:51:53         at Object.Module._extensions..js (internal/modules/cjs/loader.js:788:10)
12:51:53         at Module.load (internal/modules/cjs/loader.js:640:32)
12:51:53         at Function.Module._load (internal/modules/cjs/loader.js:555:12)
12:51:53         at Function.Module.runMain (internal/modules/cjs/loader.js:840:10)
12:51:53         at internal/main/run_main_module.js:17:11 {
12:51:53       generatedMessage: false,
12:51:53       code: 'ERR_ASSERTION',
12:51:53       actual: null,
12:51:53       expected: true,
12:51:53       operator: '=='
12:51:53     }
12:51:53   ...

https://ci.nodejs.org/job/node-test-commit-v8-linux/2359/nodes=benchmark-ubuntu1604-intel-64,v8test=v8test/console

[mmarchini]

Yeah, I noticed this version of V8 is optimizing functionOne faster than before. Increasing sampling frequency might help, but will still be flaky. Or we could run the fixture script a few times (5, 10) and search for frames on the output of all executions. Let me see if we can improve this somehow first, if not we can try alternative solutions.

[refack]

Thanks.

Since this is run only once and only for V8 updates, IMO it could be a "pummel" test. We can extend it's timeout, or generate synthetic CPU load if needed.

[mmarchini]

Ok, I was able to rewrite it in a way it's guaranteed to work:

diff --git a/test/fixtures/linux-perf.js b/test/fixtures/linux-perf.js
index 011ef19777..90ea2f2e33 100644
--- a/test/fixtures/linux-perf.js
+++ b/test/fixtures/linux-perf.js
@@ -1,26 +1,17 @@
 'use strict';
 
-const crypto = require('crypto');
+const { spawnSync } = require("child_process");
+const sleepTime = new Number(process.argv[2] || "0.1");
+const repeat = new Number(process.argv[3]) || 5;
 
-// Functions should be complex enough for V8 to run them a few times before
-// compiling, but not complex enough to always stay in interpreted mode. They
-// should also take some time to run, otherwise Linux perf might miss them
-// entirely even when sampling at a high frequency.
-function functionOne(i) {
-  for (let j=i; j > 0; j--) {
-    crypto.createHash('md5').update(functionTwo(i, j)).digest("hex");
-  }
+function functionOne() {
+  functionTwo();
 }
 
-function functionTwo(x, y) {
-  let data = ((((x * y) + (x / y)) * y) ** (x + 1)).toString();
-  if (x % 2 == 0) {
-    return crypto.createHash('md5').update(data.repeat((x % 100) + 1)).digest("hex");
-  } else {
-    return crypto.createHash('md5').update(data.repeat((y % 100) + 1)).digest("hex");
-  }
+function functionTwo() {
+  spawnSync('sleep', [`${sleepTime}`]);
 }
 
-for (let i = 0; i < 1000; i++) {
-  functionOne(i);
+for (let i = 0; i < repeat; i++) {
+  functionOne();
 }
diff --git a/test/v8-updates/test-linux-perf.js b/test/v8-updates/test-linux-perf.js
index 98f8f6c527..526dfbcd4e 100644
--- a/test/v8-updates/test-linux-perf.js
+++ b/test/v8-updates/test-linux-perf.js
@@ -22,16 +22,53 @@ tmpdir.refresh();
 if (process.config.variables.node_shared)
   common.skip("can't test Linux perf with shared libraries yet");
 
-const perfArgs = [
+if (!common.isLinux)
+  common.skip('only testing Linux for now');
+
+const frequency = 99;
+
+const repeat = 5;
+
+// Expected number of samples we'll capture per repeat
+const sampleCount = 10;
+const sleepTime = sampleCount * (1.0 / frequency);
+
+const perfFlags = [
   'record',
-  '-F999',
+  `-F${frequency}`,
   '-g',
-  '--',
-  process.execPath,
+];
+
+const nodeCommonFlags = [
   '--perf-basic-prof',
   '--interpreted-frames-native-stack',
   '--no-turbo-inlining',  // Otherwise simple functions might get inlined.
+];
+
+const perfInterpretedFramesArgs = [
+  ...perfFlags,
+  '--',
+  process.execPath,
+  ...nodeCommonFlags,
+  '--no-opt',
+  fixtures.path('linux-perf.js'),
+  `${sleepTime}`,
+  `${repeat}`,
+];
+
+const perfCompiledFramesArgs = [
+  ...perfFlags,
+  '--',
+  process.execPath,
+  ...nodeCommonFlags,
+  '--always-opt',
   fixtures.path('linux-perf.js'),
+  `${sleepTime}`,
+  `${repeat}`,
+];
+
+const perfArgsList = [
+  perfInterpretedFramesArgs, perfCompiledFramesArgs
 ];
 
 const perfScriptArgs = [
@@ -44,25 +81,27 @@ const options = {
   maxBuffer: Infinity
 };
 
-if (!common.isLinux)
-  common.skip('only testing Linux for now');
+let output = "";
+
+for (let perfArgs of perfArgsList) {
+  const perf = spawnSync('perf', perfArgs, options);
+  assert.ifError(perf.error);
+  if (perf.status !== 0)
+    throw new Error(`Failed to execute 'perf': ${perf.stderr}`);
 
-const perf = spawnSync('perf', perfArgs, options);
-assert.ifError(perf.error);
-if (perf.status !== 0)
-  throw new Error(`Failed to execute 'perf': ${perf.stderr}`);
+  const perfScript = spawnSync('perf', perfScriptArgs, options);
+  assert.ifError(perfScript.error);
+  if (perfScript.status !== 0)
+    throw new Error(`Failed to execute perf script: ${perfScript.stderr}`);
 
-const perfScript = spawnSync('perf', perfScriptArgs, options);
-assert.ifError(perfScript.error);
-if (perfScript.status !== 0)
-  throw new Error(`Failed to execute perf script: ${perfScript.stderr}`);
+  output += perfScript.stdout;
+}
 
 const interpretedFunctionOneRe = /InterpretedFunction:functionOne/;
 const compiledFunctionOneRe = /LazyCompile:\*functionOne/;
 const interpretedFunctionTwoRe = /InterpretedFunction:functionTwo/;
 const compiledFunctionTwoRe = /LazyCompile:\*functionTwo/;
 
-const output = perfScript.stdout;
 
 assert.ok(output.match(interpretedFunctionOneRe),
           "Couldn't find interpreted functionOne()");

Essentially we'll run the fixture two times, one with optimizations disabled (--no-opt) and one forcing optimization (--always-opt). Instead of using arithmetic and crypto operations to generate load that will keep frames on the CPU (which gives us no guarantee about execution time), we use spawnSync('sleep'), because this gives us a predictable minimum time each frame will stay in the CPU. The sleep time we use is calculated based on the frequency we'll use to sample stack traces and the number of samples we want to capture for a given stack trace. With this methods we guarantee that we'll get at least one sample with the functions we want on the stack trace. We repeat the function calls a few times just to be sure, but in theory this is not necessary.

I ran this 100 times (with a frequency of 99, which is way lower than what we're using today) and there was no failures.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/23683/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2362/