[WIP][tracing] Add memory dump provider for sync Directory

sync/syncable/directory.cc

Index: sync/syncable/directory.cc
diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc
index ca1efc1c95bda86f708e288e667e15b106d39b25..ae115848a5c4879923c7700567cfacce2affa6f2 100644
--- a/sync/syncable/directory.cc
+++ b/sync/syncable/directory.cc
@@ -4,6 +4,7 @@
 
 #include "sync/syncable/directory.h"
 
+#include <inttypes.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ -16,6 +17,9 @@
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
+#include "base/trace_event/memory_dump_manager.h"
+#include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event.h"
 #include "sync/internal_api/public/base/attachment_id_proto.h"
 #include "sync/internal_api/public/base/unique_position.h"
@@ -920,6 +924,58 @@ size_t Directory::GetEntriesCount() const {
   return kernel_->metahandles_map.size();
 }
 
+void Directory::OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd) {
+  size_t total = 0;
+  static const size_t kEntryKernelSize =
+      sizeof(EntryKernel) +
+      PROTO_FIELDS_COUNT * sizeof(sync_pb::EntitySpecifics) +

[stanisc, 2016/06/24 21:44:35]

This isn't correct. The actual number of EntitySpecifics and AttachmentMetadata
varies depending on whether they are shared and/or have a non-default value.
Most of the time there is just one copy of EntitySpecifics and zero copies of
AttachmentMetadata.

[ssid, 2016/07/01 22:46:52]

I am not sure how do i find if the EntitySpecifics exists. If i get the value,
it returns a default instance. So checking against default instance.

+      ATTACHMENT_METADATA_FIELDS_COUNT * sizeof(sync_pb::AttachmentMetadata);
+  {
+    ScopedKernelLock lock(this);
+    total += kernel_->metahandles_map.size() * kEntryKernelSize;

[stanisc, 2016/06/24 21:44:35]

This doesn't take the map overhead into account.

[ssid, 2016/07/01 22:46:52]

Um, I think its not really accurate to estimate the overhead using just bucket
counts and it is not worth the effort of looping through each bucket and
calculating entries in each, and this has to be done for multiple maps.

+    for (auto entry : kernel_->index_by_attachment_id) {
+      total += entry.first.size();
+      total += entry.second.size() * sizeof(int64_t);
+    }
+    for (auto handle : kernel_->metahandles_map) {

[stanisc, 2016/06/24 21:44:35]

Would be nice to add the cost of the map itself.

[ssid, 2016/07/01 22:46:52]

Do you mean the overhead mentioned at line 935, or something else?
I have added size of key and value to the above calculation.

+      // Twice because the |ids_map| stores a copy of these strings.
+      for (unsigned i = ID_FIELDS_BEGIN; i < ID_FIELDS_END; ++i)
+        total += 2 * handle.second->ref(static_cast<IdField>(i)).value().size();

[stanisc, 2016/06/24 21:44:35]

The memory used by a string in memory isn't size().
It is more like field->capacity() + 1. 
Normally sizeof(string) need to be added also but it is already counted as a
part of sizeof(EntryKernel)

[ssid, 2016/07/01 22:46:52]

I have changed to capacity. Not sure I understand why + 1?

+
+      for (unsigned i = STRING_FIELDS_BEGIN; i < STRING_FIELDS_END; ++i) {
+        if (i == UNIQUE_SERVER_TAG || i == UNIQUE_CLIENT_TAG) {
+          // These are stored again in tags_map.
+          total += 2 * handle.second->ref(static_cast<StringField>(i)).size();
+        } else {
+          total += handle.second->ref(static_cast<StringField>(i)).size();
+        }
+      }
+      for (unsigned i = PROTO_FIELDS_BEGIN; i < PROTO_FIELDS_END; ++i)
+        total += handle.second->ref(static_cast<ProtoField>(i)).ByteSize();
+      for (unsigned i = ATTACHMENT_METADATA_FIELDS_BEGIN;
+           i < ATTACHMENT_METADATA_FIELDS_END; ++i) {
+        total += handle.second->ref(static_cast<AttachmentMetadataField>(i))
+                     .ByteSize();

[stanisc, 2016/06/24 21:44:35]

We've discussed this already. ByteSize is far from the actual size.

[ssid, 2016/06/24 21:56:50]

Yes I would prefer writing the visitor code / similar to get more accurate
value. Even if it is not fast enough, it would be more accurate here.
Even a cached value of more accurate size (calculated at startup) is better than
an quick lower estimate.

What do you think I should do?

[ssid, 2016/07/01 22:46:52]

So, I have made this calculation by changing conversions to include a visitor
like pattern that return more accurate size

+      }
+      for (unsigned i = UNIQUE_POSITION_FIELDS_BEGIN;
+           i < UNIQUE_POSITION_FIELDS_END; ++i) {
+        total += handle.second->ref(static_cast<UniquePositionField>(i))
+                     .compressed_size();
+      }
+    }
+  }
+  auto dump = pmd->CreateAllocatorDump(base::StringPrintf(

[stanisc, 2016/06/24 21:44:35]

There are other maps which cost need to be added. For example there is a fairly
expensive ParentChildIndex.

[ssid, 2016/07/01 22:46:52]

I have added whatever I think is significant. Not sure if it's worth adding
others.

+      "sync/0x%" PRIXPTR, reinterpret_cast<uintptr_t>(this)));
+  dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
+                  base::trace_event::MemoryAllocatorDump::kUnitsBytes, total);
+  const char* system_allocator_name =
+      base::trace_event::MemoryDumpManager::GetInstance()
+          ->system_allocator_pool_name();
+  if (system_allocator_name) {
+    pmd->AddSuballocation(dump->guid(), system_allocator_name);
+  }
+}
+
 void Directory::SetDownloadProgress(
     ModelType model_type,
     const sync_pb::DataTypeProgressMarker& new_progress) {