From ba8dcaef0d79ae0174cdcea6d6f62015266c1d40 Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Tue, 13 Jul 2021 12:39:16 -0700 Subject: [PATCH] Revert "sanitizer_common: optimize memory drain" Breaks https://lab.llvm.org/buildbot/#/builders/anitizer-windows This reverts commit d89d3dfae17d7795dc1ef013db66272020de1959. --- .../sanitizer_allocator_local_cache.h | 19 +-- .../sanitizer_allocator_primary64.h | 165 ++++++++++----------- .../tests/sanitizer_allocator_test.cpp | 9 +- 3 files changed, 87 insertions(+), 106 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_local_cache.h b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_local_cache.h index 4344208..108dfc2 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_local_cache.h +++ b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_local_cache.h @@ -17,7 +17,6 @@ template struct SizeClassAllocator64LocalCache { typedef SizeClassAllocator Allocator; - typedef MemoryMapper MemoryMapperT; void Init(AllocatorGlobalStats *s) { stats_.Init(); @@ -54,7 +53,7 @@ struct SizeClassAllocator64LocalCache { PerClass *c = &per_class_[class_id]; InitCache(c); if (UNLIKELY(c->count == c->max_count)) - Drain(c, allocator, class_id); + Drain(c, allocator, class_id, c->max_count / 2); CompactPtrT chunk = allocator->PointerToCompactPtr( allocator->GetRegionBeginBySizeClass(class_id), reinterpret_cast(p)); @@ -63,10 +62,10 @@ struct SizeClassAllocator64LocalCache { } void Drain(SizeClassAllocator *allocator) { - MemoryMapperT memory_mapper(*allocator); for (uptr i = 1; i < kNumClasses; i++) { PerClass *c = &per_class_[i]; - while (c->count > 0) Drain(&memory_mapper, c, allocator, i, c->count); + while (c->count > 0) + Drain(c, allocator, i, c->count); } } @@ -107,18 +106,12 @@ struct SizeClassAllocator64LocalCache { return true; } - NOINLINE void Drain(PerClass *c, SizeClassAllocator *allocator, - uptr class_id) { - MemoryMapperT memory_mapper(*allocator); - Drain(&memory_mapper, c, allocator, class_id, c->max_count / 2); - } - - void Drain(MemoryMapperT *memory_mapper, PerClass *c, - SizeClassAllocator *allocator, uptr class_id, uptr count) { + NOINLINE void Drain(PerClass *c, SizeClassAllocator *allocator, uptr class_id, + uptr count) { CHECK_GE(c->count, count); const uptr first_idx_to_drain = c->count - count; c->count -= count; - allocator->ReturnToAllocator(memory_mapper, &stats_, class_id, + allocator->ReturnToAllocator(&stats_, class_id, &c->chunks[first_idx_to_drain], count); } }; diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h index f05c754..8bbed3c 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h +++ b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h @@ -42,60 +42,6 @@ struct SizeClassAllocator64FlagMasks { // Bit masks. }; }; -template -class MemoryMapper { - public: - typedef typename Allocator::CompactPtrT CompactPtrT; - - explicit MemoryMapper(const Allocator &allocator) : allocator_(allocator) {} - - ~MemoryMapper() { - if (buffer_) - UnmapOrDie(buffer_, buffer_size_); - } - - bool GetAndResetStats(uptr &ranges, uptr &bytes) { - ranges = released_ranges_count_; - released_ranges_count_ = 0; - bytes = released_bytes_; - released_bytes_ = 0; - return ranges != 0; - } - - void *MapPackedCounterArrayBuffer(uptr buffer_size) { - // TODO(alekseyshl): The idea to explore is to check if we have enough - // space between num_freed_chunks*sizeof(CompactPtrT) and - // mapped_free_array to fit buffer_size bytes and use that space instead - // of mapping a temporary one. - if (buffer_size_ < buffer_size) { - if (buffer_) - UnmapOrDie(buffer_, buffer_size_); - buffer_ = MmapOrDieOnFatalError(buffer_size, "ReleaseToOSPageCounters"); - buffer_size_ = buffer_size; - } else { - internal_memset(buffer_, 0, buffer_size); - } - return buffer_; - } - - // Releases [from, to) range of pages back to OS. - void ReleasePageRangeToOS(CompactPtrT from, CompactPtrT to, uptr class_id) { - const uptr region_base = allocator_.GetRegionBeginBySizeClass(class_id); - const uptr from_page = allocator_.CompactPtrToPointer(region_base, from); - const uptr to_page = allocator_.CompactPtrToPointer(region_base, to); - ReleaseMemoryPagesToOS(from_page, to_page); - released_ranges_count_++; - released_bytes_ += to_page - from_page; - } - - private: - const Allocator &allocator_; - uptr released_ranges_count_ = 0; - uptr released_bytes_ = 0; - void *buffer_ = nullptr; - uptr buffer_size_ = 0; -}; - template class SizeClassAllocator64 { public: @@ -111,7 +57,6 @@ class SizeClassAllocator64 { typedef SizeClassAllocator64 ThisT; typedef SizeClassAllocator64LocalCache AllocatorCache; - typedef MemoryMapper MemoryMapperT; // When we know the size class (the region base) we can represent a pointer // as a 4-byte integer (offset from the region start shifted right by 4). @@ -175,10 +120,9 @@ class SizeClassAllocator64 { } void ForceReleaseToOS() { - MemoryMapperT memory_mapper(*this); for (uptr class_id = 1; class_id < kNumClasses; class_id++) { BlockingMutexLock l(&GetRegionInfo(class_id)->mutex); - MaybeReleaseToOS(&memory_mapper, class_id, true /*force*/); + MaybeReleaseToOS(class_id, true /*force*/); } } @@ -187,8 +131,7 @@ class SizeClassAllocator64 { alignment <= SizeClassMap::kMaxSize; } - NOINLINE void ReturnToAllocator(MemoryMapperT *memory_mapper, - AllocatorStats *stat, uptr class_id, + NOINLINE void ReturnToAllocator(AllocatorStats *stat, uptr class_id, const CompactPtrT *chunks, uptr n_chunks) { RegionInfo *region = GetRegionInfo(class_id); uptr region_beg = GetRegionBeginBySizeClass(class_id); @@ -211,7 +154,7 @@ class SizeClassAllocator64 { region->num_freed_chunks = new_num_freed_chunks; region->stats.n_freed += n_chunks; - MaybeReleaseToOS(memory_mapper, class_id, false /*force*/); + MaybeReleaseToOS(class_id, false /*force*/); } NOINLINE bool GetFromAllocator(AllocatorStats *stat, uptr class_id, @@ -419,10 +362,10 @@ class SizeClassAllocator64 { // For the performance sake, none of the accessors check the validity of the // arguments, it is assumed that index is always in [0, n) range and the value // is not incremented past max_value. - template + template class PackedCounterArray { public: - PackedCounterArray(u64 num_counters, u64 max_value, MemoryMapper *mapper) + PackedCounterArray(u64 num_counters, u64 max_value, MemoryMapperT *mapper) : n(num_counters), memory_mapper(mapper) { CHECK_GT(num_counters, 0); CHECK_GT(max_value, 0); @@ -446,6 +389,11 @@ class SizeClassAllocator64 { buffer = reinterpret_cast( memory_mapper->MapPackedCounterArrayBuffer(buffer_size)); } + ~PackedCounterArray() { + if (buffer) { + memory_mapper->UnmapPackedCounterArrayBuffer(buffer, buffer_size); + } + } bool IsAllocated() const { return !!buffer; @@ -482,21 +430,18 @@ class SizeClassAllocator64 { u64 packing_ratio_log; u64 bit_offset_mask; - MemoryMapper *const memory_mapper; + MemoryMapperT* const memory_mapper; u64 buffer_size; u64* buffer; }; - template + template class FreePagesRangeTracker { public: - explicit FreePagesRangeTracker(MemoryMapperT *mapper, uptr class_id) + explicit FreePagesRangeTracker(MemoryMapperT* mapper) : memory_mapper(mapper), - class_id(class_id), page_size_scaled_log(Log2(GetPageSizeCached() >> kCompactPtrScale)), - in_the_range(false), - current_page(0), - current_range_start_page(0) {} + in_the_range(false), current_page(0), current_range_start_page(0) {} void NextPage(bool freed) { if (freed) { @@ -518,14 +463,13 @@ class SizeClassAllocator64 { void CloseOpenedRange() { if (in_the_range) { memory_mapper->ReleasePageRangeToOS( - class_id, current_range_start_page << page_size_scaled_log, + current_range_start_page << page_size_scaled_log, current_page << page_size_scaled_log); in_the_range = false; } } - MemoryMapperT *const memory_mapper; - const uptr class_id; + MemoryMapperT* const memory_mapper; const uptr page_size_scaled_log; bool in_the_range; uptr current_page; @@ -536,12 +480,11 @@ class SizeClassAllocator64 { // chunks only and returns these pages back to OS. // allocated_pages_count is the total number of pages allocated for the // current bucket. - template + template static void ReleaseFreeMemoryToOS(CompactPtrT *free_array, uptr free_array_count, uptr chunk_size, uptr allocated_pages_count, - MemoryMapper *memory_mapper, - uptr class_id) { + MemoryMapperT *memory_mapper) { const uptr page_size = GetPageSizeCached(); // Figure out the number of chunks per page and whether we can take a fast @@ -577,8 +520,9 @@ class SizeClassAllocator64 { UNREACHABLE("All chunk_size/page_size ratios must be handled."); } - PackedCounterArray counters( - allocated_pages_count, full_pages_chunk_count_max, memory_mapper); + PackedCounterArray counters(allocated_pages_count, + full_pages_chunk_count_max, + memory_mapper); if (!counters.IsAllocated()) return; @@ -603,7 +547,7 @@ class SizeClassAllocator64 { // Iterate over pages detecting ranges of pages with chunk counters equal // to the expected number of chunks for the particular page. - FreePagesRangeTracker range_tracker(memory_mapper, class_id); + FreePagesRangeTracker range_tracker(memory_mapper); if (same_chunk_count_per_page) { // Fast path, every page has the same number of chunks affecting it. for (uptr i = 0; i < counters.GetCount(); i++) @@ -642,7 +586,7 @@ class SizeClassAllocator64 { } private: - friend class MemoryMapper; + friend class MemoryMapper; ReservedAddressRange address_range; @@ -876,13 +820,57 @@ class SizeClassAllocator64 { return true; } + class MemoryMapper { + public: + MemoryMapper(const ThisT& base_allocator, uptr class_id) + : allocator(base_allocator), + region_base(base_allocator.GetRegionBeginBySizeClass(class_id)), + released_ranges_count(0), + released_bytes(0) { + } + + uptr GetReleasedRangesCount() const { + return released_ranges_count; + } + + uptr GetReleasedBytes() const { + return released_bytes; + } + + void *MapPackedCounterArrayBuffer(uptr buffer_size) { + // TODO(alekseyshl): The idea to explore is to check if we have enough + // space between num_freed_chunks*sizeof(CompactPtrT) and + // mapped_free_array to fit buffer_size bytes and use that space instead + // of mapping a temporary one. + return MmapOrDieOnFatalError(buffer_size, "ReleaseToOSPageCounters"); + } + + void UnmapPackedCounterArrayBuffer(void *buffer, uptr buffer_size) { + UnmapOrDie(buffer, buffer_size); + } + + // Releases [from, to) range of pages back to OS. + void ReleasePageRangeToOS(CompactPtrT from, CompactPtrT to) { + const uptr from_page = allocator.CompactPtrToPointer(region_base, from); + const uptr to_page = allocator.CompactPtrToPointer(region_base, to); + ReleaseMemoryPagesToOS(from_page, to_page); + released_ranges_count++; + released_bytes += to_page - from_page; + } + + private: + const ThisT& allocator; + const uptr region_base; + uptr released_ranges_count; + uptr released_bytes; + }; + // Attempts to release RAM occupied by freed chunks back to OS. The region is // expected to be locked. // // TODO(morehouse): Support a callback on memory release so HWASan can release // aliases as well. - void MaybeReleaseToOS(MemoryMapperT *memory_mapper, uptr class_id, - bool force) { + void MaybeReleaseToOS(uptr class_id, bool force) { RegionInfo *region = GetRegionInfo(class_id); const uptr chunk_size = ClassIdToSize(class_id); const uptr page_size = GetPageSizeCached(); @@ -906,16 +894,17 @@ class SizeClassAllocator64 { } } - ReleaseFreeMemoryToOS( + MemoryMapper memory_mapper(*this, class_id); + + ReleaseFreeMemoryToOS( GetFreeArray(GetRegionBeginBySizeClass(class_id)), n, chunk_size, - RoundUpTo(region->allocated_user, page_size) / page_size, memory_mapper, - class_id); + RoundUpTo(region->allocated_user, page_size) / page_size, + &memory_mapper); - uptr ranges, bytes; - if (memory_mapper->GetAndResetStats(ranges, bytes)) { + if (memory_mapper.GetReleasedRangesCount() > 0) { region->rtoi.n_freed_at_last_release = region->stats.n_freed; - region->rtoi.num_releases += ranges; - region->rtoi.last_released_bytes = bytes; + region->rtoi.num_releases += memory_mapper.GetReleasedRangesCount(); + region->rtoi.last_released_bytes = memory_mapper.GetReleasedBytes(); } region->rtoi.last_release_at_ns = MonotonicNanoTime(); } diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp index a5076da..58f1ef4 100644 --- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp +++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp @@ -1243,7 +1243,7 @@ class RangeRecorder { Log2(GetPageSizeCached() >> Allocator64::kCompactPtrScale)), last_page_reported(0) {} - void ReleasePageRangeToOS(u32 class_id, u32 from, u32 to) { + void ReleasePageRangeToOS(u32 from, u32 to) { from >>= page_size_scaled_log; to >>= page_size_scaled_log; ASSERT_LT(from, to); @@ -1253,7 +1253,6 @@ class RangeRecorder { reported_pages.append(to - from, 'x'); last_page_reported = to; } - private: const uptr page_size_scaled_log; u32 last_page_reported; @@ -1283,7 +1282,7 @@ TEST(SanitizerCommon, SizeClassAllocator64FreePagesRangeTracker) { for (auto test_case : test_cases) { RangeRecorder range_recorder; - RangeTracker tracker(&range_recorder, 1); + RangeTracker tracker(&range_recorder); for (int i = 0; test_case[i] != 0; i++) tracker.NextPage(test_case[i] == 'x'); tracker.Done(); @@ -1309,7 +1308,7 @@ class ReleasedPagesTrackingMemoryMapper { free(buffer); } - void ReleasePageRangeToOS(u32 class_id, u32 from, u32 to) { + void ReleasePageRangeToOS(u32 from, u32 to) { uptr page_size_scaled = GetPageSizeCached() >> Allocator64::kCompactPtrScale; for (u32 i = from; i < to; i += page_size_scaled) @@ -1353,7 +1352,7 @@ void TestReleaseFreeMemoryToOS() { Allocator::ReleaseFreeMemoryToOS(&free_array[0], free_array.size(), chunk_size, kAllocatedPagesCount, - &memory_mapper, class_id); + &memory_mapper); // Verify that there are no released pages touched by used chunks and all // ranges of free chunks big enough to contain the entire memory pages had -- 2.7.4