private iterator to visit all resource cache entries

private iterator to visit all resource cache entries

BUG=skia:
TBR=

Committed: https://skia.googlesource.com/skia/+/216b643fc77e754a3fabbb0ed397e7cf55d1954d

[reed1, 2015-08-04 20:05:48]

reed@google.com changed reviewers:
+ bsalomon@google.com, mtklein@google.com

[reed1, 2015-08-04 20:05:48]

as the bottom-half of hooking into chrome's memory tracker...

[mtklein_C, 2015-08-04 20:27:37]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2015-08-04 20:27:37]

seems viable to me

https://codereview.chromium.org/1271033002/diff/1/bench/ImageCacheBench.cpp
File bench/ImageCacheBench.cpp (right):

https://codereview.chromium.org/1271033002/diff/1/bench/ImageCacheBench.cpp#n...
bench/ImageCacheBench.cpp:29: const char* getCategory() const override { return
"imagecachebench-test"; }
Alternatively, defaults on Rec that return "undifferentiated" / NULL?

https://codereview.chromium.org/1271033002/diff/1/samplecode/SampleApp.cpp
File samplecode/SampleApp.cpp (right):

https://codereview.chromium.org/1271033002/diff/1/samplecode/SampleApp.cpp#ne...
samplecode/SampleApp.cpp:1429: SkResourceCache::TestDumpMemoryStatistics();
I take it this stuff is temporary?

https://codereview.chromium.org/1271033002/diff/1/src/core/SkResourceCache.h
File src/core/SkResourceCache.h (right):

https://codereview.chromium.org/1271033002/diff/1/src/core/SkResourceCache.h#...
src/core/SkResourceCache.h:125: typedef void (*Visitor)(const Rec&, void*
context);
It'd be nice to have Visitor and VisitAll right next to each other?

[reed1, 2015-08-07 16:20:00]

https://codereview.chromium.org/1271033002/diff/1/bench/ImageCacheBench.cpp
File bench/ImageCacheBench.cpp (right):

https://codereview.chromium.org/1271033002/diff/1/bench/ImageCacheBench.cpp#n...
bench/ImageCacheBench.cpp:29: const char* getCategory() const override { return
"imagecachebench-test"; }
On 2015/08/04 20:27:37, mtklein_C wrote:
> Alternatively, defaults on Rec that return "undifferentiated" / NULL? 

Acknowledged.

https://codereview.chromium.org/1271033002/diff/1/samplecode/SampleApp.cpp
File samplecode/SampleApp.cpp (right):

https://codereview.chromium.org/1271033002/diff/1/samplecode/SampleApp.cpp#ne...
samplecode/SampleApp.cpp:1429: SkResourceCache::TestDumpMemoryStatistics();
On 2015/08/04 20:27:37, mtklein_C wrote:
> I take it this stuff is temporary?

Yea, just a place-holder until we find a UI way to trigger the dump (and/or dump
after running DM or nanobench)

https://codereview.chromium.org/1271033002/diff/1/src/core/SkResourceCache.h
File src/core/SkResourceCache.h (right):

https://codereview.chromium.org/1271033002/diff/1/src/core/SkResourceCache.h#...
src/core/SkResourceCache.h:125: typedef void (*Visitor)(const Rec&, void*
context);
On 2015/08/04 20:27:37, mtklein_C wrote:
> It'd be nice to have Visitor and VisitAll right next to each other?

Done.

[reed1, 2015-08-07 19:17:28]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-07 19:17:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1271033002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1271033002/20001

[commit-bot: I haz the power, 2015-08-07 19:24:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 19:24:53]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-08-07 19:38:07]

ptal

[reed1, 2015-08-07 19:38:28]

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

[ssid, 2015-08-07 23:42:11]

This looks great. Thanks for the work! These are the statistics we need to add
to the tracing.

https://codereview.chromium.org/1271033002/diff/20001/src/core/SkGlyphCache.cpp
File src/core/SkGlyphCache.cpp (right):

https://codereview.chromium.org/1271033002/diff/20001/src/core/SkGlyphCache.c...
src/core/SkGlyphCache.cpp:423: SkDebugf("GlyphCache [     used    budget ]\n");
We are looking to add these memory statistics to chrome://tracing. Tracing can
trigger periodic dumps and also dumps on-request.

