|
|
Chromium Code Reviews
Description[WIP] Exposing memory-infra to Skia.
This is a set of the interfaces that should be exposed to skia and the
implementation of these interfaces will exist in skia/ext. The
implementations will just proxy call or exist as a wrapper around the
corresponsing base classes.
BUG=skia:
Patch Set 1 #Patch Set 2 : Prettier. #
Total comments: 1
Patch Set 3 : Update. #Patch Set 4 : Reduce more. #Patch Set 5 : One more. #Messages
Total messages: 12 (2 generated)
ssid@chromium.org changed reviewers: + bsalomon@google.com, petrcermak@chromium.org, reed@google.com
+reed, +bsalomon This is the set of interfaces that would be needed for getting the memory dumps. these are not complete, but these are sufficient for the current instrumentation plan. If more functionality is needed then it can be included in future. PTAL. https://codereview.chromium.org/1263813002/diff/20001/include/utils/SkMemoryD... File include/utils/SkMemoryDumpManager.h (right): https://codereview.chromium.org/1263813002/diff/20001/include/utils/SkMemoryD... include/utils/SkMemoryDumpManager.h:18: class SK_API SkMemoryDumpManager { These can be made singleton to remove dummy definitions of the functions.
Is this the sort of way to use your api from a single static-function that Skia
will expose?
void SkDumpProcessMemory(SkProcessMemoryDump* dumper) {
SkMemoryAllocatorDump* skia = dumper->createMemoryAllocatorDump("skia");
skia->AddScalar("font-cache", "bytes", ...);
skia->AddScalar("font-cache/strikes", "objects", ...);
skia->AddScalar("resource-cache", "bytes", ...);
delete skia; // do I do this?
}
On 2015/08/11 21:24:18, reed1 wrote:
> Is this the sort of way to use your api from a single static-function that
Skia
> will expose?
>
> void SkDumpProcessMemory(SkProcessMemoryDump* dumper) {
> SkMemoryAllocatorDump* skia = dumper->createMemoryAllocatorDump("skia");
> skia->AddScalar("font-cache", "bytes", ...);
> skia->AddScalar("font-cache/strikes", "objects", ...);
> skia->AddScalar("resource-cache", "bytes", ...);
>
A little different. So, the dump has the tree structure. and the allocator dump
has attributes.
SkMemoryAllocatorDump* font_dump =
dumper->createMemoryAllocatorDump("skia/font_cache");
font_dump->AddScalar("size", "bytes", ...)
font_dump->AddScalar("strikes", "objects", ...)
// and more attributes.
You could also do:
SkMemoryAllocatorDump* font_object_dump =
dumper->createMemoryAllocatorDump("skia/font_cache/name_roman");
font_object_dump->AddScalarF("strike_rate", "ratio", ...);
And for resource cache you will have to create a different dump:
SkMemoryAllocatorDump* resource_dump =
dumper->createMemoryAllocatorDump("skia/resource_cache/resource_[id]");
// similarly add attributes.
The statistics of the children will be aggregated and displayed in the stats of
the ancestors.
https://codereview.chromium.org/1260753004 is an example of how the objects are
dumped.
> delete skia; // do I do this?
> }
createAllocatorDump takes ownership of the dump that it creates, and delete need
not be called. It will be deleted by the manager once it gets added to the
traces.
Also, if you would like to express that the particular dump has been allocated
using malloc. You would do:
SkMemoryAllocatorDump* font_dump = dumper->getAllocatorDump("skia/font_cache");
// Already created.
SkMemoryAllocatorDump* malloc_dump =
dumper->CreateMemoryAllocatorDump("malloc/allocated_objects/skia_[id]");
dumper->AddOwnershipEdge(malloc_dump->guid(), font_dump->guid())
For the case of discardable, we are still discussing with the folks to figure
out what is the correct thing to display. It will be clear by the next week. But
as far as skia is concerned, it is just going to dump all the objects it
allocated. discardable manager will figure out what to display.
ssid@chromium.org changed reviewers: + primiano@chromium.org
+primiano
I might be misaligned here. I suggest we have a quick VC. What is the plan here? The options we have, as discussed in our doc, are: A) Expose the stats that we need from Skia -> Chromium, and do all the dumps on the chrome side B) Expose the chromium tracing infrastructure inside skia with all the plumbing and have essentially a Skia-version of memory infra. I though we were going for the A road, but this CL seems to be doing B. I'm fine with both, this decision is up to Skia owners. My personal suggestion is that A is easier and cleaner to achieve.
Ok I had a chat with ssid and explained me the evolution.
If I understood correctly the stuff we need to dump requires to access skia
internals that we don't want to expose to the public API. So the plan sounds
like: passing some object from chrome->skia when we want to dump memory, and
having skia code firing calls to that object, which SGTM.
I think the only argument of discussion here is: do we need to expose the full
memory-infra APIs form chrome-skia? Or can we have something simpler which fits
our needs (and do the plumbing on the chrome-side?). In other words, maybe we
just need to pass to skia something like:
class SkiaMemoryDump {
virtual void ReportCache(char* name, size_t size, enum
allocated_from_malloc_or_discardable, void* address) = 0;
}
On the other side, if we do need to report more, it might be easier to just
expose 1:1 the memory-infra APIs like ssid is doing here.
I think all boils down to: what are the upcoming needs that GPU folks have to
report Skia GPU memory?
I think that ericrk@ is going to look into that. Let use have a VC with him and
we can resume the discussion on this thread.
On 2015/08/12 11:25:56, Primiano Tucci wrote:
> Ok I had a chat with ssid and explained me the evolution.
> If I understood correctly the stuff we need to dump requires to access skia
> internals that we don't want to expose to the public API. So the plan sounds
> like: passing some object from chrome->skia when we want to dump memory, and
> having skia code firing calls to that object, which SGTM.
> I think the only argument of discussion here is: do we need to expose the full
> memory-infra APIs form chrome-skia? Or can we have something simpler which
fits
> our needs (and do the plumbing on the chrome-side?). In other words, maybe we
> just need to pass to skia something like:
>
> class SkiaMemoryDump {
> virtual void ReportCache(char* name, size_t size, enum
> allocated_from_malloc_or_discardable, void* address) = 0;
> }
>
> On the other side, if we do need to report more, it might be easier to just
> expose 1:1 the memory-infra APIs like ssid is doing here.
>
> I think all boils down to: what are the upcoming needs that GPU folks have to
> report Skia GPU memory?
> I think that ericrk@ is going to look into that. Let use have a VC with him
and
> we can resume the discussion on this thread.
Had a talk with ssid and primiano - I think that ssid's interface mostly handles
what we'd need from the GPU side, with two additions:
- We need a way of creating global GUID ownership edges to the objects created.
This might require some sort of function of the form:
std::string GetGUID(std::string purpose, std::string id);
this function would allow chrome to provide a global GUID for the local skia
ID, which could then be used to create a global ownership edge.
- We'd need a way to call CreateSharedGlobalAllocatorDump to allow these GUIDs
to be used for de-duping memory.
On 2015/08/12 18:11:49, ericrk wrote:
> On 2015/08/12 11:25:56, Primiano Tucci wrote:
> > Ok I had a chat with ssid and explained me the evolution.
> > If I understood correctly the stuff we need to dump requires to access skia
> > internals that we don't want to expose to the public API. So the plan sounds
> > like: passing some object from chrome->skia when we want to dump memory, and
> > having skia code firing calls to that object, which SGTM.
> > I think the only argument of discussion here is: do we need to expose the
full
> > memory-infra APIs form chrome-skia? Or can we have something simpler which
> fits
> > our needs (and do the plumbing on the chrome-side?). In other words, maybe
we
> > just need to pass to skia something like:
> >
> > class SkiaMemoryDump {
> > virtual void ReportCache(char* name, size_t size, enum
> > allocated_from_malloc_or_discardable, void* address) = 0;
> > }
> >
> > On the other side, if we do need to report more, it might be easier to just
> > expose 1:1 the memory-infra APIs like ssid is doing here.
> >
> > I think all boils down to: what are the upcoming needs that GPU folks have
to
> > report Skia GPU memory?
> > I think that ericrk@ is going to look into that. Let use have a VC with him
> and
> > we can resume the discussion on this thread.
>
> Had a talk with ssid and primiano - I think that ssid's interface mostly
handles
> what we'd need from the GPU side, with two additions:
> - We need a way of creating global GUID ownership edges to the objects
created.
> This might require some sort of function of the form:
> std::string GetGUID(std::string purpose, std::string id);
> this function would allow chrome to provide a global GUID for the local skia
> ID, which could then be used to create a global ownership edge.
> - We'd need a way to call CreateSharedGlobalAllocatorDump to allow these GUIDs
> to be used for de-duping memory.
To give an example of what I was thinking for Ganesh resources, see here:
https://codereview.chromium.org/1302663003/ note that this isn't a
ready-to-review patch, just something to give you an idea.
On 2015/08/19 16:47:21, ericrk wrote:
> On 2015/08/12 18:11:49, ericrk wrote:
> > On 2015/08/12 11:25:56, Primiano Tucci wrote:
> > > Ok I had a chat with ssid and explained me the evolution.
> > > If I understood correctly the stuff we need to dump requires to access
skia
> > > internals that we don't want to expose to the public API. So the plan
sounds
> > > like: passing some object from chrome->skia when we want to dump memory,
and
> > > having skia code firing calls to that object, which SGTM.
> > > I think the only argument of discussion here is: do we need to expose the
> full
> > > memory-infra APIs form chrome-skia? Or can we have something simpler which
> > fits
> > > our needs (and do the plumbing on the chrome-side?). In other words, maybe
> we
> > > just need to pass to skia something like:
> > >
> > > class SkiaMemoryDump {
> > > virtual void ReportCache(char* name, size_t size, enum
> > > allocated_from_malloc_or_discardable, void* address) = 0;
> > > }
> > >
> > > On the other side, if we do need to report more, it might be easier to
just
> > > expose 1:1 the memory-infra APIs like ssid is doing here.
> > >
> > > I think all boils down to: what are the upcoming needs that GPU folks have
> to
> > > report Skia GPU memory?
> > > I think that ericrk@ is going to look into that. Let use have a VC with
him
> > and
> > > we can resume the discussion on this thread.
> >
> > Had a talk with ssid and primiano - I think that ssid's interface mostly
> handles
> > what we'd need from the GPU side, with two additions:
> > - We need a way of creating global GUID ownership edges to the objects
> created.
> > This might require some sort of function of the form:
> > std::string GetGUID(std::string purpose, std::string id);
> > this function would allow chrome to provide a global GUID for the local
skia
> > ID, which could then be used to create a global ownership edge.
> > - We'd need a way to call CreateSharedGlobalAllocatorDump to allow these
GUIDs
> > to be used for de-duping memory.
>
> To give an example of what I was thinking for Ganesh resources, see here:
> https://codereview.chromium.org/1302663003/ note that this isn't a
> ready-to-review patch, just something to give you an idea.
Ignore that, responded to the wrong patch.
Message was sent while issue was closed.
Closed and re-addressed in https://codereview.chromium.org/1300103004/ |
