Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1652053004:
Add Histogram Macros to Skia (Closed)
DescriptionAdd Histogram Macros to Skia
Adds a set of histogram macros to Skia, modeled after Chrome's
UMA_HISTOGRAM_* macros. These allow logging of high frequency events,
and are useful to analyze real world usage of certain features.
By default, these macros are no-ops. Users can provide a custom
header file which defines these macros if they wish to collect
histogram data. Chrome will provide such a header.
I've currently only added two macros:
- SK_HISTOGRAM_BOOLEAN - logs a true/false type relationship (whether
we are tiling a texture or not on each draw).
- SK_HISTOGRAM_ENUMERATION - logs a set of potential values (which of
a number of choices were selected for the texture upload path).
We could add more unused macros at the moment, but it seems easier to
add these as needed, WDYT?
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1652053004
Committed: https://skia.googlesource.com/skia/+/369e9375a3ab7bb56580fc6b22690a76ad759240
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : Remove standalone header, move defines to skUserConfig #
Total comments: 2
Patch Set 4 : enum ordering change #Patch Set 5 : Fix unused constant error in release w/ enum-based constant #
Total comments: 1
Messages
Total messages: 36 (17 generated)
Description was changed from ========== Add Histogram Macros to Skia Adds a set of histogram macros to Skia, modeled after Chrome's UMA_HISTOGRAM_* macros. These allow logging of high frequency events, and are useful to analyze real world usage of certain features. By default, these macros are no-ops. Users can provide a custom header file which defines these macros if they wish to collect histogram data. Chrome will provide such a header. I've currently only added two macros: - SK_HISTOGRAM_BOOLEAN - logs a true/false type relationship (whether we are tiling a texture or not on each draw). - SK_HISTOGRAM_ENUMERATION - logs a set of potential values (which of a number of choices were selected for the texture upload path). We could add more unused macros at the moment, but it seems easier to add these as needed, WDYT? BUG=skia: ========== to ========== Add Histogram Macros to Skia Adds a set of histogram macros to Skia, modeled after Chrome's UMA_HISTOGRAM_* macros. These allow logging of high frequency events, and are useful to analyze real world usage of certain features. By default, these macros are no-ops. Users can provide a custom header file which defines these macros if they wish to collect histogram data. Chrome will provide such a header. I've currently only added two macros: - SK_HISTOGRAM_BOOLEAN - logs a true/false type relationship (whether we are tiling a texture or not on each draw). - SK_HISTOGRAM_ENUMERATION - logs a set of potential values (which of a number of choices were selected for the texture upload path). We could add more unused macros at the moment, but it seems easier to add these as needed, WDYT? BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add Histogram Macros to Skia Adds a set of histogram macros to Skia, modeled after Chrome's UMA_HISTOGRAM_* macros. These allow logging of high frequency events, and are useful to analyze real world usage of certain features. By default, these macros are no-ops. Users can provide a custom header file which defines these macros if they wish to collect histogram data. Chrome will provide such a header. I've currently only added two macros: - SK_HISTOGRAM_BOOLEAN - logs a true/false type relationship (whether we are tiling a texture or not on each draw). - SK_HISTOGRAM_ENUMERATION - logs a set of potential values (which of a number of choices were selected for the texture upload path). We could add more unused macros at the moment, but it seems easier to add these as needed, WDYT? BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add Histogram Macros to Skia Adds a set of histogram macros to Skia, modeled after Chrome's UMA_HISTOGRAM_* macros. These allow logging of high frequency events, and are useful to analyze real world usage of certain features. By default, these macros are no-ops. Users can provide a custom header file which defines these macros if they wish to collect histogram data. Chrome will provide such a header. I've currently only added two macros: - SK_HISTOGRAM_BOOLEAN - logs a true/false type relationship (whether we are tiling a texture or not on each draw). - SK_HISTOGRAM_ENUMERATION - logs a set of potential values (which of a number of choices were selected for the texture upload path). We could add more unused macros at the moment, but it seems easier to add these as needed, WDYT? BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
ericrk@chromium.org changed reviewers: + bsalomon@google.com
This is a simple attempt to get some amount of UMA histogram logging functionality into Skia. Wasn't sure what the best way to handle this was (skia file placement, etc...) but saw the optional header pattern other places and it seemed to fit the simple macro nature of the UMA_HISTOGRAM_* macros in chrome. Let me know what you think.
bsalomon@google.com changed reviewers: + reed@google.com
Adding reed@ as I think he will want to have a say in this. https://codereview.chromium.org/1652053004/diff/20001/include/config/SkUserCo... File include/config/SkUserConfig.h (right): https://codereview.chromium.org/1652053004/diff/20001/include/config/SkUserCo... include/config/SkUserConfig.h:162: //#define SK_HISTOGRAM_LOGGING_CUSTOM_SETUP_HEADER I believe Chrome already uses a custom SkUseConfig file. Perhaps they could override the macros there and Skia could set the defaults in SkPostConfig.h?
https://codereview.chromium.org/1652053004/diff/20001/include/config/SkUserCo... File include/config/SkUserConfig.h (right): https://codereview.chromium.org/1652053004/diff/20001/include/config/SkUserCo... include/config/SkUserConfig.h:162: //#define SK_HISTOGRAM_LOGGING_CUSTOM_SETUP_HEADER On 2016/02/01 21:18:08, bsalomon wrote: > I believe Chrome already uses a custom SkUseConfig file. Perhaps they could > override the macros there and Skia could set the defaults in SkPostConfig.h? hmmm... yeah, Chrome has an SkUserConfig file, that's where I was putting the SK_HISTOGRAM_LOGGING_CUSTOM_SETUP_HEADER define... I can put the macros there directly, but that ends up pulling more #includes into SkUserConfig, forcing every file in Skia to pull in the histogram headers for chrome. This approach overrides the values in a specific include file, so only those files that make use of the histogram macros need to pay the cost of the include... not sure if it really matters though, and I'm happy with either option - let me know what you think.
On 2016/02/01 21:58:33, ericrk wrote: > https://codereview.chromium.org/1652053004/diff/20001/include/config/SkUserCo... > File include/config/SkUserConfig.h (right): > > https://codereview.chromium.org/1652053004/diff/20001/include/config/SkUserCo... > include/config/SkUserConfig.h:162: //#define > SK_HISTOGRAM_LOGGING_CUSTOM_SETUP_HEADER > On 2016/02/01 21:18:08, bsalomon wrote: > > I believe Chrome already uses a custom SkUseConfig file. Perhaps they could > > override the macros there and Skia could set the defaults in SkPostConfig.h? > > hmmm... yeah, Chrome has an SkUserConfig file, that's where I was putting the > SK_HISTOGRAM_LOGGING_CUSTOM_SETUP_HEADER define... I can put the macros there > directly, but that ends up pulling more #includes into SkUserConfig, forcing > every file in Skia to pull in the histogram headers for chrome. This approach > overrides the values in a specific include file, so only those files that make > use of the histogram macros need to pay the cost of the include... not sure if > it really matters though, and I'm happy with either option - let me know what > you think. Let's go with Chrome's SkUserConfig.h file #defining your macros, directly or indirectly by #including some other file. Every file in Skia #includes tons of files. #include is quite convenient. We won't ever notice the difference the cost of the extra #include.
On 2016/02/01 22:00:24, mtklein wrote: > On 2016/02/01 21:58:33, ericrk wrote: > > > https://codereview.chromium.org/1652053004/diff/20001/include/config/SkUserCo... > > File include/config/SkUserConfig.h (right): > > > > > https://codereview.chromium.org/1652053004/diff/20001/include/config/SkUserCo... > > include/config/SkUserConfig.h:162: //#define > > SK_HISTOGRAM_LOGGING_CUSTOM_SETUP_HEADER > > On 2016/02/01 21:18:08, bsalomon wrote: > > > I believe Chrome already uses a custom SkUseConfig file. Perhaps they could > > > override the macros there and Skia could set the defaults in SkPostConfig.h? > > > > hmmm... yeah, Chrome has an SkUserConfig file, that's where I was putting the > > SK_HISTOGRAM_LOGGING_CUSTOM_SETUP_HEADER define... I can put the macros there > > directly, but that ends up pulling more #includes into SkUserConfig, forcing > > every file in Skia to pull in the histogram headers for chrome. This approach > > overrides the values in a specific include file, so only those files that make > > use of the histogram macros need to pay the cost of the include... not sure if > > it really matters though, and I'm happy with either option - let me know what > > you think. > > Let's go with Chrome's SkUserConfig.h file #defining your macros, directly or > indirectly by #including some other file. > Every file in Skia #includes tons of files. #include is quite convenient. We > won't ever notice the difference the cost of the extra #include. thanks for the context - I'll update.
mtklein@google.com changed reviewers: + mtklein@google.com
I have many questions about how this will end up used. I don't want to hold up any code landing... just curious. https://codereview.chromium.org/1652053004/diff/40001/include/config/SkUserCo... File include/config/SkUserConfig.h (right): https://codereview.chromium.org/1652053004/diff/40001/include/config/SkUserCo... include/config/SkUserConfig.h:159: * integrate with their histogram collection backend. What are the rules of evolving these things and breaking Chrome? Can we just change their types and value-range willy nilly? E.g. can we upgrade a bool to an enum? Can we add more fields to an enum without breaking Chrome? Does changing a field's name matter? Does the data logged to these fields end up in some sort of query interface? What other sorts of histograms does Chrome support (linear bucketing and exponential bucketing for floats?), and how evolvable are they? Do the strings we pass for name need to be literals, or just compile time constants, or any char*? E.g. can we pass SkString::c_str() as a temporary?
lgtm
I'm fine as a first step, but I'd feel much better knowing how skia can test this itself, so we don't bitrot this code which only gets exercised by 1 client. Also, I'd feel better if/when we know if this structure (which appears to rely on some hidden globals?) matches the sort of histograms we were imagining we wanted to build ourselves.
On 2016/02/02 18:42:46, reed1 wrote: > I'm fine as a first step, but I'd feel much better knowing how skia can test > this itself, so we don't bitrot this code which only gets exercised by 1 client. > > Also, I'd feel better if/when we know if this structure (which appears to rely > on some hidden globals?) matches the sort of histograms we were imagining we > wanted to build ourselves. Yeah, I was thinking we should have a Skia-test-tools implementation of these, and to make sure we can use this histogram/stat framework too.
responded to mtklein's comments, which I think should address some of the other questions as well. I'm happy to help with a test tools implementation of this. Finally, would you prefer something else (a singleton interface or something) rather than macros? I just went with macros as that lines up with the Chrome patterns. https://codereview.chromium.org/1652053004/diff/40001/include/config/SkUserCo... File include/config/SkUserConfig.h (right): https://codereview.chromium.org/1652053004/diff/40001/include/config/SkUserCo... include/config/SkUserConfig.h:159: * integrate with their histogram collection backend. On 2016/02/02 00:55:23, mtklein wrote: > What are the rules of evolving these things and breaking Chrome? > > Can we just change their types and value-range willy nilly? > E.g. can we upgrade a bool to an enum? Can we add more fields to an enum > without breaking Chrome? Does changing a field's name matter? > Chrome does have tables which assign each metric an owner, a type, as well as a value to string mapping (in the case of enums). If a metric doesn't line up with this table, stats may stop being collected (although chrome shouldn't experience any runtime issues). It's also ok for stats to not appear in this table - in this case they will still be collected locally (and viewable in chrome://histograms), but won't be uploaded. To work around this, I'd probably recommend that changes to values in Skia typically come with a name change - this will de-couple changes in Skia/Chrome, allowing update CLs to land separately with no risk of breakage or need for tight coordination. Changes in Skia will still be testable and should work fine, they just won't be uploaded to Chrome's servers until a corresponding Chrome change goes in. > Does the data logged to these fields end up in some sort of query interface? Yup - google internal only though - will send you a link. > What other sorts of histograms does Chrome support (linear bucketing and > exponential bucketing for floats?), and how evolvable are they? See chrome's base/metrics/histogram_macros.h for a full list, but to give an overview, chrome has: - times - used for logging the time something takes - customizable min/max range used for bucketing. - counts - used for logging the number of times something happens - allows for min/max - percentages - used for logging percentages :P - bucketized as integers 1-100 Currently no real float or exponential bucketing, although you could convert floats to large ints and go from there. > > Do the strings we pass for name need to be literals, or just compile time > constants, or any char*? E.g. can we pass SkString::c_str() as a temporary? The function eventually allows for either const std::string& or const char*. The function does not use the value past the lifetime of the call, nor does it assume it's the same every time - feel free to use any char*, even a temporary one.
On 2016/02/02 18:42:46, reed1 wrote: > I'm fine as a first step, but I'd feel much better knowing how skia can test > this itself, so we don't bitrot this code which only gets exercised by 1 client. > > Also, I'd feel better if/when we know if this structure (which appears to rely > on some hidden globals?) matches the sort of histograms we were imagining we > wanted to build ourselves. Do you have info on what you were planning on building? Hopefully we can come up with an abstraction that lines up with both Chrome + your plans.
On 2016/02/02 19:37:39, ericrk wrote: > On 2016/02/02 18:42:46, reed1 wrote: > > I'm fine as a first step, but I'd feel much better knowing how skia can test > > this itself, so we don't bitrot this code which only gets exercised by 1 > client. > > > > Also, I'd feel better if/when we know if this structure (which appears to rely > > on some hidden globals?) matches the sort of histograms we were imagining we > > wanted to build ourselves. > > Do you have info on what you were planning on building? Hopefully we can come up > with an abstraction that lines up with both Chrome + your plans. As it sounds like everyone's OK with this as a first step, I'm going to submit so we can start collecting some data. If we'd like to iterate on this or add a test implementation in Skia, I'm very happy to help. Please let me know what you'd like to see from a Skia histogram API and I'll see if it aligns with this. If not, I'll send out a follow-up CL.
The CQ bit was checked by ericrk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1652053004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1652053004/40001
The CQ bit was unchecked by ericrk@chromium.org
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1652053004/#ps100001 (title: "enum ordering change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1652053004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1652053004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
https://codereview.chromium.org/1652053004/diff/120001/src/core/SkImageCacher... File src/core/SkImageCacherator.cpp (right): https://codereview.chromium.org/1652053004/diff/120001/src/core/SkImageCacher... src/core/SkImageCacherator.cpp:254: enum { kLockTexturePathCount = kRGBA_LockTexturePath + 1 }; This appears to be the style in Skia anyway for constants - happens to avoid the issues that, when SK_HISTOGRAM_ENUMERATION is undefined, this is an unused var.
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1652053004/#ps120001 (title: "Fix unused constant error in release w/ enum-based constant")
The CQ bit was unchecked by ericrk@chromium.org
The CQ bit was checked by ericrk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1652053004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1652053004/120001
Message was sent while issue was closed.
Description was changed from ========== Add Histogram Macros to Skia Adds a set of histogram macros to Skia, modeled after Chrome's UMA_HISTOGRAM_* macros. These allow logging of high frequency events, and are useful to analyze real world usage of certain features. By default, these macros are no-ops. Users can provide a custom header file which defines these macros if they wish to collect histogram data. Chrome will provide such a header. I've currently only added two macros: - SK_HISTOGRAM_BOOLEAN - logs a true/false type relationship (whether we are tiling a texture or not on each draw). - SK_HISTOGRAM_ENUMERATION - logs a set of potential values (which of a number of choices were selected for the texture upload path). We could add more unused macros at the moment, but it seems easier to add these as needed, WDYT? BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add Histogram Macros to Skia Adds a set of histogram macros to Skia, modeled after Chrome's UMA_HISTOGRAM_* macros. These allow logging of high frequency events, and are useful to analyze real world usage of certain features. By default, these macros are no-ops. Users can provide a custom header file which defines these macros if they wish to collect histogram data. Chrome will provide such a header. I've currently only added two macros: - SK_HISTOGRAM_BOOLEAN - logs a true/false type relationship (whether we are tiling a texture or not on each draw). - SK_HISTOGRAM_ENUMERATION - logs a set of potential values (which of a number of choices were selected for the texture upload path). We could add more unused macros at the moment, but it seems easier to add these as needed, WDYT? BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/369e9375a3ab7bb56580fc6b22690a76ad759240 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://skia.googlesource.com/skia/+/369e9375a3ab7bb56580fc6b22690a76ad759240 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
