Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
83183005:
Add synthetic delay points for latency testing (Closed)
|
Created:
7 years ago by Sami Modified:
6 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, jam, piman+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd synthetic delay points for latency testing
Add synthetic delay points for simulating long computation in the
following parts of the graphics/input pipeline:
1. blink.HandleInputEvent - Expensive JavaScript input event handling.
2. cc.RasterRequiredForActivation - Long running raster tasks.
3. cc.BeginMainFrame - Complex main thread picture recording, layout or
rAF.
4. gpu.AsyncTexImage - Slow texture uploads.
5. gpu.SwapBuffers - GPU-bound rendering.
BUG=307841
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244175
Patch Set 1 #
Total comments: 5
Patch Set 2 : Avoid static initializer on raster worker pool critical path. #Patch Set 3 : Begin delay on one thread and end it on another. #
Total comments: 1
Patch Set 4 : Unconditional reset. #Patch Set 5 : Clarify effects of race condition. #
Total comments: 2
Patch Set 6 : Cleaner raster worker pool integration. #
Total comments: 6
Patch Set 7 : Also count number of image tasks needed for activation. #
Total comments: 7
Patch Set 8 : Address David's comments. #
Total comments: 2
Patch Set 9 : No need to loop over tasks in ImageRasterWorkerPool. #
Total comments: 2
Patch Set 10 : Fix nits. #Patch Set 11 : Also instrument share group uploads. #Patch Set 12 : Rebased. Fixed PerfRasterWorkerPool. #Messages
Total messages: 40 (0 generated)
Here are the delay points I added for scheduler testing. I may need to split this up for landing but I wanted to hear if any of this gives you guys allergic reactions first.
https://codereview.chromium.org/83183005/diff/1/cc/resources/raster_worker_po... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/1/cc/resources/raster_worker_po... cc/resources/raster_worker_pool.cc:111: TRACE_EVENT_SYNTHETIC_DELAY_ACTIVATE("cc.RasterRequiredForActivation"); so i'm worried about how many traces we've got on the raster workers at this point. i think there are two already, this makes the third. is this basically inevitable and I should quit whining? Is there a way to initialize the delay object elsewhere and pass it in as an argument? static initializers have a runtime penalty basically
https://codereview.chromium.org/83183005/diff/1/cc/resources/raster_worker_po... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/1/cc/resources/raster_worker_po... cc/resources/raster_worker_pool.cc:306: TRACE_EVENT_SYNTHETIC_DELAY_APPLY("cc.RasterRequiredForActivation"); I'm not up-to-date on the lates TileManager code. Does this apply the delay on the last tile required for raterization, or does it apply the delay to the first tile(s)?
https://codereview.chromium.org/83183005/diff/1/cc/resources/raster_worker_po... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/1/cc/resources/raster_worker_po... cc/resources/raster_worker_pool.cc:111: TRACE_EVENT_SYNTHETIC_DELAY_ACTIVATE("cc.RasterRequiredForActivation"); On 2013/11/24 19:45:18, nduca wrote: > so i'm worried about how many traces we've got on the raster workers at this > point. i think there are two already, this makes the third. is this basically > inevitable and I should quit whining? Is there a way to initialize the delay > object elsewhere and pass it in as an argument? > > static initializers have a runtime penalty basically Yeah, we should be careful with this stuff in tight places. I've now made this delay a global that gets initialized when the raster worker pool gets constructed. The remaining work on the critical path is now a pointer deref and a 64-bit compare against zero (base::TimeDelta). https://codereview.chromium.org/83183005/diff/1/cc/resources/raster_worker_po... cc/resources/raster_worker_pool.cc:306: TRACE_EVENT_SYNTHETIC_DELAY_APPLY("cc.RasterRequiredForActivation"); On 2013/11/26 03:14:54, Brian Anderson wrote: > I'm not up-to-date on the lates TileManager code. Does this apply the delay on > the last tile required for raterization, or does it apply the delay to the first > tile(s)? The way it roughly works is that we first schedule a bunch of RasterWorkerPoolTaskImpls to do the rasterization and then one (or more) sentinel tasks -- RasterFinishedWorkerPoolTaskImpl -- at the end to detect when the tasks that were required for activation have finished (see PixelBufferRasterWorkerPool::ScheduleMoreTasks). Like this: [raster] [raster] [raster] [raster_finished] The synthetic delay here applies to the sentinel task by delaying the notification from it to the compositor thread. This means that the raster tasks themselves run on time without delays, but the final activation signal is postponed.
https://codereview.chromium.org/83183005/diff/1/cc/resources/raster_worker_po... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/1/cc/resources/raster_worker_po... cc/resources/raster_worker_pool.cc:306: TRACE_EVENT_SYNTHETIC_DELAY_APPLY("cc.RasterRequiredForActivation"); On 2013/12/02 14:57:08, Sami wrote: > On 2013/11/26 03:14:54, Brian Anderson wrote: > > I'm not up-to-date on the lates TileManager code. Does this apply the delay on > > the last tile required for raterization, or does it apply the delay to the > first > > tile(s)? > > The way it roughly works is that we first schedule a bunch of > RasterWorkerPoolTaskImpls to do the rasterization and then one (or more) > sentinel tasks -- RasterFinishedWorkerPoolTaskImpl -- at the end to detect when > the tasks that were required for activation have finished (see > PixelBufferRasterWorkerPool::ScheduleMoreTasks). Like this: > > [raster] [raster] [raster] [raster_finished] > > The synthetic delay here applies to the sentinel task by delaying the > notification from it to the compositor thread. This means that the raster tasks > themselves run on time without delays, but the final activation signal is > postponed. Ah ok. I see now. I'm assuming the graph is set up such that the activation task is only called after the last raster required for activation even when there are multiple raster threads? If not, it'll be better to fix that as separate patch anyway, so this patch lgtm.
On 2013/12/03 03:00:30, Brian Anderson wrote: > Ah ok. I see now. I'm assuming the graph is set up such that the activation task > is only called after the last raster required for activation even when there are > multiple raster threads? If not, it'll be better to fix that as separate patch > anyway, so this patch lgtm. Yes, that should be the case if I understood the worker pool implementation correctly. It wouldn't make much sense to activate before the prerequisite tasks have finished. One problem with multiple raster workers is that if the activation task runs on a different thread than the raster tasks themselves, the delay won't get applied. I'm not sure how to best solve that -- perhaps just by adding a static delay to the activation task itself.
In a forehead-slapping moment today I realized that making it possible to start the delay interval on one thread and end it on another makes the integration code trivial. For example with raster tasks we just say that we expect N tasks before activation and the last task to run will incur the delay. Much cleaner, no?
On 2013/12/06 21:19:01, Sami wrote: > In a forehead-slapping moment today I realized that making it possible to start > the delay interval on one thread and end it on another makes the integration > code trivial. For example with raster tasks we just say that we expect N tasks > before activation and the last task to run will incur the delay. Much cleaner, > no? Good idea. One concern inline though.
https://codereview.chromium.org/83183005/diff/80001/base/debug/trace_event_sy... File base/debug/trace_event_synthetic_delay.cc (right): https://codereview.chromium.org/83183005/diff/80001/base/debug/trace_event_sy... base/debug/trace_event_synthetic_delay.cc:90: void TraceEventSyntheticDelay::BeginInternal(int begin_delta, bool reset) { When reset is true, is it possible that there is a lingering long-running task on one of the worker threads from the old graph? Does any of the logic prevent that long-running task from decrementing the new begin_count_?
> Does any of the logic prevent that long-running task from decrementing the new > begin_count_? You're right, there isn't anything preventing that, and so far I haven't come up with an elegant way of fixing it. I did play around with this a little today and noticed that this really only happens when the raster threads are 100% busy anyway. And in that case it doesn't make a huge difference where the synthetic delays end up :) Essentially normally the begin_count_ works like this: impl [3] [activate] rast [...2] [...1] [...0] [delay] With a long running task the first tree activates sooner, but all further activations are delayed: impl [3] [3] [activate] [activate] rast [...2] [...1] [...2] [...1] [...0] [delay] [...0] This way the average can end up being a little higher than the configured delay, but from my tests the error was about 2 ms for a 50 ms activation delay. Since this doesn't really happen with our test suite since everything there is intrinsically cheap, I'm wondering if it's worth trying to fix this. It's also in line with how texture upload delays work; if we are constantly busy uploading textures then no delays will get inserted. WDYT?
On 2013/12/10 19:47:22, Sami wrote: > > Does any of the logic prevent that long-running task from decrementing the new > > begin_count_? > > You're right, there isn't anything preventing that, and so far I haven't come up > with an elegant way of fixing it. I did play around with this a little today and > noticed that this really only happens when the raster threads are 100% busy > anyway. And in that case it doesn't make a huge difference where the synthetic > delays end up :) > > Essentially normally the begin_count_ works like this: > > impl [3] [activate] > rast [...2] [...1] [...0] [delay] > > With a long running task the first tree activates sooner, but all further > activations are delayed: > > impl [3] [3] [activate] [activate] > rast [...2] [...1] [...2] [...1] [...0] [delay] [...0] Interesting. I did not anticipate the difference in delays between the first and subsequent activation signals. In this case though, the first activate signal should no longer be valid since there is a new graph. I'm assuming the first activate signal is suppressed before it reaches the scheduler, otherwise we'd see an early activation. > > This way the average can end up being a little higher than the configured delay, > but from my tests the error was about 2 ms for a 50 ms activation delay. > > Since this doesn't really happen with our test suite since everything there is > intrinsically cheap, I'm wondering if it's worth trying to fix this. It's also > in line with how texture upload delays work; if we are constantly busy uploading > textures then no delays will get inserted. > > WDYT? The side-effects aren't bad, so I'm open to living with the race but would like to make sure there isn't a simple solution. Would you be able to go back to just applying the delay in the activation task, but with the cross-thread support you added in recent patches? You would have to make the delay dependent on a new timestamp in the activation task to avoid stomping the global timestamp, but then you might be able to get rid of the global all together? If we decide to live with the race, please add a comment explaining why it's not an issue.
> Interesting. I did not anticipate the difference in delays between the first and > subsequent activation signals. > > In this case though, the first activate signal should no longer be valid since > there is a new graph. I'm assuming the first activate signal is suppressed > before it reaches the scheduler, otherwise we'd see an early activation. Yeah, sorry, that was me reading the traces wrong, there's no early activation. Instead what happens is that we schedule a bunch of tasks for rasterization and then some time later replace them with different tasks because of changed tile priorities. This reshuffling just pushes the actual activation further out. This is also where the race condition happens. One task from the old set ends up decrementing the begin_count_ on behalf of one task from the new set (more if there are several raster tasks), and the delay for the new task set gets applied one task too soon. That means the activation from the second task set is delayed by O(length_of_task) + the configured delay. Since this doesn't violate the expectation that tree activation takes at least as long as the configured delay, I think this feels acceptable. Also, the synthetic delays get applied every time we change the raster task set which I also think is realistic. > Would you be able to go back to just applying the delay in the activation task, > but with the cross-thread support you added in recent patches? You would have to > make the delay dependent on a new timestamp in the activation task to avoid > stomping the global timestamp, but then you might be able to get rid of the > global all together? I tried that but turns out the activation tasks don't work like I thought they did :( The impl thread also polls for completed raster tasks every 6 ms, so delaying the activation task doesn't actually delay tree activation. Looks like the activation task is just used to reduce latency when polling doesn't coincide with the tasks finishing. > If we decide to live with the race, please add a comment explaining why it's not > an issue. Will do. I'll still try to think of ways to avoid this but it doesn't look easy :\
On 2013/12/11 18:10:24, Sami wrote: > I'll still try to think of ways to avoid this but it doesn't look easy I think I've gotten to the bottom of this now. When we change the raster task set, some of the tasks from the older set may still be required for activation, which means we can't completely ignore them when balancing the calls to Begin/End for the new set. If we did, we might overcount and end up not applying the delay at all. I therefore think having the delay sometimes be longer is the better option here. I've added a comment about this.
David, does this way of hooking up synthetic delays to raster_worker_pool.cc seem ok to you?
https://codereview.chromium.org/83183005/diff/120001/cc/resources/raster_work... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/120001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:477: "cc.RasterRequiredForActivation"); what if this is done on two threads at the same time? can we use a LazyInstance to make this thread safe? https://codereview.chromium.org/83183005/diff/120001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:506: raster_tasks_required_for_activation_.size()); This seems a bit too flaky to me. Could we do this as part of the RasterFinishedWorkerPoolTaskImpl task instead and never reset it?
On 2013/12/12 19:29:43, David Reveman wrote: > https://codereview.chromium.org/83183005/diff/120001/cc/resources/raster_work... > File cc/resources/raster_worker_pool.cc (right): > > https://codereview.chromium.org/83183005/diff/120001/cc/resources/raster_work... > cc/resources/raster_worker_pool.cc:477: "cc.RasterRequiredForActivation"); > what if this is done on two threads at the same time? can we use a LazyInstance > to make this thread safe? Looking up the delays is thread safe; all threads will end up getting a pointer to the same instance. > https://codereview.chromium.org/83183005/diff/120001/cc/resources/raster_work... > cc/resources/raster_worker_pool.cc:506: > raster_tasks_required_for_activation_.size()); > This seems a bit too flaky to me. Could we do this as part of the > RasterFinishedWorkerPoolTaskImpl task instead and never reset it? That's what I initially went for but the problem is the 6 ms polling loop on the impl thread doesn't wait for RasterFinishedWorkerPoolTaskImpl (and shouldn't!). That meant the delay happily went off on the raster thread but the impl thread was still able to activate before it finished thanks to the polling.
On 2013/12/12 19:45:09, Sami wrote: > On 2013/12/12 19:29:43, David Reveman wrote: > > > https://codereview.chromium.org/83183005/diff/120001/cc/resources/raster_work... > > File cc/resources/raster_worker_pool.cc (right): > > > > > https://codereview.chromium.org/83183005/diff/120001/cc/resources/raster_work... > > cc/resources/raster_worker_pool.cc:477: "cc.RasterRequiredForActivation"); > > what if this is done on two threads at the same time? can we use a > LazyInstance > > to make this thread safe? > > Looking up the delays is thread safe; all threads will end up getting a pointer > to the same instance. Sure but you're assuming that assigning a pointer value to g_raster_required_for_activation_delay is atomic. That might be the case but I don't think we should assume it. In theory, you could be reading g_raster_required_for_activation_delay on one thread while being half way done writing a value to it on another thread. > > > > https://codereview.chromium.org/83183005/diff/120001/cc/resources/raster_work... > > cc/resources/raster_worker_pool.cc:506: > > raster_tasks_required_for_activation_.size()); > > This seems a bit too flaky to me. Could we do this as part of the > > RasterFinishedWorkerPoolTaskImpl task instead and never reset it? > > That's what I initially went for but the problem is the 6 ms polling loop on the > impl thread doesn't wait for RasterFinishedWorkerPoolTaskImpl (and shouldn't!). > That meant the delay happily went off on the raster thread but the impl thread > was still able to activate before it finished thanks to the polling. I think we should make PixelBufferRasterWorkerPool wait for those signals before generating any DidFinish client notifications. That would make our RasterWorkerPool implementations more consistent and allow you to implement this properly. What makes you say that it shouldn't wait for these signals?
On 2013/12/12 20:07:08, David Reveman wrote: > Sure but you're assuming that assigning a pointer value to > g_raster_required_for_activation_delay is atomic. That might be the case but I > don't think we should assume it. In theory, you could be reading > g_raster_required_for_activation_delay on one thread while being half way done > writing a value to it on another thread. That's a great point. Sounds like something based on LazyInstance would be safer. I'll give it a shot. (FWIW this is only an issue with this particular initialization; the macro-based trace events all use atomics internally.) > I think we should make PixelBufferRasterWorkerPool wait for those signals before > generating any DidFinish client notifications. That would make our > RasterWorkerPool implementations more consistent and allow you to implement this > properly. What makes you say that it shouldn't wait for these signals? That would certainly simplify things. I was just thinking that waiting for RasterFinishedWorkerPoolTaskImpl would add too much latency, but if you feel it'd be acceptable I can definitely look into doing just that.
On 2013/12/12 20:17:36, Sami wrote: > On 2013/12/12 20:07:08, David Reveman wrote: > > Sure but you're assuming that assigning a pointer value to > > g_raster_required_for_activation_delay is atomic. That might be the case but I > > don't think we should assume it. In theory, you could be reading > > g_raster_required_for_activation_delay on one thread while being half way done > > writing a value to it on another thread. > > That's a great point. Sounds like something based on LazyInstance would be > safer. I'll give it a shot. (FWIW this is only an issue with this particular > initialization; the macro-based trace events all use atomics internally.) > > > I think we should make PixelBufferRasterWorkerPool wait for those signals > before > > generating any DidFinish client notifications. That would make our > > RasterWorkerPool implementations more consistent and allow you to implement > this > > properly. What makes you say that it shouldn't wait for these signals? > > That would certainly simplify things. I was just thinking that waiting for > RasterFinishedWorkerPoolTaskImpl would add too much latency, but if you feel > it'd be acceptable I can definitely look into doing just that. Would be great if you could look into doing this. It shouldn't make any significant difference as the signal message should already be in the message queue, waiting to be processed, in situations where we currently generate these client callbacks before receiving the signals.
Ok, last time I switch things around, I promise :) I've done away with the raciness by giving each RasterFinishedWorkerPoolTaskImpl their own delay deadline. This way the delay gets applied consistently regardless of whether these tasks end up getting cancelled or happen to run just as we're scheduling another one. I've also fixed the initialization race. PTAL. > Would be great if you could look into doing this. It shouldn't make any > significant difference as the signal message should already be in the message > queue, waiting to be processed, in situations where we currently generate these > client callbacks before receiving the signals. FYI this is done in https://codereview.chromium.org/99873007/
https://codereview.chromium.org/83183005/diff/140001/cc/resources/image_raste... File cc/resources/image_raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/140001/cc/resources/image_raste... cc/resources/image_raster_worker_pool.cc:89: CreateRasterRequiredForActivationFinishedTask(0u)); why 0 here instead of the number of tasks actually required for activation?
https://codereview.chromium.org/83183005/diff/140001/cc/resources/raster_work... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/140001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:323: if (tasks_pending_for_activation_) { To verify my understanding, this only applies the delay if there was at least one task required for activation? https://codereview.chromium.org/83183005/diff/140001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:324: g_raster_required_for_activation_delay.Get().delay->EndParallel( Which patch contains the EndParallel and BeginParallel implementations?
https://codereview.chromium.org/83183005/diff/140001/cc/resources/image_raste... File cc/resources/image_raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/140001/cc/resources/image_raste... cc/resources/image_raster_worker_pool.cc:89: CreateRasterRequiredForActivationFinishedTask(0u)); On 2013/12/17 20:53:05, David Reveman wrote: > why 0 here instead of the number of tasks actually required for activation? Sorry, that was just an oversight on my part; I was mainly looking at pixel buffer tasks so far. Fixed. https://codereview.chromium.org/83183005/diff/140001/cc/resources/raster_work... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/140001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:323: if (tasks_pending_for_activation_) { On 2013/12/17 21:59:16, Brian Anderson wrote: > To verify my understanding, this only applies the delay if there was at least > one task required for activation? That's correct. Without this condition we were applying the delay even when there wasn't any actual raster work, which ended up taking up almost all of the time. https://codereview.chromium.org/83183005/diff/140001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:324: g_raster_required_for_activation_delay.Get().delay->EndParallel( On 2013/12/17 21:59:16, Brian Anderson wrote: > Which patch contains the EndParallel and BeginParallel implementations? Ah, looks like I forgot to cc you; it's over here: https://codereview.chromium.org/104613003/
https://codereview.chromium.org/83183005/diff/160001/cc/resources/raster_work... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/160001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:39: template <const char* delay_name> As this is only used for cc.RasterRequiredForActivation, can we remove the template specialization and name it RasterRequiredForActivationSyntheticDelayInitializer? ::Lookup(delay_name) -> ::Lookup("cc.RasterRequiredForActivation").. https://codereview.chromium.org/83183005/diff/160001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:344: base::TimeTicks activation_delay_end_time_; Maybe cleaner to add a RasterRequiredForActivationFinishedWorkerPoolTaskImpl subclass that contains this new logic. Especially if we don't need the has_tasks_required_for_activation_ conditional. Wdyt? https://codereview.chromium.org/83183005/diff/160001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:538: bool has_tasks_required_for_activation) { Is it necessary to avoid this when has_tasks_required_for_activation is false? Maybe we shouldn't create the task in the first place in that case..
Thanks for the comments David. https://codereview.chromium.org/83183005/diff/160001/cc/resources/raster_work... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/160001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:39: template <const char* delay_name> On 2013/12/18 20:55:18, David Reveman wrote: > As this is only used for cc.RasterRequiredForActivation, can we remove the > template specialization and name it > RasterRequiredForActivationSyntheticDelayInitializer? ::Lookup(delay_name) -> > ::Lookup("cc.RasterRequiredForActivation").. Good point, done. https://codereview.chromium.org/83183005/diff/160001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:344: base::TimeTicks activation_delay_end_time_; On 2013/12/18 20:55:18, David Reveman wrote: > Maybe cleaner to add a RasterRequiredForActivationFinishedWorkerPoolTaskImpl > subclass that contains this new logic. Especially if we don't need the > has_tasks_required_for_activation_ conditional. Wdyt? Agreed, that's much nicer. https://codereview.chromium.org/83183005/diff/160001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:538: bool has_tasks_required_for_activation) { On 2013/12/18 20:55:18, David Reveman wrote: > Is it necessary to avoid this when has_tasks_required_for_activation is false? The problem is that having no tasks required for activation happens pretty often. If we apply the delay in that case, the raster worker pool ends up spinning in a delay all the time and the browser grinds to a halt. > Maybe we shouldn't create the task in the first place in that case.. That would be a reasonable thing to do, but has an unfortunate side effect for delays: if we've finished all the actual raster tasks and are spinning in the delay, CheckForCompletedRasterTasks can get triggered on the 6 ms timer and notice that all raster tasks have completed. This means we'll activate the tree before the delay completes. I don't have a good idea for working around this without always waiting for RasterRequiredForActivationFinishedTask. Potentially we could skip the fast path if the delay is active, but I wouldn't want to change production code too much to accomodate delays.
https://codereview.chromium.org/83183005/diff/160001/cc/resources/raster_work... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/160001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:538: bool has_tasks_required_for_activation) { On 2013/12/19 12:49:21, Sami wrote: > On 2013/12/18 20:55:18, David Reveman wrote: > > Is it necessary to avoid this when has_tasks_required_for_activation is false? > > The problem is that having no tasks required for activation happens pretty > often. If we apply the delay in that case, the raster worker pool ends up > spinning in a delay all the time and the browser grinds to a halt. OK, I can see why that happens. > > > Maybe we shouldn't create the task in the first place in that case.. > > That would be a reasonable thing to do, but has an unfortunate side effect for > delays: if we've finished all the actual raster tasks and are spinning in the > delay, CheckForCompletedRasterTasks can get triggered on the 6 ms timer and > notice that all raster tasks have completed. This means we'll activate the tree > before the delay completes. > > I don't have a good idea for working around this without always waiting for > RasterRequiredForActivationFinishedTask. Potentially we could skip the fast path > if the delay is active, but I wouldn't want to change production code too much > to accomodate delays. I thought some more about this and I think the current approach is good. After all, we want to simulate that the raster tasks required for activation take longer to complete, not the finish task. I think if the delay is defined as the product of the number of tasks required for activation and some per-task delay then it makes perfect sense. Current code with a constant delay when number of tasks are more than zero is fine though. That's just a special case where "delay = min(task_count * kPerTaskDelay, kPerTaskDelay)" Maybe just change "bool has_tasks_required_for_activation" here to "size_t tasks_required_for_activation_count" so the fact that we use a constant delay is not exposed outside the RasterWorkerPool implementation. https://codereview.chromium.org/83183005/diff/170001/cc/resources/image_raste... File cc/resources/image_raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/170001/cc/resources/image_raste... cc/resources/image_raster_worker_pool.cc:100: has_tasks_required_for_activation)); Can you use raster_tasks_required_for_activation().size() here instead of having to iterate over all tasks above? https://codereview.chromium.org/83183005/diff/170001/cc/resources/raster_work... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/170001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:44: "cc.RasterRequiredForActivation")) {} nit: this line should be indented 4 spaces from "base::debug::Tra.." instead of 4 spaces from "delay(ba.."
On 2013/12/19 17:29:28, David Reveman wrote: > I thought some more about this and I think the current approach is good. After > all, we want to simulate that the raster tasks required for activation take > longer to complete, not the finish task. I think if the delay is defined as the > product of the number of tasks required for activation and some per-task delay > then it makes perfect sense. > > Current code with a constant delay when number of tasks are more than zero is > fine though. That's just a special case where "delay = min(task_count * > kPerTaskDelay, kPerTaskDelay)" That's right, thanks for spelling it out. We really want to tie the delay to the activation cycle instead of a single raster task, since the number of tasks will be wildly different between platforms, pages, screen orientations, etc. > Maybe just change "bool has_tasks_required_for_activation" here to "size_t > tasks_required_for_activation_count" so the fact that we use a constant delay is > not exposed outside the RasterWorkerPool implementation. Good point, done. > https://codereview.chromium.org/83183005/diff/170001/cc/resources/image_raste... > File cc/resources/image_raster_worker_pool.cc (right): > > https://codereview.chromium.org/83183005/diff/170001/cc/resources/image_raste... > cc/resources/image_raster_worker_pool.cc:100: > has_tasks_required_for_activation)); > Can you use raster_tasks_required_for_activation().size() here instead of having > to iterate over all tasks above? Thanks, I didn't notice they'd be equivalent here. > https://codereview.chromium.org/83183005/diff/170001/cc/resources/raster_work... > File cc/resources/raster_worker_pool.cc (right): > > https://codereview.chromium.org/83183005/diff/170001/cc/resources/raster_work... > cc/resources/raster_worker_pool.cc:44: "cc.RasterRequiredForActivation")) {} > nit: this line should be indented 4 spaces from "base::debug::Tra.." instead of > 4 spaces from "delay(ba.." Weird, that was straight out of git cl format. Fixed.
cc/resources lgtm with 2 minor nits should ShareGroup implementation of AsyncPixelTransferManager also be affected by the gpu.AsyncTexImage delay? https://codereview.chromium.org/83183005/diff/190001/cc/resources/raster_work... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/83183005/diff/190001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:349: // Overridden from RasterFinishedWorkerPoolTaskImpl nit: add a ":" at the end of this line https://codereview.chromium.org/83183005/diff/190001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:365: const bool has_tasks_required_for_activation_; nit: "size_t tasks_required_for_activation_count_" here too and have the details of how this delay is implemented completely part of the RasterRequiredForActivationFinishedWorkerPoolTaskImpl implementation.
All nits addressed, thanks. Antoine, could I get your opinion on content/renderer/render_widget.cc and the changes under gpu/, thanks!
Not too familiar with the synthetic delay subsystem nor why it's linked to the tracing system, but otherwise LGTM. I assume the cost is negligible in the common case?
On 2014/01/08 02:31:54, piman (OOO back 2014-1-7) wrote: > Not too familiar with the synthetic delay subsystem nor why it's linked to the > tracing system, but otherwise LGTM. Thanks. Delays are currently part of tracing so we can reuse some of the infrastructure to experiment with them. Longer term we plan to separate them out. > I assume the cost is negligible in the common case? Yes, if synthetic delays are disabled, the cost of a delay point is a non-barrier atomic load and a couple of compares -- ~same as a trace event -- so it should not show up except in very tight loops.
lgtm if it helps
(fwiw, i'd encourage reviewers to favor landing imperfectly and iterating over having this perfect... the syntheetic delays stuff is a promising idea and I think the only way to really decide if its awesome is to try and iterate)
On 2014/01/08 20:21:37, nduca wrote: > (fwiw, i'd encourage reviewers to favor landing imperfectly and iterating over > having this perfect... the syntheetic delays stuff is a promising idea and I > think the only way to really decide if its awesome is to try and iterate) fyi, this is blocked on re-landing https://src.chromium.org/viewvc/chrome?revision=243545&view=revision
Sami, I would cq+ this for you but it needs a rebase. Better if you take care of that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/83183005/350001
> Sami, I would cq+ this for you but it needs a rebase. Better if you take care of > that. Thanks David -- rebased and cq'ing.
Retried try job too often on linux_rel for step(s) check_deps2git http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/83183005/350001
Message was sent while issue was closed.
Change committed as 244175 |
