Skip to content

GitLab

T
tangled
Commit 0ceffaa2 authored by Thomas Anderson's avatar Thomas Anderson Committed by Commit Bot
Browse files
Options

Revert "Proactively purging discardable memory on Desktop" 

This reverts commit 3558d80a.

Reason for revert: Suspected cause of build failure on android-official:

https://ci.chromium.org/p/chromium/builders/ci/android-official/1056

[13692/25621] SOLINK ./libaudio_unittests__library.so
FAILED: libaudio_unittests__library.so libaudio_unittests__library.so.TOC lib.unstripped/libaudio_unittests__library.so lib.unstripped/libaudio_unittests__library.so.map.gz
/b/s/w/ir/cipd_bin_packages/cpython/bin/python "../../build/toolchain/gcc_solink_wrapper.py" --reade...(too long)
ld.lld: error: undefined hidden symbol: base::(anonymous namespace)::g_discardable_allocator (.llvm.16724700302094337840)
>>> referenced by run_all_unittests.cc:0 (../../media/test/run_all_unittests.cc:0)
>>>               thinlto-cache/llvmcache-3F27182A7CE8A475288094E8A454CB68C4740658:(TestSuiteNoAtExit::Initialize())

Original change's description:
> Proactively purging discardable memory on Desktop
> 
> This CL adds a way for ClientDiscardableSharedMemoryManager to discard
> unlocked discardable memory locally. To achieve this, it keeps track
> of all locked and unlocked, unpurged instances. These are then purged
> all at once when ReleaseFreeMemory() is called.
> 
> This is guarded by a new feature which is disabled by default, and as
> such there is no behaviour change by default.
> 
> Bug: 1109209
> Change-Id: I7755d714ff354e831557627fda401751384392b8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2321400
> Commit-Queue: Thiabaud Engelbrecht <thiabaud@google.com>
> Reviewed-by: Peng Huang <penghuang@chromium.org>
> Reviewed-by: Benoit L <lizeb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#798630}

TBR=penghuang@chromium.org,lizeb@chromium.org,thiabaud@google.com

