Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2267233002:
Change WebSocketChannel to pass data via IOBuffer (Closed)
|
Created:
4 years, 4 months ago by darin (slow to review) Modified:
4 years, 2 months ago Reviewers:
yhirano CC:
chromium-reviews, jam, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange WebSocketChannel to pass data via IOBuffer
This avoids having to copy the data between std::vector<char> and IOBuffer within the implementation of WebSocketChannel.
As a follow-up, I plan to see about eliminating a similar copy at the Mojo bindings layer in content/browser/websockets/.
Committed: https://crrev.com/0da77e9261dad8cf86e68ccccdfd436afaf0208a
Cr-Commit-Position: refs/heads/master@{#422846}
Patch Set 1 #Patch Set 2 : snapshot #Patch Set 3 : More #
Total comments: 5
Patch Set 4 : Update per review feedback #
Total comments: 2
Patch Set 5 : Make it const char* #
Messages
Total messages: 41 (27 generated)
The CQ bit was checked by darin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by darin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by darin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Ah, now it's much more obvious what you are doing and why you were asking (and I'm guessing this offers efficiencies w/ Mojo by reusing the IOBuffer used for sending/receiving frames) There's an "blessed" way with GMock to do this, but IMO it feels pretty gross for readability, so your current approach seems more desirable/maintainable. But, for completeness, an example is below: https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... net/websockets/websocket_channel.cc:455: if (front.data().get()) { The .get() here is superfluous due to the bool operators. https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... net/websockets/websocket_channel_test.cc:2383: AsVector("FO"))); OK, now it totally makes more sense what you were asking w/r/t IOBufferWithSize If you wanted to avoid the extra casting, you could do something like: MATCHER_P(BufferAndSize, matcher, "") { // or std::vector if you want to use vector matchers std::string data(::testing::get<0>(arg)->data(), ::testing::get<1>(arg)); return matcher.MatchAndExplain(data, result_listener); } EXPECT_CALL( *event_interface_, OnDataFrame(false, WebSocketFrameHeader::kOpCodeText, _) .With(::testing::Args<2, 3>(BufferAndSize("FO"))); Arguably, the above code is subtler and assumes even more that readers have understood the subtlety of https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#... But it does avoid the need to inject your stuff in 160-168, and potentially allows you to do more complex chains like .With(::testing::Args<2, 3>(BufferAndSize(AllOf(StartsWith("Hello"), MatchesRegex("World$"))))); Or using a base::StringPiece matcher, if you wanted to avoid the copy (if you had more complex cases)
On 2016/08/30 08:07:39, Ryan Sleevi (slow) wrote: > Ah, now it's much more obvious what you are doing and why you were asking (and > I'm guessing this offers efficiencies w/ Mojo by reusing the IOBuffer used for > sending/receiving frames) This CL just avoids a couple buffer copies b/w net::WebSocketChannel and its consumer. I added more buffer copies as part of the Mojo conversion, so this is balancing that out. I plan to do a follow-up CL that attempts to avoid even more by using Mojo StructTraits. Ideally, we should be able to copy from the underlying IPC buffer into a net::IOBuffer, and then pass that along to the WebSocketChannel and vice versa. I'm not sure yet how gross this will be. > There's an "blessed" way with GMock to do this, but IMO it feels pretty gross > for readability, so your current approach seems more desirable/maintainable. > But, for completeness, an example is below: OIC. I thought there might be. Thanks for sharing. That is pretty complex and probably tough for a gmock newbie to grok. I haven't found the gmock documentation to be all that good, or maybe I just haven't found it yet. > https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... > File net/websockets/websocket_channel.cc (right): > > https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... > net/websockets/websocket_channel.cc:455: if (front.data().get()) { > The .get() here is superfluous due to the bool operators. Thanks
The CQ bit was checked by darin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== IOBuffer hacking ========== to ========== Change WebSocketChannel to pass data via IOBuffer This avoids having to copy the data between std::vector<char> and IOBuffer within the implementation of WebSocketChannel. As a follow-up, I plan to see about eliminating a similar copy at the Mojo bindings layer in content/browser/websockets/. ==========
darin@chromium.org changed reviewers: + yhirano@chromium.org - rsleevi@chromium.org
@yhirano - please let me know what you think. thanks!
This CL removes some copy operations. We can remove these more easily by replacing some vector<char> with vector<uint8_t>, but you want to use IOBuffer because you are planning to support the conversion between array<uint8> and IOBuffer, is that right? Splitting this CL to two, one for removing |cons scoped_refptr<T> &| and another for replacing vector<uint8_t> with IOBuffer, might be good. https://codereview.chromium.org/2267233002/diff/30001/content/browser/websock... File content/browser/websockets/websocket_impl.cc (right): https://codereview.chromium.org/2267233002/diff/30001/content/browser/websock... content/browser/websockets/websocket_impl.cc:443: scoped_refptr<net::IOBuffer> data_to_pass(new net::IOBuffer(data.size())); "Avoid this copy" missing? https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... net/websockets/websocket_channel.cc:133: DependentIOBuffer(const scoped_refptr<IOBuffer>& buffer, size_t offset) I think |const scoped_refptr<T>&| is discouraged. https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... net/websockets/websocket_channel.cc:434: return SendFrameFromIOBuffer(fin, op_code, std::move(buffer), buffer_size); With this change SendFrame also takes an IOBuffer. Maybe this function should be renamed, like SendFrameInternal?
On 2016/09/06 12:19:48, yhirano wrote: > This CL removes some copy operations. We can remove these more easily by > replacing some vector<char> with vector<uint8_t>, but you want to use IOBuffer > because you are planning to support the conversion between array<uint8> and > IOBuffer, is that right? Yes, that's the idea. This CL also eliminates copies within the implementation of WebSocketChannel. It wants an IOBuffer for interacting with the lower level net/ code afterall. It seems like vector<uint8_t> would not address that inefficiency. So, even if I don't succeed in changing the Mojo bindings to replace vector<uint8_t> with IOBuffer, at least I would have streamlined the net/ websocket code and encouraged consumers to use the more efficient method. > Splitting this CL to two, one for removing |cons scoped_refptr<T> &| and another > for replacing vector<uint8_t> with IOBuffer, might be good. > > https://codereview.chromium.org/2267233002/diff/30001/content/browser/websock... > File content/browser/websockets/websocket_impl.cc (right): > > https://codereview.chromium.org/2267233002/diff/30001/content/browser/websock... > content/browser/websockets/websocket_impl.cc:443: scoped_refptr<net::IOBuffer> > data_to_pass(new net::IOBuffer(data.size())); > "Avoid this copy" missing? > > https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... > File net/websockets/websocket_channel.cc (right): > > https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... > net/websockets/websocket_channel.cc:133: DependentIOBuffer(const > scoped_refptr<IOBuffer>& buffer, size_t offset) > I think |const scoped_refptr<T>&| is discouraged. > > https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... > net/websockets/websocket_channel.cc:434: return SendFrameFromIOBuffer(fin, > op_code, std::move(buffer), buffer_size); > With this change SendFrame also takes an IOBuffer. Maybe this function should be > renamed, like SendFrameInternal? Thanks for the suggestions.
On 2016/09/06 12:19:48, yhirano wrote: ... > Splitting this CL to two, one for removing |cons scoped_refptr<T> &| and another > for replacing vector<uint8_t> with IOBuffer, might be good. How strongly do you feel about this suggestion? :) > > https://codereview.chromium.org/2267233002/diff/30001/content/browser/websock... > File content/browser/websockets/websocket_impl.cc (right): > > https://codereview.chromium.org/2267233002/diff/30001/content/browser/websock... > content/browser/websockets/websocket_impl.cc:443: scoped_refptr<net::IOBuffer> > data_to_pass(new net::IOBuffer(data.size())); > "Avoid this copy" missing? Done > > https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... > File net/websockets/websocket_channel.cc (right): > > https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... > net/websockets/websocket_channel.cc:133: DependentIOBuffer(const > scoped_refptr<IOBuffer>& buffer, size_t offset) > I think |const scoped_refptr<T>&| is discouraged. Oh, I see. It is explained here: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > https://codereview.chromium.org/2267233002/diff/30001/net/websockets/websocke... > net/websockets/websocket_channel.cc:434: return SendFrameFromIOBuffer(fin, > op_code, std::move(buffer), buffer_size); > With this change SendFrame also takes an IOBuffer. Maybe this function should be > renamed, like SendFrameInternal? Done
The CQ bit was checked by darin@chromium.org to run a CQ dry run
presubmit upload checks requested that I run "git cl format net", so that added a bunch more delta to this patch :-/
presubmit upload checks requested that I run "git cl format net", so that added a bunch more delta to this patch :-/
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@yhirano - PTAL, thx!
lgtm https://codereview.chromium.org/2267233002/diff/50001/net/websockets/websocke... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/2267233002/diff/50001/net/websockets/websocke... net/websockets/websocket_channel_test.cc:164: char* data = buffer ? buffer->data() : nullptr; |const char*| might be preferable.
Thanks for the review and good suggestion. https://codereview.chromium.org/2267233002/diff/50001/net/websockets/websocke... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/2267233002/diff/50001/net/websockets/websocke... net/websockets/websocket_channel_test.cc:164: char* data = buffer ? buffer->data() : nullptr; On 2016/10/04 01:35:09, yhirano wrote: > |const char*| might be preferable. Done.
The CQ bit was checked by darin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2267233002/#ps70001 (title: "Make it const char*")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Change WebSocketChannel to pass data via IOBuffer This avoids having to copy the data between std::vector<char> and IOBuffer within the implementation of WebSocketChannel. As a follow-up, I plan to see about eliminating a similar copy at the Mojo bindings layer in content/browser/websockets/. ========== to ========== Change WebSocketChannel to pass data via IOBuffer This avoids having to copy the data between std::vector<char> and IOBuffer within the implementation of WebSocketChannel. As a follow-up, I plan to see about eliminating a similar copy at the Mojo bindings layer in content/browser/websockets/. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== Change WebSocketChannel to pass data via IOBuffer This avoids having to copy the data between std::vector<char> and IOBuffer within the implementation of WebSocketChannel. As a follow-up, I plan to see about eliminating a similar copy at the Mojo bindings layer in content/browser/websockets/. ========== to ========== Change WebSocketChannel to pass data via IOBuffer This avoids having to copy the data between std::vector<char> and IOBuffer within the implementation of WebSocketChannel. As a follow-up, I plan to see about eliminating a similar copy at the Mojo bindings layer in content/browser/websockets/. Committed: https://crrev.com/0da77e9261dad8cf86e68ccccdfd436afaf0208a Cr-Commit-Position: refs/heads/master@{#422846} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0da77e9261dad8cf86e68ccccdfd436afaf0208a Cr-Commit-Position: refs/heads/master@{#422846} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
