Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2778223005:
Plumb activation time to main (Closed)
DescriptionPlumb activation time in cc to Blink Scheduler in Main
BUG=657826, 657825
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2778223005
Cr-Commit-Position: refs/heads/master@{#465701}
Committed: https://chromium.googlesource.com/chromium/src/+/9a2eb472fde51de48c095c9c208101c534ac211e
Patch Set 1 #Patch Set 2 : remove performance observer #
Total comments: 6
Patch Set 3 : add a vector instead of single activate frame time #
Total comments: 23
Patch Set 4 : address review comments #Patch Set 5 : pass 0 for sync tree in NotifyReadyToActivate from SingleThreadProxy #
Total comments: 8
Patch Set 6 : address comments, remove printfs #Patch Set 7 : add baseline test in layer_tree_host_unittest_proxy; initialize source_frame_number is all ctors #
Total comments: 11
Patch Set 8 : address review comments #Patch Set 9 : update test; remove test-only method in proxy_main #
Total comments: 11
Patch Set 10 : update test and fix issue with sending 0 vs -1 to NotifyReadyToActivate #Patch Set 11 : only add activate time if its not already there #
Total comments: 2
Patch Set 12 : address review comment #Patch Set 13 : rebase to head #Patch Set 14 : update BeginFrameArgs::Create in renderer scheduler unittest #Patch Set 15 : update another BeginMainFrame::Create reference #
Total comments: 2
Patch Set 16 : review comment #
Messages
Total messages: 75 (33 generated)
Description was changed from ========== Scratch: explore plumbing for activate BUG= ========== to ========== Scratch: explore plumbing for activate BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
enne@chromium.org changed reviewers: + enne@chromium.org
This seems like reasonable plumbing to me. BeginFrameArgs is a good way to send information to the main thread once per frame without creating a separate ipc for it. It does require a begin main frame event for it to be sent though, so if somebody ever wants this information from javascript, then they'll need to rAF to get it. Maybe that's ok? One caveat here (and you'll probably want a test for this) is that recently it became possible for the compositor thread to generate its own pending tree/activation without a commit from Blink. This is to support atomic updates for things like lazy image decodes and eventually gif animations on the compositor thread. I think you only need to report one activation time back to the main thread, but you'll need to be careful which activation time you report and not let additional activations clobber the results from the first. Also, you could just recording the pending tree frame number during activation itself, instead of later, when it's not clear if there's a pending tree or not. (Or alternatively ask the active tree after activation.)
Thanks enne for the feedback. On 2017/03/30 15:19:25, enne wrote: > This seems like reasonable plumbing to me. > > BeginFrameArgs is a good way to send information to the main thread once per > frame without creating a separate ipc for it. It does require a begin main > frame event for it to be sent though, so if somebody ever wants this information > from javascript, then they'll need to rAF to get it. Maybe that's ok? The main consequence is that: - reporting will be delayed until next begin main frame - reporting may not happen if there is never another begin main frame (eg. user loads page and doesn't interact with it) The first case is totally fine. JS has a callback API (via Performance Observer), so it doesn't need to rAF. the second case worries me if we lose samples because there wasn't another BMF. However I expect that we'd generate a bunch of frames during page load and given that this is for First Paint, First Contentful Paint etc there's a good chance that there will be more begin-frames around then. I'm fine with implementing it this way to start with (to avoid IPC overhead + complexity) and then we can evaluate (via UMA) whether we lost too many samples. > > One caveat here (and you'll probably want a test for this) is that recently it > became possible for the compositor thread to generate its own pending > tree/activation without a commit from Blink. This is to support atomic updates > for things like lazy image decodes and eventually gif animations on the > compositor thread. I think you only need to report one activation time back to > the main thread, but you'll need to be careful which activation time you report > and not let additional activations clobber the results from the first. Thanks, this is really helpful to know. I did see a bunch of additional "NotifyReadyToActivate" signals locally - maybe due to this. Re: clobbering, at least in local testing I found that that the extra NotifyReadyToActivate signals either didn't have pending tree or didn't have source-frame-number, so I avoided clobbering by checking for non-zero source-frame-number before updating. Do you have any suggestions for how to handle clobbering? > > Also, you could just recording the pending tree frame number during activation > itself, instead of later, when it's not clear if there's a pending tree or not. > (Or alternatively ask the active tree after activation.) I record the value to local member, in Scheduler::NotifyReadyToActivate(). And then later in Scheduler::ProcessScheduledActions (case "SchedulerStateMachine::ACTION_SEND_BEGIN_MAIN_FRAME") I update begin_frame_args with the recorded value. Does that seem sane?
brianderson@chromium.org changed reviewers: + brianderson@chromium.org
https://codereview.chromium.org/2778223005/diff/20001/cc/output/begin_frame_a... File cc/output/begin_frame_args.h (right): https://codereview.chromium.org/2778223005/diff/20001/cc/output/begin_frame_a... cc/output/begin_frame_args.h:112: uint32_t ready_to_activate_source_frame_number; BeginFrameArgs are used everywhere, but these fields only apply to the main|impl interface. It's probably better to store a vector of a new struct with these fields on either 1) the ProxyImpl to be added to the BeginMainFrameAndCommitState or 2) a new BeginMainFrameArgs class and store/pass that around accordingly. https://codereview.chromium.org/2778223005/diff/20001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2778223005/diff/20001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:98: client_->SyncTreeSourceFrameNumber(); If you store this information in the ProxyImpl, you wouldn't need to expose the SyncTreeSourceFrameNumber here. If you do need it here though, you could pass it in as an argument to NotifyReadyToActivate. Also, it's possible for there to be 2 NotifyReadyToActivates between 2 ScheduledActionSendBeginMainFrames with the "main frame before activate" feature sunnyps recently implemented. So we'd need to store a vector of these pairs to forward to the main thread.
https://codereview.chromium.org/2778223005/diff/20001/cc/output/begin_frame_a... File cc/output/begin_frame_args.h (right): https://codereview.chromium.org/2778223005/diff/20001/cc/output/begin_frame_a... cc/output/begin_frame_args.h:112: uint32_t ready_to_activate_source_frame_number; On 2017/03/30 18:32:03, brianderson wrote: > BeginFrameArgs are used everywhere, but these fields only apply to the main|impl > interface. It's probably better to store a vector of a new struct with these > fields on either 1) the ProxyImpl to be added to the > BeginMainFrameAndCommitState or 2) a new BeginMainFrameArgs class and store/pass > that around accordingly. The reason I added it to BeginFrameArgs instead of BeginMainFrameAndCommitState is that we need to send this down to the Blink Scheduler which is called from here: https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo... https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host.cc?dr=C&l=241 render_widget_compositor is a client of layer_tree_host and we don't (and shouldn't?) pass BeginMainFrameAndCommitState through all these layers. Could you clarify your #2 suggestion - what do you mean by "new BeginMainFrameArgs class"? https://codereview.chromium.org/2778223005/diff/20001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2778223005/diff/20001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:98: client_->SyncTreeSourceFrameNumber(); On 2017/03/30 18:32:03, brianderson wrote: > If you store this information in the ProxyImpl, you wouldn't need to expose the > SyncTreeSourceFrameNumber here. If you do need it here though, you could pass it > in as an argument to NotifyReadyToActivate. > > Also, it's possible for there to be 2 NotifyReadyToActivates between 2 > ScheduledActionSendBeginMainFrames with the "main frame before activate" feature > sunnyps recently implemented. So we'd need to store a vector of these pairs to > forward to the main thread. The reason I did this here as opposed to ProxyImpl is because ProxyImpl does not mutate BeginFrameArgs and passes around const ref where needed. I can make it an argument to NotifyReadyToActivate as that's a bit simpler. Making a vector makes sense to me, thanks.
I tried BeginMainFrameAndCommitState instead of BeginFrameArgs but it seems wrong to me - see detailed comment. https://codereview.chromium.org/2778223005/diff/20001/cc/output/begin_frame_a... File cc/output/begin_frame_args.h (right): https://codereview.chromium.org/2778223005/diff/20001/cc/output/begin_frame_a... cc/output/begin_frame_args.h:112: uint32_t ready_to_activate_source_frame_number; On 2017/03/30 21:31:25, panicker wrote: > On 2017/03/30 18:32:03, brianderson wrote: > > BeginFrameArgs are used everywhere, but these fields only apply to the > main|impl > > interface. It's probably better to store a vector of a new struct with these > > fields on either 1) the ProxyImpl to be added to the > > BeginMainFrameAndCommitState or 2) a new BeginMainFrameArgs class and > store/pass > > that around accordingly. > > The reason I added it to BeginFrameArgs instead of BeginMainFrameAndCommitState > is that we need to send this down to the Blink Scheduler which is called from > here: > https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo... > https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host.cc?dr=C&l=241 > render_widget_compositor is a client of layer_tree_host and we don't (and > shouldn't?) pass BeginMainFrameAndCommitState through all these layers. > > Could you clarify your #2 suggestion - what do you mean by "new > BeginMainFrameArgs class"? > I tried moving to BeginMainFrameAndCommitState - however: a. this is not available at all in the single_thread_proxy codepath, so we still need to pass around BeginFrameArgs b. BeginMainFrameAndCommitState is now getting exposed to browser eg. content/renderer/gpu/render_widget_compositor.h, ui/compositor/compositor.h and in the public interface here: third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h c. including "cc/trees/proxy_common.h" from platform/scheduler/renderer/renderer_scheduler_impl.h seems wrong to me. So using BeginFrameArgs still seems like the better approach, as it seems to be intended for this type of thing (and BeginMainFrameAndCommitState doesn't seem to be)
https://codereview.chromium.org/2778223005/diff/20001/cc/output/begin_frame_a... File cc/output/begin_frame_args.h (right): https://codereview.chromium.org/2778223005/diff/20001/cc/output/begin_frame_a... cc/output/begin_frame_args.h:112: uint32_t ready_to_activate_source_frame_number; On 2017/03/30 23:09:19, panicker wrote: > On 2017/03/30 21:31:25, panicker wrote: > > On 2017/03/30 18:32:03, brianderson wrote: > > > BeginFrameArgs are used everywhere, but these fields only apply to the > > main|impl > > > interface. It's probably better to store a vector of a new struct with these > > > fields on either 1) the ProxyImpl to be added to the > > > BeginMainFrameAndCommitState or 2) a new BeginMainFrameArgs class and > > store/pass > > > that around accordingly. > > > > The reason I added it to BeginFrameArgs instead of > BeginMainFrameAndCommitState > > is that we need to send this down to the Blink Scheduler which is called from > > here: > > > https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo... > > https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host.cc?dr=C&l=241 > > render_widget_compositor is a client of layer_tree_host and we don't (and > > shouldn't?) pass BeginMainFrameAndCommitState through all these layers. > > > > Could you clarify your #2 suggestion - what do you mean by "new > > BeginMainFrameArgs class"? > > Re My #2 suggestion: That would be adding a BeginMainFrameArgs to begin_frame_args.h that is either a subclass of BeginFrameArgs or has a BeginFrameArgs. Then BeginMainFrameAndCommitState could have a BeginMainFrameArgs rather than a BeginFrameArgs. > > I tried moving to BeginMainFrameAndCommitState - however: > a. this is not available at all in the single_thread_proxy codepath, so we still > need to pass around BeginFrameArgs > b. BeginMainFrameAndCommitState is now getting exposed to browser eg. > content/renderer/gpu/render_widget_compositor.h, ui/compositor/compositor.h and > in the public interface here: > third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h > c. including "cc/trees/proxy_common.h" from > platform/scheduler/renderer/renderer_scheduler_impl.h seems wrong to me. SingleThreadProxy::ScheduledActionSendBeginMainFrame could do the same thing as the threaded ProxyImpl. Then, if you add a BeginMainFrameArgs to begin_frame_args.h, you wouldn't need to include proxy_common.h from the higher level files. Depending on the level, they could take either BeginMainFrameAndCommitState, BeginMainFrameArgs, or BeginFrameArgs as needed. > > So using BeginFrameArgs still seems like the better approach, as it seems to be > intended for this type of thing (and BeginMainFrameAndCommitState doesn't seem > to be)
Description was changed from ========== Scratch: explore plumbing for activate BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Plumb activation time in cc to Blink Scheduler in Main BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
PTAL. I've updated to vector as suggested, and this looks good in local testing. Also update CL description. Let me know if you think subclass'ing BeginFrameArgs is still warranted to add the additional field: vector for ready_to_activate_time I think the addition of frame_source_number could remain in BeginFrameArgs, this is not specific to "BeginMainFrameArgs" AFAICT. Once we've converged on this, I'll add unittests etc. Thanks for the review!
(just some drive-by comments; the approach looks reasonable to me) https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... File cc/output/begin_frame_args.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... cc/output/begin_frame_args.cc:30: BeginFrameArgs::~BeginFrameArgs() {} nit: this can be = default as well https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... File cc/output/begin_frame_args.h (right): https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... cc/output/begin_frame_args.h:108: uint32_t frame_source_number; nit: source_frame_number to be consistent with the rest of the places https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... cc/output/begin_frame_args.h:110: // This is to report Activation time back to main. Can you please elaborate on this comment a bit. Specifically, what the pair represents and why it's a vector as opposed to a single value? https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:99: fprintf(stderr, "\n\t\tScheduler::NotifyReadyToActivate: ptree: %d", this is just for debugging I assume? https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:637: begin_main_frame_args_.ready_to_activate_time = ready_to_activate_time_; std::move https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:189: std::vector<std::pair<uint32_t, base::TimeTicks>> ready_to_activate_time_; nit: can you put a comment about what this pair represents? https://codereview.chromium.org/2778223005/diff/40001/cc/trees/proxy_impl.cc File cc/trees/proxy_impl.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/trees/proxy_impl.cc#... cc/trees/proxy_impl.cc:578: int ProxyImpl::SyncTreeSourceFrameNumber() { Based on the name, is this meant to check LTHI::sync_tree()? Although this would erroneously return the active tree if the pending tree doesn't exist... So maybe this is good https://codereview.chromium.org/2778223005/diff/40001/cc/trees/proxy_main.cc File cc/trees/proxy_main.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/trees/proxy_main.cc#... cc/trees/proxy_main.cc:121: // Set frame_source_number in BeginFrameArgs I don't think you need this comment :) https://codereview.chromium.org/2778223005/diff/40001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:627: int SingleThreadProxy::SyncTreeSourceFrameNumber() { Do we have pending trees in single thread proxy? (I might not know of all the uses of this :) ).
https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... File cc/output/begin_frame_args.h (right): https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... cc/output/begin_frame_args.h:108: uint32_t frame_source_number; A few comments: * Make sure to initialize this in each of the constructors. * Add a comment like: "|source_frame_number| is set by the LayerTreeHost when the BeginFrame reaches blink and corresponds to the commit count." * "source" in this context is overloaded since it could correspond to the BeginFrameSource. Maybe rename it to "commit_number" so it's more obvious it relates to the commit. Although, I do still have a preference for a separate BeginMainFrameArgs which would also avoid overloading the "source" concept. https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:97: ready_to_activate_time_.push_back(std::make_pair( emplace_back? https://codereview.chromium.org/2778223005/diff/40001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:627: int SingleThreadProxy::SyncTreeSourceFrameNumber() { On 2017/04/05 19:11:33, vmpstr wrote: > Do we have pending trees in single thread proxy? (I might not know of all the > uses of this :) ). SingleThreadProxy is only used in the Browser, where we don't need this feature - so we could just return 0. But it does seem to break the Proxy abstraction to expose the concept of a SyncTree. Alternatively, we could pass in the source_frame_number as an argument to NotifyReadyToActivate. ProxyImpl would pass in the pending tree's number. SingleThreadProxy would pass in the active_tree's number (since we commit to active tree).
PTAL. https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... File cc/output/begin_frame_args.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... cc/output/begin_frame_args.cc:30: BeginFrameArgs::~BeginFrameArgs() {} On 2017/04/05 19:11:33, vmpstr wrote: > nit: this can be = default as well Done. https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... File cc/output/begin_frame_args.h (right): https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... cc/output/begin_frame_args.h:108: uint32_t frame_source_number; On 2017/04/06 23:43:15, brianderson wrote: > A few comments: > > * Make sure to initialize this in each of the constructors. > > * Add a comment like: > "|source_frame_number| is set by the LayerTreeHost when the BeginFrame reaches > blink and corresponds to the commit count." > > * "source" in this context is overloaded since it could correspond to the > BeginFrameSource. Maybe rename it to "commit_number" so it's more obvious it > relates to the commit. Although, I do still have a preference for a separate > BeginMainFrameArgs which would also avoid overloading the "source" concept. Updated. Happy to try out the BeginMainFrameArgs subsclassing as a follow up change, if that's fine with you. Otherwise it's adding not necessary complexity to this change. https://codereview.chromium.org/2778223005/diff/40001/cc/output/begin_frame_a... cc/output/begin_frame_args.h:110: // This is to report Activation time back to main. On 2017/04/05 19:11:33, vmpstr wrote: > Can you please elaborate on this comment a bit. Specifically, what the pair > represents and why it's a vector as opposed to a single value? Done. https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:97: ready_to_activate_time_.push_back(std::make_pair( On 2017/04/06 23:43:15, brianderson wrote: > emplace_back? Done. https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:99: fprintf(stderr, "\n\t\tScheduler::NotifyReadyToActivate: ptree: %d", On 2017/04/05 19:11:33, vmpstr wrote: > this is just for debugging I assume? Yes I plan to clean these up before submitting https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:637: begin_main_frame_args_.ready_to_activate_time = ready_to_activate_time_; On 2017/04/05 19:11:33, vmpstr wrote: > std::move Done. https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/2778223005/diff/40001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:189: std::vector<std::pair<uint32_t, base::TimeTicks>> ready_to_activate_time_; On 2017/04/05 19:11:33, vmpstr wrote: > nit: can you put a comment about what this pair represents? Done. https://codereview.chromium.org/2778223005/diff/40001/cc/trees/proxy_impl.cc File cc/trees/proxy_impl.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/trees/proxy_impl.cc#... cc/trees/proxy_impl.cc:578: int ProxyImpl::SyncTreeSourceFrameNumber() { On 2017/04/05 19:11:33, vmpstr wrote: > Based on the name, is this meant to check LTHI::sync_tree()? Although this would > erroneously return the active tree if the pending tree doesn't exist... So maybe > this is good I do want the pending tree. Should I rename the method? https://codereview.chromium.org/2778223005/diff/40001/cc/trees/proxy_main.cc File cc/trees/proxy_main.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/trees/proxy_main.cc#... cc/trees/proxy_main.cc:121: // Set frame_source_number in BeginFrameArgs On 2017/04/05 19:11:33, vmpstr wrote: > I don't think you need this comment :) Done. https://codereview.chromium.org/2778223005/diff/40001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:627: int SingleThreadProxy::SyncTreeSourceFrameNumber() { On 2017/04/06 23:43:15, brianderson wrote: > On 2017/04/05 19:11:33, vmpstr wrote: > > Do we have pending trees in single thread proxy? (I might not know of all the > > uses of this :) ). > > SingleThreadProxy is only used in the Browser, where we don't need this feature > - so we could just return 0. But it does seem to break the Proxy abstraction to > expose the concept of a SyncTree. > > Alternatively, we could pass in the source_frame_number as an argument to > NotifyReadyToActivate. ProxyImpl would pass in the pending tree's number. > SingleThreadProxy would pass in the active_tree's number (since we commit to > active tree). Updated to passing argument to NotifyReadyToActivate and passing pending-tree in ProxyImpl. Passing active-tree in SingleThreadProxy triggers a ton of NotifyReadyToActivate without corresponding BeginMainFrame. Are you sure this should be active-tree here? I'll take a closer look.
PTAL https://codereview.chromium.org/2778223005/diff/40001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/2778223005/diff/40001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:627: int SingleThreadProxy::SyncTreeSourceFrameNumber() { On 2017/04/08 00:29:07, panicker wrote: > On 2017/04/06 23:43:15, brianderson wrote: > > On 2017/04/05 19:11:33, vmpstr wrote: > > > Do we have pending trees in single thread proxy? (I might not know of all > the > > > uses of this :) ). > > > > SingleThreadProxy is only used in the Browser, where we don't need this > feature > > - so we could just return 0. But it does seem to break the Proxy abstraction > to > > expose the concept of a SyncTree. > > > > Alternatively, we could pass in the source_frame_number as an argument to > > NotifyReadyToActivate. ProxyImpl would pass in the pending tree's number. > > SingleThreadProxy would pass in the active_tree's number (since we commit to > > active tree). > > Updated to passing argument to NotifyReadyToActivate and passing pending-tree in > ProxyImpl. > Passing active-tree in SingleThreadProxy triggers a ton of NotifyReadyToActivate > without corresponding BeginMainFrame. > Are you sure this should be active-tree here? I'll take a closer look. Discussed offline and updated to passing 0 here.
lgtm once nits are addressed and debug code removed. https://codereview.chromium.org/2778223005/diff/80001/cc/output/begin_frame_a... File cc/output/begin_frame_args.h (right): https://codereview.chromium.org/2778223005/diff/80001/cc/output/begin_frame_a... cc/output/begin_frame_args.h:110: // reaches Extra line break? https://codereview.chromium.org/2778223005/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2778223005/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:98: std::make_pair(sync_tree, base::TimeTicks::Now())); I was under the impression you might be able to remove the std::make_pair here with emplace_back. Is that possible? https://codereview.chromium.org/2778223005/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:639: std::move(ready_to_activate_time_); After the move, ready_to_activate_time_ is in an valid but unspecified state. clear() should be called on it so it's in a valid and specified state. https://codereview.chromium.org/2778223005/diff/80001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/2778223005/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:80: void NotifyReadyToActivate(int sync_tree); sync_tree -> source_frame_number
Thanks for the review. Re: testing any guidance? I don't think updating scheduler_unittest.cc is useful as it can only test whether NotifyReadyToActivate() updates internal state for ready_to_activate_time What I really want to test is scenario like: - BeginMainFrame1 -> Commit1 -> BeginMainFrame2 ->Commit2 ->NotifyReadyToActivate1 -> NotifyReadyToActivate2 Now the next BeginMainFrame has the timing for both NotifyReadyToActivate1 & NotifyReadyToActivate2 I know you recommended against layouttest, any advise? https://codereview.chromium.org/2778223005/diff/80001/cc/output/begin_frame_a... File cc/output/begin_frame_args.h (right): https://codereview.chromium.org/2778223005/diff/80001/cc/output/begin_frame_a... cc/output/begin_frame_args.h:110: // reaches On 2017/04/10 23:12:47, brianderson wrote: > Extra line break? Done. https://codereview.chromium.org/2778223005/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2778223005/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:98: std::make_pair(sync_tree, base::TimeTicks::Now())); On 2017/04/10 23:12:47, brianderson wrote: > I was under the impression you might be able to remove the std::make_pair here > with emplace_back. Is that possible? Good catch, done. https://codereview.chromium.org/2778223005/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:639: std::move(ready_to_activate_time_); On 2017/04/10 23:12:47, brianderson wrote: > After the move, ready_to_activate_time_ is in an valid but unspecified state. > clear() should be called on it so it's in a valid and specified state. Done. https://codereview.chromium.org/2778223005/diff/80001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/2778223005/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:80: void NotifyReadyToActivate(int sync_tree); On 2017/04/10 23:12:47, brianderson wrote: > sync_tree -> source_frame_number Done.
The CQ bit was checked by panicker@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: This issue passed the CQ dry run.
For testing, you may want to copy then modify LayerTreeHostProxyTestCommitWaitsForActivationMFBA, which explicitly gets us into the state where we have a BeginMainFrame that's ready to commit while there's still an active tree.
On 2017/04/12 22:34:12, brianderson wrote: > For testing, you may want to copy then modify > LayerTreeHostProxyTestCommitWaitsForActivationMFBA, which explicitly gets us > into the state where we have a BeginMainFrame that's ready to commit while > there's still an active tree. * ...while there's still a pending tree.
I'm going to go ahead and submit, advise on testing the scenario I mentioned would be appreciated.
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org Link to the patchset: https://codereview.chromium.org/2778223005/#ps100001 (title: "address comments, remove printfs")
The CQ bit was unchecked by panicker@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/12 22:34:38, brianderson wrote: > On 2017/04/12 22:34:12, brianderson wrote: > > For testing, you may want to copy then modify > > LayerTreeHostProxyTestCommitWaitsForActivationMFBA, which explicitly gets us > > into the state where we have a BeginMainFrame that's ready to commit while > > there's still an active tree. > > * ...while there's still a pending tree. Whoops, just saw this. Will give it a try.
I added a baseline test layer_tree_host_unittest_proxy and left a question there. I couldn't find a way to delay activation until after two commits. Do you think I should keep (just added in this patch) the initialization of source_frame_number in copy ctor & BeginFrameArgs::Create() vs. just initializing to 0 in header? https://codereview.chromium.org/2778223005/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_proxy.cc (right): https://codereview.chromium.org/2778223005/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:434: // Frame #1, since activation was delayed, nothing here yet. I didn't actually delay this (I removed the delay that existed before), is ready_to_activate really expected to be empty here?
https://codereview.chromium.org/2778223005/diff/120001/cc/output/begin_frame_... File cc/output/begin_frame_args.h (right): https://codereview.chromium.org/2778223005/diff/120001/cc/output/begin_frame_... cc/output/begin_frame_args.h:114: uint32_t source_frame_number = 0; I like this way of initializing better, but it's probably best to keep it consistent with the rest of the class. https://codereview.chromium.org/2778223005/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_proxy.cc (right): https://codereview.chromium.org/2778223005/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:420: class LayerTreeHostProxyTestActivationTime : public LayerTreeHostProxyTest { > I couldn't find a way to delay activation until after two commits. I think you'll want to delay activation until after two BeginMainFrames (as opposed to commits), then delay the third BeginMainFrame until after two activations. The DidSendBeginMainFrameOnThread hook might be useful. https://codereview.chromium.org/2778223005/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:434: // Frame #1, since activation was delayed, nothing here yet. On 2017/04/13 22:09:22, panicker wrote: > I didn't actually delay this (I removed the delay that existed before), is > ready_to_activate really expected to be empty here? It could be since there's a race between whether activation or the next BeginMainFrame occurs first. If the first activation is slow, it would lose the race pretty often. https://codereview.chromium.org/2778223005/diff/120001/cc/trees/proxy_main.h File cc/trees/proxy_main.h (right): https://codereview.chromium.org/2778223005/diff/120001/cc/trees/proxy_main.h#... cc/trees/proxy_main.h:136: BeginFrameArgs begin_frame_args_for_testing_; Can you store this in LayerTreeTest from an overridden BeginMainFrame() or WillBeginMainFrame() instead?
https://codereview.chromium.org/2778223005/diff/120001/cc/output/begin_frame_... File cc/output/begin_frame_args.h (right): https://codereview.chromium.org/2778223005/diff/120001/cc/output/begin_frame_... cc/output/begin_frame_args.h:114: uint32_t source_frame_number = 0; On 2017/04/13 22:36:37, brianderson wrote: > I like this way of initializing better, but it's probably best to keep it > consistent with the rest of the class. Done. https://codereview.chromium.org/2778223005/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_proxy.cc (right): https://codereview.chromium.org/2778223005/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:420: class LayerTreeHostProxyTestActivationTime : public LayerTreeHostProxyTest { On 2017/04/13 22:36:37, brianderson wrote: > > I couldn't find a way to delay activation until after two commits. > > I think you'll want to delay activation until after two BeginMainFrames (as > opposed to commits), then delay the third BeginMainFrame until after two > activations. > > The DidSendBeginMainFrameOnThread hook might be useful. Not sure how this would work, when activation is blocked (say in DidSendBeginMainFrameOnThread) I cannot get the next DidCommit etc to trigger until activation is unblocked (regardless of calling SetNextCommitWaitsForActivation) i.e. cannot get 2 consequent frames DidCommit+DidBeginMainFrame without unblocking activation. Maybe there's a way to do this. Though unless you have specific suggestion here let's start with this one test? https://codereview.chromium.org/2778223005/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:434: // Frame #1, since activation was delayed, nothing here yet. On 2017/04/13 22:36:37, brianderson wrote: > On 2017/04/13 22:09:22, panicker wrote: > > I didn't actually delay this (I removed the delay that existed before), is > > ready_to_activate really expected to be empty here? > > It could be since there's a race between whether activation or the next > BeginMainFrame occurs first. If the first activation is slow, it would lose the > race pretty often. I've added back the explicit delay to make this deterministic. you expect this to make the test flaky right? https://codereview.chromium.org/2778223005/diff/120001/cc/trees/proxy_main.h File cc/trees/proxy_main.h (right): https://codereview.chromium.org/2778223005/diff/120001/cc/trees/proxy_main.h#... cc/trees/proxy_main.h:136: BeginFrameArgs begin_frame_args_for_testing_; On 2017/04/13 22:36:37, brianderson wrote: > Can you store this in LayerTreeTest from an overridden BeginMainFrame() or > WillBeginMainFrame() instead? Can you elaborate on what you mean exactly: - Store it in LayerTreeHostClientForTesting in layer_tree_test - and Update WillBeginMainFrame to take args (lots of references)? or something else ?
https://codereview.chromium.org/2778223005/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_proxy.cc (right): https://codereview.chromium.org/2778223005/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:420: class LayerTreeHostProxyTestActivationTime : public LayerTreeHostProxyTest { ============ Re: test for two ready_to_activate_time entires: ============ I'm not sure why the MFBA test uses a delayed task, but I think we can avoid it in our case. The following is a more detailed description of what you should be able to do to get two entries in ready_to_activate_time. In this BeginCommitOnThread method, for source_frame_number 1: a) BlockNotifyReadyToActivate(true) b) PostSetNeedsCommitToMainThread(). c) Set a activate_blocked_ to true. Then, in DidSendBeginMainFrameOnThread, when activate_blocked_ is true: d) Call BlockNotifyReadyToActivate(false). e) Set is_blocked to false. At this point there will be a pending tree and a BeginMainFrame on its way to the main thread. Then, in DidActivateTreeOnThread, when the active_tree source_frame_number is 2: f) PostSetNeedsCommitToMainThread(). Then, in DidBeginMainFrame, when source_frame_number is 3: g) Verify that there are two entries in ready_to_activate_time. ============ Re: test for one ready_to_activate_time entires: ============ Remove steps a-e. And check for source_frame_number 2 in g. Calling PostSetNeedsCommitToMainThread from DidActivateTreeOnThread avoids the race between activation and the next BeginMainFrame.
PTAL https://codereview.chromium.org/2778223005/diff/120001/cc/trees/proxy_main.h File cc/trees/proxy_main.h (right): https://codereview.chromium.org/2778223005/diff/120001/cc/trees/proxy_main.h#... cc/trees/proxy_main.h:136: BeginFrameArgs begin_frame_args_for_testing_; On 2017/04/14 22:02:45, panicker wrote: > On 2017/04/13 22:36:37, brianderson wrote: > > Can you store this in LayerTreeTest from an overridden BeginMainFrame() or > > WillBeginMainFrame() instead? > > Can you elaborate on what you mean exactly: > - Store it in LayerTreeHostClientForTesting in layer_tree_test > - and Update WillBeginMainFrame to take args (lots of references)? > or something else ? > Addressed https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_proxy.cc (right): https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:462: EXPECT_EQ(1UL, args.ready_to_activate_time[0].first); Hmm I think I expected this to be #0 vs. #1?
Description was changed from ========== Plumb activation time in cc to Blink Scheduler in Main BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Plumb activation time in cc to Blink Scheduler in Main BUG=657826,657825 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_proxy.cc (left): https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:324: // should not signal the completion event of the second commit. Looks like this comment was accidentally removed. https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_proxy.cc (right): https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:444: EXPECT_FALSE(activate_blocked_); Only expect during case 1. https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:462: EXPECT_EQ(1UL, args.ready_to_activate_time[0].first); On 2017/04/17 21:54:50, panicker wrote: > Hmm I think I expected this to be #0 vs. #1? I think case 1 should have the #0 and case 3 should have #1 and #2. https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:469: if (impl->sync_tree()->source_frame_number() == 2) || source_frame_number == 0. https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:476: if (impl->sync_tree()->source_frame_number() <= 0) { Change the method override to CommitCompleteOnThread (so the sync_tree is valid) and only check for source_frame_number == 1 here?
Test is working now, thanks!! https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_proxy.cc (left): https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:324: // should not signal the completion event of the second commit. On 2017/04/17 22:26:33, brianderson wrote: > Looks like this comment was accidentally removed. Done. https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_proxy.cc (right): https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:444: EXPECT_FALSE(activate_blocked_); On 2017/04/17 22:26:33, brianderson wrote: > Only expect during case 1. Acknowledged. https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:462: EXPECT_EQ(1UL, args.ready_to_activate_time[0].first); On 2017/04/17 22:26:33, brianderson wrote: > On 2017/04/17 21:54:50, panicker wrote: > > Hmm I think I expected this to be #0 vs. #1? > > I think case 1 should have the #0 and case 3 should have #1 and #2. Acknowledged. https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:469: if (impl->sync_tree()->source_frame_number() == 2) On 2017/04/17 22:26:33, brianderson wrote: > || source_frame_number == 0. Done. https://codereview.chromium.org/2778223005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_proxy.cc:476: if (impl->sync_tree()->source_frame_number() <= 0) { On 2017/04/17 22:26:33, brianderson wrote: > Change the method override to CommitCompleteOnThread (so the sync_tree is valid) > and only check for source_frame_number == 1 here? Done.
https://codereview.chromium.org/2778223005/diff/200001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2778223005/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:97: ready_to_activate_time_.back().first != (unsigned)source_frame_number) { static_cast, then lgtm.
https://codereview.chromium.org/2778223005/diff/200001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2778223005/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:97: ready_to_activate_time_.back().first != (unsigned)source_frame_number) { On 2017/04/17 23:28:50, brianderson wrote: > static_cast, then lgtm. Done.
panicker@chromium.org changed reviewers: + ccameron@chromium.org
+ccameron could you review for OWNERS for content/browser/compositor/gpu_vsync_begin_frame_source.cc
rs lgtm
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org, ccameron@chromium.org Link to the patchset: https://codereview.chromium.org/2778223005/#ps240001 (title: "rebase to head")
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
Try jobs failed on following builders: linux_chromium_asan_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 panicker@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...
panicker@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
Alex or Sami - could you approve for renderer-scheduler unittest update?
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by panicker@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: This issue passed the CQ dry run.
panicker@chromium.org changed reviewers: + miguelg@chromium.org, tedchoc@chromium.org - enne@chromium.org, skyostil@chromium.org
miguelg, tedchoc - could one of you review the minor change for /ui/android (OWNERS)
https://codereview.chromium.org/2778223005/diff/240003/ui/android/window_andr... File ui/android/window_android.cc (right): https://codereview.chromium.org/2778223005/diff/240003/ui/android/window_andr... ui/android/window_android.cc:105: BEGINFRAME_FROM_HERE, source_id(), next_sequence_number_, 0, frame_time, should this be kDefaultSourceFrameNumber instead of 0?
https://codereview.chromium.org/2778223005/diff/240003/ui/android/window_andr... File ui/android/window_android.cc (right): https://codereview.chromium.org/2778223005/diff/240003/ui/android/window_andr... ui/android/window_android.cc:105: BEGINFRAME_FROM_HERE, source_id(), next_sequence_number_, 0, frame_time, On 2017/04/18 23:11:06, Ted C wrote: > should this be kDefaultSourceFrameNumber instead of 0? Done.
The CQ bit was checked by panicker@chromium.org to run a CQ dry run
On 2017/04/19 17:22:09, panicker wrote: > https://codereview.chromium.org/2778223005/diff/240003/ui/android/window_andr... > File ui/android/window_android.cc (right): > > https://codereview.chromium.org/2778223005/diff/240003/ui/android/window_andr... > ui/android/window_android.cc:105: BEGINFRAME_FROM_HERE, source_id(), > next_sequence_number_, 0, frame_time, > On 2017/04/18 23:11:06, Ted C wrote: > > should this be kDefaultSourceFrameNumber instead of 0? > > Done. lgtm
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 panicker@chromium.org
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org, ccameron@chromium.org, alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2778223005/#ps280001 (title: "review comment")
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": 280001, "attempt_start_ts": 1492622784593170,
"parent_rev": "03427bcc41560b36b5c595eda92e35f563a6303e", "commit_rev":
"9a2eb472fde51de48c095c9c208101c534ac211e"}
Message was sent while issue was closed.
Description was changed from ========== Plumb activation time in cc to Blink Scheduler in Main BUG=657826,657825 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Plumb activation time in cc to Blink Scheduler in Main BUG=657826,657825 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2778223005 Cr-Commit-Position: refs/heads/master@{#465701} Committed: https://chromium.googlesource.com/chromium/src/+/9a2eb472fde51de48c095c9c2081... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:280001) as https://chromium.googlesource.com/chromium/src/+/9a2eb472fde51de48c095c9c2081...
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:280001) has been created in https://codereview.chromium.org/2833603002/ by rogerm@chromium.org. The reason for reverting is: Failure observed in LayerTreeHostProxyTestActivationTime.RunMultiThread_DelegatingRenderer https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.win%2FWin7_Test... .
Message was sent while issue was closed.
On 2017/04/19 21:21:12, Roger McFarlane (Chromium) wrote: > A revert of this CL (patchset #16 id:280001) has been created in > https://codereview.chromium.org/2833603002/ by mailto:rogerm@chromium.org. > > The reason for reverting is: Failure observed in > LayerTreeHostProxyTestActivationTime.RunMultiThread_DelegatingRenderer > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.win%2FWin7_Test... > . According to Findit's analysis in https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV..., Findit suggests this CL to have introduced flakiness in LayerTreeHostProxyTestActivationTime.RunMultiThread_DelegatingRenderer as pointed out by Roger. Can someone please help confirm? Thanks, Jeff on behalf of Findit team
Message was sent while issue was closed.
On 2017/04/20 17:26:33, lijeffrey wrote: > On 2017/04/19 21:21:12, Roger McFarlane (Chromium) wrote: > > A revert of this CL (patchset #16 id:280001) has been created in > > https://codereview.chromium.org/2833603002/ by mailto:rogerm@chromium.org. > > > > The reason for reverting is: Failure observed in > > LayerTreeHostProxyTestActivationTime.RunMultiThread_DelegatingRenderer > > > > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.win%2FWin7_Test... > > . > > According to Findit's analysis in > https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV..., > Findit suggests this CL to have introduced flakiness in > LayerTreeHostProxyTestActivationTime.RunMultiThread_DelegatingRenderer as > pointed out by Roger. > > Can someone please help confirm? > > Thanks, > Jeff on behalf of Findit team That is correct. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
