Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2960933002:
tools/benchmarks: Fix failures in image decoding measurements. (Closed)
Descriptiontools/benchmarks: Fix failures in image decoding measurements.
The test page uses requestAnimationFrames as a signal to rely on an
image decode completing after it has been painted by blink, which is
precisely what checker-imaging would avoid, i.e., blocking pushing
frames from blink on an image decode.
Since these tests solely want to measure the decode performance,
disable the feature for them.
BUG=736793
R=piman@chromium.org,nednguyen@google.com
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2960933002
Cr-Commit-Position: refs/heads/master@{#483254}
Committed: https://chromium.googlesource.com/chromium/src/+/83e99d0e9ce163a5b78e67d17768ed9034721ef4
Patch Set 1 #
Total comments: 5
Patch Set 2 : .. #
Messages
Total messages: 27 (13 generated)
Description was changed from ========== tools/benchmarks: Fix failures in image decoding measurements. The test page uses requestAnimationFrames as a signal to rely on an image decode completing after it has been painted by blink, which is precisely what checker-imaging would avoid, i.e., blocking pushing frames from blink on an image decode. Since these tests solely want to measure the decode performance, disable the feature for them. BUG=736793 R=piman@chromium.org,nednguyen@google.com ========== to ========== tools/benchmarks: Fix failures in image decoding measurements. The test page uses requestAnimationFrames as a signal to rely on an image decode completing after it has been painted by blink, which is precisely what checker-imaging would avoid, i.e., blocking pushing frames from blink on an image decode. Since these tests solely want to measure the decode performance, disable the feature for them. BUG=736793 R=piman@chromium.org,nednguyen@google.com CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== tools/benchmarks: Fix failures in image decoding measurements. The test page uses requestAnimationFrames as a signal to rely on an image decode completing after it has been painted by blink, which is precisely what checker-imaging would avoid, i.e., blocking pushing frames from blink on an image decode. Since these tests solely want to measure the decode performance, disable the feature for them. BUG=736793 R=piman@chromium.org,nednguyen@google.com CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== tools/benchmarks: Fix failures in image decoding measurements. The test page uses requestAnimationFrames as a signal to rely on an image decode completing after it has been painted by blink, which is precisely what checker-imaging would avoid, i.e., blocking pushing frames from blink on an image decode. Since these tests solely want to measure the decode performance, disable the feature for them. BUG=736793 R=piman@chromium.org,nednguyen@google.com CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + enne@chromium.org - piman@chromium.org
Description was changed from ========== tools/benchmarks: Fix failures in image decoding measurements. The test page uses requestAnimationFrames as a signal to rely on an image decode completing after it has been painted by blink, which is precisely what checker-imaging would avoid, i.e., blocking pushing frames from blink on an image decode. Since these tests solely want to measure the decode performance, disable the feature for them. BUG=736793 R=piman@chromium.org,nednguyen@google.com CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== tools/benchmarks: Fix failures in image decoding measurements. The test page uses requestAnimationFrames as a signal to rely on an image decode completing after it has been painted by blink, which is precisely what checker-imaging would avoid, i.e., blocking pushing frames from blink on an image decode. Since these tests solely want to measure the decode performance, disable the feature for them. BUG=736793 R=enne@chromium.org,nednguyen@google.com TBR=piman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== tools/benchmarks: Fix failures in image decoding measurements. The test page uses requestAnimationFrames as a signal to rely on an image decode completing after it has been painted by blink, which is precisely what checker-imaging would avoid, i.e., blocking pushing frames from blink on an image decode. Since these tests solely want to measure the decode performance, disable the feature for them. BUG=736793 R=enne@chromium.org,nednguyen@google.com TBR=piman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== tools/benchmarks: Fix failures in image decoding measurements. The test page uses requestAnimationFrames as a signal to rely on an image decode completing after it has been painted by blink, which is precisely what checker-imaging would avoid, i.e., blocking pushing frames from blink on an image decode. Since these tests solely want to measure the decode performance, disable the feature for them. BUG=736793 R=piman@chromium.org,nednguyen@google.com CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + piman@chromium.org - enne@chromium.org
nednguyen@google.com changed reviewers: + cblume@chromium.org
On 2017/06/27 22:53:13, Khushal wrote: Chris is working on overhaul the image decoder benchmark, so I think he should review this CL & make sure that this fits into his plan.
lgtm
Non-owner LGTM https://codereview.chromium.org/2960933002/diff/1/content/browser/gpu/composi... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/2960933002/diff/1/content/browser/gpu/composi... content/browser/gpu/compositor_util.cc:291: if (base::CommandLine::ForCurrentProcess()->HasSwitch( Just to make sure I understand this correctly, We have a command line switch to disable. We also have one to enable. So presumably we could do a slow rollout where we enable for a small number of users. But the user could override that and disable if needed. That is, if both disable and enable are specified, the disable wins.
lgtm
https://codereview.chromium.org/2960933002/diff/1/tools/perf/benchmarks/image... File tools/perf/benchmarks/image_decoding.py (right): https://codereview.chromium.org/2960933002/diff/1/tools/perf/benchmarks/image... tools/perf/benchmarks/image_decoding.py:35: '--disable-checker-imaging' Chris: I hope the overhauled version of image decode benchmark will not require this flag? Otherwise, I worry that we may drift away from testing what we ship to the users.
https://codereview.chromium.org/2960933002/diff/1/content/browser/gpu/composi... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/2960933002/diff/1/content/browser/gpu/composi... content/browser/gpu/compositor_util.cc:291: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2017/06/28 19:17:26, cblume wrote: > Just to make sure I understand this correctly, > > We have a command line switch to disable. > We also have one to enable. > > So presumably we could do a slow rollout where we enable for a small number of > users. But the user could override that and disable if needed. > > That is, if both disable and enable are specified, the disable wins. The command line switches are not exposed to the users, there is no entry in chrome:flags for them. These are just for dev testing. We are already doing a slow roll out for this on canary using a finch experiment. https://codereview.chromium.org/2960933002/diff/1/tools/perf/benchmarks/image... File tools/perf/benchmarks/image_decoding.py (right): https://codereview.chromium.org/2960933002/diff/1/tools/perf/benchmarks/image... tools/perf/benchmarks/image_decoding.py:35: '--disable-checker-imaging' On 2017/06/28 19:28:11, nednguyen wrote: > Chris: I hope the overhauled version of image decode benchmark will not require > this flag? Otherwise, I worry that we may drift away from testing what we ship > to the users. If the only thing this benchmark wants to measure is the performance of the decoding algorithm, then this flag should not have an impact. It only impacts how these decodes are scheduled.
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...
https://codereview.chromium.org/2960933002/diff/1/tools/perf/benchmarks/image... File tools/perf/benchmarks/image_decoding.py (right): https://codereview.chromium.org/2960933002/diff/1/tools/perf/benchmarks/image... tools/perf/benchmarks/image_decoding.py:35: '--disable-checker-imaging' On 2017/06/28 20:29:55, Khushal wrote: > On 2017/06/28 19:28:11, nednguyen wrote: > > Chris: I hope the overhauled version of image decode benchmark will not > require > > this flag? Otherwise, I worry that we may drift away from testing what we ship > > to the users. > > If the only thing this benchmark wants to measure is the performance of the > decoding algorithm, then this flag should not have an impact. It only impacts > how these decodes are scheduled. I think our concern is if the existing perf was thrown off, the new, overhauled version will be thrown off as well. And to add to the concern, I don't think the new version has a way to specify flags like this. It might force us to either 1.) find a better way to get the decoder time to the page, or 2.) abandon this test (which has been a real consideration for a while now)
Oh also.... Once the Finch experiment is done, we presumably plan to not have the enable flag and just enable it by default, right? Which means there would still be the disable flag? At some point, do we plan to get rid of the disable flag? Presumably, doing so means we would need to figure out how to get the correct decode timing routed. If I land the revamp soon, it'll mess up timing again and we'll need to figure out that routing.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/06/28 21:22:03, cblume wrote: > Oh also.... > > Once the Finch experiment is done, we presumably plan to not have the enable > flag and just enable it by default, right? > Which means there would still be the disable flag? There is no need to remove the disable flag if something needs to use it. > > At some point, do we plan to get rid of the disable flag? Presumably, doing so > means we would need to figure out how to get the correct decode timing routed. > > If I land the revamp soon, it'll mess up timing again and we'll need to figure > out that routing. Could you share the revamp patch? We can figure out a way to restructure it so this feature doesn't get in the way.
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, cblume@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2960933002/#ps20001 (title: "..")
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": 20001, "attempt_start_ts": 1498694605026930,
"parent_rev": "3002e5d40231945950c3b96a905662d412212d84", "commit_rev":
"83e99d0e9ce163a5b78e67d17768ed9034721ef4"}
Message was sent while issue was closed.
Description was changed from ========== tools/benchmarks: Fix failures in image decoding measurements. The test page uses requestAnimationFrames as a signal to rely on an image decode completing after it has been painted by blink, which is precisely what checker-imaging would avoid, i.e., blocking pushing frames from blink on an image decode. Since these tests solely want to measure the decode performance, disable the feature for them. BUG=736793 R=piman@chromium.org,nednguyen@google.com CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== tools/benchmarks: Fix failures in image decoding measurements. The test page uses requestAnimationFrames as a signal to rely on an image decode completing after it has been painted by blink, which is precisely what checker-imaging would avoid, i.e., blocking pushing frames from blink on an image decode. Since these tests solely want to measure the decode performance, disable the feature for them. BUG=736793 R=piman@chromium.org,nednguyen@google.com CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2960933002 Cr-Commit-Position: refs/heads/master@{#483254} Committed: https://chromium.googlesource.com/chromium/src/+/83e99d0e9ce163a5b78e67d17768... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/83e99d0e9ce163a5b78e67d17768... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
