You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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-turbofanminimum-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?
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?
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 😅).
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)
This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.
This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 240 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.
Activity
test: skip v8-updates/test-linux-perf-logger
test: skip v8-updates/test-linux-perf-logger
lukealbao commentedon 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-turbofanminimum-invocations-before-optimizationv8 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 thetest-perftests, as well. As I mentioned elsewhere, #50352 is just a port of that test suite, without the externalperfdependency.So, summing up my understanding of how to get this test suite active in all maintained branches:
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 commentedon 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 commentedon May 6, 2024
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 commentedon 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
test: unskip v8-updates/test-linux-perf-logger
lukealbao commentedon May 7, 2024
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.
test-linux-perf-loggerfile to v18 is a no-go by virtue of it being in maintenance mode?richardlau commentedon May 8, 2024
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/consoleWhile 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-perfor landingtest-linux-perf-loggeras a replacement.test: skip v8-updates/test-linux-perf-logger
richardlau commentedon May 16, 2024
The V8 CI on the
v18.x-stagingandv20.x-stagingbranches is now passing.For
v20.x-stagingI have cherry-picked 515b007, which allowstest-linux-perf-loggerto pass.I've also cherry-picked
54f1e0a6b76b77 to both branches, which will skip the still brokentest-linux-perf.lukealbao commentedon May 16, 2024
Just confirming I understand, it looks like 6b76b77 was the cherry-pick, not 54f1e0a?
richardlau commentedon May 17, 2024
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 commentedon May 27, 2024
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 commentedon May 28, 2024
@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 inv22.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).test: unskip v8-updates/test-linux-perf-logger
test: unskip v8-updates/test-linux-perf-logger
test: skip v8-updates/test-linux-perf-logger
test: skip v8-updates/test-linux-perf-logger
test: unskip v8-updates/test-linux-perf-logger
github-actions commentedon May 24, 2026
This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.
github-actions commentedon Jun 24, 2026
This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 240 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.