Issue 2084243004: [WIP][tracing] Add memory dump provider for sync Directory

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1518 lines, -453 lines)			Patch
M	components/sync_driver/glue/sync_backend_host_core.h	View		3 chunks	+6 lines, -0 lines	0 comments	Download
M	components/sync_driver/glue/sync_backend_host_core.cc	View	1 2	4 chunks	+16 lines, -0 lines	0 comments	Download
M	sync/internal_api/public/base/unique_position.h	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	sync/internal_api/public/sync_manager.h	View		2 chunks	+9 lines, -0 lines	0 comments	Download
M	sync/internal_api/public/test/fake_sync_manager.h	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	sync/internal_api/sync_manager_impl.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	sync/internal_api/sync_manager_impl.cc	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	sync/internal_api/test/fake_sync_manager.cc	View	1 2	1 chunk	+4 lines, -0 lines	0 comments	Download
M	sync/protocol/proto_value_conversions.h	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download
M	sync/protocol/proto_value_conversions.cc	View	1 2 3	16 chunks	+1299 lines, -452 lines	0 comments	Download
M	sync/syncable/directory.h	View	1	2 chunks	+11 lines, -0 lines	0 comments	Download
M	sync/syncable/directory.cc	View	1 2 3	5 chunks	+122 lines, -0 lines	3 comments	Download
M	sync/syncable/entry_kernel.h	View	1	3 chunks	+6 lines, -0 lines	0 comments	Download
M	sync/syncable/entry_kernel.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	sync/syncable/parent_child_index.h	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download
M	sync/syncable/parent_child_index.cc	View	1 2 3	2 chunks	+25 lines, -0 lines	0 comments	Download
M	tools/metrics/histograms/histograms.xml	View	1 2 3	1 chunk	+5 lines, -0 lines	0 comments	Download

Messages

