Reclaims inactive slots in the metadata recorder as it becomes full
This is a reland of https://crrev.com/c/1666806, which caused flakiness
on the Mac bots and was reverted. Since submitting that CL, I've changed
the pointer management in ScopedGetItems so that it no longer relies on
the heap. Calling unique_ptr::Reset() was causing deadlock when the
target thread was suspended while holding the heap lock.
This lays the groundwork for a follow-up patch that keys items in the
metadata recorder by a <name hash, integer ID> tuple instead of just the
name hash. This will allow us to distinguish between metadata that might
coexist with the same name within the same process. For example, if we
had a piece of metadata with a lifetime between navigation and FCP,
we're currently unable to distinguish between two tabs running in the
same process that are both loading.
This new key structure will fill up the metadata map much faster
because we don't recycle keys once an item slot is allocated for them.
This patch gives us some ability to recycle those keys.
TBR=gab@chromium.org
R=wittman@chromium.org
Bug: 976864, 982034
Change-Id: Ie4f0b941c8c9281aae7ab95e54d546ee4c13c8a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1696347
Reviewed-by: Charlie Andrews <charliea@chromium.org>
Reviewed-by: Mike Wittman <wittman@chromium.org>
Commit-Queue: Charlie Andrews <charliea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676269}
diff --git a/base/BUILD.gn b/base/BUILD.gn
index 044c081..063a042 100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -622,6 +622,7 @@
"profiler/native_unwinder_mac.h",
"profiler/native_unwinder_win.cc",
"profiler/native_unwinder_win.h",
+ "profiler/profile_builder.cc",
"profiler/profile_builder.h",
"profiler/register_context.h",
"profiler/sample_metadata.cc",
diff --git a/base/profiler/metadata_recorder.cc b/base/profiler/metadata_recorder.cc
index baca29a..b0f5ea0 100644
--- a/base/profiler/metadata_recorder.cc
+++ b/base/profiler/metadata_recorder.cc
@@ -21,20 +21,33 @@
void MetadataRecorder::Set(uint64_t name_hash, int64_t value) {
base::AutoLock lock(write_lock_);
- // Acquiring the |write_lock_| guarantees that two simultaneous writes don't
- // attempt to create items in the same slot. Use of memory_order_release
- // guarantees that all writes performed by other threads to the metadata items
- // will be seen by the time we reach this point.
+ // Acquiring the |write_lock_| ensures that:
+ //
+ // - We don't try to write into the same new slot at the same time as
+ // another thread
+ // - We see all writes by other threads (acquiring a mutex implies acquire
+ // semantics)
size_t item_slots_used = item_slots_used_.load(std::memory_order_relaxed);
for (size_t i = 0; i < item_slots_used; ++i) {
auto& item = items_[i];
if (item.name_hash == name_hash) {
item.value.store(value, std::memory_order_relaxed);
- item.is_active.store(true, std::memory_order_release);
+
+ const bool was_active =
+ item.is_active.exchange(true, std::memory_order_release);
+ if (!was_active)
+ inactive_item_count_--;
+
return;
}
}
+ item_slots_used = TryReclaimInactiveSlots(item_slots_used);
+
+ // TODO(charliea): Add an UMA histogram to track the number of occupied
+ // metadata slots.
+ // See: https://crbug.com/980308
+
// There should always be room in this data structure because there are more
// reserved slots than there are unique metadata names in Chromium.
DCHECK_NE(item_slots_used, items_.size())
@@ -44,7 +57,7 @@
// Wait until the item is fully created before setting |is_active| to true and
// incrementing |item_slots_used_|, which will signal to readers that the item
// is ready.
- auto& item = items_[item_slots_used_];
+ auto& item = items_[item_slots_used];
item.name_hash = name_hash;
item.value.store(value, std::memory_order_relaxed);
item.is_active.store(true, std::memory_order_release);
@@ -58,16 +71,42 @@
for (size_t i = 0; i < item_slots_used; ++i) {
auto& item = items_[i];
if (item.name_hash == name_hash) {
- // A removed item will occupy its slot indefinitely.
- item.is_active.store(false, std::memory_order_release);
+ // A removed item will occupy its slot until that slot is reclaimed.
+ const bool was_active =
+ item.is_active.exchange(false, std::memory_order_relaxed);
+ if (was_active)
+ inactive_item_count_++;
+
+ return;
}
}
}
-size_t MetadataRecorder::GetItems(ItemArray* const items) const {
- // TODO(charliea): Defragment the item array if we can successfully acquire
- // the write lock here. This will require either making this function
- // non-const or |items_| mutable.
+MetadataRecorder::ScopedGetItems::ScopedGetItems(
+ MetadataRecorder* metadata_recorder)
+ : metadata_recorder_(metadata_recorder),
+ auto_lock_(&metadata_recorder->read_lock_) {}
+
+MetadataRecorder::ScopedGetItems::~ScopedGetItems() {}
+
+// This function is marked as NO_THREAD_SAFETY_ANALYSIS because the analyzer
+// doesn't understand that the lock is acquired in the constructor initializer
+// list and can therefore be safely released here.
+size_t MetadataRecorder::ScopedGetItems::GetItems(
+ ProfileBuilder::MetadataItemArray* const items) NO_THREAD_SAFETY_ANALYSIS {
+ size_t item_count = metadata_recorder_->GetItems(items);
+ auto_lock_.Release();
+ return item_count;
+}
+
+std::unique_ptr<ProfileBuilder::MetadataProvider>
+MetadataRecorder::CreateMetadataProvider() {
+ return std::make_unique<MetadataRecorder::ScopedGetItems>(this);
+}
+
+size_t MetadataRecorder::GetItems(
+ ProfileBuilder::MetadataItemArray* const items) const {
+ read_lock_.AssertAcquired();
// If a writer adds a new item after this load, it will be ignored. We do
// this instead of calling item_slots_used_.load() explicitly in the for loop
@@ -87,12 +126,75 @@
// Because we wait until |is_active| is set to consider an item active and
// that field is always set last, we ignore half-created items.
if (item.is_active.load(std::memory_order_acquire)) {
- (*items)[write_index++] =
- Item{item.name_hash, item.value.load(std::memory_order_relaxed)};
+ (*items)[write_index++] = ProfileBuilder::MetadataItem{
+ item.name_hash, item.value.load(std::memory_order_relaxed)};
}
}
return write_index;
}
+size_t MetadataRecorder::TryReclaimInactiveSlots(size_t item_slots_used) {
+ const size_t remaining_slots =
+ ProfileBuilder::MAX_METADATA_COUNT - item_slots_used;
+
+ if (inactive_item_count_ == 0 || inactive_item_count_ < remaining_slots) {
+ // This reclaiming threshold has a few nice properties:
+ //
+ // - It avoids reclaiming when no items have been removed
+ // - It makes doing so more likely as free slots become more scarce
+ // - It makes doing so less likely when the benefits are lower
+ return item_slots_used;
+ }
+
+ if (read_lock_.Try()) {
+ // The lock isn't already held by a reader or another thread reclaiming
+ // slots.
+ item_slots_used = ReclaimInactiveSlots(item_slots_used);
+ read_lock_.Release();
+ }
+
+ return item_slots_used;
+}
+
+size_t MetadataRecorder::ReclaimInactiveSlots(size_t item_slots_used) {
+ // From here until the end of the reclamation, we can safely use
+ // memory_order_relaxed for all reads and writes. We don't need
+ // memory_order_acquire because acquiring the write mutex gives acquire
+ // semantics and no other threads can write after we hold that mutex. We don't
+ // need memory_order_release because no readers can read until we release the
+ // read mutex, which itself has release semantics.
+ size_t first_inactive_item_idx = 0;
+ size_t last_active_item_idx = item_slots_used - 1;
+ while (first_inactive_item_idx < last_active_item_idx) {
+ ItemInternal& inactive_item = items_[first_inactive_item_idx];
+ ItemInternal& active_item = items_[last_active_item_idx];
+
+ if (inactive_item.is_active.load(std::memory_order_relaxed)) {
+ // Keep seeking forward to an inactive item.
+ ++first_inactive_item_idx;
+ continue;
+ }
+
+ if (!active_item.is_active.load(std::memory_order_relaxed)) {
+ // Keep seeking backward to an active item. Skipping over this item
+ // indicates that we're freeing the slot at this index.
+ --last_active_item_idx;
+ item_slots_used--;
+ continue;
+ }
+
+ inactive_item.name_hash = active_item.name_hash;
+ inactive_item.value.store(active_item.value.load(std::memory_order_relaxed),
+ std::memory_order_relaxed);
+ inactive_item.is_active.store(true, std::memory_order_relaxed);
+
+ ++first_inactive_item_idx;
+ --last_active_item_idx;
+ item_slots_used--;
+ }
+
+ item_slots_used_.store(item_slots_used, std::memory_order_relaxed);
+ return item_slots_used;
+}
} // namespace base
diff --git a/base/profiler/metadata_recorder.h b/base/profiler/metadata_recorder.h
index 346a5a1..faec7c27 100644
--- a/base/profiler/metadata_recorder.h
+++ b/base/profiler/metadata_recorder.h
@@ -9,7 +9,9 @@
#include <atomic>
#include <utility>
+#include "base/profiler/profile_builder.h"
#include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
namespace base {
@@ -19,6 +21,105 @@
// with the sample.
//
// Methods on this class are safe to call unsynchronized from arbitrary threads.
+//
+// This class was designed to read metadata from a single sampling thread and
+// write metadata from many Chrome threads within the same process. These other
+// threads might be suspended by the sampling thread at any time in order to
+// collect a sample.
+//
+// This class has a few notable constraints:
+//
+// A) If a lock that's required to read the metadata might be held while writing
+// the metadata, that lock must be acquirable *before* the thread is
+// suspended. Otherwise, the sampling thread might suspend the target thread
+// while it is holding the required lock, causing deadlock.
+//
+// Ramifications:
+//
+// - When retrieving items, lock acquisition (through
+// CreateMetadataProvider()) and actual item retrieval (through
+// MetadataProvider::GetItems()) are separate.
+//
+// B) We can't allocate data on the heap while reading the metadata items. This
+// is because, on many operating systems, there's a process-wide heap lock
+// that is held while allocating on the heap. If a thread is suspended while
+// holding this lock and the sampling thread then tries to allocate on the
+// heap to read the metadata, it will deadlock trying to acquire the heap
+// lock.
+//
+// Ramifications:
+//
+// - We hold and retrieve the metadata using a fixed-size array, which
+// allows readers to preallocate the data structure that we pass back
+// the metadata in.
+//
+// C) We shouldn't guard writes with a lock that also guards reads. It can take
+// ~30us from the time that the sampling thread requests that a thread be
+// suspended and the time that it actually happens. If all metadata writes
+// block their thread during that time, we're very likely to block all Chrome
+// threads for an additional 30us per sample.
+//
+// Ramifications:
+//
+// - We use two locks to guard the metadata: a read lock and a write
+// lock. Only the write lock is required to write into the metadata, and
+// only the read lock is required to read the metadata.
+//
+// - Because we can't guard reads and writes with the same lock, we have to
+// face the possibility of writes occurring during a read. This is
+// especially problematic because there's no way to read both the key and
+// value for an item atomically without using mutexes, which violates
+// constraint A). If the sampling thread were to see the following
+// interleaving of reads and writes:
+//
+// * Reader thread reads key for slot 0
+// * Writer thread removes item at slot 0
+// * Writer thread creates new item with different key in slot 0
+// * Reader thread reads value for slot 0
+//
+// then the reader would see an invalid value for the given key. Because
+// of this possibility, we keep slots reserved for a specific key even
+// after that item has been removed. We reclaim these slots on a
+// best-effort basis during writes when the metadata recorder has become
+// sufficiently full and we can acquire the read lock.
+//
+// - We use state stored in atomic data types to ensure that readers and
+// writers are synchronized about where data should be written to and
+// read from. We must use atomic data types to guarantee that there's no
+// instruction during a write after which the recorder is in an
+// inconsistent state that might yield garbage data for a reader.
+//
+// Here are a few of the many states the recorder can be in:
+//
+// - No thread is using the recorder.
+//
+// - A single writer is writing into the recorder without a simultaneous
+// read. The write will succeed.
+//
+// - A reader is reading from the recorder without a simultaneous write. The
+// read will succeed.
+//
+// - Multiple writers attempt to write into the recorder simultaneously. All
+// writers but one will block because only one can hold the write lock.
+//
+// - A writer is writing into the recorder, which hasn't reached the threshold
+// at which it will try to reclaim inactive slots. The writer won't try to
+// acquire the read lock to reclaim inactive slots. The reader will therefore
+// be able to immediately acquire the read lock, suspend the target thread,
+// and read the metadata.
+//
+// - A writer is writing into the recorder, the recorder has reached the
+// threshold at which it needs to reclaim inactive slots, and the writer
+// thread is now in the middle of reclaiming those slots when a reader
+// arrives. The reader will try to acquire the read lock before suspending the
+// thread but will block until the writer thread finishes reclamation and
+// releases the read lock. The reader will then be able to acquire the read
+// lock and suspend the target thread.
+//
+// - A reader is reading the recorder when a writer attempts to write. The write
+// will be successful. However, if the writer deems it necessary to reclaim
+// inactive slots, it will skip doing so because it won't be able to acquire
+// the read lock.
class BASE_EXPORT MetadataRecorder {
public:
MetadataRecorder();
@@ -35,22 +136,62 @@
// If such an item does not exist, this has no effect.
void Remove(uint64_t name_hash);
- struct Item {
- // The hash of the metadata name, as produced by base::HashMetricName().
- uint64_t name_hash;
- // The value of the metadata item.
- int64_t value;
- };
-
- static const size_t MAX_METADATA_COUNT = 50;
- typedef std::array<Item, MAX_METADATA_COUNT> ItemArray;
- // Retrieves the first |available_slots| items in the metadata recorder and
- // copies them into |items|, returning the number of metadata items that were
- // copied. To ensure that all items can be copied, |available slots| should be
- // greater than or equal to |MAX_METADATA_COUNT|.
- size_t GetItems(ItemArray* const items) const;
+ // Creates a MetadataProvider object for the recorder, which acquires the
+ // necessary exclusive read lock and provides access to the recorder's items
+ // via its GetItems() function. Reclaiming of inactive slots in the recorder
+ // can't occur while this object lives, so it should be created as soon before
+ // it's needed as possible. Calling GetItems() releases the lock held by the
+ // object and can therefore only be called once during the object's lifetime.
+ //
+ // This object should be created *before* suspending the target
+ // thread. Otherwise, that thread might be suspended while reclaiming inactive
+ // slots and holding the read lock, which would cause the sampling thread to
+ // deadlock.
+ //
+ // Example usage:
+ //
+ // MetadataRecorder r;
+ // base::ProfileBuilder::MetadataItemArray arr;
+ // size_t item_count;
+ // ...
+ // {
+ // auto get_items = r.CreateMetadataProvider();
+ // item_count = get_items.GetItems(arr);
+ // }
+ std::unique_ptr<ProfileBuilder::MetadataProvider> CreateMetadataProvider();
private:
+ // An object that provides access to a MetadataRecorder's items and holds the
+ // necessary exclusive read lock until either GetItems() is called or the
+ // object is destroyed.
+ //
+ // For usage and more details, see CreateMetadataProvider().
+ class SCOPED_LOCKABLE ScopedGetItems
+ : public ProfileBuilder::MetadataProvider {
+ public:
+ // Acquires an exclusive read lock on the metadata recorder which is held
+ // until either GetItems() is called or the object is destroyed.
+ ScopedGetItems(MetadataRecorder* metadata_recorder)
+ EXCLUSIVE_LOCK_FUNCTION(metadata_recorder->read_lock_);
+ ~ScopedGetItems() override UNLOCK_FUNCTION(metadata_recorder_->read_lock_);
+ ScopedGetItems(const ScopedGetItems&) = delete;
+ ScopedGetItems& operator=(const ScopedGetItems&) = delete;
+
+ // Retrieves the first |available_slots| items in the metadata recorder and
+ // copies them into |items|, returning the number of metadata items that
+ // were copied. To ensure that all items can be copied, |available slots|
+ // should be greater than or equal to |MAX_METADATA_COUNT|.
+ //
+ // This function releases the lock held by the object and can therefore only
+ // be called once during the object's lifetime.
+ size_t GetItems(ProfileBuilder::MetadataItemArray* const items) override
+ EXCLUSIVE_LOCKS_REQUIRED(metadata_recorder_->read_lock_);
+
+ private:
+ const MetadataRecorder* const metadata_recorder_;
+ base::ReleasableAutoLock auto_lock_;
+ };
+
// TODO(charliea): Support large quantities of metadata efficiently.
struct ItemInternal {
ItemInternal();
@@ -77,6 +218,24 @@
std::atomic<int64_t> value;
};
+ // Attempts to free slots in the metadata map that are currently allocated to
+ // inactive items. May fail silently if the read lock is already held, in
+ // which case no slots will be freed. Returns the number of item slots used
+ // after the reclamation.
+ size_t TryReclaimInactiveSlots(size_t item_slots_used)
+ EXCLUSIVE_LOCKS_REQUIRED(write_lock_) LOCKS_EXCLUDED(read_lock_);
+ // Also protected by read_lock_, but current thread annotation limitations
+ // prevent us from using thread annotations with locks acquired through
+ // Lock::Try(). Updates item_slots_used_ to reflect the new item count and
+ // returns the number of item slots used after the reclamation.
+ size_t ReclaimInactiveSlots(size_t item_slots_used)
+ EXCLUSIVE_LOCKS_REQUIRED(write_lock_);
+
+ // Protected by read_lock_, but current thread annotation limitations
+ // prevent us from using thread annotations with locks acquired through
+ // Lock::Try().
+ size_t GetItems(ProfileBuilder::MetadataItemArray* const items) const;
+
// Metadata items that the recorder has seen. Rather than implementing the
// metadata recorder as a dense array, we implement it as a sparse array where
// removed metadata items keep their slot with their |is_active| bit set to
@@ -85,7 +244,7 @@
//
// For the rationale behind this design (along with others considered), see
// https://docs.google.com/document/d/18shLhVwuFbLl_jKZxCmOfRB98FmNHdKl0yZZZ3aEO4U/edit#.
- std::array<ItemInternal, MAX_METADATA_COUNT> items_;
+ std::array<ItemInternal, ProfileBuilder::MAX_METADATA_COUNT> items_;
// The number of item slots used in the metadata map.
//
@@ -95,9 +254,21 @@
// of its existence.
std::atomic<size_t> item_slots_used_{0};
- // A lock that guards against multiple threads trying to modify the same item
- // at once.
+ // The number of item slots occupied by inactive items.
+ size_t inactive_item_count_ GUARDED_BY(write_lock_) = 0;
+
+ // A lock that guards against multiple threads trying to manipulate items_,
+ // item_slots_used_, or inactive_item_count_ at the same time.
base::Lock write_lock_;
+
+ // A lock that guards against a reader trying to read items_ while inactive
+ // slots are being reclaimed.
+ //
+ // Note that we can't enforce that this lock is properly acquired through
+ // thread annotations because thread annotations doesn't understand that
+ // ScopedGetItems::GetItems() can only be called between ScopedGetItems's
+ // constructor and destructor.
+ base::Lock read_lock_;
};
} // namespace base
diff --git a/base/profiler/metadata_recorder_unittest.cc b/base/profiler/metadata_recorder_unittest.cc
index 8155687..7228ec9 100644
--- a/base/profiler/metadata_recorder_unittest.cc
+++ b/base/profiler/metadata_recorder_unittest.cc
@@ -10,15 +10,21 @@
namespace base {
-bool operator==(const MetadataRecorder::Item& lhs,
- const MetadataRecorder::Item& rhs) {
+bool operator==(const base::ProfileBuilder::MetadataItem& lhs,
+ const base::ProfileBuilder::MetadataItem& rhs) {
return lhs.name_hash == rhs.name_hash && lhs.value == rhs.value;
}
+bool operator<(const base::ProfileBuilder::MetadataItem& lhs,
+ const base::ProfileBuilder::MetadataItem& rhs) {
+ return lhs.name_hash < rhs.name_hash;
+}
+
TEST(MetadataRecorderTest, GetItems_Empty) {
MetadataRecorder recorder;
- MetadataRecorder::ItemArray items;
- size_t item_count = recorder.GetItems(&items);
+ base::ProfileBuilder::MetadataItemArray items;
+
+ size_t item_count = recorder.CreateMetadataProvider()->GetItems(&items);
ASSERT_EQ(0u, item_count);
}
@@ -28,18 +34,23 @@
recorder.Set(10, 20);
- MetadataRecorder::ItemArray items;
- size_t item_count = recorder.GetItems(&items);
- ASSERT_EQ(1u, item_count);
- ASSERT_EQ(10u, items[0].name_hash);
- ASSERT_EQ(20, items[0].value);
+ base::ProfileBuilder::MetadataItemArray items;
+ size_t item_count;
+ {
+ item_count = recorder.CreateMetadataProvider()->GetItems(&items);
+ ASSERT_EQ(1u, item_count);
+ ASSERT_EQ(10u, items[0].name_hash);
+ ASSERT_EQ(20, items[0].value);
+ }
recorder.Set(20, 30);
- item_count = recorder.GetItems(&items);
- ASSERT_EQ(2u, item_count);
- ASSERT_EQ(20u, items[1].name_hash);
- ASSERT_EQ(30, items[1].value);
+ {
+ item_count = recorder.CreateMetadataProvider()->GetItems(&items);
+ ASSERT_EQ(2u, item_count);
+ ASSERT_EQ(20u, items[1].name_hash);
+ ASSERT_EQ(30, items[1].value);
+ }
}
TEST(MetadataRecorderTest, Set_ExistingNameNash) {
@@ -47,8 +58,8 @@
recorder.Set(10, 20);
recorder.Set(10, 30);
- MetadataRecorder::ItemArray items;
- size_t item_count = recorder.GetItems(&items);
+ base::ProfileBuilder::MetadataItemArray items;
+ size_t item_count = recorder.CreateMetadataProvider()->GetItems(&items);
ASSERT_EQ(1u, item_count);
ASSERT_EQ(10u, items[0].name_hash);
ASSERT_EQ(30, items[0].value);
@@ -56,10 +67,10 @@
TEST(MetadataRecorderTest, Set_ReAddRemovedNameNash) {
MetadataRecorder recorder;
- MetadataRecorder::ItemArray items;
- std::vector<MetadataRecorder::Item> expected;
+ base::ProfileBuilder::MetadataItemArray items;
+ std::vector<base::ProfileBuilder::MetadataItem> expected;
for (size_t i = 0; i < items.size(); ++i) {
- expected.push_back(MetadataRecorder::Item{i, 0});
+ expected.push_back(base::ProfileBuilder::MetadataItem{i, 0});
recorder.Set(i, 0);
}
@@ -70,14 +81,14 @@
recorder.Remove(3);
recorder.Set(3, 0);
- size_t item_count = recorder.GetItems(&items);
+ size_t item_count = recorder.CreateMetadataProvider()->GetItems(&items);
EXPECT_EQ(items.size(), item_count);
- ASSERT_THAT(expected, ::testing::ElementsAreArray(items));
+ ASSERT_THAT(expected, ::testing::UnorderedElementsAreArray(items));
}
TEST(MetadataRecorderTest, Set_AddPastMaxCount) {
MetadataRecorder recorder;
- MetadataRecorder::ItemArray items;
+ base::ProfileBuilder::MetadataItemArray items;
for (size_t i = 0; i < items.size(); ++i) {
recorder.Set(i, 0);
}
@@ -92,8 +103,8 @@
recorder.Set(50, 60);
recorder.Remove(30);
- MetadataRecorder::ItemArray items;
- size_t item_count = recorder.GetItems(&items);
+ base::ProfileBuilder::MetadataItemArray items;
+ size_t item_count = recorder.CreateMetadataProvider()->GetItems(&items);
ASSERT_EQ(2u, item_count);
ASSERT_EQ(10u, items[0].name_hash);
ASSERT_EQ(20, items[0].value);
@@ -106,11 +117,45 @@
recorder.Set(10, 20);
recorder.Remove(20);
- MetadataRecorder::ItemArray items;
- size_t item_count = recorder.GetItems(&items);
+ base::ProfileBuilder::MetadataItemArray items;
+ size_t item_count = recorder.CreateMetadataProvider()->GetItems(&items);
ASSERT_EQ(1u, item_count);
ASSERT_EQ(10u, items[0].name_hash);
ASSERT_EQ(20, items[0].value);
}
+TEST(MetadataRecorderTest, ReclaimInactiveSlots) {
+ MetadataRecorder recorder;
+
+ std::set<base::ProfileBuilder::MetadataItem> items_set;
+ // Fill up the metadata map.
+ for (size_t i = 0; i < base::ProfileBuilder::MAX_METADATA_COUNT; ++i) {
+ recorder.Set(i, i);
+ items_set.insert(base::ProfileBuilder::MetadataItem{i, i});
+ }
+
+ // Remove every fourth entry to fragment the data.
+ size_t entries_removed = 0;
+ for (size_t i = 3; i < base::ProfileBuilder::MAX_METADATA_COUNT; i += 4) {
+ recorder.Remove(i);
+ ++entries_removed;
+ items_set.erase(base::ProfileBuilder::MetadataItem{i, i});
+ }
+
+ // Ensure that the inactive slots are reclaimed to make room for more entries.
+ for (size_t i = 1; i <= entries_removed; ++i) {
+ recorder.Set(i * 100, i * 100);
+ items_set.insert(base::ProfileBuilder::MetadataItem{i * 100, i * 100});
+ }
+
+ base::ProfileBuilder::MetadataItemArray items_arr;
+ std::copy(items_set.begin(), items_set.end(), items_arr.begin());
+
+ base::ProfileBuilder::MetadataItemArray recorder_items;
+ size_t recorder_item_count =
+ recorder.CreateMetadataProvider()->GetItems(&recorder_items);
+ ASSERT_EQ(recorder_item_count, base::ProfileBuilder::MAX_METADATA_COUNT);
+ ASSERT_THAT(recorder_items, ::testing::UnorderedElementsAreArray(items_arr));
+}
+
} // namespace base
diff --git a/base/profiler/profile_builder.cc b/base/profiler/profile_builder.cc
new file mode 100644
index 0000000..ca3db2a
--- /dev/null
+++ b/base/profiler/profile_builder.cc
@@ -0,0 +1,7 @@
+// Copyright 2019 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 "base/profiler/profile_builder.h"
+
+const size_t base::ProfileBuilder::MAX_METADATA_COUNT;
diff --git a/base/profiler/profile_builder.h b/base/profiler/profile_builder.h
index 0202c21..49355ef 100644
--- a/base/profiler/profile_builder.h
+++ b/base/profiler/profile_builder.h
@@ -26,13 +26,31 @@
// up modules from addresses.
virtual ModuleCache* GetModuleCache() = 0;
+ struct MetadataItem {
+ // The hash of the metadata name, as produced by base::HashMetricName().
+ uint64_t name_hash;
+ // The value of the metadata item.
+ int64_t value;
+ };
+
+ static constexpr size_t MAX_METADATA_COUNT = 50;
+ typedef std::array<MetadataItem, MAX_METADATA_COUNT> MetadataItemArray;
+
+ class MetadataProvider {
+ public:
+ MetadataProvider() = default;
+ virtual ~MetadataProvider() = default;
+
+ virtual size_t GetItems(ProfileBuilder::MetadataItemArray* const items) = 0;
+ };
+
// Records metadata to be associated with the current sample. To avoid
// deadlock on locks taken by the suspended profiled thread, implementations
// of this method must not execute any code that could take a lock, including
// heap allocation or use of CHECK/DCHECK/LOG statements. Generally
// implementations should simply atomically copy metadata state to be
// associated with the sample.
- virtual void RecordMetadata() {}
+ virtual void RecordMetadata(MetadataProvider* metadata_provider) {}
// Records a new set of frames. Invoked when sampling a sample completes.
virtual void OnSampleCompleted(std::vector<Frame> frames) = 0;
diff --git a/base/profiler/sample_metadata_unittest.cc b/base/profiler/sample_metadata_unittest.cc
index 27e177b..e38052f 100644
--- a/base/profiler/sample_metadata_unittest.cc
+++ b/base/profiler/sample_metadata_unittest.cc
@@ -10,19 +10,29 @@
namespace base {
TEST(SampleMetadataTest, ScopedSampleMetadata) {
- MetadataRecorder::ItemArray items;
-
- ASSERT_EQ(0u, GetSampleMetadataRecorder()->GetItems(&items));
+ base::ProfileBuilder::MetadataItemArray items;
+ {
+ auto get_items = GetSampleMetadataRecorder()->CreateMetadataProvider();
+ ASSERT_EQ(0u, get_items->GetItems(&items));
+ }
{
ScopedSampleMetadata m("myname", 100);
- ASSERT_EQ(1u, GetSampleMetadataRecorder()->GetItems(&items));
- EXPECT_EQ(base::HashMetricName("myname"), items[0].name_hash);
- EXPECT_EQ(100, items[0].value);
+ {
+ ASSERT_EQ(1u,
+ GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+ &items));
+ EXPECT_EQ(base::HashMetricName("myname"), items[0].name_hash);
+ EXPECT_EQ(100, items[0].value);
+ }
}
- ASSERT_EQ(0u, GetSampleMetadataRecorder()->GetItems(&items));
+ {
+ ASSERT_EQ(0u,
+ GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+ &items));
+ }
}
} // namespace base
diff --git a/base/profiler/stack_sampler_impl.cc b/base/profiler/stack_sampler_impl.cc
index 2c70cc8..6993664 100644
--- a/base/profiler/stack_sampler_impl.cc
+++ b/base/profiler/stack_sampler_impl.cc
@@ -8,6 +8,7 @@
#include "base/logging.h"
#include "base/profiler/profile_builder.h"
+#include "base/profiler/sample_metadata.h"
#include "base/profiler/thread_delegate.h"
#include "base/profiler/unwinder.h"
@@ -79,6 +80,12 @@
uintptr_t bottom = 0;
const uint8_t* stack_copy_bottom = nullptr;
{
+ // The MetadataProvider must be created before the ScopedSuspendThread
+ // because it acquires a lock in its constructor that might otherwise be
+ // held by the target thread, resulting in deadlock.
+ std::unique_ptr<base::ProfileBuilder::MetadataProvider> get_metadata_items =
+ base::GetSampleMetadataRecorder()->CreateMetadataProvider();
+
// Allocation of the ScopedSuspendThread object itself is OK since it
// necessarily occurs before the thread is suspended by the object.
std::unique_ptr<ThreadDelegate::ScopedSuspendThread> suspend_thread =
@@ -102,7 +109,7 @@
if (!thread_delegate_->CanCopyStack(bottom))
return false;
- profile_builder->RecordMetadata();
+ profile_builder->RecordMetadata(get_metadata_items.get());
stack_copy_bottom = CopyStackContentsAndRewritePointers(
reinterpret_cast<uint8_t*>(bottom), reinterpret_cast<uintptr_t*>(top),
diff --git a/base/profiler/stack_sampler_impl_unittest.cc b/base/profiler/stack_sampler_impl_unittest.cc
index 9884e59c..f86d433 100644
--- a/base/profiler/stack_sampler_impl_unittest.cc
+++ b/base/profiler/stack_sampler_impl_unittest.cc
@@ -33,7 +33,8 @@
// ProfileBuilder
ModuleCache* GetModuleCache() override { return module_cache_; }
- void RecordMetadata() override {}
+ void RecordMetadata(
+ base::ProfileBuilder::MetadataProvider* metadata_provider) override {}
void OnSampleCompleted(std::vector<Frame> frames) override {}
void OnProfileCompleted(TimeDelta profile_duration,
TimeDelta sampling_period) override {}
@@ -60,8 +61,7 @@
// The register context will be initialized to
// *|thread_context| if non-null.
RegisterContext* thread_context = nullptr)
- : fake_stack_(fake_stack),
- thread_context_(thread_context) {}
+ : fake_stack_(fake_stack), thread_context_(thread_context) {}
TestThreadDelegate(const TestThreadDelegate&) = delete;
TestThreadDelegate& operator=(const TestThreadDelegate&) = delete;
diff --git a/base/profiler/stack_sampling_profiler_test_util.cc b/base/profiler/stack_sampling_profiler_test_util.cc
index e560e0c..b96bbeb 100644
--- a/base/profiler/stack_sampling_profiler_test_util.cc
+++ b/base/profiler/stack_sampling_profiler_test_util.cc
@@ -35,7 +35,7 @@
// ProfileBuilder:
ModuleCache* GetModuleCache() override { return module_cache_; }
- void RecordMetadata() override {}
+ void RecordMetadata(MetadataProvider* metadata_provider) override {}
void OnSampleCompleted(std::vector<Frame> sample) override {
EXPECT_TRUE(sample_.empty());
diff --git a/base/profiler/stack_sampling_profiler_unittest.cc b/base/profiler/stack_sampling_profiler_unittest.cc
index c52fcc2d..d3035b93 100644
--- a/base/profiler/stack_sampling_profiler_unittest.cc
+++ b/base/profiler/stack_sampling_profiler_unittest.cc
@@ -172,7 +172,8 @@
// ProfileBuilder:
ModuleCache* GetModuleCache() override;
- void RecordMetadata() override;
+ void RecordMetadata(
+ base::ProfileBuilder::MetadataProvider* metadata_provider) override;
void OnSampleCompleted(std::vector<Frame> sample) override;
void OnProfileCompleted(TimeDelta profile_duration,
TimeDelta sampling_period) override;
@@ -202,7 +203,8 @@
return module_cache_;
}
-void TestProfileBuilder::RecordMetadata() {
+void TestProfileBuilder::RecordMetadata(
+ base::ProfileBuilder::MetadataProvider* metadata_provider) {
++metadata_count_;
}
diff --git a/chrome/common/thread_profiler.cc b/chrome/common/thread_profiler.cc
index bfdb6c4..b8333d3a 100644
--- a/chrome/common/thread_profiler.cc
+++ b/chrome/common/thread_profiler.cc
@@ -245,7 +245,7 @@
std::make_unique<CallStackProfileBuilder>(
CallStackProfileParams(GetProcess(), thread,
CallStackProfileParams::PROCESS_STARTUP),
- work_id_recorder_.get(), base::GetSampleMetadataRecorder()));
+ work_id_recorder_.get()));
startup_profiler_->Start();
@@ -305,7 +305,7 @@
std::make_unique<CallStackProfileBuilder>(
CallStackProfileParams(GetProcess(), thread_,
CallStackProfileParams::PERIODIC_COLLECTION),
- work_id_recorder_.get(), base::GetSampleMetadataRecorder(),
+ work_id_recorder_.get(),
base::BindOnce(&ThreadProfiler::OnPeriodicCollectionCompleted,
owning_thread_task_runner_,
weak_factory_.GetWeakPtr())));
diff --git a/components/metrics/call_stack_profile_builder.cc b/components/metrics/call_stack_profile_builder.cc
index b35165d..0b40e5b0 100644
--- a/components/metrics/call_stack_profile_builder.cc
+++ b/components/metrics/call_stack_profile_builder.cc
@@ -48,7 +48,7 @@
}
std::map<uint64_t, int64_t> CreateMetadataMap(
- base::MetadataRecorder::ItemArray items,
+ base::ProfileBuilder::MetadataItemArray items,
size_t item_count) {
std::map<uint64_t, int64_t> item_map;
for (size_t i = 0; i < item_count; ++i) {
@@ -102,10 +102,8 @@
CallStackProfileBuilder::CallStackProfileBuilder(
const CallStackProfileParams& profile_params,
const WorkIdRecorder* work_id_recorder,
- const base::MetadataRecorder* metadata_recorder,
base::OnceClosure completed_callback)
: work_id_recorder_(work_id_recorder),
- metadata_recorder_(metadata_recorder),
profile_start_time_(base::TimeTicks::Now()) {
completed_callback_ = std::move(completed_callback);
sampled_profile_.set_process(
@@ -124,7 +122,8 @@
// This function is invoked on the profiler thread while the target thread is
// suspended so must not take any locks, including indirectly through use of
// heap allocation, LOG, CHECK, or DCHECK.
-void CallStackProfileBuilder::RecordMetadata() {
+void CallStackProfileBuilder::RecordMetadata(
+ base::ProfileBuilder::MetadataProvider* metadata_provider) {
if (work_id_recorder_) {
unsigned int work_id = work_id_recorder_->RecordWorkId();
// A work id of 0 indicates that the message loop has not yet started.
@@ -134,8 +133,8 @@
}
}
- if (metadata_recorder_)
- metadata_item_count_ = metadata_recorder_->GetItems(&metadata_items_);
+ if (metadata_provider)
+ metadata_item_count_ = metadata_provider->GetItems(&metadata_items_);
}
void CallStackProfileBuilder::OnSampleCompleted(
diff --git a/components/metrics/call_stack_profile_builder.h b/components/metrics/call_stack_profile_builder.h
index d141670..9de8401 100644
--- a/components/metrics/call_stack_profile_builder.h
+++ b/components/metrics/call_stack_profile_builder.h
@@ -57,7 +57,6 @@
explicit CallStackProfileBuilder(
const CallStackProfileParams& profile_params,
const WorkIdRecorder* work_id_recorder = nullptr,
- const base::MetadataRecorder* metadata_recorder = nullptr,
base::OnceClosure completed_callback = base::OnceClosure());
~CallStackProfileBuilder() override;
@@ -69,7 +68,8 @@
// base::ProfileBuilder:
base::ModuleCache* GetModuleCache() override;
- void RecordMetadata() override;
+ void RecordMetadata(base::ProfileBuilder::MetadataProvider*
+ metadata_provider = nullptr) override;
void OnSampleCompleted(std::vector<base::Frame> frames) override;
void OnProfileCompleted(base::TimeDelta profile_duration,
base::TimeDelta sampling_period) override;
@@ -113,7 +113,6 @@
unsigned int last_work_id_ = std::numeric_limits<unsigned int>::max();
bool is_continued_work_ = false;
const WorkIdRecorder* const work_id_recorder_;
- const base::MetadataRecorder* const metadata_recorder_;
// The SampledProfile protobuf message which contains the collected stack
// samples.
@@ -135,7 +134,7 @@
const base::TimeTicks profile_start_time_;
// The data fetched from the MetadataRecorder for the next sample.
- base::MetadataRecorder::ItemArray metadata_items_;
+ base::ProfileBuilder::MetadataItemArray metadata_items_;
size_t metadata_item_count_ = 0;
// The data fetched from the MetadataRecorder for the previous sample.
std::map<uint64_t, int64_t> previous_items_;
diff --git a/components/metrics/call_stack_profile_builder_unittest.cc b/components/metrics/call_stack_profile_builder_unittest.cc
index 2d4900bb..c3788bf 100644
--- a/components/metrics/call_stack_profile_builder_unittest.cc
+++ b/components/metrics/call_stack_profile_builder_unittest.cc
@@ -53,7 +53,6 @@
TestingCallStackProfileBuilder(
const CallStackProfileParams& profile_params,
const WorkIdRecorder* work_id_recorder = nullptr,
- const base::MetadataRecorder* metadata_recorder = nullptr,
base::OnceClosure completed_callback = base::OnceClosure());
~TestingCallStackProfileBuilder() override;
@@ -72,11 +71,9 @@
TestingCallStackProfileBuilder::TestingCallStackProfileBuilder(
const CallStackProfileParams& profile_params,
const WorkIdRecorder* work_id_recorder,
- const base::MetadataRecorder* metadata_recorder,
base::OnceClosure completed_callback)
: CallStackProfileBuilder(profile_params,
work_id_recorder,
- metadata_recorder,
std::move(completed_callback)) {}
TestingCallStackProfileBuilder::~TestingCallStackProfileBuilder() = default;
@@ -94,7 +91,7 @@
EXPECT_CALL(mock_closure, Run()).Times(1);
auto profile_builder = std::make_unique<TestingCallStackProfileBuilder>(
- kProfileParams, nullptr, nullptr, mock_closure.Get());
+ kProfileParams, nullptr, mock_closure.Get());
#if defined(OS_WIN)
uint64_t module_md5 = 0x46C3E4166659AC02ULL;
@@ -436,8 +433,8 @@
TEST(CallStackProfileBuilderTest, MetadataRecorder_NoItems) {
base::MetadataRecorder metadata_recorder;
- auto profile_builder = std::make_unique<TestingCallStackProfileBuilder>(
- kProfileParams, nullptr, &metadata_recorder);
+ auto profile_builder =
+ std::make_unique<TestingCallStackProfileBuilder>(kProfileParams, nullptr);
TestModule module;
base::Frame frame = {0x10, &module};
@@ -460,16 +457,22 @@
TEST(CallStackProfileBuilderTest, MetadataRecorder_RepeatItem) {
base::MetadataRecorder metadata_recorder;
- auto profile_builder = std::make_unique<TestingCallStackProfileBuilder>(
- kProfileParams, nullptr, &metadata_recorder);
+ auto profile_builder =
+ std::make_unique<TestingCallStackProfileBuilder>(kProfileParams, nullptr);
TestModule module;
base::Frame frame = {0x10, &module};
metadata_recorder.Set(100, 10);
- profile_builder->RecordMetadata();
+ {
+ auto get_items = metadata_recorder.CreateMetadataProvider();
+ profile_builder->RecordMetadata(get_items.get());
+ }
profile_builder->OnSampleCompleted({frame});
- profile_builder->RecordMetadata();
+ {
+ auto get_items = metadata_recorder.CreateMetadataProvider();
+ profile_builder->RecordMetadata(get_items.get());
+ }
profile_builder->OnSampleCompleted({frame});
profile_builder->OnProfileCompleted(base::TimeDelta::FromMilliseconds(500),
@@ -496,17 +499,23 @@
TEST(CallStackProfileBuilderTest, MetadataRecorder_ModifiedItem) {
base::MetadataRecorder metadata_recorder;
- auto profile_builder = std::make_unique<TestingCallStackProfileBuilder>(
- kProfileParams, nullptr, &metadata_recorder);
+ auto profile_builder =
+ std::make_unique<TestingCallStackProfileBuilder>(kProfileParams, nullptr);
TestModule module;
base::Frame frame = {0x10, &module};
metadata_recorder.Set(100, 10);
- profile_builder->RecordMetadata();
+ {
+ auto get_items = metadata_recorder.CreateMetadataProvider();
+ profile_builder->RecordMetadata(get_items.get());
+ }
profile_builder->OnSampleCompleted({frame});
metadata_recorder.Set(100, 11);
- profile_builder->RecordMetadata();
+ {
+ auto get_items = metadata_recorder.CreateMetadataProvider();
+ profile_builder->RecordMetadata(get_items.get());
+ }
profile_builder->OnSampleCompleted({frame});
profile_builder->OnProfileCompleted(base::TimeDelta::FromMilliseconds(500),
@@ -536,19 +545,25 @@
TEST(CallStackProfileBuilderTest, MetadataRecorder_NewItem) {
base::MetadataRecorder metadata_recorder;
- auto profile_builder = std::make_unique<TestingCallStackProfileBuilder>(
- kProfileParams, nullptr, &metadata_recorder);
+ auto profile_builder =
+ std::make_unique<TestingCallStackProfileBuilder>(kProfileParams, nullptr);
TestModule module;
base::Frame frame = {0x10, &module};
metadata_recorder.Set(100, 10);
- profile_builder->RecordMetadata();
+ {
+ auto get_items = metadata_recorder.CreateMetadataProvider();
+ profile_builder->RecordMetadata(get_items.get());
+ }
profile_builder->OnSampleCompleted({frame});
metadata_recorder.Set(100, 11);
metadata_recorder.Set(200, 20);
- profile_builder->RecordMetadata();
+ {
+ auto get_items = metadata_recorder.CreateMetadataProvider();
+ profile_builder->RecordMetadata(get_items.get());
+ }
profile_builder->OnSampleCompleted({frame});
profile_builder->OnProfileCompleted(base::TimeDelta::FromMilliseconds(500),
@@ -582,17 +597,23 @@
TEST(CallStackProfileBuilderTest, MetadataRecorder_RemovedItem) {
base::MetadataRecorder metadata_recorder;
- auto profile_builder = std::make_unique<TestingCallStackProfileBuilder>(
- kProfileParams, nullptr, &metadata_recorder);
+ auto profile_builder =
+ std::make_unique<TestingCallStackProfileBuilder>(kProfileParams, nullptr);
TestModule module;
base::Frame frame = {0x10, &module};
metadata_recorder.Set(100, 10);
- profile_builder->RecordMetadata();
+ {
+ auto get_items = metadata_recorder.CreateMetadataProvider();
+ profile_builder->RecordMetadata(get_items.get());
+ }
profile_builder->OnSampleCompleted({frame});
metadata_recorder.Remove(100);
- profile_builder->RecordMetadata();
+ {
+ auto get_items = metadata_recorder.CreateMetadataProvider();
+ profile_builder->RecordMetadata(get_items.get());
+ }
profile_builder->OnSampleCompleted({frame});
profile_builder->OnProfileCompleted(base::TimeDelta::FromMilliseconds(500),
