Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1726873002:
Report histogram creation results. (Closed)
|
Created:
4 years, 10 months ago by bcwhite Modified:
4 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReport histogram creation results.
Add a new histogram that reports information about all the
histograms created in different procesesses, their types,
and what flags were set.
BUG=546019
TBR=grt
grt: single instantiating call in installer_metrics.cc
Committed: https://crrev.com/d723c25e72442b9381023224d3db0ed12ae3210d
Cr-Commit-Position: refs/heads/master@{#381482}
Patch Set 1 #Patch Set 2 : fix histogram name generation to be cross-platform #Patch Set 3 : fixed inclusion of looked-up histograms; add report to setup; extracted common code #Patch Set 4 : some 'git cl format' changes #
Total comments: 34
Patch Set 5 : addressed review comments by Alexei #Patch Set 6 : keep test StatisticsRecorder in a scoped_ptr #Patch Set 7 : added message for why not using macro #
Total comments: 10
Patch Set 8 : addressed review comments by Alexei #Patch Set 9 : addressed final review comments by Alexei #Patch Set 10 : removed unnecessary include #
Total comments: 10
Patch Set 11 : addressed review comments by Greg #Patch Set 12 : rebased #
Messages
Total messages: 34 (10 generated)
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_base.cc (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base.cc:130: if (existing) { Nit: != 0 https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base.cc:193: histogram->GetHistogramType()); This is a bit fragile - if the histogram type ever changes (a new value is added), then this histogram gets broken. I suggest just doing a switch on the histogram type and mapping to the value. This way, if a new type is added, then there would be a compile error if there's a new enum that's not handled in the switch - which ensures that the histogram will be updated. https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_base.h (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base.h:254: // Update the creation (or lookup) report for a histogram. Document the |created| param, please. Also I think it's slightly confusing that the function is called "UpdateReportHistogram" and takes "histogram" as a param - at first read it would sound like that's the histogram being updated. Can you rework things to avoid this possible confusion? https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_base_unittest.cc (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base_unittest.cc:31: statistics_recorder_ = new StatisticsRecorder(); Nit: While you're changing this, can you make this a scoped_ptr? https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base_unittest.cc:41: }; Nit: Add DISALLOW_COPY_AND_ASSIGN(). https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_persistence.cc (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_persistence.cc:192: if (existing) { Nit: != 0 for checking numbers. https://codereview.chromium.org/1726873002/diff/60001/base/metrics/statistics... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/statistics... base/metrics/statistics_recorder.cc:372: size_t StatisticsRecorder::NumberOfHistograms() { Nit: GetHistogramCount() https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... chrome/installer/setup/installer_metrics.cc:27: base::HistogramBase::EnableCreationReportHistogram("setup"); I find it unfortunate that we need an extra call (and an extra string param). Is there any way to make the SetPersistentHistogramMemoryAllocator() call do this internally w/ the allocator name string that's passed? Maybe there could be a param to it that specifies whether it should do this or not - if we don't always want it. A second question - why do we need it be a separate call to enable the histogram and to cache the histogram object? As opposed to e.g. just using a standard macro and making the code early return if the name hasn't been set? Is it just for testing? If so, you can use HistogramTester class and not need to actually access the histogram object. https://codereview.chromium.org/1726873002/diff/60001/content/app/content_mai... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1726873002/diff/60001/content/app/content_mai... content/app/content_main_runner.cc:759: report_type = "browser"; Is this really "browser"? Seems like this is assuming something about the user of the content/ API which is not necessarily true. https://codereview.chromium.org/1726873002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1726873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:52985: +<histogram name="UMA.Histograms.browser.Creations" I suggest using the <histogram_suffixes> construct to make the XML more concise. See top of the file for an example. https://codereview.chromium.org/1726873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:69201: + <int value="3" label="Number of general histograms"/> Nit: I think "general histogram" is not very obvious - I would call say "Log histogram" https://codereview.chromium.org/1726873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:69208: + <int value="10" label="Number of persistent histograms"/> Nit: No need to put "Total number" and "number" in these labels. Just describe the event - i.e. "Linear histogram created".
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_base.cc (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base.cc:130: if (existing) { On 2016/03/01 16:41:33, Alexei Svitkine (slow) wrote: > Nit: != 0 Done. https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base.cc:193: histogram->GetHistogramType()); On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > This is a bit fragile - if the histogram type ever changes (a new value is > added), then this histogram gets broken. > > I suggest just doing a switch on the histogram type and mapping to the value. > This way, if a new type is added, then there would be a compile error if there's > a new enum that's not handled in the switch - which ensures that the histogram > will be updated. Done. https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_base.h (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base.h:254: // Update the creation (or lookup) report for a histogram. On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > Document the |created| param, please. Also I think it's slightly confusing that > the function is called "UpdateReportHistogram" and takes "histogram" as a param > - at first read it would sound like that's the histogram being updated. Can you > rework things to avoid this possible confusion? Done. https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_base_unittest.cc (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base_unittest.cc:31: statistics_recorder_ = new StatisticsRecorder(); On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > Nit: While you're changing this, can you make this a scoped_ptr? StatisticsRecorder is explicit about what classes are allowed to construct/destruct the objects. scoped_ptr needs permission to destruct the objects it holds. I can leave it as-is or make the SR dtor public. Preference? https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base_unittest.cc:41: }; On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > Nit: Add DISALLOW_COPY_AND_ASSIGN(). Done. https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_persistence.cc (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_persistence.cc:192: if (existing) { On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > Nit: != 0 for checking numbers. Done. https://codereview.chromium.org/1726873002/diff/60001/base/metrics/statistics... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/statistics... base/metrics/statistics_recorder.cc:372: size_t StatisticsRecorder::NumberOfHistograms() { On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > Nit: GetHistogramCount() Done. https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... chrome/installer/setup/installer_metrics.cc:27: base::HistogramBase::EnableCreationReportHistogram("setup"); > I find it unfortunate that we need an extra call (and an extra string param). Is > there any way to make the SetPersistentHistogramMemoryAllocator() call do this > internally w/ the allocator name string that's passed? Maybe there could be a > param to it that specifies whether it should do this or not - if we don't always > want it. Most of the time, the Report histogram will be enabled first. Also, I don't think it is something that histogram_persistence should know about -- it's a completely orthogonal thing. > A second question - why do we need it be a separate call to enable the histogram > and to cache the histogram object? As opposed to e.g. just using a standard > macro and making the code early return if the name hasn't been set? This particular histogram doesn't get a single update per activity. The creation action, for example, could cause increments in up to four different buckets: total count, the type, uma stability|targeted, and is-persistent. A macro instantiation can't do that efficiently. > Is it just for testing? If so, you can use HistogramTester class and not need to > actually access the histogram object. It's for reporting. I wanted to see what and how histograms were being used in the field. It already found one issue which led to a 40% time reduction in creating new histograms. https://codereview.chromium.org/1726873002/diff/60001/content/app/content_mai... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1726873002/diff/60001/content/app/content_mai... content/app/content_main_runner.cc:759: report_type = "browser"; On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > Is this really "browser"? Seems like this is assuming something about the user > of the content/ API which is not necessarily true. If the process is called without a "--type=" parameter then it launches the browser which then re-launches itself with that parameter to start renderers, gpu process, and potentially others. With the change to histogram_suffixes, though, there needs to be an empty name anyway so this can go away. https://codereview.chromium.org/1726873002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1726873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:52985: +<histogram name="UMA.Histograms.browser.Creations" On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > I suggest using the <histogram_suffixes> construct to make the XML more concise. > See top of the file for an example. Okay. It doesn't seem to provide an option for omitting the source histogram but that works out well here because I can remove the "browser" one which will also remove the setting of that name (if empty) in startup. https://codereview.chromium.org/1726873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:69201: + <int value="3" label="Number of general histograms"/> On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > Nit: I think "general histogram" is not very obvious - I would call say "Log > histogram" Done. https://codereview.chromium.org/1726873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:69208: + <int value="10" label="Number of persistent histograms"/> On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > Nit: No need to put "Total number" and "number" in these labels. Just describe > the event - i.e. "Linear histogram created". Done.
https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_base_unittest.cc (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base_unittest.cc:31: statistics_recorder_ = new StatisticsRecorder(); On 2016/03/02 19:14:19, bcwhite wrote: > On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > > Nit: While you're changing this, can you make this a scoped_ptr? > > StatisticsRecorder is explicit about what classes are allowed to > construct/destruct the objects. scoped_ptr needs permission to destruct the > objects it holds. I can leave it as-is or make the SR dtor public. Preference? > I think making the dtor public should be fine, since the ctor is still private. So doing that + making this a scoped ptr would be best, I think. https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... chrome/installer/setup/installer_metrics.cc:27: base::HistogramBase::EnableCreationReportHistogram("setup"); On 2016/03/02 19:14:19, bcwhite wrote: > > I find it unfortunate that we need an extra call (and an extra string param). > Is > > there any way to make the SetPersistentHistogramMemoryAllocator() call do this > > internally w/ the allocator name string that's passed? Maybe there could be a > > param to it that specifies whether it should do this or not - if we don't > always > > want it. > > Most of the time, the Report histogram will be enabled first. Also, I don't > think it is something that histogram_persistence should know about -- it's a > completely orthogonal thing. > > > > A second question - why do we need it be a separate call to enable the > histogram > > and to cache the histogram object? As opposed to e.g. just using a standard > > macro and making the code early return if the name hasn't been set? > > This particular histogram doesn't get a single update per activity. The > creation action, for example, could cause increments in up to four different > buckets: total count, the type, uma stability|targeted, and is-persistent. A > macro instantiation can't do that efficiently. You can have a helper function that calls the macro and call it with different enum values. It should be pretty efficient - only overhead is the function call per value logged, but that would be negligible. > > > > Is it just for testing? If so, you can use HistogramTester class and not need > to > > actually access the histogram object. > > It's for reporting. I wanted to see what and how histograms were being used in > the field. It already found one issue which led to a 40% time reduction in > creating new histograms. Sorry, I understand that you want the histogram in the wild. My question is why you're constructing it manually and keeping a pointer to it? (As opposed to using a standard UMA_ macro guarded by a check if the process name has been set.) If you're only using the pointer to access it from tests, there's a HistogramTester class that can be used that doesn't require it.
https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_base_unittest.cc (right): https://codereview.chromium.org/1726873002/diff/60001/base/metrics/histogram_... base/metrics/histogram_base_unittest.cc:31: statistics_recorder_ = new StatisticsRecorder(); On 2016/03/03 17:56:23, Alexei Svitkine (slow) wrote: > On 2016/03/02 19:14:19, bcwhite wrote: > > On 2016/03/01 16:41:34, Alexei Svitkine (slow) wrote: > > > Nit: While you're changing this, can you make this a scoped_ptr? > > > > StatisticsRecorder is explicit about what classes are allowed to > > construct/destruct the objects. scoped_ptr needs permission to destruct the > > objects it holds. I can leave it as-is or make the SR dtor public. > Preference? > > > > I think making the dtor public should be fine, since the ctor is still private. > So doing that + making this a scoped ptr would be best, I think. Done. https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... chrome/installer/setup/installer_metrics.cc:27: base::HistogramBase::EnableCreationReportHistogram("setup"); > Sorry, I understand that you want the histogram in the wild. My question is why > you're constructing it manually and keeping a pointer to it? (As opposed to > using a standard UMA_ macro guarded by a check if the process name has been > set.) I guess I've done so much work on histograms that using FactoryGet seems natural. Given that this call would be done while single-threaded, it has no need for atomics and conditional creation. It also avoids saving an extra copy of the string. The std::string copy held inside the histogram may be copy-on-write but persistent histograms by necessity keep a separate copy of the histogram name in persistent space; once the transition to them is done, I hope to eliminate the extra std::string copy of the name and change FactoryGet to take a StringPiece instead. But as you say, the overhead of the multiple subroutine calls, atomic reads, conditions, and returns is pretty small and applies only to the creation of all histograms, no their use. If you'd prefer it done that way, I'll change it.
https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... chrome/installer/setup/installer_metrics.cc:27: base::HistogramBase::EnableCreationReportHistogram("setup"); On 2016/03/07 13:39:25, bcwhite wrote: > > Sorry, I understand that you want the histogram in the wild. My question is > why > > you're constructing it manually and keeping a pointer to it? (As opposed to > > using a standard UMA_ macro guarded by a check if the process name has been > > set.) > > I guess I've done so much work on histograms that using FactoryGet seems > natural. Given that this call would be done while single-threaded, it has no > need for atomics and conditional creation. > > It also avoids saving an extra copy of the string. The std::string copy held > inside the histogram may be copy-on-write but persistent histograms by necessity > keep a separate copy of the histogram name in persistent space; once the > transition to them is done, I hope to eliminate the extra std::string copy of > the name and change FactoryGet to take a StringPiece instead. > > But as you say, the overhead of the multiple subroutine calls, atomic reads, > conditions, and returns is pretty small and applies only to the creation of all > histograms, no their use. If you'd prefer it done that way, I'll change it. Right - my preference is to use the macros because does make the code less complex and is easier to understand.
https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... chrome/installer/setup/installer_metrics.cc:27: base::HistogramBase::EnableCreationReportHistogram("setup"); On 2016/03/07 16:16:46, Alexei Svitkine (slow) wrote: > Right - my preference is to use the macros because does make the code less > complex and is easier to understand. Well... We're just going to have to agree to disagree on that particular point. :-) But I did try. It adds complications to avoid recursive calls to the macro and requires an independent unit-test that doesn't try to create a StatisticsRecorder. I had to back it out, though, because it breaks other tests. The basic problem is that HistogramTester objects do not play nice with local StastiticsRecorder objects. Specifically, HistogramTester calls StatisticsRecorder::Initialize() which creates the global SR object. After that point, any attempt to create a local SR object will crash on DCHECK(!histograms_) in the SR constructor. base_unittests has one existing instance of HistogramTester and that is within histogram_tester_unittest.cc. By luck, this test conveniently runs later and only one test after it attempts to create local SR objects. The only one that does is... (drum roll, please) "HistogramTest" ...which as you may recall included the SR::ResetForTesting() method (that I now see is pretty much a hack to get around the just-described problem). The fix for this is non-trivial. If HistogramTester were to create its own instance of a StatisticsRecorder (which was obviously avoided purposely during its development) then it would crash if any previous test initialized the lazy-instance with the (global) SR. The simplest solution seems to be to simply delete the global instance inside ResetForTesting()... except there is no way to reset a LazyInstance. Regardless of if/when/how this gets fixed, it doesn't belong in this CL so I've gone back to my original solution. I can revisit this if you feel it necessary once the larger problem has been resolved. I'll add that to my list of things to do when bored. ;-) Incidentally, the reason the ResetForTesting() hack doesn't work here when it worked elsewhere is, again, test ordering. Because tests that run later that HistogramBaseTest depend on ::Initialize() to create a working SR but that method won't do so if it did so previously but was then reset. The result is that histograms don't get recorded by the SR and tests fail.
https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... chrome/installer/setup/installer_metrics.cc:27: base::HistogramBase::EnableCreationReportHistogram("setup"); On 2016/03/07 20:09:25, bcwhite wrote: > On 2016/03/07 16:16:46, Alexei Svitkine (slow) wrote: > > Right - my preference is to use the macros because does make the code less > > complex and is easier to understand. > > Well... We're just going to have to agree to disagree on that particular point. > :-) But I did try. It adds complications to avoid recursive calls to the > macro and requires an independent unit-test that doesn't try to create a > StatisticsRecorder. > > I had to back it out, though, because it breaks other tests. > > The basic problem is that HistogramTester objects do not play nice with local > StastiticsRecorder objects. > > Specifically, HistogramTester calls StatisticsRecorder::Initialize() which > creates the global SR object. After that point, any attempt to create a local > SR object will crash on DCHECK(!histograms_) in the SR constructor. > > base_unittests has one existing instance of HistogramTester and that is within > histogram_tester_unittest.cc. By luck, this test conveniently runs later and > only one test after it attempts to create local SR objects. The only one that > does is... (drum roll, please) "HistogramTest" ...which as you may recall > included the SR::ResetForTesting() method (that I now see is pretty much a hack > to get around the just-described problem). > > The fix for this is non-trivial. If HistogramTester were to create its own > instance of a StatisticsRecorder (which was obviously avoided purposely during > its development) then it would crash if any previous test initialized the > lazy-instance with the (global) SR. The simplest solution seems to be to simply > delete the global instance inside ResetForTesting()... except there is no way > to reset a LazyInstance. > > Regardless of if/when/how this gets fixed, it doesn't belong in this CL so I've > gone back to my original solution. I can revisit this if you feel it necessary > once the larger problem has been resolved. I'll add that to my list of things > to do when bored. ;-) > > Incidentally, the reason the ResetForTesting() hack doesn't work here when it > worked elsewhere is, again, test ordering. Because tests that run later that > HistogramBaseTest depend on ::Initialize() to create a working SR but that > method won't do so if it did so previously but was then reset. The result is > that histograms don't get recorded by the SR and tests fail. Okay, that's fair. Can you add a comment about this to the callsite where you use the Factory method about this reasoning? However, if we have an existing issue that depends on test ordering, we need to fix it. I tried running base_unittests a few times with --gunit_shuffle and didn't see any failures. Are you saying there are orderings that it does fail in?
https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... chrome/installer/setup/installer_metrics.cc:27: base::HistogramBase::EnableCreationReportHistogram("setup"); > Okay, that's fair. Can you add a comment about this to the callsite where you > use the Factory method about this reasoning? Done. > However, if we have an existing issue that depends on test ordering, we need to > fix it. I tried running base_unittests a few times with --gunit_shuffle and > didn't see any failures. Are you saying there are orderings that it does fail > in? Try adding "--single-process-tests" to ensure that everything runs on a single thread. With that and --gtest_shuffle, I got the DCHECK first try.
https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... chrome/installer/setup/installer_metrics.cc:27: base::HistogramBase::EnableCreationReportHistogram("setup"); On 2016/03/07 20:47:34, bcwhite wrote: > > Okay, that's fair. Can you add a comment about this to the callsite where you > > use the Factory method about this reasoning? > > Done. > > > > However, if we have an existing issue that depends on test ordering, we need > to > > fix it. I tried running base_unittests a few times with --gunit_shuffle and > > didn't see any failures. Are you saying there are orderings that it does fail > > in? > > Try adding "--single-process-tests" to ensure that everything runs on a single > thread. With that and --gtest_shuffle, I got the DCHECK first try. Yep - I see it! (I was using --gunit_shuffle instead of --gtest_shuffle - oops.) Can you file a bug for it? Thanks! https://codereview.chromium.org/1726873002/diff/140001/base/metrics/histogram... File base/metrics/histogram_base.cc (right): https://codereview.chromium.org/1726873002/diff/140001/base/metrics/histogram... base/metrics/histogram_base.cc:192: void HistogramBase::ReportHistogramActivity(HistogramBase* histogram, This param can be const. And in fact you might as well pass it by const ref since the param shouldn't be null. https://codereview.chromium.org/1726873002/diff/140001/base/metrics/histogram... base/metrics/histogram_base.cc:218: default: No need for default case. GetHistogramType() return value is HistogramType which compilers will validate against the switch and produce a compile error when a case isn't handled. Same thing for the ReportActivity switch. https://codereview.chromium.org/1726873002/diff/140001/base/metrics/statistic... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/1726873002/diff/140001/base/metrics/statistic... base/metrics/statistics_recorder.cc:71: DCHECK(histograms_ && ranges_ && lock_); Nit: Not sure why I missed that in the previous CL, but these should be separate DCHECKs so that if it fails, it's clear which one it is. https://codereview.chromium.org/1726873002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1726873002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:87036: + ordering="prefix"> Actually, I think my preference would be for this to be a suffix, that way, it's easy to add them all via UMA.Histogram.Activity* on the dashboard.
https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1726873002/diff/60001/chrome/installer/setup/... chrome/installer/setup/installer_metrics.cc:27: base::HistogramBase::EnableCreationReportHistogram("setup"); On 2016/03/07 21:14:50, Alexei Svitkine (slow) wrote: > On 2016/03/07 20:47:34, bcwhite wrote: > > > Okay, that's fair. Can you add a comment about this to the callsite where > you > > > use the Factory method about this reasoning? > > > > Done. > > > > > > > However, if we have an existing issue that depends on test ordering, we need > > to > > > fix it. I tried running base_unittests a few times with --gunit_shuffle and > > > didn't see any failures. Are you saying there are orderings that it does > fail > > > in? > > > > Try adding "--single-process-tests" to ensure that everything runs on a single > > thread. With that and --gtest_shuffle, I got the DCHECK first try. > > Yep - I see it! (I was using --gunit_shuffle instead of --gtest_shuffle - oops.) > > Can you file a bug for it? Thanks! Done. https://codereview.chromium.org/1726873002/diff/140001/base/metrics/histogram... File base/metrics/histogram_base.cc (right): https://codereview.chromium.org/1726873002/diff/140001/base/metrics/histogram... base/metrics/histogram_base.cc:192: void HistogramBase::ReportHistogramActivity(HistogramBase* histogram, On 2016/03/07 21:14:51, Alexei Svitkine (slow) wrote: > This param can be const. And in fact you might as well pass it by const ref > since the param shouldn't be null. Done. https://codereview.chromium.org/1726873002/diff/140001/base/metrics/histogram... base/metrics/histogram_base.cc:218: default: On 2016/03/07 21:14:51, Alexei Svitkine (slow) wrote: > No need for default case. GetHistogramType() return value is HistogramType which > compilers will validate against the switch and produce a compile error when a > case isn't handled. > > Same thing for the ReportActivity switch. The Windows compiler does not produce such an error. I tried omitting both "dafault" and SPARSE_HISTOGRAM from the switch and no error, debug or release. Also, without the "default" case, is reports a potentially uninitialized variable in "report_type". I can omit the default and initialize report_type to HISTOGRAM_REPORT_MAX if other compilers will indeed fail. https://codereview.chromium.org/1726873002/diff/140001/base/metrics/statistic... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/1726873002/diff/140001/base/metrics/statistic... base/metrics/statistics_recorder.cc:71: DCHECK(histograms_ && ranges_ && lock_); On 2016/03/07 21:14:51, Alexei Svitkine (slow) wrote: > Nit: Not sure why I missed that in the previous CL, but these should be separate > DCHECKs so that if it fails, it's clear which one it is. Done. https://codereview.chromium.org/1726873002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1726873002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:87036: + ordering="prefix"> On 2016/03/07 21:14:51, Alexei Svitkine (slow) wrote: > Actually, I think my preference would be for this to be a suffix, that way, it's > easy to add them all via UMA.Histogram.Activity* on the dashboard. Done.
lgtm https://codereview.chromium.org/1726873002/diff/140001/base/metrics/histogram... File base/metrics/histogram_base.cc (right): https://codereview.chromium.org/1726873002/diff/140001/base/metrics/histogram... base/metrics/histogram_base.cc:218: default: On 2016/03/07 22:20:41, bcwhite wrote: > On 2016/03/07 21:14:51, Alexei Svitkine (slow) wrote: > > No need for default case. GetHistogramType() return value is HistogramType > which > > compilers will validate against the switch and produce a compile error when a > > case isn't handled. > > > > Same thing for the ReportActivity switch. > > The Windows compiler does not produce such an error. I tried omitting both > "dafault" and SPARSE_HISTOGRAM from the switch and no error, debug or release. > > Also, without the "default" case, is reports a potentially uninitialized > variable in "report_type". > > I can omit the default and initialize report_type to HISTOGRAM_REPORT_MAX if > other compilers will indeed fail. Other platforms do - specifically clang. Since this is a cross-platform file, having one platform warn is sufficient, so omitting default and initializing the var SGTM. Omit the default in the outer switch too, please. Thanks.
Patchset #9 (id:180001) has been deleted
https://codereview.chromium.org/1726873002/diff/140001/base/metrics/histogram... File base/metrics/histogram_base.cc (right): https://codereview.chromium.org/1726873002/diff/140001/base/metrics/histogram... base/metrics/histogram_base.cc:218: default: On 2016/03/08 19:55:30, Alexei Svitkine (slow) wrote: > On 2016/03/07 22:20:41, bcwhite wrote: > > On 2016/03/07 21:14:51, Alexei Svitkine (slow) wrote: > > > No need for default case. GetHistogramType() return value is HistogramType > > which > > > compilers will validate against the switch and produce a compile error when > a > > > case isn't handled. > > > > > > Same thing for the ReportActivity switch. > > > > The Windows compiler does not produce such an error. I tried omitting both > > "dafault" and SPARSE_HISTOGRAM from the switch and no error, debug or release. > > > > Also, without the "default" case, is reports a potentially uninitialized > > variable in "report_type". > > > > I can omit the default and initialize report_type to HISTOGRAM_REPORT_MAX if > > other compilers will indeed fail. > > Other platforms do - specifically clang. Since this is a cross-platform file, > having one platform warn is sufficient, so omitting default and initializing the > var SGTM. > > Omit the default in the outer switch too, please. Thanks. Cool. I thought that was the case but was playing it safe. Done. Also applies to HistogramTypeToString, above.
bcwhite@chromium.org changed reviewers: + nasko@chromium.org
nasko@chromium.org: Please review changes in content/app/content_main_runner.cc
bcwhite@chromium.org changed reviewers: + grt@chromium.org
grt@chromium.org: Please review changes in chrome/installer/setup/installer_metrics.cc
https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... File base/metrics/histogram_base.cc (right): https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... base/metrics/histogram_base.cc:221: if (flags & (kUmaStabilityHistogramFlag & ~kUmaTargetedHistogramFlag)) is the ~kUmaTargetedHistogramFlag necessary? from histogram_base.h, it seems that the two bits in 0x3 indicate stability, not 0x2 alone. are there any histograms that have only the 0x2 bit set? https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... File base/metrics/histogram_base.h (right): https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... base/metrics/histogram_base.h:53: HISTOGRAM_REPORT_CREATED, nit: since enum histogram values are ordinarily meant to be fixed for all time, i like to put explicit " = 0" and such here in the enum's definition. it makes it nearly impossible to accidentally change the values in the code without also updating histograms.xml. since asvitkine is okay with it as-is, i won't insist on it. https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... File base/metrics/histogram_persistence.cc (right): https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... base/metrics/histogram_persistence.cc:193: DLOG(WARNING) << existing DLOG_IF(WARNING, existing) << ...
https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... File base/metrics/histogram_base.cc (right): https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... base/metrics/histogram_base.cc:221: if (flags & (kUmaStabilityHistogramFlag & ~kUmaTargetedHistogramFlag)) On 2016/03/09 04:04:43, grt wrote: > is the ~kUmaTargetedHistogramFlag necessary? Yes. Without it, it will also match those marked UMA_TARGETED because either bit set alone will cause a non-zero (true) result. > from histogram_base.h, it seems > that the two bits in 0x3 indicate stability, not 0x2 alone. are there any > histograms that have only the 0x2 bit set? No histograms have only the 0x2 bit set. https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... File base/metrics/histogram_base.h (right): https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... base/metrics/histogram_base.h:53: HISTOGRAM_REPORT_CREATED, On 2016/03/09 04:04:43, grt wrote: > nit: since enum histogram values are ordinarily meant to be fixed for all time, > i like to put explicit " = 0" and such here in the enum's definition. it makes > it nearly impossible to accidentally change the values in the code without also > updating histograms.xml. since asvitkine is okay with it as-is, i won't insist > on it. Do you mean only on the first entry or all entries? Only the latter would provide any real protection as the most likely mistake is that someone would insert a new TYPE in the middle. https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... File base/metrics/histogram_persistence.cc (right): https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... base/metrics/histogram_persistence.cc:193: DLOG(WARNING) << existing On 2016/03/09 04:04:43, grt wrote: > DLOG_IF(WARNING, existing) << ... Didn't know about that one. Will do.
https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... File base/metrics/histogram_base.cc (right): https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... base/metrics/histogram_base.cc:221: if (flags & (kUmaStabilityHistogramFlag & ~kUmaTargetedHistogramFlag)) On 2016/03/09 14:01:57, bcwhite wrote: > On 2016/03/09 04:04:43, grt wrote: > > is the ~kUmaTargetedHistogramFlag necessary? > > Yes. Without it, it will also match those marked UMA_TARGETED because either > bit set alone will cause a non-zero (true) result. > > > > from histogram_base.h, it seems > > that the two bits in 0x3 indicate stability, not 0x2 alone. are there any > > histograms that have only the 0x2 bit set? > > No histograms have only the 0x2 bit set. I'll rewrite it to be "(flags & mask) == mask" to be more clear.
Rubberstamp LGTM on content/app/content_main_runner.cc.
https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... File base/metrics/histogram_base.cc (right): https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... base/metrics/histogram_base.cc:221: if (flags & (kUmaStabilityHistogramFlag & ~kUmaTargetedHistogramFlag)) On 2016/03/09 14:03:50, bcwhite wrote: > On 2016/03/09 14:01:57, bcwhite wrote: > > On 2016/03/09 04:04:43, grt wrote: > > > is the ~kUmaTargetedHistogramFlag necessary? > > > > Yes. Without it, it will also match those marked UMA_TARGETED because either > > bit set alone will cause a non-zero (true) result. > > > > > > > from histogram_base.h, it seems > > > that the two bits in 0x3 indicate stability, not 0x2 alone. are there any > > > histograms that have only the 0x2 bit set? > > > > No histograms have only the 0x2 bit set. > > I'll rewrite it to be "(flags & mask) == mask" to be more clear. Done. https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... File base/metrics/histogram_base.h (right): https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... base/metrics/histogram_base.h:53: HISTOGRAM_REPORT_CREATED, On 2016/03/09 14:01:57, bcwhite wrote: > On 2016/03/09 04:04:43, grt wrote: > > nit: since enum histogram values are ordinarily meant to be fixed for all > time, > > i like to put explicit " = 0" and such here in the enum's definition. it makes > > it nearly impossible to accidentally change the values in the code without > also > > updating histograms.xml. since asvitkine is okay with it as-is, i won't insist > > on it. > > Do you mean only on the first entry or all entries? Only the latter would > provide any real protection as the most likely mistake is that someone would > insert a new TYPE in the middle. Done for all entries. https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... File base/metrics/histogram_persistence.cc (right): https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... base/metrics/histogram_persistence.cc:193: DLOG(WARNING) << existing On 2016/03/09 14:01:57, bcwhite wrote: > On 2016/03/09 04:04:43, grt wrote: > > DLOG_IF(WARNING, existing) << ... > > Didn't know about that one. Will do. Done.
On 2016/03/09 16:04:26, bcwhite wrote: > https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... > File base/metrics/histogram_base.cc (right): > > https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... > base/metrics/histogram_base.cc:221: if (flags & (kUmaStabilityHistogramFlag & > ~kUmaTargetedHistogramFlag)) > On 2016/03/09 14:03:50, bcwhite wrote: > > On 2016/03/09 14:01:57, bcwhite wrote: > > > On 2016/03/09 04:04:43, grt wrote: > > > > is the ~kUmaTargetedHistogramFlag necessary? > > > > > > Yes. Without it, it will also match those marked UMA_TARGETED because > either > > > bit set alone will cause a non-zero (true) result. > > > > > > > > > > from histogram_base.h, it seems > > > > that the two bits in 0x3 indicate stability, not 0x2 alone. are there any > > > > histograms that have only the 0x2 bit set? > > > > > > No histograms have only the 0x2 bit set. > > > > I'll rewrite it to be "(flags & mask) == mask" to be more clear. > > Done. > > https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... > File base/metrics/histogram_base.h (right): > > https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... > base/metrics/histogram_base.h:53: HISTOGRAM_REPORT_CREATED, > On 2016/03/09 14:01:57, bcwhite wrote: > > On 2016/03/09 04:04:43, grt wrote: > > > nit: since enum histogram values are ordinarily meant to be fixed for all > > time, > > > i like to put explicit " = 0" and such here in the enum's definition. it > makes > > > it nearly impossible to accidentally change the values in the code without > > also > > > updating histograms.xml. since asvitkine is okay with it as-is, i won't > insist > > > on it. > > > > Do you mean only on the first entry or all entries? Only the latter would > > provide any real protection as the most likely mistake is that someone would > > insert a new TYPE in the middle. > > Done for all entries. > > https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... > File base/metrics/histogram_persistence.cc (right): > > https://codereview.chromium.org/1726873002/diff/220001/base/metrics/histogram... > base/metrics/histogram_persistence.cc:193: DLOG(WARNING) << existing > On 2016/03/09 14:01:57, bcwhite wrote: > > On 2016/03/09 04:04:43, grt wrote: > > > DLOG_IF(WARNING, existing) << ... > > > > Didn't know about that one. Will do. > > Done. Greg, you good with these last changes?
Description was changed from ========== Report histogram creation results. Add a new histogram that reports information about all the histograms created in different procesesses, their types, and what flags were set. BUG=546019 ========== to ========== Report histogram creation results. Add a new histogram that reports information about all the histograms created in different procesesses, their types, and what flags were set. BUG=546019 TBR=grt grt: single instantiating call in installer_metrics.cc ==========
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1726873002/#ps260001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1726873002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1726873002/260001
Message was sent while issue was closed.
Description was changed from ========== Report histogram creation results. Add a new histogram that reports information about all the histograms created in different procesesses, their types, and what flags were set. BUG=546019 TBR=grt grt: single instantiating call in installer_metrics.cc ========== to ========== Report histogram creation results. Add a new histogram that reports information about all the histograms created in different procesesses, their types, and what flags were set. BUG=546019 TBR=grt grt: single instantiating call in installer_metrics.cc ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Report histogram creation results. Add a new histogram that reports information about all the histograms created in different procesesses, their types, and what flags were set. BUG=546019 TBR=grt grt: single instantiating call in installer_metrics.cc ========== to ========== Report histogram creation results. Add a new histogram that reports information about all the histograms created in different procesesses, their types, and what flags were set. BUG=546019 TBR=grt grt: single instantiating call in installer_metrics.cc Committed: https://crrev.com/d723c25e72442b9381023224d3db0ed12ae3210d Cr-Commit-Position: refs/heads/master@{#381482} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/d723c25e72442b9381023224d3db0ed12ae3210d Cr-Commit-Position: refs/heads/master@{#381482} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