But, I am not clear how this data can be hooked into tracing.

The dump functions added here seem to add data to logs (SkDebugf). Is there a
way to retrieve the log and add it to tracing? Or should we duplicate this code
to add these data to traces?

Even if it can be taken from logs to traces, this format seems very difficult to
parse and add this data to trace json. Could the dump format be made json?

[reed1, 2015-08-11 13:44:19]

https://codereview.chromium.org/1271033002/diff/20001/src/core/SkGlyphCache.cpp
File src/core/SkGlyphCache.cpp (right):

https://codereview.chromium.org/1271033002/diff/20001/src/core/SkGlyphCache.c...
src/core/SkGlyphCache.cpp:423: SkDebugf("GlyphCache [     used    budget ]\n");
On 2015/08/07 23:42:11, ssid wrote:
> We are looking to add these memory statistics to chrome://tracing. Tracing can
> trigger periodic dumps and also dumps on-request.
> 
> But, I am not clear how this data can be hooked into tracing.
> 
> The dump functions added here seem to add data to logs (SkDebugf). Is there a
> way to retrieve the log and add it to tracing? Or should we duplicate this
code
> to add these data to traces?
> 
> Even if it can be taken from logs to traces, this format seems very difficult
to
> parse and add this data to trace json. Could the dump format be made json?

SkGlyphCache::Dump() exists just as a client of the private iterators (and as a
test for them). My suggestion is to create an public function for chrome to
call, that will pass in your tracing-callback-objects, and it will perform a
similar iteration, but dump its info into your API object instead of printf.

[ssid, 2015-08-11 13:52:36]

On 2015/08/11 13:44:19, reed1 wrote:
>
https://codereview.chromium.org/1271033002/diff/20001/src/core/SkGlyphCache.cpp
> File src/core/SkGlyphCache.cpp (right):
> 
>
https://codereview.chromium.org/1271033002/diff/20001/src/core/SkGlyphCache.c...
> src/core/SkGlyphCache.cpp:423: SkDebugf("GlyphCache [     used    budget
]\n");
> On 2015/08/07 23:42:11, ssid wrote:
> > We are looking to add these memory statistics to chrome://tracing. Tracing
can
> > trigger periodic dumps and also dumps on-request.
> > 
> > But, I am not clear how this data can be hooked into tracing.
> > 
> > The dump functions added here seem to add data to logs (SkDebugf). Is there
a
> > way to retrieve the log and add it to tracing? Or should we duplicate this
> code
> > to add these data to traces?
> > 
> > Even if it can be taken from logs to traces, this format seems very
difficult
> to
> > parse and add this data to trace json. Could the dump format be made json?
> 
> SkGlyphCache::Dump() exists just as a client of the private iterators (and as
a
> test for them). My suggestion is to create an public function for chrome to
> call, that will pass in your tracing-callback-objects, and it will perform a
> similar iteration, but dump its info into your API object instead of printf.

Sounds good! Thanks for all your help :) I will send reduce the API that I am
exposing to just "MemoryDump" object that takes in all the dump for the tracing
and use it here.

[bsalomon, 2015-08-11 13:56:10]

+CC Eric Karl

[reed1, 2015-08-11 14:12:30]

