Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
495313003:
Let GpuChannel handle deferred IPC messages of SwapBuffer in batch (Closed)
|
Created:
6 years, 4 months ago by Hongbo Min Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, piman+watch_chromium.org, Shouqun Liu, yunchao Base URL:
https://chromium.googlesource.com/chromium/src@master Project:
chromium Visibility:
Public. |
DescriptionWe process the pending messages immediately if these messages matches
the pattern of SwapBuffers, for example, GLRenderer always issues
SwapBuffers calls with a specific IPC message patterns, it should always
be SetLatencyInfo->AsyncFlush->Echo sequence.
Instead of posting a task to message loop, it could avoid the possibility
of being blocked by other channels, and make SwapBuffers executed as soon
as possible.
BUG=407529
Committed: https://crrev.com/c4c12c1da82232d2145aa0ffd4a7f556178f782f
Cr-Commit-Position: refs/heads/master@{#292768}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : fix nits #
Total comments: 5
Patch Set 3 : fix nits II #
Messages
Total messages: 17 (1 generated)
Hi, reviewers, could you please take a review? Thanks.
ping kbr@
This looks good overall. It looks a little complicated, but I don't have any good suggestions about simplifying it. Could you please file a bug and point this change to it? How did you discover the need for this optimization? Also please get jbauman's review. I'll approve it afterward. https://codereview.chromium.org/495313003/diff/20001/content/common/gpu/gpu_c... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/495313003/diff/20001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel.cc:771: // SwapBuffers calls with a specifix IPC message patterns, for example, typo: specific. Here and in CL description. https://codereview.chromium.org/495313003/diff/20001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel.cc:785: --matched_messages_num; Before this line, please DCHECK(matched_messages_num != 0).
https://codereview.chromium.org/495313003/diff/20001/content/common/gpu/gpu_c... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/495313003/diff/20001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel.cc:777: m = deferred_messages_.front(); I think you could move these two lines to the beginning of the do while loop, and then you wouldn't need to deal with "m" and "stub" down here.
A new bug was filed at http://crbug.com/407531. https://codereview.chromium.org/495313003/diff/20001/content/common/gpu/gpu_c... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/495313003/diff/20001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel.cc:771: // SwapBuffers calls with a specifix IPC message patterns, for example, On 2014/08/25 23:03:03, Ken Russell wrote: > typo: specific. Here and in CL description. Done. https://codereview.chromium.org/495313003/diff/20001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel.cc:777: m = deferred_messages_.front(); On 2014/08/26 00:00:43, jbauman wrote: > I think you could move these two lines to the beginning of the do while loop, > and then you wouldn't need to deal with "m" and "stub" down here. Done. https://codereview.chromium.org/495313003/diff/20001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel.cc:785: --matched_messages_num; On 2014/08/25 23:03:03, Ken Russell wrote: > Before this line, please DCHECK(matched_messages_num != 0). Done.
https://codereview.chromium.org/495313003/diff/40001/content/common/gpu/gpu_c... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/495313003/diff/40001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel.cc:781: matched_messages_num = MatchSwapBufferMessagesPattern(message.get()); This isn't the same logic as in the previous patch set nor the original code. Note that in the previous code deferred_messages_.pop_front() is called higher up (line 685) before deferred_messages_.front() is called on line 722, so this used to start the scan for GpuCommandBufferMsg_Echo::ID from the message after the one currently processed. Now it'll start the look-ahead from whatever the current message is, so it won't necessarily fast-track a SwapBuffers following an AsyncFlush (think AsyncFlush -> SetLatencyInfo -> AsyncFlush -> Echo). How did you intend this to work? How was this change tested? There's a lack of unit tests for the GpuChannel.
Russell, thanks for your comments. https://codereview.chromium.org/495313003/diff/40001/content/common/gpu/gpu_c... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/495313003/diff/40001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel.cc:781: matched_messages_num = MatchSwapBufferMessagesPattern(message.get()); Russell, thanks for your time. On 2014/08/26 18:18:55, Ken Russell wrote: > This isn't the same logic as in the previous patch set nor the original code. > Note that in the previous code deferred_messages_.pop_front() is called higher > up (line 685) before deferred_messages_.front() is called on line 722, so this > used to start the scan for GpuCommandBufferMsg_Echo::ID from the message after > the one currently processed. Now it'll start the look-ahead from whatever the > current message is, so it won't necessarily fast-track a SwapBuffers following > an AsyncFlush (think AsyncFlush -> SetLatencyInfo -> AsyncFlush -> Echo). How > did you intend this to work? Such a change would not change the logics. The parameter |message| passed to MatchSwapBufferMessagesPattern is initialized from |m| which is assigned to the front of |deferred_messages_|, it is always pointing to the message currently being processed, no matter whether pop_front is called on |deferred_message_|. During the process of MatchSwapBufferMessagesPattern, the first message of |deferred_messages_| is actually the next message of the current processing message |message|, so it can look ahead by fetching the first element of |deferred_messages_|. Take AsyncFlush -> SetLatencyInfo -> AsyncFlush -> Echo sequence for example. Assume the current processing message is AsyncFlush, the MatchSwapBufferMessagesPattern would return 0 indicating no SwapBufers found for this AsyncFlush message, and let message loop schedule HandleMessage next time. The next time of HandleMessage is called, the current processing message is SetLatencyInfo, and MatchSwapBufferMessagesPattern would return 2, meaning it is followed by 2 more messages which are made up of a SwapBuffers, and we will go to next loop iteration instead of scheduling by message loop, and at the begin of loop, it will fetch the first message of deferred message and repeat again util no more pattern message. For the original code, If the current processing message is AsyncFlush followed by Echo, the MatchSwapBufferMessagesPattern will return 1, meaning the Echo message should be fast-tracked, it is the same as the origin code. > > How was this change tested? There's a lack of unit tests for the GpuChannel. Seems there is no any unittest for GpuChannel, no gpu_channel_unittest.cc file is found. Is there any other place to add test for GpuChannel or write a new one from scatch?
https://codereview.chromium.org/495313003/diff/40001/content/common/gpu/gpu_c... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/495313003/diff/40001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel.cc:783: matched_messages_num > 0 && stub && stub->IsScheduled(); I don't think you need to check for "stub && stub->IsScheduled()" here or in the else branch.
Thanks for this contribution and for your explanations. LGTM with jbauman's concerns addressed. https://codereview.chromium.org/495313003/diff/40001/content/common/gpu/gpu_c... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/495313003/diff/40001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel.cc:781: matched_messages_num = MatchSwapBufferMessagesPattern(message.get()); On 2014/08/27 01:54:00, Hongbo Min wrote: > Such a change would not change the logics. The parameter |message| passed to > MatchSwapBufferMessagesPattern is initialized from |m| which is assigned to the > front of |deferred_messages_|, it is always pointing to the message currently > being processed, no matter whether pop_front is called on |deferred_message_|. Thanks for pointing that out. I now see that message.get(), not m, was being passed to MatchSwapBufferMessagesPattern in patch set 1. Agree that the new structure is the same. > Seems there is no any unittest for GpuChannel, no gpu_channel_unittest.cc file > is found. Is there any other place to add test for GpuChannel or write a new one > from scatch? A new unit test file would be needed. I won't block this CL on the addition of new tests, but if you would consider starting to add some as future work, that would be very helpful.
jbauman, The CL is updated. thanks. https://codereview.chromium.org/495313003/diff/40001/content/common/gpu/gpu_c... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/495313003/diff/40001/content/common/gpu/gpu_c... content/common/gpu/gpu_channel.cc:783: matched_messages_num > 0 && stub && stub->IsScheduled(); On 2014/08/27 20:46:41, jbauman wrote: > I don't think you need to check for "stub && stub->IsScheduled()" here or in the > else branch. Seems stub->IsScheduled is not needed since the above if clause could ensure it is true, but we still need to check stub is null.
Patchset #3 (id:60001) has been deleted
lgtm
The CQ bit was checked by hongbo.min@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/495313003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as 4916accef68ee0cccbf1a44d65b2bc36a4679cb7
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c4c12c1da82232d2145aa0ffd4a7f556178f782f Cr-Commit-Position: refs/heads/master@{#292768} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
