Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Broken test/v8-updates/test-linux-perf-logger #51308

Open
targos opened this issue Dec 29, 2023 · 9 comments · May be fixed by #52869
Open

Broken test/v8-updates/test-linux-perf-logger #51308

targos opened this issue Dec 29, 2023 · 9 comments · May be fixed by #52869

Comments

@targos
Copy link
Member

targos commented Dec 29, 2023

/cc @lukealbao

Seen in #50115
Test added in #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
@marco-ippolito marco-ippolito mentioned this issue Mar 5, 2024
Merged
@targos targos mentioned this issue Mar 5, 2024
Open
@richardlau richardlau mentioned this issue Mar 26, 2024
Open
@daeyeon daeyeon mentioned this issue Apr 29, 2024
Open
@targos targos mentioned this issue May 3, 2024
Open
targos added a commit to targos/node that referenced this issue May 3, 2024
@targos
test: skip v8-updates/test-linux-perf-logger
18d42f0 
Refs: nodejs#51308
@targos targos mentioned this issue May 3, 2024
Merged
nodejs-github-bot pushed a commit that referenced this issue May 5, 2024
@targos
test: skip v8-updates/test-linux-perf-logger
54f1e0a 
Refs: #51308
PR-URL: #52821
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@lukealbao
Copy link
Contributor

lukealbao commented May 6, 2024 •
edited

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. nodejs/build#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
Copy link
Member Author

targos commented May 6, 2024

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
Copy link
Contributor

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?

image

@targos
Copy link
Member Author

targos commented May 6, 2024

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 added a commit to lukealbao/node that referenced this issue May 7, 2024
@lukealbao
test: unskip v8-updates/test-linux-perf-logger
cdc89d0 
Refs: nodejs#51308 nodejs#52821
@lukealbao lukealbao linked a pull request May 7, 2024 that will close this issue
Open
@lukealbao
Copy link
Contributor

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
Copy link
Member

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 #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.

Ch3nYuY pushed a commit to Ch3nYuY/node that referenced this issue May 8, 2024
@targos @Ch3nYuY
test: skip v8-updates/test-linux-perf-logger
40d385b 
Refs: nodejs#51308
PR-URL: nodejs#52821
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos added a commit that referenced this issue May 8, 2024
@targos
test: skip v8-updates/test-linux-perf-logger
1448959 
Refs: #51308
PR-URL: #52821
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@lukealbao lukealbao mentioned this issue May 9, 2024
Closed
@richardlau
Copy link
Member

richardlau commented May 16, 2024 •
edited

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

For v20.x-staging I have cherry-picked 515b007, which allows test-linux-perf-logger to pass.

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

@lukealbao
Copy link
Contributor

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?

@richardlau
Copy link
Member

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, 6b76b77 was the commit cherry-picked (fortunately I did cherry-pick the correct commit, and only got the reference wrong when updating this issue 😅).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: unskip v8-updates/test-linux-perf-logger lukealbao/node
3 participants
@targos @lukealbao @richardlau