Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1418953002:
cc: Send shared variables between main and impl side using the channel (Closed)
Descriptioncc: Send shared variables between main and impl side using the channel
ProxyImpl uses the LayerTreeHost for 2 blocking operations:
initialization and commit. Pass this through the channel to make
the main state seen by the impl side for an operation explicit.
Also route post tasks for initialization and shutdown through the channel.
This is a follow-up patch to: https://codereview.chromium.org/1377063003/
BUG=527200
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/64d1e686b0eb5e60a2d2e8f7c3f86c57698249bb
Cr-Commit-Position: refs/heads/master@{#357759}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebase. #
Total comments: 16
Patch Set 3 : Addressing comments. #
Total comments: 8
Patch Set 4 : Addressing comments. #
Total comments: 2
Patch Set 5 : Rebase. #
Messages
Total messages: 33 (10 generated)
Description was changed from ========== cc: Send shared variables between main and impl side using the channel ProxyImpl uses the LayerTreeHost for 2 blocking operations: initialization and commit. Pass this through the channel to make the main state seen by the impl side for an operation explicit. BUG= 527200 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Send shared variables between main and impl side using the channel ProxyImpl uses the LayerTreeHost for 2 blocking operations: initialization and commit. Pass this through the channel to make the main state seen by the impl side for an operation explicit. BUG= 527200 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + danakj@chromium.org, dtrainor@chromium.org, vmpstr@chromium.org
Description was changed from ========== cc: Send shared variables between main and impl side using the channel ProxyImpl uses the LayerTreeHost for 2 blocking operations: initialization and commit. Pass this through the channel to make the main state seen by the impl side for an operation explicit. BUG= 527200 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Send shared variables between main and impl side using the channel ProxyImpl uses the LayerTreeHost for 2 blocking operations: initialization and commit. Pass this through the channel to make the main state seen by the impl side for an operation explicit. This is a follow-up patch to: https://codereview.chromium.org/1377063003/ BUG= 527200 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
https://codereview.chromium.org/1418953002/diff/1/cc/trees/channel_main.h File cc/trees/channel_main.h (right): https://codereview.chromium.org/1418953002/diff/1/cc/trees/channel_main.h#new... cc/trees/channel_main.h:47: virtual void StartCommitOnImpl(CompletionEvent* completion, The aim here is to make it explicit to the user of the channel as to which operations use the LayerTreeHost on the impl side, so for a remote channel, it can be serialized at that point. Alternatively we could add methods to the ChannelImpl, CreateLayerTreeHostImpl(LayerTreeHostImplClient client); FinishCommitToLayerTreeHostImpl(LayerTreeHostImpl host_impl); and the implementation uses the LayerTreeHost to do the operations. Would that be better?
On 2015/10/23 22:42:29, Khushal wrote: > https://codereview.chromium.org/1418953002/diff/1/cc/trees/channel_main.h > File cc/trees/channel_main.h (right): > > https://codereview.chromium.org/1418953002/diff/1/cc/trees/channel_main.h#new... > cc/trees/channel_main.h:47: virtual void StartCommitOnImpl(CompletionEvent* > completion, > The aim here is to make it explicit to the user of the channel as to which > operations use the LayerTreeHost on the impl side, so for a remote channel, it > can be serialized at that point. > Alternatively we could add methods to the ChannelImpl, > > CreateLayerTreeHostImpl(LayerTreeHostImplClient client); > FinishCommitToLayerTreeHostImpl(LayerTreeHostImpl host_impl); > > and the implementation uses the LayerTreeHost to do the operations. Would that > be better? friendly ping. :)
vmpstr@chromium.org changed reviewers: + brianderson@chromium.org
+brianderson as well. https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (left): https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:805: impl().completion_event_for_commit_held_on_tree_activation = This is the part that ensured that we had both completion events if we had another request... although I'm still not clear when that can happen. https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:490: LayerTreeHost* ThreadProxy::layer_tree_host() { I think we maybe should remove these functions and call main().layer_tree_host explicitly to make it clear that it can't be accessed on the impl thread. https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:508: DCHECK(IsMainThreadBlocked() && impl().commit_completion_event); nit: Split into two dchecks, so we know which one fails. https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:735: if (hold_commit_for_activation) { Can we get multiple StartCommitOnImpl calls while waiting on the same activation? That is, can we have impl().next_commit_waits_for_activation be true here and hold_commit_for_activation be false? What situations are those? https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:752: impl().commit_completion_event = completion; DCHECK that this is currently null? It should be right? Or maybe it can be the previously held commit completion. I'm not sure overwriting it is a good idea since then we leak a completion https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:793: // Remove the LayerTreeHost reference before the completion event is signaled. Why is this important? That is, I understand that we should clear it but the comment reads that it must happen before the signal
https://codereview.chromium.org/1418953002/diff/20001/cc/trees/proxy_impl.h File cc/trees/proxy_impl.h (right): https://codereview.chromium.org/1418953002/diff/20001/cc/trees/proxy_impl.h#n... cc/trees/proxy_impl.h:51: virtual void LayerTreeHostClosedOnImpl(CompletionEvent* completion) = 0; Should bug summary mention anything about this change? https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (left): https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:805: impl().completion_event_for_commit_held_on_tree_activation = On 2015/10/29 21:36:33, vmpstr wrote: > This is the part that ensured that we had both completion events if we had > another request... although I'm still not clear when that can happen. This looks okay to me. The main thread is blocked and can't add another commit_completion_event. If it ever does happen, there's a DCHECK at the start of ThreadProxy::StartCommitOnImpl that we'll hit. https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:707: bool hold_commit_for_activation = main().commit_waits_for_activation; I think StartCommitOnImpl will not modify main().commit_waits_for_activation, so this temp variable isn't necessary and you can reset main().commit_waits_for_activation at the end of this block.
Description was changed from ========== cc: Send shared variables between main and impl side using the channel ProxyImpl uses the LayerTreeHost for 2 blocking operations: initialization and commit. Pass this through the channel to make the main state seen by the impl side for an operation explicit. This is a follow-up patch to: https://codereview.chromium.org/1377063003/ BUG= 527200 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Send shared variables between main and impl side using the channel ProxyImpl uses the LayerTreeHost for 2 blocking operations: initialization and commit. Pass this through the channel to make the main state seen by the impl side for an operation explicit. Also route post tasks for initialization and shutdown through the channel. This is a follow-up patch to: https://codereview.chromium.org/1377063003/ BUG= 527200 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
https://codereview.chromium.org/1418953002/diff/20001/cc/trees/proxy_impl.h File cc/trees/proxy_impl.h (right): https://codereview.chromium.org/1418953002/diff/20001/cc/trees/proxy_impl.h#n... cc/trees/proxy_impl.h:51: virtual void LayerTreeHostClosedOnImpl(CompletionEvent* completion) = 0; On 2015/10/29 22:38:28, brianderson wrote: > Should bug summary mention anything about this change? Done. https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:490: LayerTreeHost* ThreadProxy::layer_tree_host() { On 2015/10/29 21:36:33, vmpstr wrote: > I think we maybe should remove these functions and call main().layer_tree_host > explicitly to make it clear that it can't be accessed on the impl thread. Done. https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:508: DCHECK(IsMainThreadBlocked() && impl().commit_completion_event); On 2015/10/29 21:36:33, vmpstr wrote: > nit: Split into two dchecks, so we know which one fails. Done. https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:707: bool hold_commit_for_activation = main().commit_waits_for_activation; On 2015/10/29 22:38:28, brianderson wrote: > I think StartCommitOnImpl will not modify main().commit_waits_for_activation, so > this temp variable isn't necessary and you can reset > main().commit_waits_for_activation at the end of this block. Done. https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:735: if (hold_commit_for_activation) { On 2015/10/29 21:36:33, vmpstr wrote: > Can we get multiple StartCommitOnImpl calls while waiting on the same > activation? That is, can we have > > impl().next_commit_waits_for_activation be true here and > hold_commit_for_activation be false? > > What situations are those? Once the main side sends the hold_commit_for_activation flag to impl, it will reset it in main(). So there can be a scenario where the impl().next_commit_waits_for_activation will get set to true but we early out below. In this case the next time the start commit call is received, impl().next_commit_waits_for_activation will be true here but hold_commit_for_activation will be false since it was cleared in the last call. https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:752: impl().commit_completion_event = completion; On 2015/10/29 21:36:33, vmpstr wrote: > DCHECK that this is currently null? It should be right? Or maybe it can be the > previously held commit completion. I'm not sure overwriting it is a good idea > since then we leak a completion Its already DCHECKed at the beginning of the function. It should be null since if it isn't this means that the main thread is blocked right now and we shouldn't be receiving this call at all. https://codereview.chromium.org/1418953002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:793: // Remove the LayerTreeHost reference before the completion event is signaled. On 2015/10/29 21:36:33, vmpstr wrote: > Why is this important? That is, I understand that we should clear it but the > comment reads that it must happen before the signal The moment we signal the event, it is cleared as well. And the blocked_main_commit() accessor DCHECKs the impl().commit_completion_event to ensure that the LayerTreeHost is accessed only while we are blocked for a commit. Trying to clear it after clearing the event will fail that check.
https://chromiumcodereview.appspot.com/1418953002/diff/40001/cc/trees/layer_t... File cc/trees/layer_tree_host_unittest_proxy.cc (right): https://chromiumcodereview.appspot.com/1418953002/diff/40001/cc/trees/layer_t... cc/trees/layer_tree_host_unittest_proxy.cc:328: commits_completed_++; Pull this out of the switch? Otherwise if you somehow get more than 3 calls you won't catch it?
https://chromiumcodereview.appspot.com/1418953002/diff/40001/cc/trees/layer_t... File cc/trees/layer_tree_host_unittest_proxy.cc (right): https://chromiumcodereview.appspot.com/1418953002/diff/40001/cc/trees/layer_t... cc/trees/layer_tree_host_unittest_proxy.cc:328: commits_completed_++; On 2015/11/02 16:19:44, David Trainor wrote: > Pull this out of the switch? Otherwise if you somehow get more than 3 calls you > won't catch it? The AfterTest should catch that right? It makes sure that the test gets exactly 3 commits.
friendly ping! :)
lgtm, if brianderson is ok with it. https://codereview.chromium.org/1418953002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_proxy.cc (right): https://codereview.chromium.org/1418953002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_proxy.cc:302: EXPECT_EQ(nullptr, ThreadProxyImplOnly().commit_completion_event); nit: We usually use EXPECT_TRUE/EXPECT_FALSE for nullptr checks. https://codereview.chromium.org/1418953002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_proxy.cc:307: EXPECT_NE(nullptr, ThreadProxyImplOnly().commit_completion_event); Here as well. https://codereview.chromium.org/1418953002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_proxy.cc:312: EXPECT_EQ(nullptr, ThreadProxyImplOnly().commit_completion_event); Here as well.
Thanks Vlad. brianderson@, could you please take a look? https://codereview.chromium.org/1418953002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_proxy.cc (right): https://codereview.chromium.org/1418953002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_proxy.cc:302: EXPECT_EQ(nullptr, ThreadProxyImplOnly().commit_completion_event); On 2015/11/03 19:50:55, vmpstr wrote: > nit: We usually use EXPECT_TRUE/EXPECT_FALSE for nullptr checks. Done. https://codereview.chromium.org/1418953002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_proxy.cc:307: EXPECT_NE(nullptr, ThreadProxyImplOnly().commit_completion_event); On 2015/11/03 19:50:55, vmpstr wrote: > Here as well. Done. https://codereview.chromium.org/1418953002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_proxy.cc:312: EXPECT_EQ(nullptr, ThreadProxyImplOnly().commit_completion_event); On 2015/11/03 19:50:55, vmpstr wrote: > Here as well. Done.
1 comment, then lgtm. https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:955: impl().layer_tree_host_impl = layer_tree_host->CreateLayerTreeHostImpl(this); Can this be created on the main thread, then passed to the impl? That way we have one less place where we access LayerTreeHost on the Impl thread. I think that would leave only the commit where we access the LayerTreeHost on the impl thread. Theoretically, we could eventually commit on the main thread (https://code.google.com/p/chromium/issues/detail?id=354969) at which point we would have removed all LTH accesses from the impl thread.
https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:955: impl().layer_tree_host_impl = layer_tree_host->CreateLayerTreeHostImpl(this); On 2015/11/03 22:14:44, brianderson wrote: > Can this be created on the main thread, then passed to the impl? That way we > have one less place where we access LayerTreeHost on the Impl thread. > > I think that would leave only the commit where we access the LayerTreeHost on > the impl thread. Theoretically, we could eventually commit on the main thread > (https://code.google.com/p/chromium/issues/detail?id=354969) at which point we > would have removed all LTH accesses from the impl thread. The thread boundary separation for other channel implementations imply a network/IPC layer and any data being passed through the channel being serialized. The current plan was to align the operations that use the LTH on the impl thread with the existing implementation. Serialize the complete LTH state during a commit and send it down to the client, where it is deserialized into a dummy object that the impl thread uses for these operations. IIUC, you're suggesting that we shift the split boundary in terms of data passed a little to the impl side, so the channel sends the LTHI to the main thread rather than sending the LTH to the impl thread? So for initialization, build the LTHI on the main thread and then pass it to the impl thread, which for a network channel would mean serializing the LTHI at this point to send to the client. I would be fine with that. We probably need to serialize only a few LTHI properties needed to build it on the client. +vmpstr, +danakj, what do you think? How would the commit function in this case? Would we pass the pending tree to the main thread here with the BeginMainFrame call to commit into? I assumed that the impl thread would be reading from this tree while the main thread tries to commit to it. For the case of a network channel, this can easily work. Even right now, once we serialize the LTH for a commit, the main thread is unblocked since the data the client will commit from has already been copied. After this the server main thread can mutate the tree all it wants. If we have an implementation where the commit happens on the main thread into the pending tree, then we'd probably pass a dummy LTHI to the main thread to commit to on the server, serialize it and deserialize that into the pending tree on the client.
On 2015/11/03 23:07:52, Khushal wrote: > https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.cc > File cc/trees/thread_proxy.cc (right): > > https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.c... > cc/trees/thread_proxy.cc:955: impl().layer_tree_host_impl = > layer_tree_host->CreateLayerTreeHostImpl(this); > On 2015/11/03 22:14:44, brianderson wrote: > > Can this be created on the main thread, then passed to the impl? That way we > > have one less place where we access LayerTreeHost on the Impl thread. > > > > I think that would leave only the commit where we access the LayerTreeHost on > > the impl thread. Theoretically, we could eventually commit on the main thread > > (https://code.google.com/p/chromium/issues/detail?id=354969) at which point we > > would have removed all LTH accesses from the impl thread. > > The thread boundary separation for other channel implementations imply a > network/IPC layer and any data being passed through the channel being > serialized. > > The current plan was to align the operations that use the LTH on the impl thread > with the existing implementation. Serialize the complete LTH state during a > commit and send it down to the client, where it is deserialized into a dummy > object that the impl thread uses for these operations. > > IIUC, you're suggesting that we shift the split boundary in terms of data passed > a little to the impl side, so the channel sends the LTHI to the main thread Woops, sorry. I meant the channel sends the LTHI to the impl thread. > rather than sending the LTH to the impl thread? > > So for initialization, build the LTHI on the main thread and then pass it to the > impl thread, which for a network channel would mean serializing the LTHI at this > point to send to the client. I would be fine with that. We probably need to > serialize only a few LTHI properties needed to build it on the client. +vmpstr, > +danakj, what do you think? > > How would the commit function in this case? Would we pass the pending tree to > the main thread here with the BeginMainFrame call to commit into? I assumed that > the impl thread would be reading from this tree while the main thread tries to > commit to it. > > For the case of a network channel, this can easily work. Even right now, once we > serialize the LTH for a commit, the main thread is unblocked since the data the > client will commit from has already been copied. After this the server main > thread can mutate the tree all it wants. If we have an implementation where the > commit happens on the main thread into the pending tree, then we'd probably pass > a dummy LTHI to the main thread to commit to on the server, serialize it and > deserialize that into the pending tree on the client.
On 2015/11/04 00:42:09, Khushal wrote: > On 2015/11/03 23:07:52, Khushal wrote: > > https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.cc > > File cc/trees/thread_proxy.cc (right): > > > > > https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.c... > > cc/trees/thread_proxy.cc:955: impl().layer_tree_host_impl = > > layer_tree_host->CreateLayerTreeHostImpl(this); > > On 2015/11/03 22:14:44, brianderson wrote: > > > Can this be created on the main thread, then passed to the impl? That way we > > > have one less place where we access LayerTreeHost on the Impl thread. > > > > > > I think that would leave only the commit where we access the LayerTreeHost > on > > > the impl thread. Theoretically, we could eventually commit on the main > thread > > > (https://code.google.com/p/chromium/issues/detail?id=354969) at which point > we > > > would have removed all LTH accesses from the impl thread. > > > > The thread boundary separation for other channel implementations imply a > > network/IPC layer and any data being passed through the channel being > > serialized. > > > > The current plan was to align the operations that use the LTH on the impl > thread > > with the existing implementation. Serialize the complete LTH state during a > > commit and send it down to the client, where it is deserialized into a dummy > > object that the impl thread uses for these operations. > > > > IIUC, you're suggesting that we shift the split boundary in terms of data > passed > > a little to the impl side, so the channel sends the LTHI to the main thread > > Woops, sorry. I meant the channel sends the LTHI to the impl thread. Yes. > > > rather than sending the LTH to the impl thread? > > > > So for initialization, build the LTHI on the main thread and then pass it to > the > > impl thread, which for a network channel would mean serializing the LTHI at > this > > point to send to the client. I would be fine with that. We probably need to > > serialize only a few LTHI properties needed to build it on the client. > +vmpstr, > > +danakj, what do you think? > > > > How would the commit function in this case? Would we pass the pending tree to > > the main thread here with the BeginMainFrame call to commit into? I assumed > that > > the impl thread would be reading from this tree while the main thread tries to > > commit to it. I don't want the main-thread commit stuff to block this patch at all - it may never become a thing. But for discussion, it depends: Currently, we never send a BeginMainFrame until we've activated the pending tree and the main thread could effectively pass the pending tree to the main thread as an argument to the BeginMainFrame. There's probably a lot more to it than that though, especially regarding any shared state between the pending/active trees like scroll offset. There's an experimental mode that does send BeginMainFrames before activation, but it's not a mode we've decided to pursue strongly yet (search for "main_frame_before_activation" or see https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...). In that case, we'd need to either add a 3rd tree before the pending tree, or do what the commit already does today which is block the main thread until the pending tree is free (https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...). > > > > For the case of a network channel, this can easily work. Even right now, once > we > > serialize the LTH for a commit, the main thread is unblocked since the data > the > > client will commit from has already been copied. After this the server main > > thread can mutate the tree all it wants. If we have an implementation where > the > > commit happens on the main thread into the pending tree, then we'd probably > pass > > a dummy LTHI to the main thread to commit to on the server, serialize it and > > deserialize that into the pending tree on the client.
On 2015/11/04 01:09:32, brianderson wrote: > On 2015/11/04 00:42:09, Khushal wrote: > > On 2015/11/03 23:07:52, Khushal wrote: > > > > https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.cc > > > File cc/trees/thread_proxy.cc (right): > > > > > > > > > https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.c... > > > cc/trees/thread_proxy.cc:955: impl().layer_tree_host_impl = > > > layer_tree_host->CreateLayerTreeHostImpl(this); > > > On 2015/11/03 22:14:44, brianderson wrote: > > > > Can this be created on the main thread, then passed to the impl? That way > we > > > > have one less place where we access LayerTreeHost on the Impl thread. > > > > > > > > I think that would leave only the commit where we access the LayerTreeHost > > on > > > > the impl thread. Theoretically, we could eventually commit on the main > > thread > > > > (https://code.google.com/p/chromium/issues/detail?id=354969) at which > point > > we > > > > would have removed all LTH accesses from the impl thread. > > > > > > The thread boundary separation for other channel implementations imply a > > > network/IPC layer and any data being passed through the channel being > > > serialized. > > > > > > The current plan was to align the operations that use the LTH on the impl > > thread > > > with the existing implementation. Serialize the complete LTH state during a > > > commit and send it down to the client, where it is deserialized into a dummy > > > object that the impl thread uses for these operations. > > > > > > IIUC, you're suggesting that we shift the split boundary in terms of data > > passed > > > a little to the impl side, so the channel sends the LTHI to the main thread > > > > Woops, sorry. I meant the channel sends the LTHI to the impl thread. > > Yes. > > > > > > rather than sending the LTH to the impl thread? > > > > > > So for initialization, build the LTHI on the main thread and then pass it to > > the > > > impl thread, which for a network channel would mean serializing the LTHI at > > this > > > point to send to the client. I would be fine with that. We probably need to > > > serialize only a few LTHI properties needed to build it on the client. > > +vmpstr, > > > +danakj, what do you think? > > > > > > How would the commit function in this case? Would we pass the pending tree > to > > > the main thread here with the BeginMainFrame call to commit into? I assumed > > that > > > the impl thread would be reading from this tree while the main thread tries > to > > > commit to it. > > I don't want the main-thread commit stuff to block this patch at all - it may > never become a thing. But for discussion, it depends: > > Currently, we never send a BeginMainFrame until we've activated the pending tree > and the main thread could effectively pass the pending tree to the main thread > as an argument to the BeginMainFrame. There's probably a lot more to it than > that though, especially regarding any shared state between the pending/active > trees like scroll offset. > > There's an experimental mode that does send BeginMainFrames before activation, > but it's not a mode we've decided to pursue strongly yet (search for > "main_frame_before_activation" or see > https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...). > In that case, we'd need to either add a 3rd tree before the pending tree, or do > what the commit already does today which is block the main thread until the > pending tree is free > (https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...). > Thanks Brian. I think if we ever do want to consider this approach, we can easily change the channel for it. For creating the LTHI on the main thread, there is currently a DCHECK in the LTHI constructor to ensure its created on the impl thread. So I'm not sure if its safe to do create it on the main thread. > > > > > > > For the case of a network channel, this can easily work. Even right now, > once > > we > > > serialize the LTH for a commit, the main thread is unblocked since the data > > the > > > client will commit from has already been copied. After this the server main > > > thread can mutate the tree all it wants. If we have an implementation where > > the > > > commit happens on the main thread into the pending tree, then we'd probably > > pass > > > a dummy LTHI to the main thread to commit to on the server, serialize it and > > > deserialize that into the pending tree on the client.
On 2015/11/04 01:44:58, Khushal wrote: > On 2015/11/04 01:09:32, brianderson wrote: > > On 2015/11/04 00:42:09, Khushal wrote: > > > On 2015/11/03 23:07:52, Khushal wrote: > > > > > > https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.cc > > > > File cc/trees/thread_proxy.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.c... > > > > cc/trees/thread_proxy.cc:955: impl().layer_tree_host_impl = > > > > layer_tree_host->CreateLayerTreeHostImpl(this); > > > > On 2015/11/03 22:14:44, brianderson wrote: > > > > > Can this be created on the main thread, then passed to the impl? That > way > > we > > > > > have one less place where we access LayerTreeHost on the Impl thread. > > > > > > > > > > I think that would leave only the commit where we access the > LayerTreeHost > > > on > > > > > the impl thread. Theoretically, we could eventually commit on the main > > > thread > > > > > (https://code.google.com/p/chromium/issues/detail?id=354969) at which > > point > > > we > > > > > would have removed all LTH accesses from the impl thread. > > > > > > > > The thread boundary separation for other channel implementations imply a > > > > network/IPC layer and any data being passed through the channel being > > > > serialized. > > > > > > > > The current plan was to align the operations that use the LTH on the impl > > > thread > > > > with the existing implementation. Serialize the complete LTH state during > a > > > > commit and send it down to the client, where it is deserialized into a > dummy > > > > object that the impl thread uses for these operations. > > > > > > > > IIUC, you're suggesting that we shift the split boundary in terms of data > > > passed > > > > a little to the impl side, so the channel sends the LTHI to the main > thread > > > > > > Woops, sorry. I meant the channel sends the LTHI to the impl thread. > > > > Yes. > > > > > > > > > rather than sending the LTH to the impl thread? > > > > > > > > So for initialization, build the LTHI on the main thread and then pass it > to > > > the > > > > impl thread, which for a network channel would mean serializing the LTHI > at > > > this > > > > point to send to the client. I would be fine with that. We probably need > to > > > > serialize only a few LTHI properties needed to build it on the client. > > > +vmpstr, > > > > +danakj, what do you think? > > > > > > > > How would the commit function in this case? Would we pass the pending tree > > to > > > > the main thread here with the BeginMainFrame call to commit into? I > assumed > > > that > > > > the impl thread would be reading from this tree while the main thread > tries > > to > > > > commit to it. > > > > I don't want the main-thread commit stuff to block this patch at all - it may > > never become a thing. But for discussion, it depends: > > > > Currently, we never send a BeginMainFrame until we've activated the pending > tree > > and the main thread could effectively pass the pending tree to the main thread > > as an argument to the BeginMainFrame. There's probably a lot more to it than > > that though, especially regarding any shared state between the pending/active > > trees like scroll offset. > > > > There's an experimental mode that does send BeginMainFrames before activation, > > but it's not a mode we've decided to pursue strongly yet (search for > > "main_frame_before_activation" or see > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...). > > In that case, we'd need to either add a 3rd tree before the pending tree, or > do > > what the commit already does today which is block the main thread until the > > pending tree is free > > > (https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...). > > > > Thanks Brian. I think if we ever do want to consider this approach, we can > easily change the channel for it. > > For creating the LTHI on the main thread, there is currently a DCHECK in the > LTHI constructor to ensure its created on the impl thread. So I'm not sure if > its safe to do create it on the main thread. Okay. lgtm, as is. Let's not worry about this for now - it's not super important for any real functionality and would probably be better to do as a separate patch anyway. In my mind, there is no LTHI and therefore nothing running on the impl thread to be worried about being in conflict with when creating the LTHI (unless creation of the LTHI posts tasks itself - but I would not expect that). > > > > > > > > > > > For the case of a network channel, this can easily work. Even right now, > > once > > > we > > > > serialize the LTH for a commit, the main thread is unblocked since the > data > > > the > > > > client will commit from has already been copied. After this the server > main > > > > thread can mutate the tree all it wants. If we have an implementation > where > > > the > > > > commit happens on the main thread into the pending tree, then we'd > probably > > > pass > > > > a dummy LTHI to the main thread to commit to on the server, serialize it > and > > > > deserialize that into the pending tree on the client.
On 2015/11/04 02:36:30, brianderson wrote: > On 2015/11/04 01:44:58, Khushal wrote: > > On 2015/11/04 01:09:32, brianderson wrote: > > > On 2015/11/04 00:42:09, Khushal wrote: > > > > On 2015/11/03 23:07:52, Khushal wrote: > > > > > > > > > https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.cc > > > > > File cc/trees/thread_proxy.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1418953002/diff/60001/cc/trees/thread_proxy.c... > > > > > cc/trees/thread_proxy.cc:955: impl().layer_tree_host_impl = > > > > > layer_tree_host->CreateLayerTreeHostImpl(this); > > > > > On 2015/11/03 22:14:44, brianderson wrote: > > > > > > Can this be created on the main thread, then passed to the impl? That > > way > > > we > > > > > > have one less place where we access LayerTreeHost on the Impl thread. > > > > > > > > > > > > I think that would leave only the commit where we access the > > LayerTreeHost > > > > on > > > > > > the impl thread. Theoretically, we could eventually commit on the main > > > > thread > > > > > > (https://code.google.com/p/chromium/issues/detail?id=354969) at which > > > point > > > > we > > > > > > would have removed all LTH accesses from the impl thread. > > > > > > > > > > The thread boundary separation for other channel implementations imply a > > > > > network/IPC layer and any data being passed through the channel being > > > > > serialized. > > > > > > > > > > The current plan was to align the operations that use the LTH on the > impl > > > > thread > > > > > with the existing implementation. Serialize the complete LTH state > during > > a > > > > > commit and send it down to the client, where it is deserialized into a > > dummy > > > > > object that the impl thread uses for these operations. > > > > > > > > > > IIUC, you're suggesting that we shift the split boundary in terms of > data > > > > passed > > > > > a little to the impl side, so the channel sends the LTHI to the main > > thread > > > > > > > > Woops, sorry. I meant the channel sends the LTHI to the impl thread. > > > > > > Yes. > > > > > > > > > > > > rather than sending the LTH to the impl thread? > > > > > > > > > > So for initialization, build the LTHI on the main thread and then pass > it > > to > > > > the > > > > > impl thread, which for a network channel would mean serializing the LTHI > > at > > > > this > > > > > point to send to the client. I would be fine with that. We probably need > > to > > > > > serialize only a few LTHI properties needed to build it on the client. > > > > +vmpstr, > > > > > +danakj, what do you think? > > > > > > > > > > How would the commit function in this case? Would we pass the pending > tree > > > to > > > > > the main thread here with the BeginMainFrame call to commit into? I > > assumed > > > > that > > > > > the impl thread would be reading from this tree while the main thread > > tries > > > to > > > > > commit to it. > > > > > > I don't want the main-thread commit stuff to block this patch at all - it > may > > > never become a thing. But for discussion, it depends: > > > > > > Currently, we never send a BeginMainFrame until we've activated the pending > > tree > > > and the main thread could effectively pass the pending tree to the main > thread > > > as an argument to the BeginMainFrame. There's probably a lot more to it than > > > that though, especially regarding any shared state between the > pending/active > > > trees like scroll offset. > > > > > > There's an experimental mode that does send BeginMainFrames before > activation, > > > but it's not a mode we've decided to pursue strongly yet (search for > > > "main_frame_before_activation" or see > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...). > > > In that case, we'd need to either add a 3rd tree before the pending tree, or > > do > > > what the commit already does today which is block the main thread until the > > > pending tree is free > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...). > > > > > > > Thanks Brian. I think if we ever do want to consider this approach, we can > > easily change the channel for it. > > > > For creating the LTHI on the main thread, there is currently a DCHECK in the > > LTHI constructor to ensure its created on the impl thread. So I'm not sure if > > its safe to do create it on the main thread. > > Okay. lgtm, as is. > > Let's not worry about this for now - it's not super important for any real > functionality and would probably be better to do as a separate patch anyway. In > my mind, there is no LTHI and therefore nothing running on the impl thread to be > worried about being in conflict with when creating the LTHI (unless creation of > the LTHI posts tasks itself - but I would not expect that). > Thanks Brian. :) > > > > > > > > > > > > > > > > > For the case of a network channel, this can easily work. Even right now, > > > once > > > > we > > > > > serialize the LTH for a commit, the main thread is unblocked since the > > data > > > > the > > > > > client will commit from has already been copied. After this the server > > main > > > > > thread can mutate the tree all it wants. If we have an implementation > > where > > > > the > > > > > commit happens on the main thread into the pending tree, then we'd > > probably > > > > pass > > > > > a dummy LTHI to the main thread to commit to on the server, serialize it > > and > > > > > deserialize that into the pending tree on the client.
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1418953002/#ps60001 (title: "Addressing comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418953002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418953002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, brianderson@chromium.org Link to the patchset: https://codereview.chromium.org/1418953002/#ps80001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418953002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418953002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/64d1e686b0eb5e60a2d2e8f7c3f86c57698249bb Cr-Commit-Position: refs/heads/master@{#357759} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
