build: fix the heap profiling build and cover it in CI - #312
Merged
Merged
Conversation
The sampled memory profile tests compared std::vector<allocation_site>
with BOOST_CHECK_EQUAL, which requires an operator<< for the vector
itself. Nothing provides one: the test defines operator<< for a single
allocation_site, and Boost.Test has no printer for collections, so the
only thing that ever satisfied this was the generic vector operator<< in
sstring.hh, which sits behind SEASTAR_DEPRECATED_OSTREAM_FORMATTERS and
is off by default. The result was that alloc_test did not compile in a
heap profiling build:
print_helper.hpp:53:39: error: static assertion failed ...
Type has to implement operator<< to be printable
Use BOOST_CHECK_EQUAL_COLLECTIONS instead, which compares and prints
element by element and so only needs the per-allocation_site operator<<
that the test already defines. That operator was previously dead code,
since no assertion ever passed a lone allocation_site to Boost. This
also reports the first differing index rather than dumping both
vectors.
(cherry picked from commit fcfebba)
abseil LTS 20220623, which is what Ubuntu's 24.04 libabsl-dev ships, includes
<ciso646> from absl/base/options.h. libstdc++ 16 emits a #warning for
that header for C++20 and later:
ciso646:49:6: error: "<ciso646> is not a standard header since C++20,
use <version> to detect implementation-specific macros"
This breaks the build due to warnings-as-errors: disable the warning
at the include site.
Redpanda-only, this can be squashed into the change which originated absl
in our fork in a future rebase.
test_sampled_profile_collection_small and _large each run two identical allocation loops so that there are two distinct call sites, then require sampled_memory_profile() to report exactly 2. Whether a loop is sampled at all is probabilistic: the sampler draws the gap to the next sample from an exponential distribution whose mean is the sampling interval, so a loop that allocates N intervals worth of bytes records nothing with probability e^-N. Both tests sized their loops at only N=5 (500 bytes against a 100 byte interval, and 5000000 bytes against a 1000000 byte interval), which leaves each test failing on about 2*e^-5 = 1.3% of runs with critical check stats.size() == 2 has failed [1 != 2] Give both loops a much wider margin. _small raises count to 1000 for N=50. _large lowers the interval to 200000 instead, for N=25, which keeps the allocation size and the total memory footprint unchanged; it cannot lower it further because sample_size() accounts a sample as max(allocated_size, interval) and a 100000 byte request allocates 131072, so the interval has to stay above that for the neighbouring size == count * sample_rate assertion to hold. Measured over 500 runs of each test: 8/400 and 1/400 failures before, 0/500 and 0/500 after. Closes scylladb#3580 (cherry picked from commit 3bb2e37)
We should use --heap-profiling in our CI as that's how we build it on the Redpanda side, and so we should bench/build/test in that context (this is helpful also as upstream does not build much with --heap-profiling so this is our chance to catch more stuff).
There was a problem hiding this comment.
🟡 Changes recommended
The updated sampled-profile assertions still don’t validate count/size because allocation_site::operator== compares only backtraces, so the tests can pass even when sampled values differ.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes build/test breakages that only appear when Seastar is configured with heap profiling enabled, and extends CI to ensure --heap-profiling builds are continuously covered.
Changes:
- Make heap profiling sampled-profile unit tests both compile and be less flaky by adjusting sampling margins and replacing
BOOST_CHECK_EQUALvector comparisons. - Silence a
-Werror-promoted warning triggered by older Abseil versions including<ciso646>, scoped to the Abseil include site. - Enable
--heap-profilingin the shared CI configure step so all CI jobs build with heap profiling enabled.
File summaries
| File | Description |
|---|---|
tests/unit/alloc_test.cc |
Adjusts sampled heap profiling tests for determinism and compilation under heap-profiling builds. |
include/seastar/core/chunked_hash_map.hh |
Adds compiler-specific diagnostic suppression around the Abseil hash include to avoid <ciso646> warnings-as-errors. |
.github/workflows/install-build-env.sh |
Enables heap profiling in CI builds by passing --heap-profiling to configure.py. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
625
to
+632
| // two back-to-back copies of the sample should have the same value | ||
| BOOST_CHECK_EQUAL(stats0, stats1); | ||
| BOOST_CHECK_EQUAL_COLLECTIONS(stats0.begin(), stats0.end(), stats1.begin(), stats1.end()); | ||
|
|
||
| // check that we get the same value from the raw array iterface | ||
| std::vector<seastar::memory::allocation_site> stats2(stats0.size()); | ||
| auto sz2 = seastar::memory::sampled_memory_profile(stats2.data(), stats2.size()); | ||
| BOOST_CHECK_EQUAL(stats0.size(), sz2); | ||
| BOOST_CHECK_EQUAL(stats0, stats2); | ||
| BOOST_CHECK_EQUAL_COLLECTIONS(stats0.begin(), stats0.end(), stats2.begin(), stats2.end()); |
Member
Author
There was a problem hiding this comment.
Fair but, pre-existing, filed CORE-17350.
Member
|
Is this a downstream? |
Member
Author
2/4 are, each commit has "cherry picked" if it's a downstream. The other two are specific to us. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A few build/test fixes. Downstream one build fix and one test fix,
add one additional build fix to our fork, and enable --heap-profiling
in CI.
No prod code changes.
.
A heap profiling build of seastar did not compile. There were two independent
breaks, and CI saw neither of them, because
--heap-profilingis off bydefault and no job passed it. Redpanda builds seastar with heap profiling, so
the breaks reached developers and redpanda while CI stayed green.
The first break is in
alloc_test: the sampled profile tests comparedstd::vector<allocation_site>withBOOST_CHECK_EQUAL, which needs anoperator<<for the vector itself, and nothing provides one. The second is the<ciso646>warning reached through the abseil include inchunked_hash_map.hh: abseil LTS 20220623, which Ubuntu 24.04 ships, includes<ciso646>unconditionally fromabsl/base/options.h, libstdc++ 16 warns onthat header for C++20 and later, and seastar builds with
-Werror.Commits 1 and 3 are backports of commits already in scylladb/master, so they
carry cherry-pick trailers. Commit 2 has no upstream counterpart:
chunked_hash_map.hhis fork-only. Commit 4 closes the coverage gap that letboth breaks through in the first place.
Two notes on the fixes. The abseil suppression is scoped to the one include
rather than a build-wide
-Wno-error=#warnings, andchunked_hash_map.hhisthe only file in the tree that includes abseil. Dropping abseil in favour of
unordered_dense alone was considered and rejected: the default hash
deliberately dispatches to
absl::Hashfor keys that defineAbslHashValue,so removing it would change which hash those keys get. And commit 3 is a
prerequisite for commit 4, not an optional extra: the sampled profile tests sit
behind
#ifdef SEASTAR_HEAPPROF, so commit 4 is what makes them run at all,and at their previous sizing they failed on about 1.3% of runs.
--heap-profilinggoes into the sharedconfigure.pycall ininstall-build-env.shrather than into a matrix entry, so every job buildswith it. It has no effect where the default allocator is in use, such as the
sanitize jobs.
Testing: a full
ninjabuild of a--heap-profilingbuild/devgoes from twohard failures to clean, and
chunked_hash_map_testandalloc_testboth pass,with
alloc_test's log confirming the heap profiler engages. The abseilsuppression was checked against a standalone repro. The CI image was checked
too:
ubuntu:26.04ships abseil 20260107, which takes the<version>branchinstead, and including
absl/hash/hash.hthere underg++-16 -Wall -Werrorisclean at both
-std=gnu++23and-std=gnu++26, so CI never needed thesuppression.
Two follow-ups worth knowing. The suppression can go once the minimum abseil is
new enough to take the
<version>branch;cmake/SeastarDependencies.cmakecurrently asks for abseil with no version floor, so an alternative is to add
one and let configure fail with a clear message on older distros. And the
sampled profile tests have never run on GitHub runners: if the widened margin
still is not enough on noisier CPUs, that is where it will surface.