Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
914403003:
cc: Add main frame timing info to frame timing tracker. (Closed)
Descriptioncc: Add main frame timing info to frame timing tracker.
This patch adds main frame timing info to frame timing tracker.
The main frame start time is the timestamp of the frame in which we
did a SendBeginMainFrame. The "end" of the frame is the timestamp of
the beginning of the frame that follows the frame in which we activated.
We report start and end_time on each of the frames.
If a main frame is aborted, then we don't report the time unless we
aborted due to the fact that there were no changes (ie a "successful"
frame).
R=danakj, brianderson, mpb@chromium.org
Committed: https://crrev.com/d704c875efd1204a4128d66685fc95b204dc353d
Cr-Commit-Position: refs/heads/master@{#323806}
Patch Set 1 #
Total comments: 3
Patch Set 2 : tests #
Total comments: 7
Patch Set 3 : update #
Total comments: 5
Patch Set 4 : update #
Total comments: 2
Patch Set 5 : update #
Total comments: 15
Patch Set 6 : update #
Total comments: 7
Patch Set 7 : #
Total comments: 19
Patch Set 8 : #Patch Set 9 : comment #Patch Set 10 : #
Total comments: 3
Patch Set 11 : rebase #
Messages
Total messages: 38 (5 generated)
Please take a look. I'm gonna make some unittests, but if you have some changes that you'd like made to the code, let me know! https://codereview.chromium.org/914403003/diff/1/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/914403003/diff/1/cc/scheduler/scheduler.cc#ne... cc/scheduler/scheduler.cc:332: return begin_impl_frame_args_.frame_time + begin_impl_frame_args_.interval; +brianderson, you mentioned that this might not be correct in webview right now. Is there a bug out there to track/fix this? Or is there a better way of getting this information? I want to add a TODO or something like that, but I'm kind of missing context. https://codereview.chromium.org/914403003/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/914403003/diff/1/cc/trees/layer_tree_impl.cc#... cc/trees/layer_tree_impl.cc:161: // get them. Alternatively, I can store requests ids on layer_tree_impl() keyed by layer_id. Would that be better? https://codereview.chromium.org/914403003/diff/1/cc/trees/single_thread_proxy.cc File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:828: base::TimeTicks SingleThreadProxy::GetNextBeginImplFrameTimeIfRequested() Will add a TODO.
danakj@chromium.org changed reviewers: + enne@chromium.org - danakj@chromium.org
enne@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/914403003/diff/20001/cc/debug/frame_timing_tr... File cc/debug/frame_timing_tracker.h (right): https://codereview.chromium.org/914403003/diff/20001/cc/debug/frame_timing_tr... cc/debug/frame_timing_tracker.h:43: base::TimeDelta duration; It's a little weird to have a duration, when you can get that from multiple timestamps? https://codereview.chromium.org/914403003/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:2097: client_->GetNextBeginImplFrameTimeIfRequested() - main_frame_time_; So for SingleThreadProxy this will always be negative? https://codereview.chromium.org/914403003/diff/20001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/914403003/diff/20001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.cc:40: void ProcessLayersRecursive(LayerImpl* current, const Lambda& lambda) { Maybe add a LayerTreeHostCommon::CallFunctionForSubtree(LayerType*, const Lambda& lambda) function, rather than a one-off here?
mithro@mithis.com changed reviewers: + mithro@mithis.com
PTAL https://codereview.chromium.org/914403003/diff/20001/cc/debug/frame_timing_tr... File cc/debug/frame_timing_tracker.h (right): https://codereview.chromium.org/914403003/diff/20001/cc/debug/frame_timing_tr... cc/debug/frame_timing_tracker.h:43: base::TimeDelta duration; On 2015/02/12 23:41:07, enne wrote: > It's a little weird to have a duration, when you can get that from multiple > timestamps? This struct just represents what we would actually send back to the client. I'm not sure why it's weird :) But I changed it. https://codereview.chromium.org/914403003/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:2097: client_->GetNextBeginImplFrameTimeIfRequested() - main_frame_time_; On 2015/02/12 23:41:07, enne wrote: > So for SingleThreadProxy this will always be negative? I changed this to use the same value. As far as I know, this is only used in browser compositors, where we wouldn't expect to see these requests, but it should work the same. https://codereview.chromium.org/914403003/diff/20001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/914403003/diff/20001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.cc:40: void ProcessLayersRecursive(LayerImpl* current, const Lambda& lambda) { On 2015/02/12 23:41:07, enne wrote: > Maybe add a LayerTreeHostCommon::CallFunctionForSubtree(LayerType*, const > Lambda& lambda) function, rather than a one-off here? Done.
Sorry for the O_o? but I really don't quite follow what these values are supposed to be representing. https://codereview.chromium.org/914403003/diff/40001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:2095: base::TimeTicks end_time = client_->GetNextBeginImplFrameTimeIfRequested(); Maybe I just don't understand the spec, but what is "end time" supposed to represent? I had the same question about duration. It's just an estimate of how long this particular thing will be shown? https://codereview.chromium.org/914403003/diff/40001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/914403003/diff/40001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.cc:40: void ProcessLayersRecursive(LayerImpl* current, const Lambda& lambda) { This isn't needed anymore, yeah? https://codereview.chromium.org/914403003/diff/40001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/40001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:694: impl().layer_tree_host_impl->SetBeginMainFrameTime( I'm not quite sure I follow what this is doing. When you send a begin main frame message, you record a time. But, we could have many BeginImplFrames between the time this BeginMainFrame happens and activation later. Maybe this is part of my confusion about what start and end are supposed to represent.
PTAL. An explanation is below. Let me know if it's unclear... Also let me know if there's a better way to accomplish what I describe there. That is, if the code doesn't match the description, then the description is more correct :) https://codereview.chromium.org/914403003/diff/40001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/914403003/diff/40001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.cc:40: void ProcessLayersRecursive(LayerImpl* current, const Lambda& lambda) { On 2015/03/06 00:00:57, enne wrote: > This isn't needed anymore, yeah? Whoops, removed. https://codereview.chromium.org/914403003/diff/40001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/40001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:694: impl().layer_tree_host_impl->SetBeginMainFrameTime( On 2015/03/06 00:00:57, enne wrote: > I'm not quite sure I follow what this is doing. When you send a begin main > frame message, you record a time. But, we could have many BeginImplFrames > between the time this BeginMainFrame happens and activation later. Maybe this > is part of my confusion about what start and end are supposed to represent. So this is meant to capture the time it takes for a content update to make it to screen. Or in general, how long it takes the layout, javascript, whatever else, rasterization, and activation to run. Start time refers to the beginning of the main frame. It's the time we would use in rAF if rAF was fired. The end time represents the time after all of the above work has finished. To be a bit more specific, it's the time of rAF that would happen after we can display new content, if such a rAF was scheduled. So end - start should be a multiple of 16ms in most cases and it provides a signal that says "you made the frame" vs "you didn't make the frame". A sequence of 16ms 16ms 32ms 32ms 16ms should represent "good 60fps for the first 2 frames", followed by "something slow-ish that skipped a frame twice", followed by "another good frame". Does that make sense? This is what I'm trying to capture. The fact that there are multiple impl frames is ok, as long as we record the main frame once on commit and measure it until the next activation. It's entirely possible that this code isn't correct, but what I described is what I'm trying to accomplish :)
https://codereview.chromium.org/914403003/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:2097: client_->GetNextBeginImplFrameTimeIfRequested() - main_frame_time_; On 2015/03/03 18:56:46, vmpstr wrote: > On 2015/02/12 23:41:07, enne wrote: > > So for SingleThreadProxy this will always be negative? > > I changed this to use the same value. As far as I know, this is only used in > browser compositors, where we wouldn't expect to see these requests, but it > should work the same. Oh. I figured that requests were coming from Javascript, so were in the renderer? I guess then I don't quite understand what the goals here are. https://codereview.chromium.org/914403003/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:309: RecordMainFrameTiming(); Is it possible to just do this unconditionally in ActivateSyncTree? I think we call it in all cases: non-impl-side painting, impl-side painting + pending tree, impl-side painting + commit to active. https://codereview.chromium.org/914403003/diff/60001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/60001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:695: impl().scheduler->LastBeginImplFrameTime()); I still think this is a little weird, in that a lost context on a previous frame will make the next frame look long, even if that content hadn't even requested a begin main frame / needs commit. If the goal is "time to get content on screen", then maybe it makes sense to time from SetNeedsCommit on the main thread instead? Sorry for all the confusion here. I still feel like maybe I'm just missing something.
Given the strange spec decisions, I think is implemented correctly. https://codereview.chromium.org/914403003/diff/80001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/80001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:694: impl().layer_tree_host_impl->SetBeginMainFrameTime( Can you rename this to be something other than "BeginMainFrameTime", and tie it more to like "this value is passed to raf as the current time to animate at"?
Hi vmpstr, Just going through my code review backlog, sorry for taking so long to respond. Couple of things; * We are actively trying to remove people using base::TimeTicks for frame time - instead they should be using the full BeginFrameArgs to make it clear that it's a frame time rather than some random TimeTicks used for other things. * At the moment it is impossible to get a reliable value for the next frame time. I'm yet to really understand the cc/debug/frame_timing_tracker.h code - but I assume enne or danakj has review that part for you. Tim 'mithro' Ansell https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.c... cc/scheduler/scheduler.cc:306: base::TimeTicks Scheduler::NextBeginImplFrameTimeIfRequested() const { Why do you need this method? https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.h... cc/scheduler/scheduler.h:157: base::TimeTicks NextBeginImplFrameTimeIfRequested() const; Why do you need the NextBeginImplFrameTime? At the moment the value you are returning is a *extremely rough* approximate to the next BeginImplFrameTime value. https://codereview.chromium.org/914403003/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/914403003/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:163: base::TimeTicks GetNextBeginImplFrameTimeIfRequested() const override { If this method should never be called, then can you please use NOT_REACHED() here rather than returning a random value which will make it hard to track down issues in the future - otherwise we should figure out what the correct value is to return. https://codereview.chromium.org/914403003/diff/80001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/80001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:695: impl().scheduler->LastBeginImplFrameTime()); What is LastBeginImplFrameTime here? If it is as enne describes, then it is already stored in begin_main_frame_state->begin_frame_args https://codereview.chromium.org/914403003/diff/80001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:1399: base::TimeTicks ThreadProxy::GetNextBeginImplFrameTimeIfRequested() const { What is this used for?
Please take another look. Sorry for the delay in updating the review. I think I've addressed all comments, but let me know if I missed something. https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.c... cc/scheduler/scheduler.cc:306: base::TimeTicks Scheduler::NextBeginImplFrameTimeIfRequested() const { On 2015/03/11 04:01:32, mithro wrote: > Why do you need this method? This returns the next predicted raf time which is one of the values I need to report. I've changed the name to hopefully better reflect the intent. https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.h... cc/scheduler/scheduler.h:157: base::TimeTicks NextBeginImplFrameTimeIfRequested() const; On 2015/03/11 04:01:33, mithro wrote: > Why do you need the NextBeginImplFrameTime? At the moment the value you are > returning is a *extremely rough* approximate to the next BeginImplFrameTime > value. The value that I need to get is the rAF time if rAF was scheduled this frame. I understand that this is a rough estimate and a variety of things can cause the actual raf time to be further (eg process being descheduled). Do you know of a better estimate I can use short of actually scheduling a rAF only for the purposes of getting this time? https://codereview.chromium.org/914403003/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/914403003/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:163: base::TimeTicks GetNextBeginImplFrameTimeIfRequested() const override { On 2015/03/11 04:01:33, mithro wrote: > If this method should never be called, then can you please use NOT_REACHED() > here rather than returning a random value which will make it hard to track down > issues in the future - otherwise we should figure out what the correct value is > to return. I've added a NOTREACHED. https://codereview.chromium.org/914403003/diff/80001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/80001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:694: impl().layer_tree_host_impl->SetBeginMainFrameTime( On 2015/03/10 00:32:29, enne wrote: > Can you rename this to be something other than "BeginMainFrameTime", and tie it > more to like "this value is passed to raf as the current time to animate at"? Done. https://codereview.chromium.org/914403003/diff/80001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:695: impl().scheduler->LastBeginImplFrameTime()); On 2015/03/11 04:01:33, mithro wrote: > What is LastBeginImplFrameTime here? > > If it is as enne describes, then it is already stored in > begin_main_frame_state->begin_frame_args Done. https://codereview.chromium.org/914403003/diff/80001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:1399: base::TimeTicks ThreadProxy::GetNextBeginImplFrameTimeIfRequested() const { On 2015/03/11 04:01:33, mithro wrote: > What is this used for? This plumbs the next predicted raf time to lthi via the client interface.
mithro: can you take a look at this? I want to make sure your concerns are addressed.
On 2015/03/17 21:24:04, enne wrote: > mithro: can you take a look at this? I want to make sure your concerns are > addressed. ping.
Hi vmpstr, Sorry about the delay in reviewing. As mentioned in the previous email, we should be trying to use BeginFrameArgs instead of TimeTicks in this CL. This would mean that you get the interval with the frame time together and can't accidently mix and match. I'm wondering if we could record the timing information at the scheduler and then plumb the whole record as needed? Tim https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.c... cc/scheduler/scheduler.cc:306: base::TimeTicks Scheduler::NextBeginImplFrameTimeIfRequested() const { On 2015/03/16 18:36:43, vmpstr wrote: > On 2015/03/11 04:01:32, mithro wrote: > > Why do you need this method? > > This returns the next predicted raf time which is one of the values I need to > report. I've changed the name to hopefully better reflect the intent. Can you link me to why you need this? I was pretty sure the spec didn't require it.. Using frame_time + interval as the "next frame time" is also fundamentally broken when not using vsync throttling. https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:285: RecordMainFrameTiming(); The fact that this method is being called in two places makes me think it is in the wrong place. https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:523: void RecordMainFrameTiming(); This method should definitely be taking a BeginFrameArgs not a base::TimeTicks. SetCurrentRequestAnimationFrameTime is a bit confusing because there is the animation frame time used for the impl thread and the animation frame time used for the main thread which are not equal. I can quickly see this value getting used in the wrong places.
I will be working on addressing your comments. In the mean time please see my comments and questions below. From my understanding the patch you have here: https://codereview.chromium.org/787763006/ will make the frame_time refer to the proper beginning of the frame. However, this patch is meant to get the time that would be used in the raf callback, which may or may not be the vsync time. https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.c... cc/scheduler/scheduler.cc:306: base::TimeTicks Scheduler::NextBeginImplFrameTimeIfRequested() const { On 2015/03/20 02:17:53, mithro wrote: > On 2015/03/16 18:36:43, vmpstr wrote: > > On 2015/03/11 04:01:32, mithro wrote: > > > Why do you need this method? > > > > This returns the next predicted raf time which is one of the values I need to > > report. I've changed the name to hopefully better reflect the intent. > > Can you link me to why you need this? I was pretty sure the spec didn't require > it.. > > Using frame_time + interval as the "next frame time" is also fundamentally > broken when not using vsync throttling. The spec I'm working off is here: http://w3c.github.io/frame-timing/#performancerendertiming In particular, "The end of the frame is defined as what the beginning time of the next frame's requestAnimationFrame callback (see [Animation-Timing]) would have been, had one been scheduled in the current frame. If a requestAnimationFrame is already requested, its beginning time should be used." The last sentence is a recent change that isn't a part of this patch though. What time is the appropriate one to use in this situation? That is, if there is no raf scheduled and this frame is over, we need to know that the next raf time would have been if it was scheduled. My understanding from talking to brianderson@ that you have a patch that would ensure that the impl frame time is always updated at the right time. Since (also from my possibly flawed understanding), this is the frame args that would also be used for the main frame time, it would make a good candidate for predicting what the next raf would be. Please let me know if this is not the case. Also, if it isn't the case, what do you think is the right time to use here? https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:285: RecordMainFrameTiming(); On 2015/03/20 02:17:54, mithro wrote: > The fact that this method is being called in two places makes me think it is in > the wrong place. > Whether this is done in the scheduler or in lthi, I'm not sure where a single place is that would capture both of these cases: 1. We aborted a commit due to no changes in the trees and properties 2. We processed the commit, rasterized the sync tree, and activated it. Please let me know if you had a better place in mind. https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:523: void RecordMainFrameTiming(); On 2015/03/20 02:17:54, mithro wrote: > This method should definitely be taking a BeginFrameArgs not a base::TimeTicks. > > SetCurrentRequestAnimationFrameTime is a bit confusing because there is the > animation frame time used for the impl thread and the animation frame time used > for the main thread which are not equal. I can quickly see this value getting > used in the wrong places. Ok, I will work on passing BeginFrameArgs here instead. My, again clearly flawed, understanding was that the impl thread time and the raf animation time was in fact the same time. Please let me know where they diverge.
New patch is up. Please take a look.
https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:696: impl().last_begin_main_frame_args = begin_main_frame_state->begin_frame_args; If main_frame_before_activation_enabled is true in SchedulerSettings, it's possible for this line to run twice before DidActivateSyncTree gets called once. main_frame_before_activation_enabled is experimental, so this isn't important to fix as part of your patch, but can you add a TODO here so we don't accidentally break your code in the future? Alternatively, make impl().last_begin_main_frame_args a queue. https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1364: RecordMainFrameTiming(); Regarding the interval / prediction issue, we should be able to wait for the next BeginImpFrame's frame_time instead of predicting it. We always request one more BeginFrame than we need before going idle to avoid excess toggling of SetNeedsBeginFrames. The logic that does that is here: https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched... Can you do something like: 1) Add a last_begin_main_frame_activated_args and assign it to last_begin_main_frame_args here. 2) If last_begin_main_frame_activated_args.IsValid() in WillBeginImplFrame, call RecordMainFrameTiming there. https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1385: impl().last_begin_main_frame_args, impl().last_begin_impl_frame_args); Although this is slightly wrong right now because begin_main_frame_args uses Now() for it's frame time, while begin_impl_frame_args uses the original frame time, I think it's good enough to land. Tim's patch should make this 100% correct once it lands and I don't see a reason to introduce workarounds in this patch, especially since getting this 100% correct isn't urgent.
https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:696: impl().last_begin_main_frame_args = begin_main_frame_state->begin_frame_args; On 2015/03/20 21:14:05, brianderson wrote: > If main_frame_before_activation_enabled is true in SchedulerSettings, it's > possible for this line to run twice before DidActivateSyncTree gets called once. > > main_frame_before_activation_enabled is experimental, so this isn't important to > fix as part of your patch, but can you add a TODO here so we don't accidentally > break your code in the future? > > Alternatively, make impl().last_begin_main_frame_args a queue. What's the expected behavior (if say this is made non experimental) from this? That is, in terms of measuring frame times, this would mean that we have overlapping frames? If so, would we also end up with two activates? https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1364: RecordMainFrameTiming(); On 2015/03/20 21:14:06, brianderson wrote: > Regarding the interval / prediction issue, we should be able to wait for the > next BeginImpFrame's frame_time instead of predicting it. > > We always request one more BeginFrame than we need before going idle to avoid > excess toggling of SetNeedsBeginFrames. The logic that does that is here: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched... > > Can you do something like: > 1) Add a last_begin_main_frame_activated_args and assign it to > last_begin_main_frame_args here. > 2) If last_begin_main_frame_activated_args.IsValid() in WillBeginImplFrame, call > RecordMainFrameTiming there. I think that's an excellent idea. I'm guessing the whole kick-off one more frame before becoming idle idea is sticking around, right? :) Also, is this going to happen in the case of CommitEarlyOutHandledCommit? https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1385: impl().last_begin_main_frame_args, impl().last_begin_impl_frame_args); On 2015/03/20 21:14:06, brianderson wrote: > Although this is slightly wrong right now because begin_main_frame_args uses > Now() for it's frame time, while begin_impl_frame_args uses the original frame > time, I think it's good enough to land. > > Tim's patch should make this 100% correct once it lands and I don't see a reason > to introduce workarounds in this patch, especially since getting this 100% > correct isn't urgent. Acknowledged.
https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:696: impl().last_begin_main_frame_args = begin_main_frame_state->begin_frame_args; On 2015/03/20 22:31:27, vmpstr wrote: > On 2015/03/20 21:14:05, brianderson wrote: > > If main_frame_before_activation_enabled is true in SchedulerSettings, it's > > possible for this line to run twice before DidActivateSyncTree gets called > once. > > > > main_frame_before_activation_enabled is experimental, so this isn't important > to > > fix as part of your patch, but can you add a TODO here so we don't > accidentally > > break your code in the future? > > > > Alternatively, make impl().last_begin_main_frame_args a queue. > > What's the expected behavior (if say this is made non experimental) from this? > That is, in terms of measuring frame times, this would mean that we have > overlapping frames? If so, would we also end up with two activates? Yes, we would have overlapping frames and we would end up with two activates, unless the second commit gets aborted. That second commit can get aborted before the fist commit activates, which is an interesting case... Maybe best to make this a TODO =) https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1364: RecordMainFrameTiming(); On 2015/03/20 22:31:27, vmpstr wrote: > On 2015/03/20 21:14:06, brianderson wrote: > > Regarding the interval / prediction issue, we should be able to wait for the > > next BeginImpFrame's frame_time instead of predicting it. > > > > We always request one more BeginFrame than we need before going idle to avoid > > excess toggling of SetNeedsBeginFrames. The logic that does that is here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched... > > > > Can you do something like: > > 1) Add a last_begin_main_frame_activated_args and assign it to > > last_begin_main_frame_args here. > > 2) If last_begin_main_frame_activated_args.IsValid() in WillBeginImplFrame, > call > > RecordMainFrameTiming there. > > I think that's an excellent idea. I'm guessing the whole kick-off one more frame > before becoming idle idea is sticking around, right? :) There are no plans to get rid of it. It's especially important for requesting BeginFrames from the Browser where we want to hide the latency of the thread hops. We are moving all platforms to that model. > > Also, is this going to happen in the case of CommitEarlyOutHandledCommit? No. Good catch. You'd need to modify SchedulerStateMachine::UpdateStateOnCommit(true) to somehow make ProactiveBeginFrameWanted true, which is something we should probably do anyway.
Hi vmpstr, Looks like Brian has pointed you in the right direction. Pretty much just reiterated the approach Brian suggested (as only read his comments after I had added mine). Hope that helps! Tim https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/914403003/diff/80001/cc/scheduler/scheduler.c... cc/scheduler/scheduler.cc:306: base::TimeTicks Scheduler::NextBeginImplFrameTimeIfRequested() const { On 2015/03/20 18:47:25, vmpstr wrote: > On 2015/03/20 02:17:53, mithro wrote: > > On 2015/03/16 18:36:43, vmpstr wrote: > > > On 2015/03/11 04:01:32, mithro wrote: > > > > Why do you need this method? > > > > > > This returns the next predicted raf time which is one of the values I need > to > > > report. I've changed the name to hopefully better reflect the intent. > > > > Can you link me to why you need this? I was pretty sure the spec didn't > require > > it.. > > > > Using frame_time + interval as the "next frame time" is also fundamentally > > broken when not using vsync throttling. > > The spec I'm working off is here: > http://w3c.github.io/frame-timing/#performancerendertiming > > In particular, > > "The end of the frame is defined as what the beginning time of the next frame's > requestAnimationFrame callback (see [Animation-Timing]) would have been, had one > been scheduled in the current frame. If a requestAnimationFrame is already > requested, its beginning time should be used." > > The last sentence is a recent change that isn't a part of this patch though. > > What time is the appropriate one to use in this situation? That is, if there is > no raf scheduled and this frame is over, we need to know that the next raf time > would have been if it was scheduled. > > My understanding from talking to brianderson@ that you have a patch that would > ensure that the impl frame time is always updated at the right time. Since (also > from my possibly flawed understanding), this is the frame args that would also > be used for the main frame time, it would make a good candidate for predicting > what the next raf would be. Please let me know if this is not the case. Also, if > it isn't the case, what do you think is the right time to use here? Reading the spec, my understanding is that you should be using the actual frame time of the next frame, this means waiting until that frame is generated rather than trying to predict the value. The "had one been scheduled in the current frame" means the scheduler will need to receive one more BeginFrameArgs after SetNeedsBeginFrame has become false to get the ending time - which as brianderson points out we already do. I don't think this information gets down to the LTHI however. https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:285: RecordMainFrameTiming(); On 2015/03/20 18:47:26, vmpstr wrote: > On 2015/03/20 02:17:54, mithro wrote: > > The fact that this method is being called in two places makes me think it is > in > > the wrong place. > > > > Whether this is done in the scheduler or in lthi, I'm not sure where a single > place is that would capture both of these cases: > > 1. We aborted a commit due to no changes in the trees and properties If we aborted the commit, should we be generating a frame time value anyway? We effectively didn't actually generate a main frame or call raf. > 2. We processed the commit, rasterized the sync tree, and activated it. > > Please let me know if you had a better place in mind. I was pondering the NotifyReadyToCommit / BeginMainFrameAborted methods in the scheduler for main frame times and the OnBeginImplFrameDeadline for composite frame times. https://codereview.chromium.org/914403003/diff/120001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/120001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2108: const BeginFrameArgs& current_frame_args) { This is "current_main_frame_args" or "current_impl_frame_args"? https://codereview.chromium.org/914403003/diff/120001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2118: // current frame's frame_time. I really don't think we should be sending an estimate because the frame timing specification is about providing developers with accurate information about what is going on. https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1364: RecordMainFrameTiming(); On 2015/03/20 23:01:58, brianderson wrote: > On 2015/03/20 22:31:27, vmpstr wrote: > > On 2015/03/20 21:14:06, brianderson wrote: > > > Regarding the interval / prediction issue, we should be able to wait for the > > > next BeginImpFrame's frame_time instead of predicting it. > > > > > > We always request one more BeginFrame than we need before going idle to > avoid > > > excess toggling of SetNeedsBeginFrames. The logic that does that is here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched... > > > > > > Can you do something like: > > > 1) Add a last_begin_main_frame_activated_args and assign it to > > > last_begin_main_frame_args here. > > > 2) If last_begin_main_frame_activated_args.IsValid() in WillBeginImplFrame, > > call > > > RecordMainFrameTiming there. > > > > I think that's an excellent idea. I'm guessing the whole kick-off one more > frame > > before becoming idle idea is sticking around, right? :) > > There are no plans to get rid of it. It's especially important for requesting > BeginFrames from the Browser where we want to hide the latency of the thread > hops. We are moving all platforms to that model. > > > > > Also, is this going to happen in the case of CommitEarlyOutHandledCommit? > > No. Good catch. > > You'd need to modify SchedulerStateMachine::UpdateStateOnCommit(true) to somehow > make ProactiveBeginFrameWanted true, which is something we should probably do > anyway. > I agree with Brian here. https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.h File cc/trees/thread_proxy.h (right): https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.h... cc/trees/thread_proxy.h:147: BeginFrameArgs last_begin_impl_frame_args; We now have "animation_time", "last_begin_main_frame_args" and "last_begin_impl_frame_args" here - I'm 100% sure people will end up using the wrong values here. At a minimum put a comment that these values are only for recording frame timing. As Brian mentions I have a patch which should hopefully clean this up a bit, will try and get that in ASAP.
PTAL https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:285: RecordMainFrameTiming(); On 2015/03/23 00:15:08, mithro wrote: > On 2015/03/20 18:47:26, vmpstr wrote: > > On 2015/03/20 02:17:54, mithro wrote: > > > The fact that this method is being called in two places makes me think it is > > in > > > the wrong place. > > > > > > > Whether this is done in the scheduler or in lthi, I'm not sure where a single > > place is that would capture both of these cases: > > > > 1. We aborted a commit due to no changes in the trees and properties > > If we aborted the commit, should we be generating a frame time value anyway? We > effectively didn't actually generate a main frame or call raf. > This happens after apply scroll deltas, animate layers, layout, etc. I think it's just a bit of a misnomer to call this an abort, since we did everything we would normally do short of creating a pending tree and later activating it. That is, we successfully finished the frame and we know that we don't have to do extra work. I'm counting that as a frame that happened... What do you think? https://codereview.chromium.org/914403003/diff/120001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/120001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2108: const BeginFrameArgs& current_frame_args) { On 2015/03/23 00:15:08, mithro wrote: > This is "current_main_frame_args" or "current_impl_frame_args"? That's where I'm a little bit hesitant. It's really impl frame args, but my understanding is that it would become main frame args if there is a main frame? So I don't really wanna call it impl frame, since then it's kind of weird that I'm measuring main.start to impl.start, but I also don't want to call it a main frame arg, since it isn't at this point. Do you have any suggestions? https://codereview.chromium.org/914403003/diff/120001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2118: // current frame's frame_time. On 2015/03/23 00:15:08, mithro wrote: > I really don't think we should be sending an estimate because the frame timing > specification is about providing developers with accurate information about what > is going on. Done. With the latest patch, this is not an estimate. https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:696: impl().last_begin_main_frame_args = begin_main_frame_state->begin_frame_args; On 2015/03/20 23:01:58, brianderson wrote: > On 2015/03/20 22:31:27, vmpstr wrote: > > On 2015/03/20 21:14:05, brianderson wrote: > > > If main_frame_before_activation_enabled is true in SchedulerSettings, it's > > > possible for this line to run twice before DidActivateSyncTree gets called > > once. > > > > > > main_frame_before_activation_enabled is experimental, so this isn't > important > > to > > > fix as part of your patch, but can you add a TODO here so we don't > > accidentally > > > break your code in the future? > > > > > > Alternatively, make impl().last_begin_main_frame_args a queue. > > > > What's the expected behavior (if say this is made non experimental) from this? > > That is, in terms of measuring frame times, this would mean that we have > > overlapping frames? If so, would we also end up with two activates? > > Yes, we would have overlapping frames and we would end up with two activates, > unless the second commit gets aborted. > > That second commit can get aborted before the fist commit activates, which is an > interesting case... Maybe best to make this a TODO =) > Done. https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1364: RecordMainFrameTiming(); On 2015/03/23 00:15:08, mithro wrote: > On 2015/03/20 23:01:58, brianderson wrote: > > On 2015/03/20 22:31:27, vmpstr wrote: > > > On 2015/03/20 21:14:06, brianderson wrote: > > > > Regarding the interval / prediction issue, we should be able to wait for > the > > > > next BeginImpFrame's frame_time instead of predicting it. > > > > > > > > We always request one more BeginFrame than we need before going idle to > > avoid > > > > excess toggling of SetNeedsBeginFrames. The logic that does that is here: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched... > > > > > > > > Can you do something like: > > > > 1) Add a last_begin_main_frame_activated_args and assign it to > > > > last_begin_main_frame_args here. > > > > 2) If last_begin_main_frame_activated_args.IsValid() in > WillBeginImplFrame, > > > call > > > > RecordMainFrameTiming there. > > > > > > I think that's an excellent idea. I'm guessing the whole kick-off one more > > frame > > > before becoming idle idea is sticking around, right? :) > > > > There are no plans to get rid of it. It's especially important for requesting > > BeginFrames from the Browser where we want to hide the latency of the thread > > hops. We are moving all platforms to that model. > > > > > > > > Also, is this going to happen in the case of CommitEarlyOutHandledCommit? > > > > No. Good catch. > > > > You'd need to modify SchedulerStateMachine::UpdateStateOnCommit(true) to > somehow > > make ProactiveBeginFrameWanted true, which is something we should probably do > > anyway. > > > > I agree with Brian here. Done. https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.h File cc/trees/thread_proxy.h (right): https://codereview.chromium.org/914403003/diff/120001/cc/trees/thread_proxy.h... cc/trees/thread_proxy.h:147: BeginFrameArgs last_begin_impl_frame_args; On 2015/03/23 00:15:08, mithro wrote: > We now have "animation_time", "last_begin_main_frame_args" and > "last_begin_impl_frame_args" here - I'm 100% sure people will end up using the > wrong values here. At a minimum put a comment that these values are only for > recording frame timing. > > As Brian mentions I have a patch which should hopefully clean this up a bit, > will try and get that in ASAP. Added a comment. I look forward to your patch landing.
https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:285: RecordMainFrameTiming(); Despite having worked in this area for a while, I'm always feel like I don't really understand what is going on most of the time :/ Taking a step back and looking at the specification; * PerformanceRenderTiming captures the work performed by the renderer to process the frame: DOM modification, JavaScript execution, CSS calculation, and similar activities. * PerformanceCompositeTiming captures events performed by the compositor to place rendered content onto the screen. I think this maps the following; * Create a PerformanceRenderTiming any time we generate new "content" which maps to any work as part of the "BeginMainFrame" to get stuff ready for the impl thread including the commit and activate stages. * Create a PerformanceCompositeTiming any time we are going to put something onto the screen (IE Draw and Swap) which maps to any work as part of the BeginImplFrameDeadline. Which is why I'm pondering this could be cleaner up at the scheduling level. On 2015/03/23 20:26:43, vmpstr wrote: > > This happens after apply scroll deltas, animate layers, layout, etc. To be exact in your case only the CommitEarlyOutReason::FINISHED_NO_UPDATES reason matters (which is the only one CommitEarlyOutHandledCommit returns true on). Looking at that pathway in ThreadProxy::BeginMainFrame there is the following comment; // Although the commit is internally aborted, this is because it has been // detected to be a no-op. From the perspective of an embedder, this commit // went through, and input should no longer be throttled, etc. But you are correct that this happens *after* all the stuff you mentioned. From what I can tell, by "no-op" it actually means "there were no evicted UI resources". The code also does a "BreakSwapPromises" however looking at the scheduler, there doesn't appear to be an explicit guarantee for or against a Draw happening after an aborted commit. > I think it's just a bit of a misnomer to call this an abort, Yeah, there are lots of things here which could be better named. Makes it even more important to get new things named correctly so things trend in the right direction. > since we did everything we > would normally do short of creating a pending tree and later activating it. That > is, we successfully finished the frame and we know that we don't have to do > extra work. I'm counting that as a frame that happened... What do you think? https://codereview.chromium.org/914403003/diff/120001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/120001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2108: const BeginFrameArgs& current_frame_args) { On 2015/03/23 20:26:43, vmpstr wrote: > On 2015/03/23 00:15:08, mithro wrote: > > This is "current_main_frame_args" or "current_impl_frame_args"? > > That's where I'm a little bit hesitant. It's really impl frame args, but my > understanding is that it would become main frame args if there is a main frame? > So I don't really wanna call it impl frame, since then it's kind of weird that > I'm measuring main.start to impl.start, but I also don't want to call it a main > frame arg, since it isn't at this point. > > Do you have any suggestions? I think you need to be *explicitly* clear what is going on here. I think you should be trying to measure the time between two successive start_of_main_frame_args right? I think this confusion here is probably pointing to it being in the wrong place?
I think this patch does the correct thing (modulo naming nits). Also, I understand the desire to get this perfect on the first patch. However, I also think it's important to get something landed so that we can implement and test different types of situations and find any bugs that will inevitably creep up (be it in this code or some other code surrounding this feature). Let me know what I can do to move this review forward. https://codereview.chromium.org/914403003/diff/120001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/914403003/diff/120001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2108: const BeginFrameArgs& current_frame_args) { On 2015/03/24 07:37:42, mithro wrote: > On 2015/03/23 20:26:43, vmpstr wrote: > > On 2015/03/23 00:15:08, mithro wrote: > > > This is "current_main_frame_args" or "current_impl_frame_args"? > > > > That's where I'm a little bit hesitant. It's really impl frame args, but my > > understanding is that it would become main frame args if there is a main > frame? > > So I don't really wanna call it impl frame, since then it's kind of weird that > > I'm measuring main.start to impl.start, but I also don't want to call it a > main > > frame arg, since it isn't at this point. > > > > Do you have any suggestions? > > I think you need to be *explicitly* clear what is going on here. > > I think you should be trying to measure the time between two successive > start_of_main_frame_args right? > In the situation where main frames don't happen one after the other, we still want to capture the end of the frame as the time where the next main frame _would_ begin. That's what these frame args are. That is, they are impl side that are slated to become main frame args _if_ there is a main frame that is going to happen. In situations where main frame does happen, this is the main frame args. In situations where it doesn't, however, this isn't main frame args it's just args that _would_ be main frame. I renamed this to be "expected_next_main_frame_args" and left a comment in the header. I'm not sure how better to be explicit about it while at the same time being correct in that it doesn't have to be main frame args here. > I think this confusion here is probably pointing to it being in the wrong place?
I've ask brainderson to take a close look and see what he thinks.
lgtm https://codereview.chromium.org/914403003/diff/180001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/914403003/diff/180001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1048: for (const auto& request : frame_timing_requests_) To avoid potential multiple resizes: request_ids->insert(request_ids.end(), frame_timing_requests_.begin(), frame_timing_requests_.end()) Or: DCHECK(request_ids.empty()); request_ids.swap(frame_timing_requests_);
https://codereview.chromium.org/914403003/diff/180001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/914403003/diff/180001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1048: for (const auto& request : frame_timing_requests_) On 2015/03/25 19:46:15, brianderson wrote: > To avoid potential multiple resizes: > > request_ids->insert(request_ids.end(), frame_timing_requests_.begin(), > frame_timing_requests_.end()) > > Or: > > DCHECK(request_ids.empty()); > request_ids.swap(frame_timing_requests_); It goes from a vector<FrameTimingRequest> to vector<int64_t>, so I'm not sure that would work here. (It does a request.id() below). I could reserve request_ids to requests_ids.size() + frame_timing_requests_.size()? I think that's a bit premature though.
https://codereview.chromium.org/914403003/diff/180001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/914403003/diff/180001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1048: for (const auto& request : frame_timing_requests_) On 2015/03/26 21:02:22, vmpstr wrote: > On 2015/03/25 19:46:15, brianderson wrote: > > To avoid potential multiple resizes: > > > > request_ids->insert(request_ids.end(), frame_timing_requests_.begin(), > > frame_timing_requests_.end()) > > > > Or: > > > > DCHECK(request_ids.empty()); > > request_ids.swap(frame_timing_requests_); > > It goes from a vector<FrameTimingRequest> to vector<int64_t>, so I'm not sure > that would work here. > (It does a request.id() below). I could reserve request_ids to > requests_ids.size() + frame_timing_requests_.size()? I think that's a bit > premature though. Didn't notice that. Nevermind then.
mithro@, enne@, could you both take another look at this?
On 2015/04/01 17:01:24, vmpstr wrote: > mithro@, enne@, could you both take another look at this? I'm hoping to look at this today, but if it doesn't happen it will be Tuesday next week before I can comment (due to the easter public holidays here in Australia, 4 day weekend, yay!). If landing this patch is blocking further work and both enne@ and brianderson@ are happy with it I'm okay for you to not waiting on my approval. If I come across anything serious in that case I'll just log a bug and assign it to you. Sorry once again about holding up this patch. Tim 'mithro' Ansell
lgtm
On 2015/04/02 02:14:35, mithro wrote: > On 2015/04/01 17:01:24, vmpstr wrote: > > mithro@, enne@, could you both take another look at this? > > I'm hoping to look at this today, but if it doesn't happen it will be Tuesday > next week before I can comment (due to the easter public holidays here in > Australia, 4 day weekend, yay!). Lucky. > > If landing this patch is blocking further work and both enne@ and brianderson@ > are happy with it I'm okay for you to not waiting on my approval. If I come > across anything serious in that case I'll just log a bug and assign it to you. > Ok that sounds good. I don't expect anything critical to come up since this code is not really being fully exercised until some other patches land, but please do look out for problems. > Sorry once again about holding up this patch. No worries :) > > Tim 'mithro' Ansell
The CQ bit was checked by vmpstr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/914403003/#ps200001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914403003/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/d704c875efd1204a4128d66685fc95b204dc353d Cr-Commit-Position: refs/heads/master@{#323806} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
