Broken `test/v8-updates/test-linux-perf-logger`

[targos]

/cc @lukealbao

Seen in https://github.com/nodejs/node/pull/50115 Test added in https://github.com/nodejs/node/pull/50352

https://ci.nodejs.org/job/node-test-commit-v8-linux/5743/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/consoleFull

12:00:23 not ok 2 v8-updates/test-linux-perf-logger
12:00:23   ---
12:00:23   duration_ms: 714.05100
12:00:23   severity: fail
12:00:23   exitcode: 1
12:00:23   stack: |-
12:00:23     node:assert:126
12:00:23       throw new AssertionError(obj);
12:00:23       ^
12:00:23     
12:00:23     AssertionError [ERR_ASSERTION]: 2 tests failed
12:00:23     
12:00:23     [ERROR 1] --perf-basic-prof compiled
12:00:23     Errors:
12:00:23     1. Expected to match /[a-fA-F0-9]+ [a-fA-F0-9]+.*:\*functionOne .+\/linux-perf-logger.js/
12:00:23     2. Expected to match /[a-fA-F0-9]+ [a-fA-F0-9]+.*:\*functionTwo .+\/linux-perf-logger.js/
12:00:23     Perf map content:
...
12:00:24     </end perf map content>
12:00:24     
12:00:24         at runSuite (/home/iojs/build/workspace/node-test-commit-v8-linux/test/v8-updates/test-linux-perf-logger.js:145:10)
12:00:24         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-v8-linux/test/v8-updates/test-linux-perf-logger.js:148:1)
12:00:24         at Module._compile (node:internal/modules/cjs/loader:1378:14)
12:00:24         at Module._extensions..js (node:internal/modules/cjs/loader:1437:10)
12:00:24         at Module.load (node:internal/modules/cjs/loader:1212:32)
12:00:24         at Module._load (node:internal/modules/cjs/loader:1028:12)
12:00:24         at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:142:12)
12:00:24         at node:internal/main/run_main_module:28:49 {
12:00:24       generatedMessage: false,
12:00:24       code: 'ERR_ASSERTION',
12:00:24       actual: 2,
12:00:24       expected: 0,
12:00:24       operator: 'strictEqual'
12:00:24     }
12:00:24     
12:00:24     Node.js v22.0.0-pre
12:00:24   ...
12:00:24 
12:00:24 Failed tests:
12:00:24 out/Release/node /home/iojs/build/workspace/node-test-commit-v8-linux/test/v8-updates/test-linux-perf-logger.js

[lukealbao]

Hi @targos sorry I haven't followed up on this -- this happened while I was out of pocket, and I thought I'd seen it get resolved before I returned. In particular, I missed the comment that the #50352 landed but wasn't actually being exercised in CI. I see that conversations are still happening around this test (cf. https://github.com/nodejs/build/issues/3645).

I'm unable to view CI runs. Has auth around that changed? I'd like to help if I can. I believe @richardlau is right that nodejs/node@515b007 would need to be backported to the v18.x branch.

Separately but relatedly, it looks like more recent v8 versions are causing the compiled test cases to fail due to the fact that they don't exceed the newly added ~invocation-count-for-turbofan~ minimum-invocations-before-optimization v8 option's default. Setting that to zero in the child process options make these cases pass for me when running on the main branch. Without view access to CI, I'm just taking a stab, but I suspect that's what's going on with the test-perf tests, as well. As I mentioned elsewhere, #50352 is just a port of that test suite, without the external perf dependency.

So, summing up my understanding of how to get this test suite active in all maintained branches:

• v18.x needs to have the changes from 515b007 backported. That will prevent the child process from exiting due to a bad v8 option.
• all others will need to add new invocation* count-based options to force JIT. Previously, that seems to have been accomplished simply by the --always-<jit> option.

Would it be helpful for me to submit a PR for the above?

[targos]

Yes, it would be very helpful!

Note that the test was just skipped in CI yesterday so a fixing PR would have to re-enable it.

I'm not sure why you don't have access to CI. Do you get an error message or something when you try to access it?

[lukealbao]

I tried logging out and in before, but now that I look at my notifications, I see this warning about the git plugin needing to be patched, so maybe it's that?

[targos]

Ah no, it's an old run that's not available anymore. Try https://ci.nodejs.org/job/node-test-commit-v8-linux/5954/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/console

[lukealbao]

Thanks @targos! Re the first bullet point: I think I was mistaken. On closer look, looks like nodejs/build#3645 was running on v21.x.

• Do I understand correctly that adding the test-linux-perf-logger file to v18 is a no-go by virtue of it being in maintenance mode?
• Should I keep an eye out to do anything special about backporting fixes into other branches? I see that 52869 is labelled as don't land on v20.
• AFAICT, CI for 52869 is failing because of windows heap exhaustion, but please LMK if there's something material I can help with around this issue.

[richardlau]

I ran a V8 CI for v18.x-staging -- it failed because of test-linux-perf: https://ci.nodejs.org/job/node-test-commit-v8-linux/5966/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/console While v18.x is in maintenance, we do still occassionally need to land V8 backports (see https://github.com/nodejs/node/pull/52337 for example) so it would be really useful to be able to get the V8 CI passing again, whether that would be fixing test-linux-perf or landing test-linux-perf-logger as a replacement.

[richardlau]

The V8 CI on the v18.x-staging and v20.x-staging branches is now passing.

For v20.x-staging I have cherry-picked https://github.com/nodejs/node/commit/515b007faedf529861b22823f8a722eebed837fa, which allows test-linux-perf-logger to pass.

I've also cherry-picked ~https://github.com/nodejs/node/commit/54f1e0a48882c7818e771ad918528a5dd5cfd6e4~ https://github.com/nodejs/node/commit/6b76b7782cb4281ac11dcf58ff14fdf88bc5a5d7 to both branches, which will skip the still broken test-linux-perf.

[lukealbao]

I've also cherry-picked https://github.com/nodejs/node/commit/54f1e0a48882c7818e771ad918528a5dd5cfd6e4 to both branches, which will skip the still broken test-linux-perf.

Just confirming I understand, it looks like 6b76b7782cb4281ac11dcf58ff14fdf88bc5a5d7 was the cherry-pick, not 54f1e0a?

[richardlau]

I've also cherry-picked 54f1e0a to both branches, which will skip the still broken test-linux-perf.

Just confirming I understand, it looks like 6b76b77 was the cherry-pick, not 54f1e0a?

Whoops, you're correct, https://github.com/nodejs/node/commit/6b76b7782cb4281ac11dcf58ff14fdf88bc5a5d7 was the commit cherry-picked (fortunately I did cherry-pick the correct commit, and only got the reference wrong when updating this issue 😅).

[davidfiala]

As this test was being skipped in the v18x and v20x branches, was there a deliberate reason it isn't also being skipped in the v22x branch as well? I only ask as it appears to be a blocker preventing v8 lite mode from being fixed in the v22x series: https://github.com/nodejs/node/pull/52725#issuecomment-2082371779

Thanks!

[richardlau]

@davidfiala This test is not skipped on the v18.x and v20.x branches, only test-linux-perf, which is an older test and is also skipped on v22.x.

This test (test-linux-perf-logger) is currently skipped in v22.x-staging (by https://github.com/nodejs/node/pull/52821).

For main, I'll try to land https://github.com/nodejs/node/pull/52869 later today (I think it needs a manual land to properly squash the commits).