Commit 08f75150 authored by Patrick Noland's avatar Patrick Noland Committed by Commit Bot

Revert "base/allocator: Report the size of mapped PageAllocator space when crashing."

This reverts commit 402cee70.

Reason for revert: PageAllocatorTest.MappedPagesAccounting is failing on lollipop; see https://ci.chromium.org/p/chromium/builders/ci/Lollipop%20Phone%20Tester/25868 and https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=base_unittests&tests=PageAllocatorTest.MappedPagesAccounting

Original change's description:
> base/allocator: Report the size of mapped PageAllocator space when crashing.
> 
> PartitionAllocator can occupy a lot of address space, as freed pages for
> normal buckets are never unmapped, and these are not shared across
> buckets. This is a known issue, tracked in the linked bug.
> 
> However the reporting currently in place only looks at a single
> partition when reporting that an OOM is likely due to too many mapped
> pages. It does not look at all PageAllocator allocations. This commit
> adds a crash key reporting the total amount of mapped PageAllocator
> pages to crash reports, to allow distinguishing between "true" OOM and
> address space exhaustion.
> 
> For instance, on Windows 32, an address space exhaustion crash would
> happen with a reasonable committed set, remaining system commit limit,
> and a large amount of mapped pages. All these data points are available
> in crash reports, except for the last one, which this commit adds.
> 
> Bug: 421387
> Change-Id: If3eac64489b61297cb5da6da328cfc70fecc4c8c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2264438
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Commit-Queue: Benoit L <lizeb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#782459}

TBR=haraken@chromium.org,lizeb@chromium.org

Change-Id: Ib18a3623e09c3c278f355ffd4cc63aee47aeacce
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 421387
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2267425Reviewed-by: default avatarPatrick Noland <pnoland@chromium.org>
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782528}
parent e1425005
......@@ -42,8 +42,6 @@ Lock& GetReserveLock() {
return *lock;
}
std::atomic<size_t> g_total_mapped_address_space;
// We only support a single block of reserved address space.
void* s_reservation_address GUARDED_BY(GetReserveLock()) = nullptr;
size_t s_reservation_size GUARDED_BY(GetReserveLock()) = 0;
......@@ -99,12 +97,8 @@ void* SystemAllocPages(void* hint,
PA_DCHECK(!(reinterpret_cast<uintptr_t>(hint) &
kPageAllocationGranularityOffsetMask));
PA_DCHECK(commit || accessibility == PageInaccessible);
void* ptr =
SystemAllocPagesInternal(hint, length, accessibility, page_tag, commit);
if (ptr)
g_total_mapped_address_space.fetch_add(length, std::memory_order_relaxed);
return ptr;
return SystemAllocPagesInternal(hint, length, accessibility, page_tag,
commit);
}
void* AllocPages(void* address,
......@@ -194,8 +188,6 @@ void FreePages(void* address, size_t length) {
kPageAllocationGranularityOffsetMask));
PA_DCHECK(!(length & kPageAllocationGranularityOffsetMask));
FreePagesInternal(address, length);
PA_DCHECK(g_total_mapped_address_space.load(std::memory_order_relaxed) > 0);
g_total_mapped_address_space.fetch_sub(length, std::memory_order_relaxed);
}
bool TrySetSystemPagesAccess(void* address,
......@@ -269,8 +261,4 @@ uint32_t GetAllocPageErrorCode() {
return s_allocPageErrorCode;
}
size_t GetTotalMappedSize() {
return g_total_mapped_address_space;
}
} // namespace base
......@@ -193,11 +193,6 @@ BASE_EXPORT bool HasReservationForTesting();
// (POSIX) or |VirtualAlloc| (Windows) fails.
BASE_EXPORT uint32_t GetAllocPageErrorCode();
// Returns the total amount of mapped pages from all clients of
// PageAllocator. These pages may or may not be committed. This is mostly useful
// to assess address space pressure.
BASE_EXPORT size_t GetTotalMappedSize();
} // namespace base
#endif // BASE_ALLOCATOR_PARTITION_ALLOCATOR_PAGE_ALLOCATOR_H_
......@@ -270,31 +270,10 @@ TEST(PageAllocatorTest, DecommitErasesMemory) {
sum += recommitted_buffer[i];
}
EXPECT_EQ(0u, sum) << "Data was not erased";
FreePages(buffer, size);
}
#endif // defined(OS_MACOSX)
TEST(PageAllocatorTest, MappedPagesAccounting) {
size_t size = kPageAllocationGranularity;
size_t mapped_size_before = GetTotalMappedSize();
// Ask for a large alignment to make sure that trimming doesn't change the
// accounting.
void* data = AllocPages(nullptr, size, 128 * kPageAllocationGranularity,
PageInaccessible, PageTag::kChromium, true);
ASSERT_TRUE(data);
EXPECT_EQ(mapped_size_before + size, GetTotalMappedSize());
DecommitSystemPages(data, size);
EXPECT_EQ(mapped_size_before + size, GetTotalMappedSize());
FreePages(data, size);
EXPECT_EQ(mapped_size_before, GetTotalMappedSize());
}
} // namespace base
#endif // !defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
......@@ -207,7 +207,6 @@ jumbo_component("wtf") {
public_deps = [
"//base",
"//base/third_party/double_conversion",
"//components/crash/core/common:crash_key",
"//third_party/icu",
]
......
......@@ -34,8 +34,6 @@
#include "base/allocator/partition_allocator/oom.h"
#include "base/allocator/partition_allocator/page_allocator.h"
#include "base/debug/alias.h"
#include "base/strings/safe_sprintf.h"
#include "components/crash/core/common/crash_key.h"
#include "third_party/blink/renderer/platform/wtf/allocator/partition_allocator.h"
#include "third_party/blink/renderer/platform/wtf/wtf.h"
......@@ -247,21 +245,6 @@ void Partitions::HandleOutOfMemory(size_t size) {
uint32_t alloc_page_error_code = base::GetAllocPageErrorCode();
base::debug::Alias(&alloc_page_error_code);
// Report the total mapped size from PageAllocator. This is intended to
// distinguish better between address space exhaustion and out of memory on 32
// bit platforms. PartitionAlloc can use a lot of address space, as free pages
// are not shared between buckets (see crbug.com/421387). There is already
// reporting for this, however it only looks at the address space usage of a
// single partition. This allows to look across all the partitions, and other
// users such as V8.
char value[24];
// %d works for 64 bit types as well with SafeSPrintf(), see its unit tests
// for an example.
base::strings::SafeSPrintf(value, "%d", base::GetTotalMappedSize());
static crash_reporter::CrashKeyString<24> g_page_allocator_mapped_size(
"page-allocator-mapped-size");
g_page_allocator_mapped_size.Set(value);
if (total_usage >= 2UL * 1024 * 1024 * 1024)
PartitionsOutOfMemoryUsing2G(size);
if (total_usage >= 1UL * 1024 * 1024 * 1024)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment