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

[libcxx] [test] Add a test parameter for disabling memory intensive tests #68214

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

mstorsjo
Copy link
Member

@mstorsjo mstorsjo commented Oct 4, 2023

Specifically, the test
std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp allocates a std::string with INT_MAX-1 elements, and then writes this to a std::stringstream. On Linux, running this test consumes around 5.0 GB of memory; on Windows, it ends up using up to 6.8 GB of memory.

This limits whether such tests can run on e.g. GitHub Actions runners, where the free runners are limited to 8 GB of memory.

This is somewhat similar to, but still notably different, from the existing test parameter long_tests.

…ests

Specifically, the test
std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp
allocates a std::string with INT_MAX-1 elements, and then writes
this to a std::stringstream. On Linux, running this test consumes
around 5.0 GB of memory; on Windows, it ends up using up to
6.8 GB of memory.

This limits whether such tests can run on e.g. GitHub Actions
runners, where the free runners are limited to 8 GB of memory.

This is somewhat similar to, but still notably different, from
the existing test parameter long_tests.
@mstorsjo mstorsjo requested a review from a team as a code owner October 4, 2023 12:32
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 4, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 4, 2023

@llvm/pr-subscribers-libcxx

Changes

Specifically, the test
std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp allocates a std::string with INT_MAX-1 elements, and then writes this to a std::stringstream. On Linux, running this test consumes around 5.0 GB of memory; on Windows, it ends up using up to 6.8 GB of memory.

This limits whether such tests can run on e.g. GitHub Actions runners, where the free runners are limited to 8 GB of memory.

This is somewhat similar to, but still notably different, from the existing test parameter long_tests.


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

2 Files Affected:

  • (modified) libcxx/test/std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp (+1)
  • (modified) libcxx/utils/libcxx/test/params.py (+8)
diff --git a/libcxx/test/std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp b/libcxx/test/std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp
index 3a5edac6c58b4fe..8dc74421e789590 100644
--- a/libcxx/test/std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp
+++ b/libcxx/test/std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: 32-bit-pointer
+// REQUIRES: large_tests
 
 // Test that tellp() does not break the stringstream after INT_MAX, due to use
 // of pbump() that accept int.
diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index d4e4b722347d623..5911af85c1c8169 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -278,6 +278,14 @@ def getStdFlag(cfg, std):
         help="Whether to enable tests that take longer to run. This can be useful when running on a very slow device.",
         actions=lambda enabled: [] if not enabled else [AddFeature("long_tests")],
     ),
+    Parameter(
+        name="large_tests",
+        choices=[True, False],
+        type=bool,
+        default=True,
+        help="Whether to enable tests that use a lot of memory. This can be useful when running on a device with limited amounts of memory.",
+        actions=lambda enabled: [] if not enabled else [AddFeature("large_tests")],
+    ),
     Parameter(
         name="hardening_mode",
         choices=["unchecked", "hardened", "safe", "debug"],

@ldionne
Copy link
Member

ldionne commented Oct 4, 2023

I have a slight problem with these kinds of Lit features, because what's considered "large" or "slow" is entirely subjective. This is also the reason why long-tests is not a great Lit parameter, and in fact I've contemplated removing it IDK how many times.

Instead, I think it would be better to have a way to exclude tests that time out after a certain amount of time, or that bypass some system limits like rlimits. WDYT?

@mstorsjo
Copy link
Member Author

mstorsjo commented Oct 5, 2023

I have a slight problem with these kinds of Lit features, because what's considered "large" or "slow" is entirely subjective.

Yes, that's true. In absolute terms, whether something is large might depend on how much memory you have. But if you look at the whole testsuite, where most tests run "reasonably sized" things, and we have one test that uses 5-7 GB of memory, it's pretty clear that there's a distinction there.

This is also the reason why long-tests is not a great Lit parameter, and in fact I've contemplated removing it IDK how many times.

Instead, I think it would be better to have a way to exclude tests that time out after a certain amount of time, or that bypass some system limits like rlimits. WDYT?

At least for the memory intensive test, it's not workable to just try running it and seeing if it works and tolerate failures. Half of the time, this test itself fails due to OOM, but half of the time, another test that is being compiled at the same time gets the OOM error. So we need to proactively stop it from trying to run.

If you mean that the tests (and the compiler itself) would be run within some wrapping that limits it from using more than x GB of memory etc, I guess that could work - but it'd be a fair amount of work - way out of scope for what I could take on right here.

@ldionne ldionne merged commit 122064a into llvm:main Oct 17, 2023
@mstorsjo mstorsjo deleted the libcxx-large-tests branch October 17, 2023 21:38
@mstorsjo
Copy link
Member Author

Thanks!

@ldionne - how would you feel about backporting this to 17.x? It's not a regression fix in itself, but it would allow running the tests on 17.x successfully.

@ldionne
Copy link
Member

ldionne commented Oct 17, 2023

I'd be OK with that -- this is not a risky change for the release, so I think that's acceptable.

mstorsjo added a commit to mstorsjo/llvm-project that referenced this pull request Oct 18, 2023
…ests (llvm#68214)

Specifically, the test std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp
allocates a std::string with INT_MAX-1 elements, and then writes this to
a std::stringstream. On Linux, running this test consumes around 5.0 GB
of memory; on Windows, it ends up using up to 6.8 GB of memory.

This limits whether such tests can run on e.g. GitHub Actions runners,
where the free runners are limited to 8 GB of memory.

This is somewhat similar to, but still notably different, from the
existing test parameter long_tests.

(cherry picked from commit 122064a)
Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Oct 20, 2023
Local branch amd-gfx ba5461f Merged main:761c9dd92789 into amd-gfx:5dff4b923208
Remote branch main 122064a [libcxx] [test] Add a test parameter for disabling memory intensive tests (llvm#68214)
tru pushed a commit that referenced this pull request Oct 24, 2023
…ests (#68214)

Specifically, the test std/input.output/string.streams/stringstream/stringstream.members/gcount.pass.cpp
allocates a std::string with INT_MAX-1 elements, and then writes this to
a std::stringstream. On Linux, running this test consumes around 5.0 GB
of memory; on Windows, it ends up using up to 6.8 GB of memory.

This limits whether such tests can run on e.g. GitHub Actions runners,
where the free runners are limited to 8 GB of memory.

This is somewhat similar to, but still notably different, from the
existing test parameter long_tests.

(cherry picked from commit 122064a)
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. test-suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants