|
|
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/ |