From f09b426f5fd873fab53bab924fd12d504256ec44 Mon Sep 17 00:00:00 2001 From: Chris Kennelly CA Date: Sun, 20 Sep 2026 19:07:33 -0700 Subject: [PATCH] Suppress all-zero compressibility columns in heap profiles. MakeProfileProto advertises the space_compressed and zero_space sample types whenever compressibility collection is enabled, even when not a single sample was analyzed. Every sample then carries a literal 0 for both, which is indistinguishable downstream from a heap that was measured and found to hold no compressible or zero bytes. A consumer computing savings as (resident + swapped) - compressed scores such a profile as 100% compressible. Root Cause: the decision to emit the columns was derived from FLAGS_heapz_collect_compressibility alone, which says the analyzer is allowed to run, not that it ever produced a result. Analysis is skipped whenever residency reports nothing for the span, span_start_address is null, requested_size is 0, or CompressionAnalyzer::Analyze fails, and SampleMergedData::compressed_size and ::zero_size then keep their zero initializers. Have the merge report whether the analyzer ever succeeded and add the two sample types only when it did. A profile with no measurements now looks exactly like one from a binary that does not collect compressibility at all: the columns are absent rather than uniformly zero. PiperOrigin-RevId: 984957126 --- tcmalloc/internal/profile_builder.cc | 33 ++++++++++---- tcmalloc/internal/profile_builder_test.cc | 54 ++++++++++++++++++++++- 2 files changed, 77 insertions(+), 10 deletions(-) diff --git a/tcmalloc/internal/profile_builder.cc b/tcmalloc/internal/profile_builder.cc index a52f0fc9b..a07e61744 100644 --- a/tcmalloc/internal/profile_builder.cc +++ b/tcmalloc/internal/profile_builder.cc @@ -145,10 +145,19 @@ using SampleMergedMap = absl::flat_hash_map; -SampleMergedMap MergeProfileSamplesAndMaybeGetResidencyInfo( +struct SampleMergeResult { + SampleMergedMap samples; + // Whether CompressionAnalyzer produced a result for at least one sample. + // Collection being enabled does not imply this: analysis is skipped for any + // sample lacking residency information, a span address, or a requested size. + bool measured_compressibility = false; +}; + +SampleMergeResult MergeProfileSamplesAndMaybeGetResidencyInfo( const tcmalloc::Profile& profile, PageFlagsBase* pageflags, - Residency* residency, bool exporting_compressibility) { - SampleMergedMap map; + Residency* residency, bool collecting_compressibility) { + SampleMergeResult result; + SampleMergedMap& map = result.samples; CompressionAnalyzer compression_analyzer; profile.Iterate([&](const tcmalloc::Profile::Sample& entry) { @@ -206,18 +215,19 @@ SampleMergedMap MergeProfileSamplesAndMaybeGetResidencyInfo( } } - if (exporting_compressibility && residency_info.has_value() && + if (collecting_compressibility && residency_info.has_value() && entry.span_start_address != nullptr && entry.requested_size > 0) { absl::Span sample_mem( reinterpret_cast(entry.span_start_address), size); absl::StatusOr res = compression_analyzer.Analyze(sample_mem, *residency_info); if (res.ok()) { + result.measured_compressibility = true; data.zero_size += entry.count * res->zero_bytes; } } }); - return map; + return result; } } // namespace @@ -926,7 +936,16 @@ absl::StatusOr> MakeProfileProto( sample_type->set_unit(bytes_id); } - bool exporting_compressibility = false; + bool collecting_compressibility = false; + + // Only advertise space_compressed/zero_space when at least one sample was + // actually analyzed. Emitting the columns with a zero in every sample is + // indistinguishable downstream from a genuinely incompressible heap, so a + // profile that never ran the analyzer would otherwise be reported as 100% + // compressible rather than as having no data. + const auto [samples, exporting_compressibility] = + MergeProfileSamplesAndMaybeGetResidencyInfo(profile, pageflags, residency, + collecting_compressibility); if (exporting_compressibility) { perftools::profiles::ValueType* sample_type = nullptr; @@ -950,8 +969,6 @@ absl::StatusOr> MakeProfileProto( converted.set_default_sample_type(default_sample_type_id); - SampleMergedMap samples = MergeProfileSamplesAndMaybeGetResidencyInfo( - profile, pageflags, residency, exporting_compressibility); for (const auto& [entry, data] : samples) { perftools::profiles::Profile& profile = builder.profile(); perftools::profiles::Sample& sample = *profile.add_sample(); diff --git a/tcmalloc/internal/profile_builder_test.cc b/tcmalloc/internal/profile_builder_test.cc index d6bc64aad..99507ae2b 100644 --- a/tcmalloc/internal/profile_builder_test.cc +++ b/tcmalloc/internal/profile_builder_test.cc @@ -112,7 +112,7 @@ class StubPageFlags final : public PageFlagsBase { private: absl::flat_hash_map stale_bytes_; absl::flat_hash_map locked_bytes_; - uint64_t stale_scan_period_; + uint64_t stale_scan_period_ = 0; }; class StubResidency final : public Residency { @@ -1449,7 +1449,7 @@ TEST(ProfileConverterTest, CompressedSizeDoesNotExceedAnalyzedSize) { Profile profile = ProfileAccessor::MakeProfile(std::move(fake_profile)); auto converted_or = MakeProfileProto(profile, &pageflags, &residency); - ASSERT_TRUE(converted_or.ok()); + CHECK_OK(converted_or.status()); const auto& converted = **converted_or; // Find the space_compressed sample type index. @@ -1499,6 +1499,56 @@ TEST(ProfileConverterTest, CompressedSizeDoesNotExceedAnalyzedSize) { EXPECT_LE(sizes[1], kAllocatedSize); } +// A heap profile whose samples were never analyzed must not advertise the +// compressibility columns at all. Emitting a zero in every sample is +// indistinguishable downstream from a fully compressible heap. +TEST(ProfileConverterTest, NoCompressibilityColumnsWithoutMeasurements) { + std::vector buf(128); + + Profile::Sample sample = {}; + sample.sum = buf.size(); + sample.count = 1; + sample.requested_size = buf.size(); + sample.requested_alignment = std::nullopt; + sample.requested_size_returning = false; + sample.allocated_size = buf.size(); + sample.span_start_address = buf.data(); + sample.depth = 1; + sample.stack[0] = reinterpret_cast(&RealPath); + sample.access_hint = hot_cold_t{0}; + sample.access_allocated = Profile::Sample::Access::Hot; + sample.token_id = TokenId{0}; + sample.guarded_status = Profile::Sample::GuardedStatus::NotAttempted; + sample.type = AllocationType::Malloc; + + StubPageFlags pageflags; + // Residency has nothing for this address, so compressibility is never + // computed even though collection is enabled. + StubResidency residency; + + auto fake_profile = std::make_unique(); + fake_profile->SetType(ProfileType::kHeap); + fake_profile->SetDuration(absl::Milliseconds(100)); + fake_profile->SetSamples({sample}); + Profile profile = ProfileAccessor::MakeProfile(std::move(fake_profile)); + + auto converted_or = MakeProfileProto(profile, &pageflags, &residency); + CHECK_OK(converted_or.status()); + const auto& converted = **converted_or; + + std::vector types; + for (const auto& s : converted.sample_type()) { + types.push_back(converted.string_table(s.type())); + } + EXPECT_THAT(types, Not(Contains("space_compressed"))); + EXPECT_THAT(types, Not(Contains("zero_space"))); + + // Every sample must still agree with the advertised sample types. + for (const auto& s : converted.sample()) { + EXPECT_EQ(s.value_size(), types.size()); + } +} + } // namespace } // namespace tcmalloc_internal } // namespace tcmalloc