On 2015/08/11 13:52:36, ssid wrote:
> On 2015/08/11 13:44:19, reed1 wrote:
> >
>
https://codereview.chromium.org/1271033002/diff/20001/src/core/SkGlyphCache.cpp
> > File src/core/SkGlyphCache.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1271033002/diff/20001/src/core/SkGlyphCache.c...
> > src/core/SkGlyphCache.cpp:423: SkDebugf("GlyphCache [     used    budget
> ]\n");
> > On 2015/08/07 23:42:11, ssid wrote:
> > > We are looking to add these memory statistics to chrome://tracing. Tracing
> can
> > > trigger periodic dumps and also dumps on-request.
> > > 
> > > But, I am not clear how this data can be hooked into tracing.
> > > 
> > > The dump functions added here seem to add data to logs (SkDebugf). Is
there
> a
> > > way to retrieve the log and add it to tracing? Or should we duplicate this
> > code
> > > to add these data to traces?
> > > 
> > > Even if it can be taken from logs to traces, this format seems very
> difficult
> > to
> > > parse and add this data to trace json. Could the dump format be made json?
> > 
> > SkGlyphCache::Dump() exists just as a client of the private iterators (and
as
> a
> > test for them). My suggestion is to create an public function for chrome to
> > call, that will pass in your tracing-callback-objects, and it will perform a
> > similar iteration, but dump its info into your API object instead of printf.
> 
> Sounds good! Thanks for all your help :) I will send reduce the API that I am
> exposing to just "MemoryDump" object that takes in all the dump for the
tracing
> and use it here.

Yes, if you could sketch out what interface(s) you would need to expose, then we
can see how to glue the private iterators into your API.

[ssid, 2015-08-11 18:01:56]

> Yes, if you could sketch out what interface(s) you would need to expose, then
we
> can see how to glue the private iterators into your API.

I have updated the CL https://codereview.chromium.org/1263813002/ with the
minimal interfaces that is required for dumping these data into traces. PTAL,
thanks

[ssid, 2015-08-12 10:18:26]

+primiano

[Primiano Tucci (use gerrit), 2015-08-12 10:36:27]

On 2015/08/12 10:18:26, ssid wrote:
> +primiano

Oh, just came back from vacation, trying to context switch back in.
What is the plan here? I see that this CL is introducing all the internals in
the skia code to visit all the caches, which looks great.
What is the API used by the tracing infrastructure going to look like from our
side?
Will we use a custom visitor? In other words is the SkResourceCache::visitAll
the API that we are going to use from the chrome side to get the details?
Maybe we should have a quick VC just to syncup

[reed1, 2015-08-12 13:58:25]

On 2015/08/12 10:36:27, Primiano Tucci wrote:
> On 2015/08/12 10:18:26, ssid wrote:
> > +primiano
> 
> Oh, just came back from vacation, trying to context switch back in.
> What is the plan here? I see that this CL is introducing all the internals in
> the skia code to visit all the caches, which looks great.
> What is the API used by the tracing infrastructure going to look like from our
> side?
> Will we use a custom visitor? In other words is the SkResourceCache::visitAll
> the API that we are going to use from the chrome side to get the details?
> Maybe we should have a quick VC just to syncup

A custom visitor is exactly the direction I want to support, BUT I want that
visitor code to live in skia, not elsewhere, as the protocal for the visitor is
NOT stable nor public, and I want to be able to freely modify it over time. If
we have a (mostly) stable callback API from chrome (e.g. the
SkProcessMemoryDump) to pour the data into, I should easily be able to keep that
glue up to date as we evolve skia's internals.

[Primiano Tucci (use gerrit), 2015-08-12 18:19:30]

> A custom visitor is exactly the direction I want to support, BUT I want that
> visitor code to live in skia, not elsewhere, as the protocal for the visitor
is
> NOT stable nor public, and I want to be able to freely modify it over time. If
> we have a (mostly) stable callback API from chrome (e.g. the
> SkProcessMemoryDump) to pour the data into, I should easily be able to keep
that
> glue up to date as we evolve skia's internals.


LGTM. I think we all agree that the knowledge about what to dump (kind of
objects, columns etc) should stay inside skia and should NOT be exported outside
to Chrome.
The thing we need to still decide is how the Skia->Chrome interface to actually
dump the values is going to look like. We are brainstorming that right now.
But that decision is orthogonal to this CL. Regardless of how that interface
will be designed, that will carry only strings and ints from skia -> Chrome, in
a more or less elegant form. All the visitor infrastructure here in this CL
holds anyways. It's just a matter of how the code that we'll put in the visitor
(inside skia) is going to look like.

Does it make sense?

[reed1, 2015-08-19 19:15:52]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-08-19 19:16:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1271033002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1271033002/20001

[commit-bot: I haz the power, 2015-08-19 19:25:43]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/216b643fc77e754a3fabbb0ed397e7cf55d1954d