Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23804004:
[cc] Update UIResource test threading (Closed)
|
Created:
7 years, 3 months ago by ccameron Modified:
7 years, 3 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[cc] Update UIResource test threading
Update UIResource tests to obey the restriction that ScopedUIResource
create and destroy calls may only be made on the main thread.
BUG=279438
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222132
Patch Set 1 #Patch Set 2 : Remove whitespace #
Total comments: 2
Patch Set 3 : Explicitly reference extra commits in context lost #Patch Set 4 : Add missed comment #
Total comments: 4
Patch Set 5 : Incorporate review feedback #Patch Set 6 : Consolidate common code #Messages
Total messages: 12 (0 generated)
This change makes the tests follow the threading rules that the UI resources follow. This turned out to be necessary to keep the tests green (well, without changing them substantially) when I add the eviction code.
https://codereview.chromium.org/23804004/diff/3001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/23804004/diff/3001/cc/trees/layer_tree_host.c... cc/trees/layer_tree_host.cc:1183: std::set<UIResourceId> already_recreated_resources; Hmm, I'm not wholly opposed to having this, but we considered it originally, and figured it would be a somewhat fake protection against duplicate creation calls since they could still be sitting in the pending tree when context is lost. So we kept this function simple and made sure the impl-side could handle duplicate creation. Could you clarify in what way it makes the tests behave more consistently, is it actually a race or just that the creation counts look more reasonable?
Thanks! I've removed that functionality and replaced it with a comment explaining what I was working around (which is not clearly intentional behavior, though it is documented in other places). https://codereview.chromium.org/23804004/diff/3001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/23804004/diff/3001/cc/trees/layer_tree_host.c... cc/trees/layer_tree_host.cc:1183: std::set<UIResourceId> already_recreated_resources; On 2013/09/07 21:34:09, aelias wrote: > Hmm, I'm not wholly opposed to having this, but we considered it originally, and > figured it would be a somewhat fake protection against duplicate creation calls > since they could still be sitting in the pending tree when context is lost. So > we kept this function simple and made sure the impl-side could handle duplicate > creation. Could you clarify in what way it makes the tests behave more > consistently, is it actually a race or just that the creation counts look more > reasonable? The behavior that this works around is a strange thing where SingleThreadProxy will issue two commits after a context lost ... I'm not sure if that is intentional or not, so I filed a bug against the issue (http://crbug.com/287250), and I changed the test expectations to differ on single-versus multi-threaded proxy (explicitly referencing the bug).
On 2013/09/08 01:54:33, ccameron1 wrote: > Thanks! > > I've removed that functionality and replaced it with a comment explaining what I > was working around (which is not clearly intentional behavior, though it is > documented in other places). > > https://codereview.chromium.org/23804004/diff/3001/cc/trees/layer_tree_host.cc > File cc/trees/layer_tree_host.cc (right): > > https://codereview.chromium.org/23804004/diff/3001/cc/trees/layer_tree_host.c... > cc/trees/layer_tree_host.cc:1183: std::set<UIResourceId> > already_recreated_resources; > On 2013/09/07 21:34:09, aelias wrote: > > Hmm, I'm not wholly opposed to having this, but we considered it originally, > and > > figured it would be a somewhat fake protection against duplicate creation > calls > > since they could still be sitting in the pending tree when context is lost. > So > > we kept this function simple and made sure the impl-side could handle > duplicate > > creation. Could you clarify in what way it makes the tests behave more > > consistently, is it actually a race or just that the creation counts look more > > reasonable? > > The behavior that this works around is a strange thing where SingleThreadProxy > will issue two commits after a context lost ... I'm not sure if that is > intentional or not, so I filed a bug against the issue > (http://crbug.com/287250), and I changed the test expectations to differ on > single-versus multi-threaded proxy (explicitly referencing the bug). Looks good. Just some nits from me. You're right, UI resource should be created only on the main thread. A little clarification for me though: the extra commmit from single-thread proxy means that recreation is skipped, but the original creation request still goes through, correct? And do you know why this wasn't observed in the original test? And thanks very much for cleaning this up.
https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:8: #include <set> probably not necessary anymore? https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest_context.cc:1650: // the the call to StepCompleteOnMainThread will not occur until after nit: double the
Thanks!! > A little clarification for me though: the > extra commmit from single-thread proxy means that recreation is skipped, but the > original creation request still goes through, correct? Actually, I think the recreation does happen, but happens later on in the test -- it just so happens that the commit 1 comes in before the output surface is recreated -- so the code is fairly brittle. It's a really strange situation -- we know that the output surface is gone but do a commit to it anyway. I'm going to clean most of this up the bug that I filed on the issue (but I'll need to discuss if the current behavior is intentional there first). Fortunately, fixing that bug gets easier when this change is in. > And do you know why this > wasn't observed in the original test? IIUC this is because we happened to be calling LoseContext during the commit flow. https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:8: #include <set> On 2013/09/08 04:49:13, powei wrote: > probably not necessary anymore? Oops! Done. https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest_context.cc:1650: // the the call to StepCompleteOnMainThread will not occur until after On 2013/09/08 04:49:13, powei wrote: > nit: double the Done.
On 2013/09/08 21:08:24, ccameron1 wrote: > Thanks!! > > > A little clarification for me though: the > > extra commmit from single-thread proxy means that recreation is skipped, but > the > > original creation request still goes through, correct? > > Actually, I think the recreation does happen, but happens later on in the test > -- it just so happens that the commit 1 comes in before the output surface is > recreated -- so the code is fairly brittle. > > It's a really strange situation -- we know that the output surface is gone but > do a commit to it anyway. > > I'm going to clean most of this up the bug that I filed on the issue (but I'll > need to discuss if the current behavior is intentional there first). > Fortunately, fixing that bug gets easier when this change is in. > > > And do you know why this > > wasn't observed in the original test? > > IIUC this is because we happened to be calling LoseContext during the commit > flow. > > https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host.cc > File cc/trees/layer_tree_host.cc (right): > > https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host.... > cc/trees/layer_tree_host.cc:8: #include <set> > On 2013/09/08 04:49:13, powei wrote: > > probably not necessary anymore? > > Oops! Done. > > https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host_... > File cc/trees/layer_tree_host_unittest_context.cc (right): > > https://codereview.chromium.org/23804004/diff/10001/cc/trees/layer_tree_host_... > cc/trees/layer_tree_host_unittest_context.cc:1650: // the the call to > StepCompleteOnMainThread will not occur until after > On 2013/09/08 04:49:13, powei wrote: > > nit: double the > > Done. lgtm from me. You might want to remove the second paragraph in the description about no redundant entries since you removed that part. It still seems a little strange that the single thread checks for UIResourceLostBeforeCommit is saying that no resource recreation occurred, when what we really want to check is that recreation does occur, but I understand that might have to wait until (http://crbug.com/287250) gets cleared up. Thanks for the hard work.
Thanks!
lgtm
Thanks!!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/23804004/20001
Message was sent while issue was closed.
Change committed as 222132 |
