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

[libc++] Make benchmarks dry-run by default on the release branch #126441

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Feb 9, 2025

As reported in #125510, doing a full run of the benchmarks during release testing breaks for some of the testers, and it also takes a long time. The proper fix would be for the release testing process to call check-cxx instead of running lit directly inside libc++'s test directory: that will also have the benefit of actually running all of our tests, not only the Lit ones.

However, since that fix may take longer to happen, this patch tries to reduce the pain of release testers by dry-running benchmarks by default instead.

@ldionne ldionne requested a review from a team as a code owner February 9, 2025 21:47
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Feb 9, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 9, 2025

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

As reported in #125510, doing a full run of the benchmarks during release testing breaks for some of the testers, and it also takes a long time. The proper fix would be for the release testing process to call check-cxx instead of running lit directly inside libc++'s test directory: that will also have the benefit of actually running all of our tests, not only the Lit ones.

However, since that fix may take longer to happen, this patch tries to reduce the pain of release testers by dry-running benchmarks by default instead.


Full diff: https://github.com/llvm/llvm-project/pull/126441.diff

1 Files Affected:

  • (modified) libcxx/utils/libcxx/test/params.py (+1-1)
diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index 8fd3872cd8cbb05..6e13ad75ea94971 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -371,7 +371,7 @@ def getSuitableClangTidy(cfg):
         name="enable_benchmarks",
         choices=["no", "run", "dry-run"],
         type=str,
-        default="run",
+        default="dry-run",
         help="Whether to run the benchmarks in the test suite, to only dry-run them or to disable them entirely.",
         actions=lambda mode: [AddFeature(f"enable-benchmarks={mode}")],
     ),

@rorth rorth added this to the LLVM 20.X Release milestone Feb 10, 2025
Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

As reported in llvm#125510, doing a full run of the benchmarks during
release testing breaks for some of the testers, and it also takes a
long time. The proper fix would be for the release testing process to
call `check-cxx` instead of running lit directly inside libc++'s test
directory: that will also have the benefit of actually running all of
our tests, not only the Lit ones.

However, since that fix may take longer to happen, this patch tries
to reduce the pain of release testers by dry-running benchmarks by
default instead.
@tstellar tstellar merged commit 1f38351 into llvm:release/20.x Feb 11, 2025
13 of 18 checks passed
Copy link

@ldionne (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
Development

Successfully merging this pull request may close these issues.

5 participants