[tracing] Adding SkGlyphCache detailed statistics to skia dump provider

[tracing] Adding SkGlyphCache detailed statistics to skia dump provider

This CL adds name and memory usage of each glyph cache attached in the
current process, to skia dump provider. The api exposing the cache
statistics is added in crrev.com/1256323003.

BUG=503168

[ssid, 2015-07-28 16:01:55]

ssid@chromium.org changed reviewers:
+ petrcermak@chromium.org

[ssid, 2015-07-28 16:01:56]

PTAL.

[petrcermak, 2015-07-28 19:40:29]

LGTM with 3 style nits.

Thanks,
Petr

https://codereview.chromium.org/1260753004/diff/1/skia/ext/skia_memory_dump_p...
File skia/ext/skia_memory_dump_provider.cc (right):

https://codereview.chromium.org/1260753004/diff/1/skia/ext/skia_memory_dump_p...
skia/ext/skia_memory_dump_provider.cc:12: #include
"third_party/skia/include/core/SkGlyphCacheStats.h"
nit: I think this should come before the include on line 11 (unless case is
taken into order).

https://codereview.chromium.org/1260753004/diff/1/skia/ext/skia_memory_dump_p...
skia/ext/skia_memory_dump_provider.cc:42:
cache_mad->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
nit: I'd find this more readable if both calls had the same format:

cache_mad->AddScalar(
    base::trace_event::MemoryAllocatorDump::kNameSize,
    base::trace_event::MemoryAllocatorDump::kUnitsBytes,
    memoryUsed);
cache_mad->AddScalar(
    base::trace_event::MemoryAllocatorDump::kNameObjectsCount,
    base::trace_event::MemoryAllocatorDump::kUnitsObjects,
    1u);

https://codereview.chromium.org/1260753004/diff/1/skia/ext/skia_memory_dump_p...
skia/ext/skia_memory_dump_provider.cc:76:
process_memory_dump->CreateAllocatorDump("skia/sk_resource_cache");
nit: I'd factor this dump name out into a constant as well (to be consistent
with kFontCacheDumpName).

[petrcermak, 2015-07-28 19:41:25]

One more nit: Please remove the fullstop from the end of the patch title and the
first line of the description.

Thanks,
Petr

[ssid, 2015-07-28 20:19:11]

Thanks for the review, Petr!

https://codereview.chromium.org/1260753004/diff/1/skia/ext/skia_memory_dump_p...
File skia/ext/skia_memory_dump_provider.cc (right):

https://codereview.chromium.org/1260753004/diff/1/skia/ext/skia_memory_dump_p...
skia/ext/skia_memory_dump_provider.cc:12: #include
"third_party/skia/include/core/SkGlyphCacheStats.h"
On 2015/07/28 19:40:29, petrcermak wrote:
> nit: I think this should come before the include on line 11 (unless case is
> taken into order).

I would expect upload to give me error. Fixed it, thanks

https://codereview.chromium.org/1260753004/diff/1/skia/ext/skia_memory_dump_p...
skia/ext/skia_memory_dump_provider.cc:42:
cache_mad->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
On 2015/07/28 19:40:29, petrcermak wrote:
> nit: I'd find this more readable if both calls had the same format:
> 
> cache_mad->AddScalar(
>     base::trace_event::MemoryAllocatorDump::kNameSize,
>     base::trace_event::MemoryAllocatorDump::kUnitsBytes,
>     memoryUsed);
> cache_mad->AddScalar(
>     base::trace_event::MemoryAllocatorDump::kNameObjectsCount,
>     base::trace_event::MemoryAllocatorDump::kUnitsObjects,
>     1u);

Hm, it was git cl format. I don't want to touch this because next time someone
changes this file, it will rearrange back to this.

https://codereview.chromium.org/1260753004/diff/1/skia/ext/skia_memory_dump_p...
skia/ext/skia_memory_dump_provider.cc:76:
process_memory_dump->CreateAllocatorDump("skia/sk_resource_cache");
On 2015/07/28 19:40:29, petrcermak wrote:
> nit: I'd factor this dump name out into a constant as well (to be consistent
> with kFontCacheDumpName).

Done.

[ssid, 2015-07-28 20:20:05]

ssid@chromium.org changed reviewers:
+ reed@google.com

[ssid, 2015-07-28 20:20:05]

+reed This is the chrome side change for SkGlyphCache statistics. PTAL.