Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12217105:
cc: Check for completed raster tasks at interval. (Closed)
|
Created:
7 years, 10 months ago by reveman Modified:
7 years, 10 months ago CC:
chromium-reviews, cc-bugs_chromium.org, Sami Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc: Check for completed raster tasks at interval.
This significantly reduces the context switching overhead for
impl-side painting.
Instead of posting reply task to the impl thread after completing
each raster job, post a reply only when worker pool becomes idle
and poll for completed tasks at a 6ms interval.
This doesn't just make rasterization more efficient but also
reduces the number of shallow flushes, which makes async uploads
more efficient.
BUG=173802
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182404
Patch Set 1 #
Total comments: 14
Patch Set 2 : #Patch Set 3 : address review feedback #Patch Set 4 : Post task to impl thread when worker pool becomes idle. #
Total comments: 6
Patch Set 5 : confine polling to inside worker pool #
Total comments: 13
Patch Set 6 : Add DidFinishDispatchingCompletionCallbacks #
Total comments: 4
Patch Set 7 : tweak names #Patch Set 8 : Remove MoreTasksCompleted. #Patch Set 9 : Switch back to kNumPendingTasksPerWorker, seems good enough for now #
Total comments: 4
Patch Set 10 : Fix HasCompleted. #Patch Set 11 : Fix race condition #
Total comments: 1
Patch Set 12 : Add missing CC_EXPORT. #
Messages
Total messages: 28 (0 generated)
I think polling at regular intervals is a good method for batching our uploads such that a batch is followed by a single shallow flush. Any method that batches the uploads will increase our latency. The polling interval of 4ms limits how much we increase our latency by. I wonder if we might want to check for completed rasters in two other cases though: 1) A very fast system that finishes pending rasters in <4ms. In this case, should a raster worker send a task to the compositor thread that checks for completed rasters early if the raster worker detects that there are only N raster tasks left. 2) Should we check for raster tasks immediately once the raster pool is out of work to do? We might also want to check less often for slow devices, where a single raster takes much more than 4ms. I'm less worried about this case though. https://codereview.chromium.org/12217105/diff/1/cc/raster_worker_pool.cc File cc/raster_worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/1/cc/raster_worker_pool.cc#newc... cc/raster_worker_pool.cc:26: base::subtle::NoBarrier_Store(&completed_, 1); I think we need a barrier before setting completed_. Otherwise, compiler/cpu reordering can mark completed_ before we are are actually done. https://codereview.chromium.org/12217105/diff/1/cc/thread_proxy.cc File cc/thread_proxy.cc (right): https://codereview.chromium.org/12217105/diff/1/cc/thread_proxy.cc#newcode377 cc/thread_proxy.cc:377: { DCHECK(isImplThread())? https://codereview.chromium.org/12217105/diff/1/cc/thread_proxy.cc#newcode388 cc/thread_proxy.cc:388: { DCHECK(isImplThread())? https://codereview.chromium.org/12217105/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12217105/diff/1/cc/tile_manager.cc#newcode581 cc/tile_manager.cc:581: size_t new_upload_bytes_pending = bytes_pending_upload_; Shouldn't this be: size_t new_upload_bytes_pending = bytes_pending_upload_ + bytes_pending_raster_; https://codereview.chromium.org/12217105/diff/1/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12217105/diff/1/cc/tile_manager.h#newcode59 cc/tile_manager.h:59: UPLOAD_STATE = 3, Is changing "set_pixels" to "upload" just a cleanup unrelated to this patch? Would it make sense to split the cleanup into it's own patch? https://codereview.chromium.org/12217105/diff/1/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/1/cc/worker_pool.cc#newcode31 cc/worker_pool.cc:31: base::subtle::NoBarrier_Store(&completed_, 1); I think you need a barrier before this store (or a release store). https://codereview.chromium.org/12217105/diff/1/cc/worker_pool.cc#newcode46 cc/worker_pool.cc:46: base::subtle::NoBarrier_Store(&completed_, 0); I think you need a barrier after this store (or an acquire store).
I like the idea of posting a task to compositor thread once the raster pool is out of work. That will make us faster when only a very few tiles that need update. Not sure about when running low as that would cause the raster thread with pending work to yield but it's worth investigating. btw, I used on 8ms as poll interval for some of my testing and on slow devices like the N10 this might be better. https://codereview.chromium.org/12217105/diff/1/cc/raster_worker_pool.cc File cc/raster_worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/1/cc/raster_worker_pool.cc#newc... cc/raster_worker_pool.cc:26: base::subtle::NoBarrier_Store(&completed_, 1); On 2013/02/12 01:44:31, Brian Anderson wrote: > I think we need a barrier before setting completed_. Otherwise, compiler/cpu > reordering can mark completed_ before we are are actually done. yea, this should already be fixed in patch set 2. https://codereview.chromium.org/12217105/diff/1/cc/thread_proxy.cc File cc/thread_proxy.cc (right): https://codereview.chromium.org/12217105/diff/1/cc/thread_proxy.cc#newcode377 cc/thread_proxy.cc:377: { On 2013/02/12 01:44:31, Brian Anderson wrote: > DCHECK(isImplThread())? Sure. I'll add that to setNeedsManageTilesOnImplThread() above too. https://codereview.chromium.org/12217105/diff/1/cc/thread_proxy.cc#newcode388 cc/thread_proxy.cc:388: { On 2013/02/12 01:44:31, Brian Anderson wrote: > DCHECK(isImplThread())? Done. https://codereview.chromium.org/12217105/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12217105/diff/1/cc/tile_manager.cc#newcode581 cc/tile_manager.cc:581: size_t new_upload_bytes_pending = bytes_pending_upload_; On 2013/02/12 01:44:31, Brian Anderson wrote: > Shouldn't this be: > size_t new_upload_bytes_pending = bytes_pending_upload_ + bytes_pending_raster_; makes sense. https://codereview.chromium.org/12217105/diff/1/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12217105/diff/1/cc/tile_manager.h#newcode59 cc/tile_manager.h:59: UPLOAD_STATE = 3, On 2013/02/12 01:44:31, Brian Anderson wrote: > Is changing "set_pixels" to "upload" just a cleanup unrelated to this patch? > > Would it make sense to split the cleanup into it's own patch? Yes, that's just cleanup and I'm fine moving that out of this patch if you prefer. https://codereview.chromium.org/12217105/diff/1/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/1/cc/worker_pool.cc#newcode31 cc/worker_pool.cc:31: base::subtle::NoBarrier_Store(&completed_, 1); On 2013/02/12 01:44:31, Brian Anderson wrote: > I think you need a barrier before this store (or a release store). Done. https://codereview.chromium.org/12217105/diff/1/cc/worker_pool.cc#newcode46 cc/worker_pool.cc:46: base::subtle::NoBarrier_Store(&completed_, 0); On 2013/02/12 01:44:31, Brian Anderson wrote: > I think you need a barrier after this store (or an acquire store). Done.
Latest patch implements the idea of posting a task to the impl thread when the worker pool becomes idle. This seems to be working well. PTAL.
I like the new idle detection. We can look into the other optimizations at a later time as this patch should handle most cases. https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.cc#newcode230 cc/worker_pool.cc:230: idle_handler_ = base::Bind(&WorkerPool::OnIdle, I do not see where idle_handler_ is reset or set to anything else. Does PostTask implicitly reset idle_handler_? If not, can you initialize idle_handler_ once somewhere else? https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.h File cc/worker_pool.h (right): https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.h#newcode43 cc/worker_pool.h:43: virtual void OnIdle() = 0; Can we rename this to OnWorkerPoolIdle?
To recap, I think we dont have to delegate through the proxy for the message loop stuff. The way rasterziation works these days, you pretty much have to spin a message loop. I also think it'd be interesting to try posting a delayed task of +4ms to the main thread from the worker threads to check for completed events, if its not already posted [as by doing an atomic set of the bool to true]. That way the only thing that you need to do outside the worker pool is to tell the embedder "hey, you should probably glFlush now" https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.cc#newcode119 cc/worker_pool.cc:119: worker_pool->OnWorkCompleted(); Use suffix to OnWorkCompletedOnWorkerThread
On 2013/02/13 01:33:18, nduca wrote: > To recap, I think we dont have to delegate through the proxy for the message > loop stuff. The way rasterziation works these days, you pretty much have to > spin a message loop. > > I also think it'd be interesting to try posting a delayed task of +4ms to the > main thread from the worker threads to check for completed events, if its not > already posted [as by doing an atomic set of the bool to true]. That way the > only thing that you need to do outside the worker pool is to tell the embedder > "hey, you should probably glFlush now" I tried this and posting a delayed task will immediately wake up the main (impl) thread and cause the raster thread that has more work left to do to yield. I'm not sure how complicated it would be to make the message pump not do this. > > https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.cc > File cc/worker_pool.cc (right): > > https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.cc#newcode119 > cc/worker_pool.cc:119: worker_pool->OnWorkCompleted(); > Use suffix to OnWorkCompletedOnWorkerThread
Cool thanks for checking. Lets do your approach, though maybe we can confine the polling to inside worker pool? Also, lets file a crbug about this causing a wakeup.
latest patch keeps all the polling logic inside the worker pool and adds a "more_tasks_completed" argument to the task completion reply provided by the tile manager. the tile manager is then using this argument to determine when it makes sense to try dispatch more tasks and flush uploads. PTAL. https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.cc#newcode119 cc/worker_pool.cc:119: worker_pool->OnWorkCompleted(); On 2013/02/13 01:33:18, nduca wrote: > Use suffix to OnWorkCompletedOnWorkerThread Done. https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.cc#newcode230 cc/worker_pool.cc:230: idle_handler_ = base::Bind(&WorkerPool::OnIdle, On 2013/02/13 01:19:57, Brian Anderson wrote: > I do not see where idle_handler_ is reset or set to anything else. Does > PostTask implicitly reset idle_handler_? If not, can you initialize > idle_handler_ once somewhere else? latest patch initialize this once in the constructor. https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.h File cc/worker_pool.h (right): https://codereview.chromium.org/12217105/diff/1029/cc/worker_pool.h#newcode43 cc/worker_pool.h:43: virtual void OnIdle() = 0; On 2013/02/13 01:19:57, Brian Anderson wrote: > Can we rename this to OnWorkerPoolIdle? Latest patch doesn't need this client interface.
https://codereview.chromium.org/12217105/diff/4007/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12217105/diff/4007/cc/tile_manager.cc#newcode655 cc/tile_manager.cc:655: if (need_shallow_flush_) { hmmm the position of this group surprised me, I was sort of hoping to see this on the raster task callback... whys it not called after we get the raster task and do the upload? https://codereview.chromium.org/12217105/diff/4007/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12217105/diff/4007/cc/tile_manager.h#newcode169 cc/tile_manager.h:169: bool more_tasks_completed); might tweak the name, i cant quite tell if this means more_tasks_will_come or there_are_no_more_tasks_that_will_arrive :) https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.h File cc/worker_pool.h (left): https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.h#oldcode58 cc/worker_pool.h:58: // of pending tasks. I wonder, can you shift the raster throttle stuff into here? Where the task you post has a cost? Cost might be bytes instead, but i think the bookeeping doesn't have to be in tilemanager. https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.h File cc/worker_pool.h (right): https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.h#newcode28 cc/worker_pool.h:28: bool IsPending(); Can you say HasCompleted? So we dont mix completed and pending words? https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.h#newcode157 cc/worker_pool.h:157: base::subtle::Atomic32 work_count_; better name
https://codereview.chromium.org/12217105/diff/4007/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12217105/diff/4007/cc/tile_manager.cc#newcode655 cc/tile_manager.cc:655: if (need_shallow_flush_) { On 2013/02/13 08:29:32, nduca wrote: > hmmm the position of this group surprised me, I was sort of hoping to see this > on the raster task callback... whys it not called after we get the raster task > and do the upload? this is just common code that is always executed after task completion and it shouldn't hurt to have it here if DispatchMoreTasks is called for a different reason. But I can move this to OnImageDecodeTaskCompleted and OnRasterTaskCompleted if you prefer that? https://codereview.chromium.org/12217105/diff/4007/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12217105/diff/4007/cc/tile_manager.h#newcode169 cc/tile_manager.h:169: bool more_tasks_completed); On 2013/02/13 08:29:32, nduca wrote: > might tweak the name, i cant quite tell if this means more_tasks_will_come or > there_are_no_more_tasks_that_will_arrive :) more_tasks_have_completed? https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.h File cc/worker_pool.h (left): https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.h#oldcode58 cc/worker_pool.h:58: // of pending tasks. On 2013/02/13 08:29:32, nduca wrote: > I wonder, can you shift the raster throttle stuff into here? Where the task you > post has a cost? Cost might be bytes instead, but i think the bookeeping doesn't > have to be in tilemanager. my current throttle values are derived from the kMaxPendingUploadBytes constants and I'd prefer to keep them in one place but I guess we can initialize the pool with MaxPendingRasterBytes. I'll try this. https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.h File cc/worker_pool.h (right): https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.h#newcode28 cc/worker_pool.h:28: bool IsPending(); On 2013/02/13 08:29:32, nduca wrote: > Can you say HasCompleted? So we dont mix completed and pending words? Done.
small nits, and to recap offline chat, lets maybe route the bool about last compelted via a client interface,, which will clarify the logic i think... https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.cc#newcode133 cc/worker_pool.cc:133: while (!pending_tasks_.empty()) { any reason we return tasks fifo? doesnt seem required... https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.cc#newcode246 cc/worker_pool.cc:246: if (base::subtle::Barrier_AtomicIncrement(&work_count_, -1) == 0) { Do we not have atomicdec?
This looks really solid. I think we're getting really close here. Nit below, and lets sync up tomorrow (erm when its light out) about throttling. https://codereview.chromium.org/12217105/diff/18002/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12217105/diff/18002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:550: if (need_shallow_flush_) { has_performed_uploads_since_last_shallow_flush_? https://codereview.chromium.org/12217105/diff/18002/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12217105/diff/18002/cc/tile_manager.h#newcode130 cc/tile_manager.h:130: virtual void DidFinishDispatchingCompletionCallbacks() OVERRIDE; Might tweak the name so it has Worker in it so its clearer which completions
https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.cc#newcode133 cc/worker_pool.cc:133: while (!pending_tasks_.empty()) { On 2013/02/13 09:06:29, nduca wrote: > any reason we return tasks fifo? doesnt seem required... not really but they will complete in fifo order on the worker so doing it any other way seems unjustified https://codereview.chromium.org/12217105/diff/4007/cc/worker_pool.cc#newcode246 cc/worker_pool.cc:246: if (base::subtle::Barrier_AtomicIncrement(&work_count_, -1) == 0) { On 2013/02/13 09:06:29, nduca wrote: > Do we not have atomicdec? no, but Barrier_AtomicIncrement(-1) is commonly used in chromium to do decrement. https://codereview.chromium.org/12217105/diff/18002/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12217105/diff/18002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:550: if (need_shallow_flush_) { On 2013/02/13 10:53:37, nduca wrote: > has_performed_uploads_since_last_shallow_flush_? ok, I changed to has_performed_uploads_since_last_flush_. https://codereview.chromium.org/12217105/diff/18002/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12217105/diff/18002/cc/tile_manager.h#newcode130 cc/tile_manager.h:130: virtual void DidFinishDispatchingCompletionCallbacks() OVERRIDE; On 2013/02/13 10:53:37, nduca wrote: > Might tweak the name so it has Worker in it so its clearer which completions Now DidFinishDispatchingWorkerPoolCompletionCallbacks.
I think the simple handling IsBusy in latest patch is good enough for now. We'll need to revisit this as we start using multiple raster threads though.
LGTM. I think a good followup patch would be to remove completely the Worker object as visible to the tile manager. Basically, the tile manager would say if (not throttled) worker_pool->Post() And internally the post call would figure out what thread to assign to.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/12217105/21007
https://codereview.chromium.org/12217105/diff/21007/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/21007/cc/worker_pool.cc#newcode140 cc/worker_pool.cc:140: if (pending_tasks_.front()->HasCompleted()) Should the condition be the inverse here? if (!pending_tasks_.front()->HasCompleted())
https://codereview.chromium.org/12217105/diff/21007/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/21007/cc/worker_pool.cc#newcode280 cc/worker_pool.cc:280: I think there is a race condition if the remaining raster jobs finish after we checked them and before the next line. We might not schedule another check for them which will cause us to never upload them, which could cause us to display incomplete frames without user intervention. You need a way to track the pending tasks that have not been checked.
https://codereview.chromium.org/12217105/diff/21007/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/21007/cc/worker_pool.cc#newcode140 cc/worker_pool.cc:140: if (pending_tasks_.front()->HasCompleted()) On 2013/02/14 00:26:50, Brian Anderson wrote: > Should the condition be the inverse here? > if (!pending_tasks_.front()->HasCompleted()) yes, good call. it's doing the right thing now as I changed the name but not the implementation but this is going to be super confusing if not fixed.
https://codereview.chromium.org/12217105/diff/21007/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/21007/cc/worker_pool.cc#newcode280 cc/worker_pool.cc:280: On 2013/02/14 00:42:40, Brian Anderson wrote: > I think there is a race condition if the remaining raster jobs finish after we > checked them and before the next line. We might not schedule another check for > them which will cause us to never upload them, which could cause us to display > incomplete frames without user intervention. > > You need a way to track the pending tasks that have not been checked. > Moving the if statement to be above the for loop will also fix the race condition at the cost of scheduling a check sometimes when you don't need to.
lgtm https://codereview.chromium.org/12217105/diff/21008/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12217105/diff/21008/cc/worker_pool.cc#newcode284 cc/worker_pool.cc:284: if (worker->num_pending_tasks()) { No race condition and no unnecessary checks scheduled. Looks good!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/12217105/21008
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, cacheinvalidation_unittests, check_deps, chromedriver2_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, nacl_integration, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, unit_tests 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/reveman@chromium.org/12217105/21008
Retried try job too often on linux_chromeos for step(s) aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, check_deps, chromeos_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, dbus_unittests, device_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, net_unittests, ppapi_unittests, printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/12217105/26012
We should maybe dcommit? Or did we miss the build?
build is still waiting for this CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
