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.

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

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

Open
targos opened this issue Dec 29, 2023 · 11 comments
Open

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

targos opened this issue Dec 29, 2023 · 11 comments

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
targos added a commit to targos/node that referenced this issue May 3, 2024
nodejs-github-bot pushed a commit that referenced this issue May 5, 2024
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

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

richardlau commented May 16, 2024

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 😅).

@davidfiala
Copy link

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: #52725 (comment)

Thanks!

@richardlau
Copy link
Member

@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 #52821).

For main, I'll try to land #52869 later today (I think it needs a manual land to properly squash the commits).

richardlau pushed a commit that referenced this issue May 29, 2024
Refs: #51308
Refs: #52821
PR-URL: #52869
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
targos pushed a commit that referenced this issue Jun 1, 2024
Refs: #51308
Refs: #52821
PR-URL: #52869
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jun 17, 2024
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>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
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>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
Refs: nodejs#51308
Refs: nodejs#52821
PR-URL: nodejs#52869
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
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>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
Refs: nodejs#51308
Refs: nodejs#52821
PR-URL: nodejs#52869
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants