Skip to content

Commit

Permalink
memory-infra: MemoryAllocatorDump stores values instead of TracedValue
Browse files Browse the repository at this point in the history 
We want to be able to serialize MemoryAllocatorDumps for IPC,
this means keeping track of values set via AddScalar/AddScalarF/
AddString instead of serializing directly to a TracedValue.

Bug: 728199
Change-Id: I9fa86b61a07369782b1fc58802bc60ec9d11563b
Reviewed-on: https://chromium-review.googlesource.com/618721
Commit-Queue: Hector Dearman <hjd@chromium.org>
Reviewed-by: Siddhartha S <ssid@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498796}
  • Loading branch information
@chromy
chromy authored and Commit Bot committed Aug 31, 2017
1 parent 8b3e357 commit 0fdf71e
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 20 deletions.
73 changes: 59 additions & 14 deletions base/trace_event/memory_allocator_dump.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "base/trace_event/memory_allocator_dump.h"

#include "base/format_macros.h"
#include "base/memory/ptr_util.h"
#include "base/strings/stringprintf.h"
#include "base/trace_event/memory_dump_manager.h"
#include "base/trace_event/memory_dump_provider.h"
Expand Down Expand Up @@ -34,7 +35,6 @@ MemoryAllocatorDump::MemoryAllocatorDump(const std::string& absolute_name,
const MemoryAllocatorDumpGuid& guid)
: absolute_name_(absolute_name),
process_memory_dump_(process_memory_dump),
attributes_(new TracedValue),
guid_(guid),
flags_(Flags::DEFAULT),
size_(0) {
Expand All @@ -55,7 +55,6 @@ MemoryAllocatorDump::MemoryAllocatorDump(const std::string& absolute_name,
: MemoryAllocatorDump(absolute_name,
process_memory_dump,
GetDumpIdFromName(absolute_name)) {
string_conversion_buffer_.reserve(16);
}

MemoryAllocatorDump::~MemoryAllocatorDump() {
Expand All @@ -66,12 +65,7 @@ void MemoryAllocatorDump::AddScalar(const char* name,
uint64_t value) {
if (strcmp(kNameSize, name) == 0)
size_ = value;
SStringPrintf(&string_conversion_buffer_, "%" PRIx64, value);
attributes_->BeginDictionary(name);
attributes_->SetString("type", kTypeScalar);
attributes_->SetString("units", units);
attributes_->SetString("value", string_conversion_buffer_);
attributes_->EndDictionary();
entries_.emplace_back(name, units, value);
}

void MemoryAllocatorDump::AddString(const char* name,
Expand All @@ -83,22 +77,73 @@ void MemoryAllocatorDump::AddString(const char* name,
NOTREACHED();
return;
}
entries_.emplace_back(name, units, value);
}

void MemoryAllocatorDump::DumpAttributes(TracedValue* value) const {
std::string string_conversion_buffer;

attributes_->BeginDictionary(name);
attributes_->SetString("type", kTypeString);
attributes_->SetString("units", units);
attributes_->SetString("value", value);
attributes_->EndDictionary();
for (const Entry& entry : entries_) {
value->BeginDictionaryWithCopiedName(entry.name);
switch (entry.entry_type) {
case Entry::kUint64:
SStringPrintf(&string_conversion_buffer, "%" PRIx64,
entry.value_uint64);
value->SetString("type", kTypeScalar);
value->SetString("units", entry.units);
value->SetString("value", string_conversion_buffer);
break;
case Entry::kString:
value->SetString("type", kTypeString);
value->SetString("units", entry.units);
value->SetString("value", entry.value_string);
break;
}
value->EndDictionary();
}
}

void MemoryAllocatorDump::AsValueInto(TracedValue* value) const {
value->BeginDictionaryWithCopiedName(absolute_name_);
value->SetString("guid", guid_.ToString());
value->SetValue("attrs", *attributes_);
value->BeginDictionary("attrs");
DumpAttributes(value);
value->EndDictionary(); // "attrs": { ... }
if (flags_)
value->SetInteger("flags", flags_);
value->EndDictionary(); // "allocator_name/heap_subheap": { ... }
}

std::unique_ptr<TracedValue> MemoryAllocatorDump::attributes_for_testing()
const {
std::unique_ptr<TracedValue> attributes = base::MakeUnique<TracedValue>();
DumpAttributes(attributes.get());
return attributes;
}

MemoryAllocatorDump::Entry::Entry(Entry&& other) = default;

MemoryAllocatorDump::Entry::Entry(std::string name,
std::string units,
uint64_t value)
: name(name), units(units), entry_type(kUint64), value_uint64(value) {}
MemoryAllocatorDump::Entry::Entry(std::string name,
std::string units,
std::string value)
: name(name), units(units), entry_type(kString), value_string(value) {}

bool MemoryAllocatorDump::Entry::operator==(const Entry& rhs) const {
if (!(name == rhs.name && units == rhs.units && entry_type == rhs.entry_type))
return false;
switch (entry_type) {
case EntryType::kUint64:
return value_uint64 == rhs.value_uint64;
case EntryType::kString:
return value_string == rhs.value_string;
}
NOTREACHED();
return false;
}

} // namespace trace_event
} // namespace base
46 changes: 40 additions & 6 deletions base/trace_event/memory_allocator_dump.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "base/logging.h"
#include "base/macros.h"
#include "base/trace_event/memory_allocator_dump_guid.h"
#include "base/trace_event/trace_event_argument.h"
#include "base/values.h"

namespace base {
Expand All @@ -33,6 +34,35 @@ class BASE_EXPORT MemoryAllocatorDump {
WEAK = 1 << 0,
};

// In the TraceViewer UI table each MemoryAllocatorDump becomes
// a row and each Entry generates a column (if it doesn't already
// exist).
struct BASE_EXPORT Entry {
enum EntryType {
kUint64,
kString,
};

// By design name, units and value_string are always coming from
// indefinitely lived const char* strings, the only reason we copy
// them into a std::string is to handle Mojo (de)serialization.
// TODO(hjd): Investigate optimization (e.g. using StringPiece).
Entry(std::string name, std::string units, uint64_t value);
Entry(std::string name, std::string units, std::string value);
Entry(Entry&& other);
bool operator==(const Entry& rhs) const;

std::string name;
std::string units;

EntryType entry_type;

uint64_t value_uint64;
std::string value_string;

DISALLOW_COPY_AND_ASSIGN(Entry);
};

// Returns the Guid of the dump for the given |absolute_name| for the
// current process.
static MemoryAllocatorDumpGuid GetDumpIdFromName(
Expand Down Expand Up @@ -62,7 +92,7 @@ class BASE_EXPORT MemoryAllocatorDump {
// - "size" column (all dumps are expected to have at least this one):
// AddScalar(kNameSize, kUnitsBytes, 1234);
// - Some extra-column reporting internal details of the subsystem:
// AddScalar("number_of_freelist_entires", kUnitsObjects, 42)
// AddScalar("number_of_freelist_entries", kUnitsObjects, 42)
// - Other informational column:
// AddString("kitten", "name", "shadow");
void AddScalar(const char* name, const char* units, uint64_t value);
Expand Down Expand Up @@ -92,19 +122,23 @@ class BASE_EXPORT MemoryAllocatorDump {
// expected to have the same guid.
const MemoryAllocatorDumpGuid& guid() const { return guid_; }

TracedValue* attributes_for_testing() const { return attributes_.get(); }
const std::vector<Entry>& entries_for_testing() const { return entries_; }

// Decprecated testing method. Use entries_for_testing instead.
// TODO(hjd): Remove this and refactor callers to use entries_for_testing then
// inline DumpAttributes.
std::unique_ptr<TracedValue> attributes_for_testing() const;

private:
void DumpAttributes(TracedValue* value) const;

const std::string absolute_name_;
ProcessMemoryDump* const process_memory_dump_; // Not owned (PMD owns this).
std::unique_ptr<TracedValue> attributes_;
MemoryAllocatorDumpGuid guid_;
int flags_; // See enum Flags.
uint64_t size_;

// A local buffer for Sprintf conversion on fastpath. Avoids allocating
// temporary strings on each AddScalar() call.
std::string string_conversion_buffer_;
std::vector<Entry> entries_;

DISALLOW_COPY_AND_ASSIGN(MemoryAllocatorDump);
};
Expand Down
35 changes: 35 additions & 0 deletions base/trace_event/memory_allocator_dump_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,13 @@
#include "base/trace_event/trace_event_argument.h"
#include "base/values.h"
#include "build/build_config.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"

using testing::ElementsAre;
using testing::Eq;
using testing::ByRef;

namespace base {
namespace trace_event {

Expand Down Expand Up @@ -149,6 +154,7 @@ TEST(MemoryAllocatorDumpTest, DumpIntoProcessMemoryDump) {
pmd.GetAllocatorDump("foobar_allocator/sub_heap/empty");
ASSERT_NE(nullptr, empty_sub_heap);
EXPECT_EQ("foobar_allocator/sub_heap/empty", empty_sub_heap->absolute_name());

auto raw_attrs = empty_sub_heap->attributes_for_testing()->ToBaseValue();
DictionaryValue* attrs = nullptr;
ASSERT_TRUE(raw_attrs->GetAsDictionary(&attrs));
Expand All @@ -170,6 +176,27 @@ TEST(MemoryAllocatorDumpTest, GetSize) {
EXPECT_EQ(1u, dump->GetSizeInternal());
}

TEST(MemoryAllocatorDumpTest, ReadValues) {
MemoryDumpArgs dump_args = {MemoryDumpLevelOfDetail::DETAILED};
ProcessMemoryDump pmd(new HeapProfilerSerializationState, dump_args);
MemoryAllocatorDump* dump = pmd.CreateAllocatorDump("allocator_for_size");
dump->AddScalar("one", "byte", 1);
dump->AddString("one", "object", "one");

MemoryAllocatorDump::Entry expected_scalar("one", "byte", 1);
MemoryAllocatorDump::Entry expected_string("one", "object", "one");
EXPECT_THAT(
dump->entries_for_testing(),
ElementsAre(Eq(ByRef(expected_scalar)), Eq(ByRef(expected_string))));
}

TEST(MemoryAllocatorDumpTest, MovingAnEntry) {
MemoryAllocatorDump::Entry expected_entry("one", "byte", 1);
MemoryAllocatorDump::Entry from_entry("one", "byte", 1);
MemoryAllocatorDump::Entry to_entry = std::move(from_entry);
EXPECT_EQ(expected_entry, to_entry);
}

// DEATH tests are not supported in Android/iOS/Fuchsia.
#if !defined(NDEBUG) && !defined(OS_ANDROID) && !defined(OS_IOS) && \
!defined(OS_FUCHSIA)
Expand All @@ -183,6 +210,14 @@ TEST(MemoryAllocatorDumpTest, ForbidDuplicatesDeathTest) {
ASSERT_DEATH(pmd.CreateAllocatorDump("bar_allocator/heap"), "");
ASSERT_DEATH(pmd.CreateAllocatorDump(""), "");
}

TEST(MemoryAllocatorDumpTest, ForbidStringsInBackgroundModeDeathTest) {
MemoryDumpArgs dump_args = {MemoryDumpLevelOfDetail::BACKGROUND};
ProcessMemoryDump pmd(new HeapProfilerSerializationState, dump_args);
MemoryAllocatorDump* dump = pmd.CreateAllocatorDump("malloc");
ASSERT_DEATH(dump->AddString("foo", "bar", "baz"), "");
}

#endif

} // namespace trace_event
Expand Down

0 comments on commit 0fdf71e

Please sign in to comment.