Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2904743002:
cc: Add UMA for tracking decode duration for out of raster decodes. (Closed)
Descriptioncc: Add UMA for tracking decode duration for out of raster decodes.
We currently bucket UMA for all decode tasks in a single histogram.
With checker-imaging enabled, we expect to see a decline in the decode
duration for images decoded with raster. Add UMA to track these
seperately.
BUG=725344
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2904743002
Cr-Commit-Position: refs/heads/master@{#476527}
Committed: https://chromium.googlesource.com/chromium/src/+/97474434c7f06805f9bdc97c2848ef25cf07f550
Patch Set 1 #
Total comments: 6
Patch Set 2 : .. #
Total comments: 8
Patch Set 3 : addressed comments #
Total comments: 8
Patch Set 4 : addressed comments #
Total comments: 2
Patch Set 5 : move #
Messages
Total messages: 30 (11 generated)
Description was changed from ========== cc: Add UMA for tracking decode duration for out of raster decodes. We currently bucket UMA for all decode tasks in a single histogram. With checker-imaging enabled, we expect to see a decline in the decode duration for images decoded with raster. Add UMA to track these seperately. BUG=725344 ========== to ========== cc: Add UMA for tracking decode duration for out of raster decodes. We currently bucket UMA for all decode tasks in a single histogram. With checker-imaging enabled, we expect to see a decline in the decode duration for images decoded with raster. Add UMA to track these seperately. BUG=725344 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + vmpstr@chromium.org
khushalsagar@chromium.org changed reviewers: + enne@chromium.org
I didn't realize vmpstr@ is OOO. enne@, could you take a look at this one?
khushalsagar@chromium.org changed reviewers: + danakj@chromium.org
And one more bounce. Dana, could you take a look? :P
pingy.
https://codereview.chromium.org/2904743002/diff/1/cc/tiles/image_decode_cache.h File cc/tiles/image_decode_cache.h (right): https://codereview.chromium.org/2904743002/diff/1/cc/tiles/image_decode_cache... cc/tiles/image_decode_cache.h:39: IN_RASTER, enums should be kFoo style now https://codereview.chromium.org/2904743002/diff/1/cc/tiles/image_decode_cache... cc/tiles/image_decode_cache.h:52: return ScopedTaskType::CHECKER_IMAGING; I think itd be less confusing if these names matched maybe? https://codereview.chromium.org/2904743002/diff/1/cc/tiles/image_decode_cache... cc/tiles/image_decode_cache.h:62: struct TracingInfo { With these changes, this struct now has 4 constructors. a) Can we not mix overloading + default values? b) Do we need so many?
https://codereview.chromium.org/2904743002/diff/1/cc/tiles/image_decode_cache.h File cc/tiles/image_decode_cache.h (right): https://codereview.chromium.org/2904743002/diff/1/cc/tiles/image_decode_cache... cc/tiles/image_decode_cache.h:39: IN_RASTER, On 2017/05/26 22:36:23, danakj wrote: > enums should be kFoo style now Done. https://codereview.chromium.org/2904743002/diff/1/cc/tiles/image_decode_cache... cc/tiles/image_decode_cache.h:52: return ScopedTaskType::CHECKER_IMAGING; On 2017/05/26 22:36:23, danakj wrote: > I think itd be less confusing if these names matched maybe? Done. https://codereview.chromium.org/2904743002/diff/1/cc/tiles/image_decode_cache... cc/tiles/image_decode_cache.h:62: struct TracingInfo { On 2017/05/26 22:36:23, danakj wrote: > With these changes, this struct now has 4 constructors. > a) Can we not mix overloading + default values? > b) Do we need so many? I just removed both.
https://codereview.chromium.org/2904743002/diff/20001/cc/base/devtools_instru... File cc/base/devtools_instrumentation.h (right): https://codereview.chromium.org/2904743002/diff/20001/cc/base/devtools_instru... cc/base/devtools_instrumentation.h:60: enum Type { SOFTWARE, GPU }; Can you change this to be consistent with the naming, please. Can you also change the enum name to be DecodeType and DecodeTaskType to be TaskType https://codereview.chromium.org/2904743002/diff/20001/cc/base/devtools_instru... cc/base/devtools_instrumentation.h:63: ScopedImageDecodeTask(const void* imagePtr, image_ptr while here https://codereview.chromium.org/2904743002/diff/20001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2904743002/diff/20001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:215: ImageDecodeCache::ImageDecodeType::kCheckerImaging); This is a bit awkward. This means that to use this interface, you need to qualify the caller as one of the available ImageDecodeTypes. What's the reason for distinguishing between say checker imaging and js decode? Can we just say that one source of images comes from the image decode queue, and the other one comes from raster? In other words, I'm not sure what you would be able to infer from checker imaging times vs js decode times (they use the same method of decoding) https://codereview.chromium.org/2904743002/diff/20001/cc/tiles/image_decode_c... File cc/tiles/image_decode_cache.h (right): https://codereview.chromium.org/2904743002/diff/20001/cc/tiles/image_decode_c... cc/tiles/image_decode_cache.h:38: enum class ImageDecodeType { kInRaster, kCheckerImaging, kJSDecode }; I think we should be consistent wrt to what we call a thing like CheckerImages/JSDecode vs Software/GPU. Earlier, you had the _decode_ type as software/gpu and _task_ type as this. Here, it's the _decode_ type that's checker/js and not the task. It's a bit confusing.
https://codereview.chromium.org/2904743002/diff/20001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2904743002/diff/20001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:215: ImageDecodeCache::ImageDecodeType::kCheckerImaging); On 2017/05/30 20:15:12, vmpstr wrote: > This is a bit awkward. This means that to use this interface, you need to > qualify the caller as one of the available ImageDecodeTypes. > > What's the reason for distinguishing between say checker imaging and js decode? > Can we just say that one source of images comes from the image decode queue, and > the other one comes from raster? > > In other words, I'm not sure what you would be able to infer from checker > imaging times vs js decode times (they use the same method of decoding) Since we use heuristics to decide whether images should be checkered or not, I wanted to see that we generally checker only on long decodes. And whether that logic needs more fine-tuning. Do you think that's unnecessary?
On 2017/05/30 21:30:33, Khushal wrote: > https://codereview.chromium.org/2904743002/diff/20001/cc/tiles/checker_image_... > File cc/tiles/checker_image_tracker.cc (right): > > https://codereview.chromium.org/2904743002/diff/20001/cc/tiles/checker_image_... > cc/tiles/checker_image_tracker.cc:215: > ImageDecodeCache::ImageDecodeType::kCheckerImaging); > On 2017/05/30 20:15:12, vmpstr wrote: > > This is a bit awkward. This means that to use this interface, you need to > > qualify the caller as one of the available ImageDecodeTypes. > > > > What's the reason for distinguishing between say checker imaging and js > decode? > > Can we just say that one source of images comes from the image decode queue, > and > > the other one comes from raster? > > > > In other words, I'm not sure what you would be able to infer from checker > > imaging times vs js decode times (they use the same method of decoding) > > Since we use heuristics to decide whether images should be checkered or not, I > wanted to see that we generally checker only on long decodes. And whether that > logic needs more fine-tuning. Do you think that's unnecessary? I think given different machines that this will run on, you'll find a pretty varied distribution of times. That coupled with the fact that we expect to see far more checker cases that img.decode cases makes me believe that we can just clump the two together... I'd rather have simpler code than add complexity for the sake of UMA
Okay. I bucketed them in one OutOfRaster UMA. PTAL. https://codereview.chromium.org/2904743002/diff/20001/cc/base/devtools_instru... File cc/base/devtools_instrumentation.h (right): https://codereview.chromium.org/2904743002/diff/20001/cc/base/devtools_instru... cc/base/devtools_instrumentation.h:60: enum Type { SOFTWARE, GPU }; On 2017/05/30 20:15:12, vmpstr wrote: > Can you change this to be consistent with the naming, please. > > Can you also change the enum name to be DecodeType and DecodeTaskType to be > TaskType Done. https://codereview.chromium.org/2904743002/diff/20001/cc/base/devtools_instru... cc/base/devtools_instrumentation.h:63: ScopedImageDecodeTask(const void* imagePtr, On 2017/05/30 20:15:12, vmpstr wrote: > image_ptr while here Done. https://codereview.chromium.org/2904743002/diff/20001/cc/tiles/image_decode_c... File cc/tiles/image_decode_cache.h (right): https://codereview.chromium.org/2904743002/diff/20001/cc/tiles/image_decode_c... cc/tiles/image_decode_cache.h:38: enum class ImageDecodeType { kInRaster, kCheckerImaging, kJSDecode }; On 2017/05/30 20:15:12, vmpstr wrote: > I think we should be consistent wrt to what we call a thing like > CheckerImages/JSDecode vs Software/GPU. Earlier, you had the _decode_ type as > software/gpu and _task_ type as this. Here, it's the _decode_ type that's > checker/js and not the task. It's a bit confusing. Done.
pingy.
lgtm thanks! https://codereview.chromium.org/2904743002/diff/40001/cc/base/devtools_instru... File cc/base/devtools_instrumentation.h (right): https://codereview.chromium.org/2904743002/diff/40001/cc/base/devtools_instru... cc/base/devtools_instrumentation.h:66: : type_(type), nit: decode_type_(decode_type) https://codereview.chromium.org/2904743002/diff/40001/cc/tiles/image_decode_c... File cc/tiles/image_decode_cache.h (right): https://codereview.chromium.org/2904743002/diff/40001/cc/tiles/image_decode_c... cc/tiles/image_decode_cache.h:49: TracingInfo() : TracingInfo(0, TilePriority::NOW, TaskType::kInRaster) {} While here, can you just = default this and put the initializers inline below? Does that work with const members? https://codereview.chromium.org/2904743002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2904743002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58701: <histogram name="Renderer4.ImageDecodeTaskDurationUs" units="microseconds"> There was some magic with suffixes that I think you can use here. Or maybe this is OK. I'll defer to histograms owners on this.
khushalsagar@chromium.org changed reviewers: + isherman@chromium.org
Thanks! +isherman@ for histograms.xml. https://codereview.chromium.org/2904743002/diff/40001/cc/base/devtools_instru... File cc/base/devtools_instrumentation.h (right): https://codereview.chromium.org/2904743002/diff/40001/cc/base/devtools_instru... cc/base/devtools_instrumentation.h:66: : type_(type), On 2017/06/01 21:40:56, vmpstr wrote: > nit: decode_type_(decode_type) Done. https://codereview.chromium.org/2904743002/diff/40001/cc/tiles/image_decode_c... File cc/tiles/image_decode_cache.h (right): https://codereview.chromium.org/2904743002/diff/40001/cc/tiles/image_decode_c... cc/tiles/image_decode_cache.h:49: TracingInfo() : TracingInfo(0, TilePriority::NOW, TaskType::kInRaster) {} On 2017/06/01 21:40:56, vmpstr wrote: > While here, can you just = default this and put the initializers inline below? > Does that work with const members? Done. https://codereview.chromium.org/2904743002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2904743002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58701: <histogram name="Renderer4.ImageDecodeTaskDurationUs" units="microseconds"> On 2017/06/01 21:40:56, vmpstr wrote: > There was some magic with suffixes that I think you can use here. Or maybe this > is OK. I'll defer to histograms owners on this. Yeah, I don't know how it works with 2 suffixes.
Metrics LGTM https://codereview.chromium.org/2904743002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2904743002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58701: <histogram name="Renderer4.ImageDecodeTaskDurationUs" units="microseconds"> On 2017/06/01 23:45:09, Khushal wrote: > On 2017/06/01 21:40:56, vmpstr wrote: > > There was some magic with suffixes that I think you can use here. Or maybe > this > > is OK. I'll defer to histograms owners on this. > > Yeah, I don't know how it works with 2 suffixes. You can use suffixes recursively, but I think it's fine the way it's written too. https://codereview.chromium.org/2904743002/diff/60001/cc/base/devtools_instru... File cc/base/devtools_instrumentation.h (right): https://codereview.chromium.org/2904743002/diff/60001/cc/base/devtools_instru... cc/base/devtools_instrumentation.h:105: } Why is there so much non-trivial code implemented in the header file? Could this be moved to the .cc file instead?
https://codereview.chromium.org/2904743002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2904743002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58701: <histogram name="Renderer4.ImageDecodeTaskDurationUs" units="microseconds"> On 2017/06/01 23:50:50, Ilya Sherman wrote: > On 2017/06/01 23:45:09, Khushal wrote: > > On 2017/06/01 21:40:56, vmpstr wrote: > > > There was some magic with suffixes that I think you can use here. Or maybe > > this > > > is OK. I'll defer to histograms owners on this. > > > > Yeah, I don't know how it works with 2 suffixes. > > You can use suffixes recursively, but I think it's fine the way it's written > too. Acknowledged. https://codereview.chromium.org/2904743002/diff/60001/cc/base/devtools_instru... File cc/base/devtools_instrumentation.h (right): https://codereview.chromium.org/2904743002/diff/60001/cc/base/devtools_instru... cc/base/devtools_instrumentation.h:105: } On 2017/06/01 23:50:50, Ilya Sherman wrote: > Why is there so much non-trivial code implemented in the header file? Could > this be moved to the .cc file instead? I guess it was just following the rest of the style in this file. Its mostly trivial utility classes or inline functions. This one seems to have gotten somewhat large. Move to .cc file.
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2904743002/#ps80001 (title: "move")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by khushalsagar@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496368318595240,
"parent_rev": "1edede81c91a1057842e803f87ee1393ac5c172f", "commit_rev":
"97474434c7f06805f9bdc97c2848ef25cf07f550"}
Message was sent while issue was closed.
Description was changed from ========== cc: Add UMA for tracking decode duration for out of raster decodes. We currently bucket UMA for all decode tasks in a single histogram. With checker-imaging enabled, we expect to see a decline in the decode duration for images decoded with raster. Add UMA to track these seperately. BUG=725344 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Add UMA for tracking decode duration for out of raster decodes. We currently bucket UMA for all decode tasks in a single histogram. With checker-imaging enabled, we expect to see a decline in the decode duration for images decoded with raster. Add UMA to track these seperately. BUG=725344 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2904743002 Cr-Commit-Position: refs/heads/master@{#476527} Committed: https://chromium.googlesource.com/chromium/src/+/97474434c7f06805f9bdc97c2848... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/97474434c7f06805f9bdc97c2848... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
