Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7618050:
Improve GPU software rendering list histograms. (Closed)
|
Created:
9 years, 4 months ago by Zhenyao Mo Modified:
9 years, 3 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, jam, apatrick_chromium Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionImprove GPU software rendering list histograms.
Implements per feature histograms. At the moment three features: accelerated-compositing, accelerated-2d-canvas, and webgl. Also, implemented each feature's allowed case percentage, per each OS (under Windows, they are divided into XP, Vista, and Win7).
BUG=91875
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98840
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 4
Patch Set 4 : '' #
Total comments: 4
Patch Set 5 : '' #
Total comments: 7
Patch Set 6 : '' #Patch Set 7 : '' #Messages
Total messages: 13 (0 generated)
Please review.
http://codereview.chromium.org/7618050/diff/3/content/browser/gpu/gpu_data_ma... File content/browser/gpu/gpu_data_manager.cc (right): http://codereview.chromium.org/7618050/diff/3/content/browser/gpu/gpu_data_ma... content/browser/gpu/gpu_data_manager.cc:98: if (version_numbers[0] == 5) Probably needs a check that version_numbers.size() >= 2 http://codereview.chromium.org/7618050/diff/3/content/browser/gpu/gpu_data_ma... content/browser/gpu/gpu_data_manager.cc:422: // names in this loop. Hmm, that seems strange. What names get reused between invocations of the loop? If inlining the code in the body of the loop doesn't work, them maybe put the code in a function and call it 3 times to avoid replicating code?
Revised. Please review again. http://codereview.chromium.org/7618050/diff/3/content/browser/gpu/gpu_data_ma... File content/browser/gpu/gpu_data_manager.cc (right): http://codereview.chromium.org/7618050/diff/3/content/browser/gpu/gpu_data_ma... content/browser/gpu/gpu_data_manager.cc:98: if (version_numbers[0] == 5) On 2011/08/16 00:23:41, vangelis wrote: > Probably needs a check that version_numbers.size() >= 2 Done. http://codereview.chromium.org/7618050/diff/3/content/browser/gpu/gpu_data_ma... content/browser/gpu/gpu_data_manager.cc:422: // names in this loop. On 2011/08/16 00:23:41, vangelis wrote: > Hmm, that seems strange. What names get reused between invocations of the loop? > > > If inlining the code in the body of the loop doesn't work, them maybe put the > code in a function and call it 3 times to avoid replicating code? Per discussion with Vangelis and JAR, we will use histogram functions directly instead of the UMA_ macro to avoid the static variable problem.
http://codereview.chromium.org/7618050/diff/11001/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager.cc (right): http://codereview.chromium.org/7618050/diff/11001/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager.cc:98: DCHECK_GE(version_numbers.size(), 2); I think that in addition to the DCHECK we'll need to bail out safely if for some reason we don't get enough fields in the version (to avoid nasty crashes). http://codereview.chromium.org/7618050/diff/11001/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager.cc:433: LogToHistogram(kGpuBlacklistHistogramNames[i], 0, 2); Sorry, I should have spotted that earlier but I don't think we want to be adding a tick to the kGpuBlacklistHistograPerEntry for every feature that's disabled. I think a better strategy would be to update that histogram if any of the features is blacklisted.
http://codereview.chromium.org/7618050/diff/11001/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager.cc (right): http://codereview.chromium.org/7618050/diff/11001/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager.cc:98: DCHECK_GE(version_numbers.size(), 2); On 2011/08/17 17:40:17, vangelis wrote: > I think that in addition to the DCHECK we'll need to bail out safely if for some > reason we don't get enough fields in the version (to avoid nasty crashes). Done. http://codereview.chromium.org/7618050/diff/11001/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager.cc:433: LogToHistogram(kGpuBlacklistHistogramNames[i], 0, 2); On 2011/08/17 17:40:17, vangelis wrote: > Sorry, I should have spotted that earlier but I don't think we want to be adding > a tick to the kGpuBlacklistHistograPerEntry for every feature that's disabled. > I think a better strategy would be to update that histogram if any of the > features is blacklisted. > Done.
http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager.cc (right): http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager.cc:410: if (flags == 0) { shouldn't you also include here the case where flags == kDisableMultisampling ? http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager.cc:441: histogram_pointer->Add(((flags & kGpuFeatures[i]) == 0) ? 0 : 1); Could the expression above be simplified as (flags & kGpuFeatures[i]) ? Potentially with a static cast to the expected integer value.
Revised. http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager.cc (right): http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager.cc:410: if (flags == 0) { On 2011/08/18 06:06:44, vangelis wrote: > shouldn't you also include here the case where flags == kDisableMultisampling ? We have an entry which counts the number of use-cases that multisampling is disabled. So we don't need to include the case here. It should be easy to add this allowed-case and the multisampling-disabled-case in the histogram. In the histograms that counts allowed/disallowed gpu features, since we do it per feature cases, so multisampling won't affect those histograms. http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager.cc:441: histogram_pointer->Add(((flags & kGpuFeatures[i]) == 0) ? 0 : 1); On 2011/08/18 06:06:44, vangelis wrote: > Could the expression above be simplified as (flags & kGpuFeatures[i]) ? > Potentially with a static cast to the expected integer value. Done.
http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager.cc (right): http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager.cc:441: histogram_pointer->Add(((flags & kGpuFeatures[i]) == 0) ? 0 : 1); On 2011/08/18 18:16:10, Zhenyao Mo wrote: > On 2011/08/18 06:06:44, vangelis wrote: > > Could the expression above be simplified as (flags & kGpuFeatures[i]) ? > > Potentially with a static cast to the expected integer value. > > Done. The Windows try bot fails to compile because of the implicit conversion from bool to uint32. So we have to cast it to bool then cast it to int again. Maybe the original ((flags & kGpuFeatures[i]) == 0) ? 0 : 1 is easier to read than two nested cast.
On 2011/08/18 20:12:28, Zhenyao Mo wrote: > http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... > File content/browser/gpu/gpu_data_manager.cc (right): > > http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... > content/browser/gpu/gpu_data_manager.cc:441: histogram_pointer->Add(((flags & > kGpuFeatures[i]) == 0) ? 0 : 1); > On 2011/08/18 18:16:10, Zhenyao Mo wrote: > > On 2011/08/18 06:06:44, vangelis wrote: > > > Could the expression above be simplified as (flags & kGpuFeatures[i]) ? > > > Potentially with a static cast to the expected integer value. > > > > Done. > > The Windows try bot fails to compile because of the implicit conversion from > bool to uint32. So we have to cast it to bool then cast it to int again. > > Maybe the original ((flags & kGpuFeatures[i]) == 0) ? 0 : 1 is easier to read > than two nested cast. Would this: ((flags & kGpuFeatures[i]) ? 1 : 0) work?
http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager.cc (right): http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager.cc:410: if (flags == 0) { On 2011/08/18 18:16:10, Zhenyao Mo wrote: > On 2011/08/18 06:06:44, vangelis wrote: > > shouldn't you also include here the case where flags == kDisableMultisampling > ? > > We have an entry which counts the number of use-cases that multisampling is > disabled. So we don't need to include the case here. It should be easy to add > this allowed-case and the multisampling-disabled-case in the histogram. > > In the histograms that counts allowed/disallowed gpu features, since we do it > per feature cases, so multisampling won't affect those histograms. Why do we then just explicitly skip the multisampling flag when you set the value for features in the loop below? It looks like you should be using features = kGpuFeaturesAll to get results for multisampling blacklisting too.
Revised. http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager.cc (right): http://codereview.chromium.org/7618050/diff/11002/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager.cc:410: if (flags == 0) { On 2011/08/18 21:05:41, vangelis wrote: > On 2011/08/18 18:16:10, Zhenyao Mo wrote: > > On 2011/08/18 06:06:44, vangelis wrote: > > > shouldn't you also include here the case where flags == > kDisableMultisampling > > ? > > > > We have an entry which counts the number of use-cases that multisampling is > > disabled. So we don't need to include the case here. It should be easy to > add > > this allowed-case and the multisampling-disabled-case in the histogram. > > > > In the histograms that counts allowed/disallowed gpu features, since we do it > > per feature cases, so multisampling won't affect those histograms. > > Why do we then just explicitly skip the multisampling flag when you set the > value for features in the loop below? It looks like you should be using > features = kGpuFeaturesAll to get results for multisampling blacklisting too. Ah you are right. Fixed.
Ping: Vangelis, this one is still pending.
LGTM |
