Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887453002:
SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed (Closed)
|
Created:
3 years, 7 months ago by Fady Samuel Modified:
3 years, 6 months ago Reviewers:
vmpstr CC:
chromium-reviews, cc-bugs_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed
Always observing deadlines could potentially prevent the CPU from idling because
that requires a timer to always fire. This CL only observes BeginFrames if there are
unresolved dependencies.
A new object type, SurfaceDependencyDeadline is introduced that
observes BeginFrames within its scope and reports back to
SurfaceDependencyTracker when a deadline hits. The deadline must be
canceled prior to going out of scope or else a DCHECK will fail.
BUG=722064, 672962, 727162
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2887453002
Cr-Commit-Position: refs/heads/master@{#475601}
Committed: https://chromium.googlesource.com/chromium/src/+/505078044cfca8e67070b4daed6e6157d7641774
Patch Set 1 #
Total comments: 2
Patch Set 2 : Updated #Patch Set 3 : Removed BeginFrameSource include #Patch Set 4 : Fixed build #Patch Set 5 : Fixed an incorrect DCHECK #
Messages
Total messages: 34 (28 generated)
Description was changed from ========== SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. BUG=722064, 672962 ========== to ========== SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. BUG=722064, 672962 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
fsamuel@chromium.org changed reviewers: + vmpstr@chromium.org
PTAL Vlad!
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
look good with the suggestion below. Theoretically I agree with the patch, but do you have any practical perf numbers to show that this is a win? This does add a bit of complexity to the code, so I want to see if there is a real benefit. https://codereview.chromium.org/2887453002/diff/1/cc/surfaces/surface_depende... File cc/surfaces/surface_dependency_tracker.cc (right): https://codereview.chromium.org/2887453002/diff/1/cc/surfaces/surface_depende... cc/surfaces/surface_dependency_tracker.cc:54: begin_frame_source_->AddObserver(this); I wonder if we should have some sort of a scoped object that would remove itself from the begin_frame_source_ observer list on destruction. My concern is that it's hard to reason whether any of the operations that remove the observer are guaranteed to happen before the dtor of this class is called. You would also not need the && frames_since_deadline_set_ checks, since you can just reset the observer thing when it's already empty without any bad behavior.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. BUG=722064, 672962 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. A new object type, SurfaceDependencyDeadline is introduced that observes BeginFrames within its scope and reports back to SurfaceDependencyTracker when a deadline hits. The deadline must be canceled prior to going out of scope or else a DCHECK will fail. BUG=722064, 672962 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL Vlad, once you're back. Note that this addresses some regressions. https://codereview.chromium.org/2887453002/diff/1/cc/surfaces/surface_depende... File cc/surfaces/surface_dependency_tracker.cc (right): https://codereview.chromium.org/2887453002/diff/1/cc/surfaces/surface_depende... cc/surfaces/surface_dependency_tracker.cc:54: begin_frame_source_->AddObserver(this); On 2017/05/15 15:49:39, vmpstr_ooo wrote: > I wonder if we should have some sort of a scoped object that would remove itself > from the begin_frame_source_ observer list on destruction. > > My concern is that it's hard to reason whether any of the operations that remove > the observer are guaranteed to happen before the dtor of this class is called. > > You would also not need the && frames_since_deadline_set_ checks, since you can > just reset the observer thing when it's already empty without any bad behavior. I've introduced a SurfaceDependencyDeadline to deal with this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. A new object type, SurfaceDependencyDeadline is introduced that observes BeginFrames within its scope and reports back to SurfaceDependencyTracker when a deadline hits. The deadline must be canceled prior to going out of scope or else a DCHECK will fail. BUG=722064, 672962 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. A new object type, SurfaceDependencyDeadline is introduced that observes BeginFrames within its scope and reports back to SurfaceDependencyTracker when a deadline hits. The deadline must be canceled prior to going out of scope or else a DCHECK will fail. BUG=722064, 672962 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
That's a really nice class, thanks! lgtm.
The CQ bit was checked by fsamuel@chromium.org
The CQ bit was unchecked by fsamuel@chromium.org
The CQ bit was checked by fsamuel@chromium.org
The CQ bit was unchecked by fsamuel@chromium.org
Description was changed from ========== SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. A new object type, SurfaceDependencyDeadline is introduced that observes BeginFrames within its scope and reports back to SurfaceDependencyTracker when a deadline hits. The deadline must be canceled prior to going out of scope or else a DCHECK will fail. BUG=722064, 672962 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. A new object type, SurfaceDependencyDeadline is introduced that observes BeginFrames within its scope and reports back to SurfaceDependencyTracker when a deadline hits. The deadline must be canceled prior to going out of scope or else a DCHECK will fail. BUG=722064, 672962, 727162 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by fsamuel@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": 1496168153084840,
"parent_rev": "d771e4b28b5878d005596283b88a3296518abf65", "commit_rev":
"505078044cfca8e67070b4daed6e6157d7641774"}
Message was sent while issue was closed.
Description was changed from ========== SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. A new object type, SurfaceDependencyDeadline is introduced that observes BeginFrames within its scope and reports back to SurfaceDependencyTracker when a deadline hits. The deadline must be canceled prior to going out of scope or else a DCHECK will fail. BUG=722064, 672962, 727162 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. A new object type, SurfaceDependencyDeadline is introduced that observes BeginFrames within its scope and reports back to SurfaceDependencyTracker when a deadline hits. The deadline must be canceled prior to going out of scope or else a DCHECK will fail. BUG=722064, 672962, 727162 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2887453002 Cr-Commit-Position: refs/heads/master@{#475601} Committed: https://chromium.googlesource.com/chromium/src/+/505078044cfca8e67070b4daed6e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/505078044cfca8e67070b4daed6e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
