Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1099703004:
cc: Make SingleThreadProxy::CompositeImmediately match scheduler path. (Closed)
|
Created:
5 years, 8 months ago by mithro-old Modified:
5 years, 7 months ago CC:
cc-bugs_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Make SingleThreadProxy::CompositeImmediately match scheduler path.
Only send the BeginMainFrame inside a BeginImplFrame period.
As this path is used without the scheduler it was missed in
https://codereview.chromium.org/798323003/
BUG=346230
Committed: https://crrev.com/c76d7031e5f07c2c68ec2b76c9d898ed72cab28f
Cr-Commit-Position: refs/heads/master@{#328220}
Patch Set 1 #Patch Set 2 : Only reset in DidBeginImplFrameDeadline method. #Patch Set 3 : Adding DCHECKs around impl frame usage. #Patch Set 4 : [DONT REVIEW] - Upload with deps for try bot running. #
Total comments: 8
Patch Set 5 : Fixing review comments. #Messages
Total messages: 22 (4 generated)
mithro@mithis.com changed reviewers: + brianderson@chromium.org, danakj@chromium.org, enne@chromium.org
Hi, Just a very small patch for an issue I discovered while trying to land the Now() removal from LayerTreeHostImpl. I also logged https://code.google.com/p/chromium/issues/detail?id=480763 about trying to remove this code path. Tim 'mithro' Ansell
On 2015/04/24 04:23:11, mithro wrote: > Hi, > > Just a very small patch for an issue I discovered while trying to land the Now() > removal from LayerTreeHostImpl. I also logged > https://code.google.com/p/chromium/issues/detail?id=480763 about trying to > remove this code path. > > Tim 'mithro' Ansell This patch is blocking the relanding of https://codereview.chromium.org/340743002/ Tim 'mithro' Ansell
Is there something that would fail or assert without this change? Could you make a unit test as a regression test for this assumption?
On 2015/04/24 06:26:28, enne wrote: > Is there something that would fail or assert without this change? Could you make > a unit test as a regression test for this assumption? The patch at https://codereview.chromium.org/1053653008 would be one way to enforce this constraint but I'm not sure it is the best way to do it. I can't see a good way to enforce this constraint with a test. I originally found this issue when trying to track down failures from a bunch of asserts around BeginFrameArgs that I'm trying to land. Tim
Could you add a class that derives from LayerTreeTest that overrides WillBeginImplFrame and Will/DidBeginMainFrame and test that they get called in the right order when calling CompositeImmediately?
I'm thinking of something like https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre....
sunnyps@chromium.org changed reviewers: + sunnyps@chromium.org
Looks good overall. Left a few comments. About testing this: I found that this is required for LayoutTests to work correctly with the new video rendering code (which also requires another CL to work: https://codereview.chromium.org/1113283002/ ). The only reason a lot of things don't break without this CL is that LTHI invents it's own BeginFrameArgs if it didn't get any. https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (left): https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:721: tracked_objects::ScopedTracker tracking_profile6( This is related to the other CL right? https://codereview.chromium.org/1111743002/ https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:607: DebugScopedSetImplThread impl(const_cast<SingleThreadProxy*>(this)); Why do you need a const_cast here? 'this' is not const in a non-const method, right? If this is not needed, can you change the other uses of const_cast in this method too? https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:613: DCHECK(inside_impl_frame_); nit: move this DCHECK to immediately under the WillBeginImplFrame. https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:817: DCHECK(inside_impl_frame_) We're pretty sure this is true for SingleThreadProxy, right?
On 2015/05/01 00:46:38, sunnyps wrote: > Looks good overall. Left a few comments. > > About testing this: I found that this is required for LayoutTests to work > correctly with the new video rendering code (which also requires another CL to > work: https://codereview.chromium.org/1113283002/ ). I'll notify you when this patch lands, but that requires https://codereview.chromium.org/1110183004/ to land first. > > The only reason a lot of things don't break without this CL is that LTHI invents > it's own BeginFrameArgs if it didn't get any. Yes, the behaviour that the LTHI invents it's own BeginFrameArgs is ultimately what I'm trying to remove. Hence how I found this behaviour and created this patch. Tim 'mithro' Ansell https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (left): https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:721: tracked_objects::ScopedTracker tracking_profile6( On 2015/05/01 00:46:38, sunnyps wrote: > This is related to the other CL right? > https://codereview.chromium.org/1111743002/ Yes, this patch is dependent on that CL. I uploaded it to test on the try bots, you should be looking at patchset #3 -> https://codereview.chromium.org/1099703004/#ps40001 Reitveld / Commit Queue doesn't deal with dependent patches :( https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:607: DebugScopedSetImplThread impl(const_cast<SingleThreadProxy*>(this)); On 2015/05/01 00:46:38, sunnyps wrote: > Why do you need a const_cast here? 'this' is not const in a non-const method, > right? If this is not needed, can you change the other uses of const_cast in > this method too? Done. https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:613: DCHECK(inside_impl_frame_); On 2015/05/01 00:46:38, sunnyps wrote: > nit: move this DCHECK to immediately under the WillBeginImplFrame. This DCHCEK is checking that when you call DoBeginMainFrame we are inside an impl frame. This means it should be logically in the same scope as the DoBeginMainFrame (IE if you were to split the stuff out into it's own function then you'd take the DCHECK with it). https://codereview.chromium.org/1099703004/diff/60001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:817: DCHECK(inside_impl_frame_) On 2015/05/01 00:46:38, sunnyps wrote: > We're pretty sure this is true for SingleThreadProxy, right? We'd hope so, but as SRE says "hope is not a strategy" so we check. Then when someone breaks the assumption this will fail before they commit it.
LGTM if you add a test like enne said.
On 2015/05/01 20:28:14, sunnyps wrote: > LGTM if you add a test like enne said. I can add a test for the specific bug addressed here but was unsure if it was useful to do so. I've taken a couple of days to try and come up with a good generic test to check the root issue (the "Only send the BeginMainFrame inside a BeginImplFrame period" part) which lead to the other patches but not a good test here. A number of other CLs do add checks and tests around this area; * https://codereview.chromium.org/1111743002/ - Adds a test around BeginImplFrame periods which should catch the fact that the BeginImplFrame was never called. I need to check that SINGLE_AND_MULTI_THREAD_TEST_F macro covers the CompositeImmediately path. * https://codereview.chromium.org/787763006/ - Adds a huge number of DCHECK and asserts which strongly verify the root issue here (and that patch is how I found the problem here). * https://codereview.chromium.org/1104193003/ - Adds another, very strict, test around the BeginImpl and BeginMain frame time usage which would have also caught the problem specified here. I'm a little worried that adding the specific test here might just make it harder to refactor in the future. Do you think a test which only addresses the CompositeImmediately and BeginImplFrame bits is worth adding? I'm leaning towards the CHECKs and tests from the other CLs being enough here. Sunny / Enne - thoughts? I believe landing this patch is blocking Dale and Sunny's work around YouTube video, so it would be good to figure this out ASAP. Thanks for your review! Tim 'mithro' Ansell
If the concern is a lack of tests rather than having a specific test, the new video rendering pipeline requires this to even pass any layout tests.
I looked at the other changes and I agree that they would probably fail, but I would like a more directed test to test the assumption that these calls happen in the right order. That way when it regresses due to a future refactoring, it will be much more clear about what the issue is. This should just be like a 30-40 line test. We've probably spent more time talking about it than it would be to write. :)
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Definitely agree. In the further interest of time, I've put together here, if it looks sufficient to you, can we go ahead and CQ this patch? https://codereview.chromium.org/1124733003
lgtm
The CQ bit was checked by sunnyps@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1099703004/80001
I've CQ'd this patch. I don't think the test you put together is what I had in mind we'll have a test soon enough. On Mon, May 4, 2015 at 3:28 PM, <enne@chromium.org> wrote: > lgtm > > > > https://codereview.chromium.org/1099703004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c76d7031e5f07c2c68ec2b76c9d898ed72cab28f Cr-Commit-Position: refs/heads/master@{#328220} |