Change-Id: I39ddd002be3753d1339501c74df866d898ad53a4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1109209
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2360121Reviewed-by: default avatarThomas Anderson <thomasanderson@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798796}
parent 9f9ebab3
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 75 additions and 492 deletions
components/BUILD.gn
View file @ 0ceffaa2
......@@ -236,7 +236,6 @@ test("components_unittests") {
"//components/content_settings/browser:unit_tests",
"//components/contextual_search/core:unit_tests",
"//components/data_use_measurement/core:unit_tests",
"//components/discardable_memory/client:unit_tests",
"//components/discardable_memory/common:unit_tests",
"//components/discardable_memory/service:unit_tests",
"//components/dom_distiller/content/browser:unit_tests",
......
components/discardable_memory/client/BUILD.gn
View file @ 0ceffaa2
......@@ -21,15 +21,3 @@ component("client") {
"//components/discardable_memory/public/mojom",
]
}
source_set("unit_tests") {
testonly = true
sources = [ "client_discardable_shared_memory_manager_unittest.cc" ]
deps = [
":client",
"//base",
"//testing/gtest",
]
}
components/discardable_memory/client/client_discardable_shared_memory_manager.h
View file @ 0ceffaa2
......@@ -8,8 +8,9 @@
#include <stddef.h>
#include <memory>
#include <set>
#include "base/callback_helpers.h"
#include "base/macros.h"
#include "base/memory/discardable_memory_allocator.h"
#include "base/memory/ref_counted.h"
#include "base/memory/unsafe_shared_memory_region.h"
......@@ -46,16 +47,12 @@ class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
base::trace_event::ProcessMemoryDump* pmd) override;
// Purge any unlocked memory that was allocated by this manager.
void PurgeUnlockedMemory();
// Release memory and associated resources that have been purged.
void ReleaseFreeMemory() override;
bool LockSpan(DiscardableSharedMemoryHeap::Span* span)
EXCLUSIVE_LOCKS_REQUIRED(GetLock());
void UnlockSpan(DiscardableSharedMemoryHeap::Span* span)
EXCLUSIVE_LOCKS_REQUIRED(GetLock());
bool LockSpan(DiscardableSharedMemoryHeap::Span* span);
void UnlockSpan(DiscardableSharedMemoryHeap::Span* span);
void ReleaseSpan(std::unique_ptr<DiscardableSharedMemoryHeap::Span> span);
base::trace_event::MemoryAllocatorDump* CreateMemoryAllocatorDump(
DiscardableSharedMemoryHeap::Span* span,
......@@ -73,47 +70,8 @@ class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
bytes_allocated_limit_for_testing_ = limit;
}
// We only have protected members for testing, everything else should be
// either public or private.
protected:
explicit ClientDiscardableSharedMemoryManager(
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
std::unique_ptr<DiscardableSharedMemoryHeap> heap_ GUARDED_BY(lock_);
mutable base::Lock lock_;
private:
class DiscardableMemoryImpl : public base::DiscardableMemory {
public:
DiscardableMemoryImpl(
ClientDiscardableSharedMemoryManager* manager,
std::unique_ptr<DiscardableSharedMemoryHeap::Span> span);
~DiscardableMemoryImpl() override;
DiscardableMemoryImpl(const DiscardableMemoryImpl&) = delete;
DiscardableMemoryImpl& operator=(const DiscardableMemoryImpl&) = delete;
// Overridden from base::DiscardableMemory:
bool Lock() override;
void Unlock() override;
void* data() const override;
void DiscardForTesting() override;
base::trace_event::MemoryAllocatorDump* CreateMemoryAllocatorDump(
const char* name,
base::trace_event::ProcessMemoryDump* pmd) const override;
// Returns |span_| if unlocked, otherwise nullptr.
std::unique_ptr<DiscardableSharedMemoryHeap::Span> Purge()
EXCLUSIVE_LOCKS_REQUIRED(manager_->GetLock());
private:
friend class ClientDiscardableSharedMemoryManager;
ClientDiscardableSharedMemoryManager* const manager_;
std::unique_ptr<DiscardableSharedMemoryHeap::Span> span_;
bool is_locked_ GUARDED_BY(manager_->GetLock());
};
// This is only virtual for testing.
virtual std::unique_ptr<base::DiscardableSharedMemory>
std::unique_ptr<base::DiscardableSharedMemory>
AllocateLockedDiscardableSharedMemory(size_t size, int32_t id);
void AllocateOnIO(size_t size,
int32_t id,
......@@ -123,27 +81,18 @@ class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
base::ScopedClosureRunner closure_runner,
base::UnsafeSharedMemoryRegion ret_region);
// This is only virtual for testing.
virtual void DeletedDiscardableSharedMemory(int32_t id);
void DeletedDiscardableSharedMemory(int32_t id);
void MemoryUsageChanged(size_t new_bytes_allocated,
size_t new_bytes_free) const;
void ReleaseFreeMemoryImpl();
void ReleaseMemory(DiscardableMemoryImpl* memory,
std::unique_ptr<DiscardableSharedMemoryHeap::Span> span)
EXCLUSIVE_LOCKS_REQUIRED(lock_);
void ReleaseSpan(std::unique_ptr<DiscardableSharedMemoryHeap::Span> span)
EXCLUSIVE_LOCKS_REQUIRED(lock_);
base::Lock& GetLock() { return lock_; }
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
// TODO(penghuang): Switch to SharedRemote when it starts supporting
// sync method call.
std::unique_ptr<mojo::Remote<mojom::DiscardableSharedMemoryManager>>
manager_mojo_;
// Holds all locked and unlocked instances which have not yet been purged.
std::set<DiscardableMemoryImpl*> allocated_memory_ GUARDED_BY(lock_);
mutable base::Lock lock_;
std::unique_ptr<DiscardableSharedMemoryHeap> heap_ GUARDED_BY(lock_);
size_t bytes_allocated_limit_for_testing_ = 0;
DISALLOW_COPY_AND_ASSIGN(ClientDiscardableSharedMemoryManager);
......
components/discardable_memory/client/client_discardable_shared_memory_manager_unittest.cc deleted 100644 → 0
View file @ 9f9ebab3
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "components/discardable_memory/client/client_discardable_shared_memory_manager.h"
#include "base/memory/discardable_memory.h"
#include "base/memory/discardable_shared_memory.h"
#include "base/process/process_metrics.h"
#include "base/synchronization/lock.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace discardable_memory {
namespace {
using base::Location;
using base::OnceClosure;
using base::TimeDelta;
class TestSingleThreadTaskRunner : public base::SingleThreadTaskRunner {
~TestSingleThreadTaskRunner() override = default;
bool PostTask(const Location& from_here, OnceClosure task) { return true; }
template <class T>
bool DeleteSoon(const Location& from_here, const T* object) {
return true;
}
bool PostDelayedTask(const Location& from_here,
OnceClosure task,
TimeDelta delay) override {
return true;
}
bool PostNonNestableDelayedTask(const Location& from_here,
OnceClosure task,
TimeDelta delay) override {
return true;
}
bool RunsTasksInCurrentSequence() const override { return true; }
};
class TestClientDiscardableSharedMemoryManager
: public ClientDiscardableSharedMemoryManager {
public:
TestClientDiscardableSharedMemoryManager()
: ClientDiscardableSharedMemoryManager(
base::MakeRefCounted<TestSingleThreadTaskRunner>()) {}
~TestClientDiscardableSharedMemoryManager() override = default;
std::unique_ptr<base::DiscardableSharedMemory>
AllocateLockedDiscardableSharedMemory(size_t size, int32_t id) override {
auto shared_memory = std::make_unique<base::DiscardableSharedMemory>();
shared_memory->CreateAndMap(size);
return shared_memory;
}
void DeletedDiscardableSharedMemory(int32_t id) override {}
size_t GetSize() const {
base::AutoLock lock(lock_);
return heap_->GetSize();
}
size_t GetSizeOfFreeLists() const {
base::AutoLock lock(lock_);
return heap_->GetSizeOfFreeLists();
}
};
// This test allocates a single piece of memory, then verifies that calling
// |PurgeUnlockedMemory| only affects the memory when it is unlocked.
TEST(ClientDiscardableSharedMemoryManagerTest, Simple) {
const size_t page_size = base::GetPageSize();
TestClientDiscardableSharedMemoryManager client;
// Initially, we should have no memory allocated
ASSERT_EQ(client.GetBytesAllocated(), 0u);
ASSERT_EQ(client.GetSizeOfFreeLists(), 0u);
auto mem = client.AllocateLockedDiscardableMemory(page_size);
// After allocation, we should have allocated a single piece of memory.
EXPECT_EQ(client.GetBytesAllocated(), page_size);
client.PurgeUnlockedMemory();
// All our memory is locked, so calling |PurgeUnlockedMemory| should have no
// effect.
EXPECT_EQ(client.GetBytesAllocated(), base::GetPageSize());
mem->Unlock();
// Unlocking has no effect on the amount of memory we have allocated.
EXPECT_EQ(client.GetBytesAllocated(), base::GetPageSize());
client.PurgeUnlockedMemory();
// Now that |mem| is unlocked, the call to |PurgeUnlockedMemory| will
// remove it.
EXPECT_EQ(client.GetBytesAllocated(), 0u);
}
// This test allocates multiple pieces of memory, then unlocks them one by one,
// verifying that |PurgeUnlockedMemory| only affects the unlocked pieces of
// memory.
TEST(ClientDiscardableSharedMemoryManagerTest, MultipleOneByOne) {
const size_t page_size = base::GetPageSize();
TestClientDiscardableSharedMemoryManager client;
ASSERT_EQ(client.GetBytesAllocated(), 0u);
ASSERT_EQ(client.GetSizeOfFreeLists(), 0u);
auto mem1 = client.AllocateLockedDiscardableMemory(page_size * 2.2);
auto mem2 = client.AllocateLockedDiscardableMemory(page_size * 1.1);
auto mem3 = client.AllocateLockedDiscardableMemory(page_size * 3.5);
auto mem4 = client.AllocateLockedDiscardableMemory(page_size * 0.2);
EXPECT_EQ(client.GetBytesAllocated(), 10 * page_size);
// Does nothing because everything is locked.
client.PurgeUnlockedMemory();
EXPECT_EQ(client.GetBytesAllocated(), 10 * page_size);
mem1->Unlock();
// Does nothing, since we don't have any free memory, just unlocked memory.
client.ReleaseFreeMemory();
EXPECT_EQ(client.GetBytesAllocated(), 10 * page_size);
// This gets rid of |mem1| (which is unlocked), but not the rest of the
// memory.
client.PurgeUnlockedMemory();
EXPECT_EQ(client.GetBytesAllocated(), 7 * page_size);
// We do similar checks to above for the rest of the memory.
mem2->Unlock();
client.PurgeUnlockedMemory();
EXPECT_EQ(client.GetBytesAllocated(), 5 * page_size);
mem3->Unlock();
client.PurgeUnlockedMemory();
EXPECT_EQ(client.GetBytesAllocated(), 1 * page_size);
mem4->Unlock();
client.PurgeUnlockedMemory();
EXPECT_EQ(client.GetBytesAllocated(), 0 * page_size);
}
// This test allocates multiple pieces of memory, then unlocks them all,
// verifying that |PurgeUnlockedMemory| only affects the unlocked pieces of
// memory.
TEST(ClientDiscardableSharedMemoryManagerTest, MultipleAtOnce) {
const size_t page_size = base::GetPageSize();
TestClientDiscardableSharedMemoryManager client;
ASSERT_EQ(client.GetBytesAllocated(), 0u);
ASSERT_EQ(client.GetSizeOfFreeLists(), 0u);
auto mem1 = client.AllocateLockedDiscardableMemory(page_size * 2.2);
auto mem2 = client.AllocateLockedDiscardableMemory(page_size * 1.1);
auto mem3 = client.AllocateLockedDiscardableMemory(page_size * 3.5);
auto mem4 = client.AllocateLockedDiscardableMemory(page_size * 0.2);
EXPECT_EQ(client.GetBytesAllocated(), 10 * page_size);
// Does nothing because everything is locked.
client.PurgeUnlockedMemory();
EXPECT_EQ(client.GetBytesAllocated(), 10 * page_size);
// Unlock all pieces of memory at once.
mem1->Unlock();
mem2->Unlock();
mem3->Unlock();
mem4->Unlock();
client.PurgeUnlockedMemory();
EXPECT_EQ(client.GetBytesAllocated(), 0 * page_size);
}
// Tests that FreeLists are only released once all memory has been released.
TEST(ClientDiscardableSharedMemoryManagerTest, Release) {
const size_t page_size = base::GetPageSize();
TestClientDiscardableSharedMemoryManager client;
ASSERT_EQ(client.GetBytesAllocated(), 0u);
ASSERT_EQ(client.GetSizeOfFreeLists(), 0u);
auto mem1 = client.AllocateLockedDiscardableMemory(page_size * 3);
auto mem2 = client.AllocateLockedDiscardableMemory(page_size * 2);
size_t freelist_size = client.GetSizeOfFreeLists();
EXPECT_EQ(client.GetBytesAllocated(), 5 * page_size);
mem1 = nullptr;
// Less memory is now allocated, but freelists are grown.
EXPECT_EQ(client.GetBytesAllocated(), page_size * 2);
EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 3);
client.PurgeUnlockedMemory();
// Purging doesn't remove any memory since none is unlocked, also doesn't
// remove freelists since we still have some.
EXPECT_EQ(client.GetBytesAllocated(), page_size * 2);
EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 3);
mem2 = nullptr;
// No memory is allocated, but freelists are grown.
EXPECT_EQ(client.GetBytesAllocated(), 0u);
EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 5);
client.PurgeUnlockedMemory();
// Purging now shrinks freelists as well.
EXPECT_EQ(client.GetBytesAllocated(), 0u);
EXPECT_EQ(client.GetSizeOfFreeLists(), 0u);
}
// Similar to previous test, but makes sure that freelist still shrinks when
// last piece of memory was just unlocked instead of released.
TEST(ClientDiscardableSharedMemoryManagerTest, ReleaseUnlocked) {
const size_t page_size = base::GetPageSize();
TestClientDiscardableSharedMemoryManager client;
ASSERT_EQ(client.GetBytesAllocated(), 0u);
ASSERT_EQ(client.GetSizeOfFreeLists(), 0u);
auto mem1 = client.AllocateLockedDiscardableMemory(page_size * 3);
auto mem2 = client.AllocateLockedDiscardableMemory(page_size * 2);
size_t freelist_size = client.GetSizeOfFreeLists();
EXPECT_EQ(client.GetBytesAllocated(), 5 * page_size);
mem1 = nullptr;
// Less memory is now allocated, but freelists are grown.
EXPECT_EQ(client.GetBytesAllocated(), page_size * 2);
EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 3);
client.PurgeUnlockedMemory();
// Purging doesn't remove any memory since none is unlocked, also doesn't
// remove freelists since we still have some.
EXPECT_EQ(client.GetBytesAllocated(), page_size * 2);
EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 3);
mem2->Unlock();
// No change in memory usage, since memory was only unlocked not released.
EXPECT_EQ(client.GetBytesAllocated(), page_size * 2);
EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 3);
client.PurgeUnlockedMemory();
// Purging now shrinks freelists as well.
EXPECT_EQ(client.GetBytesAllocated(), 0u);
EXPECT_EQ(client.GetSizeOfFreeLists(), 0u);
}
} // namespace
} // namespace discardable_memory
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