Total messages: 18 (10 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

stanisc

https://codereview.chromium.org/2084243004/diff/10009/sync/syncable/directory.cc File sync/syncable/directory.cc (right): https://codereview.chromium.org/2084243004/diff/10009/sync/syncable/directory.cc#newcode931 sync/syncable/directory.cc:931: PROTO_FIELDS_COUNT * sizeof(sync_pb::EntitySpecifics) + This isn't correct. The actual ...

4 years, 6 months ago (2016-06-24 21:44:35 UTC) #4

ssid

https://codereview.chromium.org/2084243004/diff/10009/sync/syncable/directory.cc File sync/syncable/directory.cc (right): https://codereview.chromium.org/2084243004/diff/10009/sync/syncable/directory.cc#newcode958 sync/syncable/directory.cc:958: .ByteSize(); On 2016/06/24 21:44:35, stanisc wrote: > We've discussed ...

4 years, 6 months ago (2016-06-24 21:56:50 UTC) #5

ssid

I changed the conversions file to give multiple types of result. If i used a ...

4 years, 5 months ago (2016-07-01 22:46:52 UTC) #11

I changed the conversions file to give multiple types of result. If i used a
visitor interface with functions like "SetString", each of the set macro will
cause a function call to virtual function, instead here I am including if
conditions for each macro, which i think could be faster. Please take a look at
the code, I will add tests once you think this pattern is fine.

https://codereview.chromium.org/2084243004/diff/10009/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/2084243004/diff/10009/sync/syncable/directory...
sync/syncable/directory.cc:931: PROTO_FIELDS_COUNT *
sizeof(sync_pb::EntitySpecifics) +
On 2016/06/24 21:44:35, stanisc wrote:
> 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.

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.

https://codereview.chromium.org/2084243004/diff/10009/sync/syncable/directory...
sync/syncable/directory.cc:935: total += kernel_->metahandles_map.size() *
kEntryKernelSize;
On 2016/06/24 21:44:35, stanisc wrote:
> This doesn't take the map overhead into account.

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.

https://codereview.chromium.org/2084243004/diff/10009/sync/syncable/directory...
sync/syncable/directory.cc:940: for (auto handle : kernel_->metahandles_map) {
On 2016/06/24 21:44:35, stanisc wrote:
> Would be nice to add the cost of the map itself.

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

https://codereview.chromium.org/2084243004/diff/10009/sync/syncable/directory...
sync/syncable/directory.cc:943: total += 2 *
handle.second->ref(static_cast<IdField>(i)).value().size();
On 2016/06/24 21:44:35, stanisc wrote:
> 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)

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

https://codereview.chromium.org/2084243004/diff/10009/sync/syncable/directory...
sync/syncable/directory.cc:958: .ByteSize();
On 2016/06/24 21:56:50, ssid wrote:
> On 2016/06/24 21:44:35, stanisc wrote:
> > We've discussed this already. ByteSize is far from the actual size.
> 
> 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?

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

https://codereview.chromium.org/2084243004/diff/10009/sync/syncable/directory...
sync/syncable/directory.cc:967: auto dump =
pmd->CreateAllocatorDump(base::StringPrintf(
On 2016/06/24 21:44:35, stanisc wrote:
> There are other maps which cost need to be added. For example there is a
fairly
> expensive ParentChildIndex.

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

stanisc

Here are my comments from a somewhat cursory review. I didn't have time for a ...

4 years, 5 months ago (2016-07-06 21:50:32 UTC) #12

ssid

Thanks! made changes as suggested. https://codereview.chromium.org/2084243004/diff/110001/sync/internal_api/public/base/unique_position.h File sync/internal_api/public/base/unique_position.h (right): https://codereview.chromium.org/2084243004/diff/110001/sync/internal_api/public/base/unique_position.h#newcode98 sync/internal_api/public/base/unique_position.h:98: size_t compressed_size() const { ...

4 years, 5 months ago (2016-07-19 22:10:35 UTC) #15

Thanks! made changes as suggested.

https://codereview.chromium.org/2084243004/diff/110001/sync/internal_api/publ...
File sync/internal_api/public/base/unique_position.h (right):

https://codereview.chromium.org/2084243004/diff/110001/sync/internal_api/publ...
sync/internal_api/public/base/unique_position.h:98: size_t compressed_size()
const { return compressed_.capacity(); }
On 2016/07/06 21:50:31, stanisc wrote:
> Perhaps the function name should be compressed_byte_size() or simply
> byte_size(). Also everywhere capacity() is used you need to add 1 for the
> terminating zero.

Done.

https://codereview.chromium.org/2084243004/diff/110001/sync/protocol/proto_va...
File sync/protocol/proto_value_conversions.cc (right):

https://codereview.chromium.org/2084243004/diff/110001/sync/protocol/proto_va...
sync/protocol/proto_value_conversions.cc:54: enum ResultType {
On 2016/07/06 21:50:31, stanisc wrote:
> Someone from the current Sync team should review this (I am not in the team
> anymore). This file was already quite complex and now it is even harder to
> understand.
> The reason I didn't want the visitor pattern to start with was to avoid this
> kind of complexity. 

Not sure what is the alternative here. I can't think of any other way to get
size of the object other than visiting all nodes.

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/director...
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/director...
sync/syncable/directory.cc:51: return map.size() * (sizeof(Key) +
sizeof(Value));
On 2016/07/06 21:50:32, stanisc wrote:
> This is too simplistic. See
> http://info.prelert.com/blog/stl-container-memory-usage

Made it more accurate. Apart from these there are a few compiler specific usages
which are ignored here.

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/director...
sync/syncable/directory.cc:231:
UMA_HISTOGRAM_COUNTS("Sync.DirectoryMemoryUsageKB",
On 2016/07/06 21:50:32, stanisc wrote:
> Someone from the current sync team should comment on this. I think the
original
> idea was to get distribution by sync type but perhaps this should be OK.

Maybe that can be added later on top of this implementation. Should be easy to
add

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/director...
sync/syncable/directory.cc:956: if (handle.second->cached_size()) {
On 2016/07/06 21:50:32, stanisc wrote:
> A comment here would be useful

Done.

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/director...
sync/syncable/directory.cc:965: 2 *
handle.second->ref(static_cast<IdField>(i)).value().capacity();
On 2016/07/06 21:50:31, stanisc wrote:
> I am not sure this is right. client ID might be the same as the server ID and
> then they wouldn't be entered into ids_map twice. Also most of the parent IDs
> are empty e.g. deref to the same shared empty string. I am not sure if their
> capacity would be zero or not - worth double checking.

counting the ids_map separately. The string capacities are usually 10.

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/director...
sync/syncable/directory.cc:969: // These are stored again in tags_map.
On 2016/07/06 21:50:32, stanisc wrote:
> The same is here - if the server tag is the same as the client tag they
wouldn't
> appear twice in tags_map.

This one will appear twice if the id is same. There are two maps,
client_tags_map and server_tags_map. Updated the comment.

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/director...
sync/syncable/directory.cc:980: entry_size += GetEntitySpecificsSize(field);
On 2016/07/06 21:50:32, stanisc wrote:
> Proto fields are often shared which means any consecutive fields with 
> non-default values may deref to the same allocated EntitySpecifics object in
> memory. It would be incorrect to add them twice (or 3 times).

Done.

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/director...
sync/syncable/directory.cc:988: entry_size +=
sizeof(sync_pb::AttachmentMetadata);
On 2016/07/06 21:50:32, stanisc wrote:
> The same is here.

Done.

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/parent_c...
File sync/syncable/parent_child_index.cc (right):

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/parent_c...
sync/syncable/parent_child_index.cc:88: bytes_used_ +=
parent_id.value().capacity() + sizeof(OrderedChildSet);
On 2016/07/06 21:50:32, stanisc wrote:
> I would prefer to not mix the code for the size estimation with the rest of
the
> logic. It would be cleaner to calculate the size on demand.

counting the usage on demand.

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/parent_c...
sync/syncable/parent_child_index.cc:128: 2 * type_root_id.value().capacity() +
sizeof(OrderedChildSet*);
On 2016/07/06 21:50:32, stanisc wrote:
> This doesn't take into account sizeof(Id) which should be ~32 bytes. Also I
> don't think you need to double the capacity. Internally characters are stored
in
> UTF-8 rather than unicode.
> 
> Also you need to add one to the capacity because it doesn't count the
> terminating zero (here and everywhere capacity is used).
> 
> I recommend that you look at the actual binary layout with a debugger.

Oh the 2 is multiplied not for the encoding. It is because the string is stored
twice, once at model_type_root_ids_ and once at parent_children_map_. I have
moved the calculation to the usage function, that should be clear.

https://codereview.chromium.org/2084243004/diff/110001/sync/syncable/parent_c...
sync/syncable/parent_child_index.cc:132: bytes_used_ += sizeof(EntryKernel*);
On 2016/07/06 21:50:32, stanisc wrote:
> This is a bit simplistic. See this:
> http://info.prelert.com/blog/stl-container-memory-usage
> 
> Also this doesn't take into account the dynamic growth. When you add an entry
in
> the map the underlying storage sometimes doesn't grow at all, and sometimes it
> doubles. That all depends on the implementation of course.

Fixed with more accurate values.

https://codereview.chromium.org/2084243004/diff/110001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2084243004/diff/110001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:56413: + 
<owner>stanisc@chromium.org</owner>
On 2016/07/06 21:50:32, stanisc wrote:
> I am not in Sync team anymore. 

changed to gangwu@

DmitrySkiba

4 years, 4 months ago (2016-08-09 17:44:10 UTC) #18

So as I understand, there are two parts:

(1) protobuf stuff in proto_value_conversions.cc
(2) C++ stuff (mostly in directory.cc)

Which part is bigger? Can we measure that?

As they are now, the changes are pretty heavy. Do we really need to estimate
EntryKernels? Maybe we can skip them and sacrifice some accuracy?

As for (1), it seems that protobuf supports "arenas":
https://developers.google.com/protocol-buffers/docs/reference/arenas, and arena
class has SpaceUsed() method. I would first look into that.

If that doesn't work (or is hard especially around strings), then I can think of
faking 'Value' classes and compiling them twice, first with 'real' Value
classes, and then with 'fake' ones. I.e. given

std::unique_ptr<base::DictionaryValue> AppSettingsToValue(
    const sync_pb::AppNotificationSettings& proto) {
  std::unique_ptr<base::DictionaryValue> value(new base::DictionaryValue());
  SET_BOOL(initial_setup_done);
  SET_BOOL(disabled);
  SET_STR(oauth_client_id);
  return value;
}

we first remove base:: specifier and add a way to override value types:

#if !defined(USING_FAKE_VALUES)
using DictionaryValue = base::DictionaryValue;
#endif

std::unique_ptr<DictionaryValue> AppSettingsToValue(
    const sync_pb::AppNotificationSettings& proto) {
  std::unique_ptr<DictionaryValue> value(new DictionaryValue());
  SET_BOOL(initial_setup_done);
  SET_BOOL(disabled);
  SET_STR(oauth_client_id);
  return value;
}

then in some other .cc file we introduce fake DictionaryValue class and use it:

class DictionaryValue {
 public:
  void* operator new (size_t) { return &instance_; }
  void operator delete(void*) {}

  void SetBoolean(const std::string& path, bool in_value) {}

  void SetString(const std::string& path, const std::string& in_value) {
    memory_size_ += sizeof(std::string) + in_value.capacity();
  }

  static size_t memory_size() { return memory_size_; }

 private:
  static size_t memory_size_;
  static DictionaryValue instance_;
};

#define USING_FAKE_VALUES
#include "proto_value_conversions.cc"

...

AppSettingsToValue(proto);
proto_memory_size = DictionaryValue::memory_size();


This is hacky, but at least the hackery is hidden. We can reduce changes further
(and make it even more hacky) by creating fake 'base' namespace and placing fake
DictionaryValue there. That avoids most changes to proto_value_conversions.cc.

https://codereview.chromium.org/2084243004/diff/170001/sync/syncable/director...
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/2084243004/diff/170001/sync/syncable/director...
sync/syncable/directory.cc:50: size_t GetUnorderedMapMemoryUsage(const
std::unordered_map<Key, Value> map) {
I think you missed & here: const std::unordered_map<>& map.

https://codereview.chromium.org/2084243004/diff/170001/sync/syncable/director...
sync/syncable/directory.cc:957: for (auto entry :
kernel_->index_by_attachment_id)
const auto&, all other places too.

https://codereview.chromium.org/2084243004/diff/170001/sync/syncable/director...
sync/syncable/directory.cc:970: size_t entry_size = 0;
I think this whole thing should be moved into EntryKernel.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Issue 2084243004: [WIP][tracing] Add memory dump provider for sync Directory (Closed)

Description

Patch Set 1 #

Patch Set 2 : Add visitors for memory. #

Patch Set 3 : Add visitors for memory. #

Patch Set 4 : Fixes. #

Messages