Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1300103004:
Introduce interface for memory dumps (Closed)
|
Created:
5 years, 4 months ago by Primiano Tucci (use gerrit) Modified:
5 years, 4 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionIntroduce interface for memory dumps
BUG=chromium:503168
Committed: https://skia.googlesource.com/skia/+/9a5bd7e860a6b132146e0247c0da0a1510225b97
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Move to include/core + use const char* + nits #
Total comments: 4
Patch Set 3 : Re reed1 @ #21 #
Messages
Total messages: 29 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
ericrk@chromium.org changed reviewers: + ericrk@chromium.org
Looks good - a number of nits, but nothing major. https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... File src/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... src/core/SkTraceMemoryDump.h:27: /** skia uses 4 space indent. https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... src/core/SkTraceMemoryDump.h:40: virtual void DumpScalarValue(const SkString &dumpName, const char *name, Skia uses different coding style - lowercase for member fns I believe (dumpScalarValue)? https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... src/core/SkTraceMemoryDump.h:49: virtual void SetMemoryBacking(const SkString &dumpName, * and & appear to bind left in skia - "const SkString& dumpName" https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... src/core/SkTraceMemoryDump.h:61: virtual ~SkTraceMemoryDump() = 0; {}? Other cases (SkDiscardable) appear to inline this. https://codereview.chromium.org/1300103004/diff/60001/tests/TraceMemoryDumpTe... File tests/TraceMemoryDumpTest.cpp (right): https://codereview.chromium.org/1300103004/diff/60001/tests/TraceMemoryDumpTe... tests/TraceMemoryDumpTest.cpp:16: SkTraceMemoryDump::~SkTraceMemoryDump() {} This is a bit odd? shouldn't be needed if you inline the destructor - which we seem to do in other cases. https://codereview.chromium.org/1300103004/diff/60001/tests/TraceMemoryDumpTe... tests/TraceMemoryDumpTest.cpp:21: virtual ~TestSkTraceMemoryDump() {} override not virtual?
reed@google.com changed reviewers: + reed@google.com
Why the mix of SkString or const char* as parameter types? Can they all be const char* (that seems simpler, and doesn't rely on any specifics of SkString).
On 2015/08/19 13:00:30, reed1 wrote:
> Why the mix of SkString or const char* as parameter types? Can they all be
const
> char* (that seems simpler, and doesn't rely on any specifics of SkString).
The reason is the following:
the column "name" is expected to be known at compile time (the const char*),
e.g. "size" "bytes" etc. This is a pretty wide convention in the tracing world.
The dumpName, instead, is not. I expect that the code inside the skia dumpers
will make those up on the stack, e.g. something like
string dump_name = SkSprintf("skia/obj%x/id_%d", ...)
DumpScalarValue(dump_name, "size", "bytes")
how would I pass dump_name? I could make the dump_name a const char* anyways,
and take care of copying it on the embedder side.
The awkward thing in that case would be than that the API doesn't make clear
which arguments are copied and which are supposed to be long lived.
So, I thought that passing a const string was a good way to make it explicit
what is copied and what is supposed to be long-lived from the caller viewpoint.
If SkString is an issue can we pass const std::string&?
The other option would be passing const char* everywhere and just add to the
comment (dump_name is copied, but not name and units).
Which is still fine to me, but I find a bit less clear from an API viewpoint
On 2015/08/19 13:08:48, Primiano Tucci wrote:
> On 2015/08/19 13:00:30, reed1 wrote:
> > Why the mix of SkString or const char* as parameter types? Can they all be
> const
> > char* (that seems simpler, and doesn't rely on any specifics of SkString).
>
> The reason is the following:
> the column "name" is expected to be known at compile time (the const char*),
> e.g. "size" "bytes" etc. This is a pretty wide convention in the tracing
world.
> The dumpName, instead, is not. I expect that the code inside the skia dumpers
> will make those up on the stack, e.g. something like
> string dump_name = SkSprintf("skia/obj%x/id_%d", ...)
> DumpScalarValue(dump_name, "size", "bytes")
>
> how would I pass dump_name? I could make the dump_name a const char* anyways,
> and take care of copying it on the embedder side.
> The awkward thing in that case would be than that the API doesn't make clear
> which arguments are copied and which are supposed to be long lived.
> So, I thought that passing a const string was a good way to make it explicit
> what is copied and what is supposed to be long-lived from the caller
viewpoint.
> If SkString is an issue can we pass const std::string&?
>
> The other option would be passing const char* everywhere and just add to the
> comment (dump_name is copied, but not name and units).
> Which is still fine to me, but I find a bit less clear from an API viewpoint
Hmmm. const SkString& also implies that the receiver must make a copy if they
want the string to live longer than their stack-frame, since it does not have a
ref() method. To me, assuming that some const char* parameters are NOT copied
feels like the exception. I wonder if those could be a different type (almost
like an enum) since that seems to be the intent.
Either way, I think I would prefer to not use SkString, at least for now.
1. I think this new header will end up in include/core since it will need to be subclassed by clients. 2. I had envisioned only one entry-point to invoke this for skia... e.g. void SkGraphics::DumpMemoryTrace(SkTraceMemoryDumper*); Is that what you also had in mind?
On 2015/08/19 13:19:21, reed1 wrote: > 1. I think this new header will end up in include/core since it will need to be > subclassed by clients. > > 2. I had envisioned only one entry-point to invoke this for skia... > > e.g. > > void SkGraphics::DumpMemoryTrace(SkTraceMemoryDumper*); > > Is that what you also had in mind? re #2 I think for all software components, one entry point makes sense. For Gpu resources, I don't think there's any singleton managing all resources, so we'll likely need to dump each GrContext (and through it each GrResourceCache) via an entry point on GrContext? Does this make sense to you in the Ganesh case?
reed@google.com changed reviewers: + bsalomon@google.com
On 2015/08/19 15:49:28, ericrk wrote: > On 2015/08/19 13:19:21, reed1 wrote: > > 1. I think this new header will end up in include/core since it will need to > be > > subclassed by clients. > > > > 2. I had envisioned only one entry-point to invoke this for skia... > > > > e.g. > > > > void SkGraphics::DumpMemoryTrace(SkTraceMemoryDumper*); > > > > Is that what you also had in mind? > > re #2 > I think for all software components, one entry point makes sense. For Gpu > resources, I don't think there's any singleton managing all resources, so we'll > likely need to dump each GrContext (and through it each GrResourceCache) via an > entry point on GrContext? Does this make sense to you in the Ganesh case? 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.
reed@google.com changed reviewers: + robertphillips@google.com
I don't know how the caller (chrome) can "find" all of the GrContexts. If that is already solved, great. If not, I was assuming the single Skia entry-point would create some global list of them (privately) and then call the Dumper on each.
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Thanks for the comments. I updated the patch: - moved the interface to include/core - fixed nits / indentation. - Made all the arguments const char*, highlighting the two that are not copied. > I wonder if those could be a different type (almost like an enum) since that seems to be the intent. Correct, those are "almost" like an enum. Very practically these define the column names, which often are fixed. All boils down to a design decision in tracing where those args are not copied for perf reasons. The only reason why I am not making these enums here is because if we need an extra column right now we can just pass an extra string and that will be just fine. If those are enums, having an extra column ends up in a multi-side patch... more boilerplate. I went for documenting the non-copied exception of (valueName, and units) in their docstrings. Defer to ericrk@ the discussion on entry points, as he has a more clear idea than me about where this will be used from. https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... File src/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... src/core/SkTraceMemoryDump.h:27: /** On 2015/08/19 12:45:28, ericrk wrote: > skia uses 4 space indent. Ah, I guess I was relying too much on clang-format :) https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... src/core/SkTraceMemoryDump.h:40: virtual void DumpScalarValue(const SkString &dumpName, const char *name, On 2015/08/19 12:45:28, ericrk wrote: > Skia uses different coding style - lowercase for member fns I believe > (dumpScalarValue)? Done. https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... src/core/SkTraceMemoryDump.h:49: virtual void SetMemoryBacking(const SkString &dumpName, On 2015/08/19 12:45:28, ericrk wrote: > * and & appear to bind left in skia - "const SkString& dumpName" Done. https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... src/core/SkTraceMemoryDump.h:61: virtual ~SkTraceMemoryDump() = 0; On 2015/08/19 12:45:28, ericrk wrote: > {}? Other cases (SkDiscardable) appear to inline this. Done. https://codereview.chromium.org/1300103004/diff/60001/tests/TraceMemoryDumpTe... File tests/TraceMemoryDumpTest.cpp (right): https://codereview.chromium.org/1300103004/diff/60001/tests/TraceMemoryDumpTe... tests/TraceMemoryDumpTest.cpp:16: SkTraceMemoryDump::~SkTraceMemoryDump() {} On 2015/08/19 12:45:28, ericrk wrote: > This is a bit odd? shouldn't be needed if you inline the destructor - which we > seem to do in other cases. Done. https://codereview.chromium.org/1300103004/diff/60001/tests/TraceMemoryDumpTe... tests/TraceMemoryDumpTest.cpp:21: virtual ~TestSkTraceMemoryDump() {} On 2015/08/19 12:45:28, ericrk wrote: > override not virtual? Done.
On 2015/08/20 08:48:01, Primiano Tucci wrote: > Thanks for the comments. > I updated the patch: > - moved the interface to include/core > - fixed nits / indentation. > - Made all the arguments const char*, highlighting the two that are not copied. > > > I wonder if those could be a different type (almost like an enum) since that > seems to be the intent. > Correct, those are "almost" like an enum. Very practically these define the > column names, which often are fixed. > All boils down to a design decision in tracing where those args are not copied > for perf reasons. > > The only reason why I am not making these enums here is because if we need an > extra column right now we can just pass an extra string and that will be just > fine. If those are enums, having an extra column ends up in a multi-side > patch... more boilerplate. > I went for documenting the non-copied exception of (valueName, and units) in > their docstrings. > > Defer to ericrk@ the discussion on entry points, as he has a more clear idea > than me about where this will be used from. > > https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... > File src/core/SkTraceMemoryDump.h (right): > > https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... > src/core/SkTraceMemoryDump.h:27: /** > On 2015/08/19 12:45:28, ericrk wrote: > > skia uses 4 space indent. > > Ah, I guess I was relying too much on clang-format :) > > https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... > src/core/SkTraceMemoryDump.h:40: virtual void DumpScalarValue(const SkString > &dumpName, const char *name, > On 2015/08/19 12:45:28, ericrk wrote: > > Skia uses different coding style - lowercase for member fns I believe > > (dumpScalarValue)? > > Done. > > https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... > src/core/SkTraceMemoryDump.h:49: virtual void SetMemoryBacking(const SkString > &dumpName, > On 2015/08/19 12:45:28, ericrk wrote: > > * and & appear to bind left in skia - "const SkString& dumpName" > > Done. > > https://codereview.chromium.org/1300103004/diff/60001/src/core/SkTraceMemoryD... > src/core/SkTraceMemoryDump.h:61: virtual ~SkTraceMemoryDump() = 0; > On 2015/08/19 12:45:28, ericrk wrote: > > {}? Other cases (SkDiscardable) appear to inline this. > > Done. > > https://codereview.chromium.org/1300103004/diff/60001/tests/TraceMemoryDumpTe... > File tests/TraceMemoryDumpTest.cpp (right): > > https://codereview.chromium.org/1300103004/diff/60001/tests/TraceMemoryDumpTe... > tests/TraceMemoryDumpTest.cpp:16: SkTraceMemoryDump::~SkTraceMemoryDump() {} > On 2015/08/19 12:45:28, ericrk wrote: > > This is a bit odd? shouldn't be needed if you inline the destructor - which we > > seem to do in other cases. > > Done. > > https://codereview.chromium.org/1300103004/diff/60001/tests/TraceMemoryDumpTe... > tests/TraceMemoryDumpTest.cpp:21: virtual ~TestSkTraceMemoryDump() {} > On 2015/08/19 12:45:28, ericrk wrote: > > override not virtual? > > Done. The Chrome side knows about and is able to dump all of the GrContexts without much issue. I actually think we have to do this from the chrome side, as that is where we own the locks that are held around multi-threaded GrContext use, so Skia itself wouldn't be able to safely iterate the GrContexts from one thread? Unless GrContext is more thread-safe than I realize? In any event, we can do this from chrome pretty easily at the moment (assuming a "onMemoryDump" type entry point on GrContext), so if that works for you, I think it should be fine. Thanks!
https://codereview.chromium.org/1300103004/diff/120001/include/core/SkTraceMe... File include/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1300103004/diff/120001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:14: class SkString; unneeded https://codereview.chromium.org/1300103004/diff/120001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:38: virtual void dumpScalarValue(const char* dumpName, trival nit: Skia uses the word Scalar very specifically to mean a float or double (see SkScalar). We can leave this here, but it might cause less confusion if this was dumpNumericValue or dumpSingleValue or something like that.
Agreed about GrContexts -- we will expose a way to "dump" a single context, and Chrome will have to manage finding and safely (from a thread-p.o.v.) calling them.
https://codereview.chromium.org/1300103004/diff/120001/include/core/SkTraceMe... File include/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1300103004/diff/120001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:14: class SkString; On 2015/08/20 14:40:48, reed1 wrote: > unneeded Oh good catch. done. https://codereview.chromium.org/1300103004/diff/120001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:38: virtual void dumpScalarValue(const char* dumpName, On 2015/08/20 14:40:48, reed1 wrote: > trival nit: > > Skia uses the word Scalar very specifically to mean a float or double (see > SkScalar). > > We can leave this here, but it might cause less confusion if this was > dumpNumericValue or dumpSingleValue or something like that. Oh I see. dumpNumericValue SGTM
lgtm
On 2015/08/20 14:46:31, reed1 wrote: > lgtm Thanks. I see that there are other reviewers looped in here. Should I wait for them or can I just tick commit?
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1300103004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1300103004/140001
Message was sent while issue was closed.
Committed patchset #3 (id:140001) as https://skia.googlesource.com/skia/+/9a5bd7e860a6b132146e0247c0da0a1510225b97
Message was sent while issue was closed.
On 2015/08/20 15:00:34, commit-bot: I haz the power wrote: > Committed patchset #3 (id:140001) as > https://skia.googlesource.com/skia/+/9a5bd7e860a6b132146e0247c0da0a1510225b97 I'll take that as a yes, thanks ;-) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
