Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
578053002:
cc: Have 1-copy rasterizer issue copy commands at the rate of raster. (Closed)
Descriptioncc: Have 1-copy rasterizer issue copy commands at the rate of raster.
This makes the OneCopyRasterWorkerPool implementation
issue copy commands at the same rate that raster tasks
complete instead of issuing all commands when
CheckForCompletedTasks is called.
BUG=406404
Committed: https://crrev.com/9d6f1c46da0a3be9600d4abcf2974d58626efd8d
Cr-Commit-Position: refs/heads/master@{#300935}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #
Total comments: 14
Patch Set 4 : address review feedback #
Total comments: 3
Patch Set 5 : add raster_finished_weak_ptr_factory_ comment #
Messages
Total messages: 15 (4 generated)
reveman@chromium.org changed reviewers: + vmpstr@chromium.org
FYI, this is the first out of 3 changes that will make one-copy ready to be turned on by default. The other changes are; blocking on worker thread when going to fast and worker thread allocation of buffers. Please take a look.
https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... File cc/resources/one_copy_raster_worker_pool.cc (right): https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.cc:272: if ((pending_copy_operations_.size() % kBatchSize) == 0) { We only flush if the pending size grows to be 4? When would we flush if we do 1 schedule, 1 copy, 1 schedule, 1 copy, etc? Or do we not need to flush in that case? (I guess what I'm asking is should this be (sequence % kBatchSize) == 0)?) https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.cc:282: void OneCopyRasterWorkerPool::IssueCopy(CopySequenceNumber sequence) { I think these functions warrant a bit more comments. I'm sitting with a pen and paper trying to figure out if there's some timing bug lurking here :) I think it's all good, but it would help if there was a brief explanation of why. In particular, something like "pending copies will always correspond to a monotonically increasing sequence numbers; issuing a copy of a sequence will ensure that copies are issued for all sequences that precede it"... Maybe there's more, but I'm pretty convinced that this works fine (modulo my other comment about kBatchSize). https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.cc:316: void OneCopyRasterWorkerPool::IssueCopyOperations(int64 count) { I'm not sure if there's a better name for this? Maybe like IssueGivenCountOfCopies.. that's maybe a bit awkward. If you can come up with something else, I would prefer a name that's less similar to IssueCopy that also takes an int64 https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... File cc/resources/one_copy_raster_worker_pool.h (right): https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.h:28: typedef int64 CopySequenceNumber; Can this be uint64? https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.h:64: // Issue copy associated with |sequence| unless already issued. Reading the code, this isn't exactly true. This will issue copies for all "unissued" sequences less or equal to |sequence|, right? https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.h:67: // Flush copy associated with |sequence| unless already flushed. Likewise here, I think you should mention that copies will be issued if they weren't already. https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.h:68: void FlushCopy(CopySequenceNumber sequence); Can this be private? Or does Bind complain?
PTAL https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... File cc/resources/one_copy_raster_worker_pool.cc (right): https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.cc:272: if ((pending_copy_operations_.size() % kBatchSize) == 0) { On 2014/10/22 23:25:16, vmpstr wrote: > We only flush if the pending size grows to be 4? When would we flush if we do 1 > schedule, 1 copy, 1 schedule, 1 copy, etc? Or do we not need to flush in that > case? > > (I guess what I'm asking is should this be (sequence % kBatchSize) == 0)?) Good idea, I changed it to what you suggested here in latest patch. FYI, I don't think it makes much of a difference in practice as it's unlikely that we'll re-schedule tasks this quickly without any drawing and a flush for that reason. https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.cc:282: void OneCopyRasterWorkerPool::IssueCopy(CopySequenceNumber sequence) { On 2014/10/22 23:25:16, vmpstr wrote: > I think these functions warrant a bit more comments. I'm sitting with a pen and > paper trying to figure out if there's some timing bug lurking here :) I think > it's all good, but it would help if there was a brief explanation of why. > > In particular, something like > "pending copies will always correspond to a monotonically increasing sequence > numbers; issuing a copy of a sequence will ensure that copies are issued for all > sequences that precede it"... Maybe there's more, but I'm pretty convinced that > this works fine (modulo my other comment about kBatchSize). I changed the naming of these functions in latest patch so it would be more clear how this works. Let me know if you think some additional comments are still needed. https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.cc:316: void OneCopyRasterWorkerPool::IssueCopyOperations(int64 count) { On 2014/10/22 23:25:16, vmpstr wrote: > I'm not sure if there's a better name for this? Maybe like > IssueGivenCountOfCopies.. that's maybe a bit awkward. If you can come up with > something else, I would prefer a name that's less similar to IssueCopy that also > takes an int64 Kept this as is in latest patch after changing the name of some other functions. Let me know if you still think this needs a better name. https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... File cc/resources/one_copy_raster_worker_pool.h (right): https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.h:28: typedef int64 CopySequenceNumber; On 2014/10/22 23:25:17, vmpstr wrote: > Can this be uint64? Yes, but I don't see a good reason for that. style guide explicitly mentions not using unsigned types just because the value is not supposed to be negative. https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.h:64: // Issue copy associated with |sequence| unless already issued. On 2014/10/22 23:25:17, vmpstr wrote: > Reading the code, this isn't exactly true. This will issue copies for all > "unissued" sequences less or equal to |sequence|, right? Yes, it's not incorrect though. Just doesn't reveal all the details of how this is done :) I had some trouble finding names for these functions. I changed it in latest patch to be more explicit about what it does. PTAL. https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.h:67: // Flush copy associated with |sequence| unless already flushed. On 2014/10/22 23:25:17, vmpstr wrote: > Likewise here, I think you should mention that copies will be issued if they > weren't already. PTAL. https://codereview.chromium.org/578053002/diff/40001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.h:68: void FlushCopy(CopySequenceNumber sequence); On 2014/10/22 23:25:17, vmpstr wrote: > Can this be private? Or does Bind complain? Done.
lgtm https://codereview.chromium.org/578053002/diff/60001/cc/resources/one_copy_ra... File cc/resources/one_copy_raster_worker_pool.h (right): https://codereview.chromium.org/578053002/diff/60001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.h:119: base::WeakPtrFactory<OneCopyRasterWorkerPool> Consider using just one weak factory
https://codereview.chromium.org/578053002/diff/60001/cc/resources/one_copy_ra... File cc/resources/one_copy_raster_worker_pool.h (right): https://codereview.chromium.org/578053002/diff/60001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.h:119: base::WeakPtrFactory<OneCopyRasterWorkerPool> On 2014/10/23 16:45:58, vmpstr wrote: > Consider using just one weak factory raster_finished_weak_ptr_factory_ is unique as it's used to cancel any in flight "raster finished" tasks when ScheduleTasks is called. We don't want that behavior for flush tasks.
The CQ bit was checked by reveman@chromium.org
https://codereview.chromium.org/578053002/diff/60001/cc/resources/one_copy_ra... File cc/resources/one_copy_raster_worker_pool.h (right): https://codereview.chromium.org/578053002/diff/60001/cc/resources/one_copy_ra... cc/resources/one_copy_raster_worker_pool.h:119: base::WeakPtrFactory<OneCopyRasterWorkerPool> On 2014/10/23 18:05:18, reveman wrote: > On 2014/10/23 16:45:58, vmpstr wrote: > > Consider using just one weak factory > > raster_finished_weak_ptr_factory_ is unique as it's used to cancel any in flight > "raster finished" tasks when ScheduleTasks is called. We don't want that > behavior for flush tasks. Maybe a comment about that then?
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/578053002/60001
The CQ bit was unchecked by reveman@chromium.org
On 2014/10/23 18:06:13, danakj wrote: > https://codereview.chromium.org/578053002/diff/60001/cc/resources/one_copy_ra... > File cc/resources/one_copy_raster_worker_pool.h (right): > > https://codereview.chromium.org/578053002/diff/60001/cc/resources/one_copy_ra... > cc/resources/one_copy_raster_worker_pool.h:119: > base::WeakPtrFactory<OneCopyRasterWorkerPool> > On 2014/10/23 18:05:18, reveman wrote: > > On 2014/10/23 16:45:58, vmpstr wrote: > > > Consider using just one weak factory > > > > raster_finished_weak_ptr_factory_ is unique as it's used to cancel any in > flight > > "raster finished" tasks when ScheduleTasks is called. We don't want that > > behavior for flush tasks. > > Maybe a comment about that then? Done.
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/578053002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9d6f1c46da0a3be9600d4abcf2974d58626efd8d Cr-Commit-Position: refs/heads/master@{#300935} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
