Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1909063003:
ImageQualityController should not post hundreds of canceled tasks. (Closed)
|
Created:
4 years, 8 months ago by esprehn Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImageQualityController should not post hundreds of canceled tasks.
The MessageLoop can't actually cancel tasks, we just cancel the task in blink
by nulling out the callback and then repost a new one. ImageQualityController was
calling restartTimer() for every image in the entire page to schedule it's 500ms
timer. For a for a page with many moving images like Animometer we would be
processing hundreds of cancelled tasks between every frame as they got reposted.
This patch avoids reposting the task if one is already scheduled. That seems fine
since the timer only fires every 500ms anyway so waiting until the original fire
time instead of moving the fire time forward by a few ms doesn't really matter.
BUG=605701
Patch Set 1 #
Total comments: 1
Messages
Total messages: 18 (8 generated)
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1909063003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1909063003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== ImageQualityController should not post hundreds of canceled tasks. BUG= ========== to ========== ImageQualityController should not post hundreds of canceled tasks. The MessageLoop can't actually cancel tasks, we just cancel the task in blink by nulling out the callback and then repost a new one. ImageQualityController was calling restartTimer() for every image in the entire page to schedule it's 500ms timer. For a for a page with many moving images like Animometer we would be processing hundreds of cancelled tasks between every frame as they got reposted. This patch avoids reposting the task if one is already scheduled. That seems fine since the timer only fires every 500ms anyway so waiting until the original fire time instead of moving the fire time forward by a few ms doesn't really matter. BUG=605701 ==========
Description was changed from ========== ImageQualityController should not post hundreds of canceled tasks. The MessageLoop can't actually cancel tasks, we just cancel the task in blink by nulling out the callback and then repost a new one. ImageQualityController was calling restartTimer() for every image in the entire page to schedule it's 500ms timer. For a for a page with many moving images like Animometer we would be processing hundreds of cancelled tasks between every frame as they got reposted. This patch avoids reposting the task if one is already scheduled. That seems fine since the timer only fires every 500ms anyway so waiting until the original fire time instead of moving the fire time forward by a few ms doesn't really matter. BUG=605701 ========== to ========== ImageQualityController should not post hundreds of canceled tasks. The MessageLoop can't actually cancel tasks, we just cancel the task in blink by nulling out the callback and then repost a new one. ImageQualityController was calling restartTimer() for every image in the entire page to schedule it's 500ms timer. For a for a page with many moving images like Animometer we would be processing hundreds of cancelled tasks between every frame as they got reposted. This patch avoids reposting the task if one is already scheduled. That seems fine since the timer only fires every 500ms anyway so waiting until the original fire time instead of moving the fire time forward by a few ms doesn't really matter. BUG=605701 ==========
esprehn@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org
dglazkov@chromium.org changed reviewers: + dglazkov@chromium.org
lgtm
chrishtr@chromium.org changed reviewers: - dglazkov@chromium.org
https://codereview.chromium.org/1909063003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ImageQualityController.cpp (right): https://codereview.chromium.org/1909063003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ImageQualityController.cpp:145: // Avoid reposting the timer if it's already active. While it does cancel restartTimer() is called in order to push the 500ms period forward. If you don't call it, then after 500ms of continuous animation the timer will expire, at which point a high-quality resize will happen. Instead I think something is needed to only push it forward if most of the time of the timer has expired.
chrishtr@chromium.org changed reviewers: + dglazkov@chromium.org
Also: I can do this for you if you like, to increase paralleism.
On 2016/04/21 at 21:00:04, chrishtr wrote: > Also: I can do this for you if you like, to increase paralleism. We should really just remove this timer in general, issuing random repaints after 500ms doesn't make sense, it should be tied to pumping frames or something. I think this patch works though, the 500ms just runs the for loop which looks for isResizing, if anything is still resizing it won't repaint. So we don't need to keep pushing the timer out?
And sure you can take this, note that calling ::Now() is very expensive, you don't want to do that thousands of times. So we can't just keeping calling ::Now() to look at how much time has elapsed.
On 2016/04/21 at 22:35:00, esprehn wrote: > On 2016/04/21 at 21:00:04, chrishtr wrote: > > Also: I can do this for you if you like, to increase paralleism. > > We should really just remove this timer in general, issuing random repaints after 500ms doesn't make sense, it should be tied to pumping frames or something. I think this patch works though, the 500ms just runs the for loop which looks for isResizing, if anything is still resizing it won't repaint. So we don't need to keep pushing the timer out? What if the animation runs for more than 500ms though? The current code avoids re-painting at high quality until the animation is done (no matter how long it takes), plus 500ms. The code you wrote will I think re-paint all images every 500ms. Also, the for loop checks for !isResizing and avoids invalidating such objects.
On 2016/04/21 at 22:36:04, esprehn wrote: > And sure you can take this, note that calling ::Now() is very expensive, you don't want to do that thousands of times. So we can't just keeping calling ::Now() to look at how much time has elapsed. Ok I'll work on it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
