Enable sharding of AllocationRegister on desktop.

base/trace_event/sharded_allocation_register.cc

Index: base/trace_event/sharded_allocation_register.cc
diff --git a/base/trace_event/sharded_allocation_register.cc b/base/trace_event/sharded_allocation_register.cc
new file mode 100644
index 0000000000000000000000000000000000000000..80895211637802da59b943fc3ec37b38be951687
--- /dev/null
+++ b/base/trace_event/sharded_allocation_register.cc
@@ -0,0 +1,105 @@
+// Copyright 2017 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/trace_event/sharded_allocation_register.h"
+
+#include "base/trace_event/trace_event_memory_overhead.h"
+#include "build/build_config.h"
+
+namespace base {
+namespace trace_event {
+
+// This number affects the bucket and capacity counts of AllocationRegister at
+// "base/trace_event/heap_profiler_allocation_register.h".
+#if defined(OS_ANDROID) || defined(OS_IOS)
+size_t ShardCount = 1;
+#else
+size_t ShardCount = 64;
+#endif
+
+size_t HashAddress(const void* address) {

[Primiano Tucci (use gerrit), 2017/05/22 16:37:51]

since this is copy-pasted from a header that is included here (well in the .h
file) can you just reuse AddressHasher ? If you are worried about perf, you can
move it to a static inline HashAddress function in
heap_profiler_allocation_register.h and use in both.
Feels a bit odd that we have to have two identical copy-pasted function in two
files that include each other to achieve the same goal (hash an address)

[erikchen, 2017/05/22 17:10:43]

Done.

+  // The multiplicative hashing scheme from [Knuth 1998].
+  const uintptr_t key = reinterpret_cast<uintptr_t>(address);
+  const uintptr_t a = 131101;
+  const uintptr_t shift = 15;
+  const uintptr_t h = (key * a) >> shift;
+  return h;
+}
+
+ShardedAllocationRegister::ShardedAllocationRegister() : enabled_(false) {}
+
+ShardedAllocationRegister::~ShardedAllocationRegister() = default;
+
+void ShardedAllocationRegister::Initialize() {
+  allocation_registers_.resize(ShardCount);
+  for (std::unique_ptr<RegisterAndLock>& ral : allocation_registers_)
+    ral.reset(new RegisterAndLock);

[Primiano Tucci (use gerrit), 2017/05/22 16:37:51]

doesn't make a difference here (so I don't care about what you do in this line)
but as a general suggestion, get used to () around new. If RegisterAndLock was
POD this would have skipped the initialization.
 (this is one of the arguments why people on chromium-dev push for MakeUnique,
because it enforces the initialization)

[erikchen, 2017/05/22 17:10:43]

Done.

+}
+
+bool ShardedAllocationRegister::IsInitialized() const {
+  return !allocation_registers_.empty();
+}
+
+void ShardedAllocationRegister::SetEnabled(bool enabled) {

[Primiano Tucci (use gerrit), 2017/05/22 16:37:50]

I would remove this method and do this in initialize().
this will remove a potential bug if the client does the Setenabled(),
Initialize() in the wrong order.
Also there doesn't seem to be a valid semantic for doing SetEnabled on its own.
So I would have only two methods (or one) in total, not 4. Up to you if you want
to call them:
- SetEnabled() (which would do the initialization also) and SetDisabled()
- SetEnabled(bool), where the "true" path does also the initialization

[erikchen, 2017/05/22 17:10:43]

Done. [SetEnabled and SetDisabled]

+  base::subtle::Release_Store(&enabled_, enabled ? 1 : 0);
+}
+
+bool ShardedAllocationRegister::IsEnabled() const {
+  return base::subtle::Acquire_Load(&enabled_);

[Primiano Tucci (use gerrit), 2017/05/22 16:37:51]

I'd make move this to the header file and turn into is_enabled(). This will
introduce an extra call to do the check that: (i) is not needed at all; (ii)
wasn't there before this cl;  (iii) shouldn't emit any more code than a function
call as this boils down to a read op.

[erikchen, 2017/05/22 17:10:43]

Done.

+}
+
+bool ShardedAllocationRegister::Insert(const void* address,
+                                       size_t size,
+                                       const AllocationContext& context) {
+  size_t index = HashAddress(address) % ShardCount;
+  std::unique_ptr<RegisterAndLock>& ral = allocation_registers_[index];
+  AutoLock lock(ral->lock);
+  return ral->allocation_register.Insert(address, size, context);
+}
+
+void ShardedAllocationRegister::Remove(const void* address) {
+  size_t index = HashAddress(address) % ShardCount;
+  std::unique_ptr<RegisterAndLock>& ral = allocation_registers_[index];
+  AutoLock lock(ral->lock);
+  return ral->allocation_register.Remove(address);
+}
+
+void ShardedAllocationRegister::EstimateTraceMemoryOverhead(
+    TraceEventMemoryOverhead* overhead) const {
+  size_t allocated = 0;
+  size_t resident = 0;
+  for (const std::unique_ptr<RegisterAndLock>& ral : allocation_registers_) {
+    AutoLock lock(ral->lock);
+    allocated += ral->allocation_register.EstimateAllocatedMemory();
+    resident += ral->allocation_register.EstimateResidentMemory();
+  }
+
+  overhead->Add(TraceEventMemoryOverhead::kHeapProfilerAllocationRegister,
+                allocated, resident);
+}
+
+ShardedAllocationRegister::OutputMetrics
+ShardedAllocationRegister::UpdateAndReturnsMetrics(MetricsMap& map) const {
+  OutputMetrics output_metrics;
+  output_metrics.size = 0;
+  output_metrics.count = 0;
+  for (const std::unique_ptr<RegisterAndLock>& ral : allocation_registers_) {
+    AutoLock lock(ral->lock);
+    for (const auto& alloc_size : ral->allocation_register) {
+      AllocationMetrics& metrics = map[alloc_size.context];
+      metrics.size += alloc_size.size;
+      metrics.count++;
+
+      output_metrics.size += alloc_size.size;
+      output_metrics.count++;
+    }
+  }
+  return output_metrics;
+}
+
+ShardedAllocationRegister::RegisterAndLock::RegisterAndLock() = default;
+ShardedAllocationRegister::RegisterAndLock::~RegisterAndLock() = default;
+
+}  // namespace trace_event
+}  // namespace base