Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
861963002:
UDP: Windows implementation using non-blocking IO (Closed)
|
Created:
5 years, 11 months ago by Alpha Left Google Modified:
5 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUDP: Windows implementation using non-blocking IO
Non-blocking IO has higher throughput than the previous implementation
that used overlapped IO. 15% increase in throughput for sending to
localhost is observed. There could be additional performance advantage
in WiFi network.
All existing UDP socket tests passed. I added a unit test to do
benchmarking. Also tested this manually with WebRTC and verified that
all UDP send and receive activities are logged correctly.
BUG=442392
Committed: https://crrev.com/5a5ee680f9fc9647e3b7228d181fc7e2bb7ddecf
Cr-Commit-Position: refs/heads/master@{#314752}
Patch Set 1 #Patch Set 2 : WSAEventSelect #
Total comments: 12
Patch Set 3 : fixed comments #Patch Set 4 : added perf test #
Total comments: 46
Patch Set 5 : git cl try #
Total comments: 12
Patch Set 6 : nits #Patch Set 7 : fixed test #Patch Set 8 : nits #
Total comments: 12
Patch Set 9 : Moved out of core and used only one event #Patch Set 10 : overlapped io by default #Patch Set 11 : style nits #Patch Set 12 : fix build #
Total comments: 15
Patch Set 13 : review comments #Patch Set 14 : added boolean #Patch Set 15 : use DCHECK #
Total comments: 1
Patch Set 16 : git cl formatted #Patch Set 17 : use IsWatching() #
Messages
Total messages: 35 (5 generated)
hclam@chromium.org changed reviewers: + rch@chromium.org
rch@chromium.org changed reviewers: + rvargas@chromium.org
This change looks goodish to me, but it's window specific and I'm not super familiar with Windows programming, so I'll defer to the Windows expert Ricardo. Thanks for doing this! https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:581: } else if (network_events.lNetworkEvents) { nit: since there was a return in the previous block, you can drop the else. https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:590: core_->WatchForRead(); Out of curiosity, what conditions lead to this branch?
In thinking about this more, I think we should make overlapped vs non-blocking determined at runtime. In particular, I'd love to be able to set up a field trial which would allow us to measure the impact of this change for QUIC.
On 2015/01/21 04:54:32, Ryan Hamilton wrote: > In thinking about this more, I think we should make overlapped vs non-blocking > determined at runtime. In particular, I'd love to be able to set up a field > trial which would allow us to measure the impact of this change for QUIC. How can we going to evaluate the performance of overlapped vs non-blocking in the field trial then?
On 2015/01/21 20:56:43, Alpha wrote: > On 2015/01/21 04:54:32, Ryan Hamilton wrote: > > In thinking about this more, I think we should make overlapped vs non-blocking > > determined at runtime. In particular, I'd love to be able to set up a field > > trial which would allow us to measure the impact of this change for QUIC. > > How can we going to evaluate the performance of overlapped vs non-blocking in > the field trial then? Various QUIC-enabled Google services have CSI dashboard in which we can break out performance by finch experiment group. So we can set up a group in which we use the new non-blocking IO and confirm what effect it has. What do you think?
On 2015/01/21 21:09:15, Ryan Hamilton wrote: > On 2015/01/21 20:56:43, Alpha wrote: > > On 2015/01/21 04:54:32, Ryan Hamilton wrote: > > > In thinking about this more, I think we should make overlapped vs > non-blocking > > > determined at runtime. In particular, I'd love to be able to set up a field > > > trial which would allow us to measure the impact of this change for QUIC. > > > > How can we going to evaluate the performance of overlapped vs non-blocking in > > the field trial then? > > Various QUIC-enabled Google services have CSI dashboard in which we can break > out performance by finch experiment group. So we can set up a group in which we > use the new non-blocking IO and confirm what effect it has. What do you think? Okay. That sounds good to me.
I think it's going to take me a while to go ever this change, but in the mean time, finch looks like a good idea. https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_unitt... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:657: TEST_F(UDPSocketTest, DISABLED_WriteBenchmark) { benchmarks belong to net_perftests https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:63: int read_iobuffer_len_; We are not supposed to have public variables. We should add set/getters as needed. https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:677: // After this call we'll receive FD_READ signals when asynchronous read nit: after this call the event will be signaled https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:692: int rv = WSARecvFrom(socket_, &read_buffer, 1, &bytes_read, &flags, Looks like there's no reason to use WSARecvFrom over recvfrom
Addressing the comments first. I'll add the code to do overlapped IO back in the next patchset. https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_unitt... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:657: TEST_F(UDPSocketTest, DISABLED_WriteBenchmark) { On 2015/01/21 22:10:09, rvargas wrote: > benchmarks belong to net_perftests Doing this in a later patchset. https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:63: int read_iobuffer_len_; On 2015/01/21 22:10:09, rvargas wrote: > We are not supposed to have public variables. We should add set/getters as > needed. Done. https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:581: } else if (network_events.lNetworkEvents) { On 2015/01/21 04:03:35, Ryan Hamilton wrote: > nit: since there was a return in the previous block, you can drop the else. Done. https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:590: core_->WatchForRead(); On 2015/01/21 04:03:35, Ryan Hamilton wrote: > Out of curiosity, what conditions lead to this branch? This comes from tcp_socket_win.cc. The comment there reads: // This may happen because Read() may succeed synchronously and // consume all the received data without resetting the event object. Interestingly tcp_socket_win uses non-blocking IO for read but overlapped IO for write.. https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:677: // After this call we'll receive FD_READ signals when asynchronous read On 2015/01/21 22:10:09, rvargas wrote: > nit: after this call the event will be signaled Done. https://codereview.chromium.org/861963002/diff/20001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:692: int rv = WSARecvFrom(socket_, &read_buffer, 1, &bytes_read, &flags, On 2015/01/21 22:10:09, rvargas wrote: > Looks like there's no reason to use WSARecvFrom over recvfrom Done.
I've added a method UDPSocketWin::UseNonBlocking() to switch to non blocking IO. I have retained the code to do overlapped IO. Switching to non blocking IO can be done in runtime. Also added a perf test.
https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... File net/udp/udp_socket_perftest.cc (right): https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_perftest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: no (c) and 2015 https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... File net/udp/udp_socket_win.cc (right): https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:49: // Start watching for the end of a read or write operation. nit: the comment is stale (for nb) https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:60: WSAEVENT read_event() const { return read_event_; } nit: I think we need some empty lines somewhere here. Maybe between unrelated pairs. https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:126: WSAEVENT read_event_; Don't do manual handle management. Use a version of ScopedHandle https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:167: memset(&read_overlapped_, 0xaf, sizeof(read_overlapped_)); I know this is the old behavior, but we should not do this. It makes things harder to debug. https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:179: AddRef(); I don't think this is needed for non-blocking. In fact, the whole core object is not needed if there are no async operations. https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:209: core_->socket_->DidCompleteReadNonBlocking(); There is no pending read (no read is completing) https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:443: int nread = core_->use_overlapped_io() use_overlapped_io_ ? I mean, why ask the core about this? https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:653: void UDPSocketWin::DidCompleteReadNonBlocking() { follow declaration order. https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:653: void UDPSocketWin::DidCompleteReadNonBlocking() { DCHECK(buffer)? And we should dcheck(!buffer) on RecvFrom https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:665: // If network_events.iErrorCode[FD_READ_BIT] is nonzero, still call I don't really follow this comment. https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:678: if (recv_from_address_ && rv >= 0) { Wasn't this (and logging the read), already done inside InternalRecvFromNonBlocking? https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:871: WSABUF read_buffer; Not needed https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:880: if (os_error != WSAEWOULDBLOCK) { The code will flow better if processing of wouldblock happens here. https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:910: if (!address) { nit: if (address) https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:921: WSABUF write_buffer; not needed https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:928: if (os_error != WSAEWOULDBLOCK) { ditto
https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_perft... File net/udp/udp_socket_perftest.cc (right): https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_perft... net/udp/udp_socket_perftest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/01/22 23:45:13, rvargas wrote: > nit: no (c) and 2015 Done. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:49: // Start watching for the end of a read or write operation. On 2015/01/22 23:45:13, rvargas wrote: > nit: the comment is stale (for nb) Done. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:60: WSAEVENT read_event() const { return read_event_; } On 2015/01/22 23:45:14, rvargas wrote: > nit: I think we need some empty lines somewhere here. Maybe between unrelated > pairs. Done. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:126: WSAEVENT read_event_; On 2015/01/22 23:45:14, rvargas wrote: > Don't do manual handle management. Use a version of ScopedHandle I don't see one for WSAEvent. I also think it's strange that we'll use a ScopedHandle for read_event_ and write_event_ but have to manually manage it for read_overlapped_.hEvent. I seems to me coherence (i.e. manage manually just like read_overlapped_.hEvent) is important here. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:167: memset(&read_overlapped_, 0xaf, sizeof(read_overlapped_)); On 2015/01/22 23:45:14, rvargas wrote: > I know this is the old behavior, but we should not do this. It makes things > harder to debug. Done. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:179: AddRef(); On 2015/01/22 23:45:14, rvargas wrote: > I don't think this is needed for non-blocking. In fact, the whole core object is > not needed if there are no async operations. Core contains a large amount of member variables that are shared between overlapped mode and non-blocking mode. I seems cleaner to retain this structure until we decided to get rid of the overlapped mode. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:209: core_->socket_->DidCompleteReadNonBlocking(); On 2015/01/22 23:45:14, rvargas wrote: > There is no pending read (no read is completing) Changed the naming to OnReadSignaledNonBlocking. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:443: int nread = core_->use_overlapped_io() On 2015/01/22 23:45:14, rvargas wrote: > use_overlapped_io_ ? > > I mean, why ask the core about this? Done. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:653: void UDPSocketWin::DidCompleteReadNonBlocking() { On 2015/01/22 23:45:13, rvargas wrote: > follow declaration order. Done. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:653: void UDPSocketWin::DidCompleteReadNonBlocking() { On 2015/01/22 23:45:14, rvargas wrote: > DCHECK(buffer)? And we should dcheck(!buffer) on RecvFrom Done for the first one. InternalRecvFromNonBlocking is called by OnReadSiangledNonBlocking and core_->read_iobuffer() is already defined. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:665: // If network_events.iErrorCode[FD_READ_BIT] is nonzero, still call On 2015/01/22 23:45:14, rvargas wrote: > I don't really follow this comment. This is taken from tcp_socket_win but I updated the comment to make it clearer. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:678: if (recv_from_address_ && rv >= 0) { On 2015/01/22 23:45:14, rvargas wrote: > Wasn't this (and logging the read), already done inside > InternalRecvFromNonBlocking? Done. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:871: WSABUF read_buffer; On 2015/01/22 23:45:14, rvargas wrote: > Not needed Done. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:880: if (os_error != WSAEWOULDBLOCK) { On 2015/01/22 23:45:13, rvargas wrote: > The code will flow better if processing of wouldblock happens here. Done. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:910: if (!address) { On 2015/01/22 23:45:14, rvargas wrote: > nit: if (address) Done. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:921: WSABUF write_buffer; On 2015/01/22 23:45:13, rvargas wrote: > not needed Done. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:928: if (os_error != WSAEWOULDBLOCK) { On 2015/01/22 23:45:13, rvargas wrote: > ditto Done.
https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... File net/udp/udp_socket_unittest.cc (right): https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_unittest.cc:33: : buffer_(new IOBufferWithSize(kMaxRead)), weak_factory_(this) {} nit: I'd say this is not allowed by the style guide. Bug in the tool? https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_unittest.cc:128: void ConnectTest(bool use_nonblocking_io) { This method is way too long to be defined at the declaration point. I'd actually say that the class is now to long to do that for the other methods. https://chromiumcodereview.appspot.com/861963002/diff/80001/net/udp/udp_socke... File net/udp/udp_socket_perftest.cc (right): https://chromiumcodereview.appspot.com/861963002/diff/80001/net/udp/udp_socke... net/udp/udp_socket_perftest.cc:23: namespace net { Please take the test out of the net namespace. The reason is that I (as a random reader) don't want to have to figure out where each thing is coming from, and having that spelled out in the code as I read it is useful. https://chromiumcodereview.appspot.com/861963002/diff/80001/net/udp/udp_socke... net/udp/udp_socket_perftest.cc:40: void WritePacketsToSocket(UDPClientSocket* socket, This class is too long to use inline definitions. https://chromiumcodereview.appspot.com/861963002/diff/80001/net/udp/udp_socke... net/udp/udp_socket_perftest.cc:44: scoped_refptr<StringIOBuffer> io_buffer(new StringIOBuffer(msg)); I know this is just a test, but having a memcpy here just so that the previous line can be done with a std template instead of memset seems like setting a bad example. https://chromiumcodereview.appspot.com/861963002/diff/80001/net/udp/udp_socke... net/udp/udp_socket_perftest.cc:46: while (num_of_packets) { for ? https://chromiumcodereview.appspot.com/861963002/diff/80001/net/udp/udp_socke... net/udp/udp_socket_perftest.cc:87: server->AllowAddressReuse(); This generates unreliable behavior on Windows. https://chromiumcodereview.appspot.com/861963002/diff/80001/net/udp/udp_socke... net/udp/udp_socket_perftest.cc:114: server.reset(); Don't really need these, do you?
https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_unitt... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:33: : buffer_(new IOBufferWithSize(kMaxRead)), weak_factory_(this) {} On 2015/01/23 03:05:34, rvargas wrote: > nit: I'd say this is not allowed by the style guide. Bug in the tool? I used git cl format and it didn't correct this line. Not sure. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:128: void ConnectTest(bool use_nonblocking_io) { On 2015/01/23 03:05:34, rvargas wrote: > This method is way too long to be defined at the declaration point. I'd actually > say that the class is now to long to do that for the other methods. Defined it out of the class. https://codereview.chromium.org/861963002/diff/80001/net/udp/udp_socket_perft... File net/udp/udp_socket_perftest.cc (right): https://codereview.chromium.org/861963002/diff/80001/net/udp/udp_socket_perft... net/udp/udp_socket_perftest.cc:23: namespace net { On 2015/01/23 03:05:35, rvargas wrote: > Please take the test out of the net namespace. The reason is that I (as a random > reader) don't want to have to figure out where each thing is coming from, and > having that spelled out in the code as I read it is useful. Done. https://codereview.chromium.org/861963002/diff/80001/net/udp/udp_socket_perft... net/udp/udp_socket_perftest.cc:40: void WritePacketsToSocket(UDPClientSocket* socket, On 2015/01/23 03:05:35, rvargas wrote: > This class is too long to use inline definitions. Defined it out of class. https://codereview.chromium.org/861963002/diff/80001/net/udp/udp_socket_perft... net/udp/udp_socket_perftest.cc:44: scoped_refptr<StringIOBuffer> io_buffer(new StringIOBuffer(msg)); On 2015/01/23 03:05:35, rvargas wrote: > I know this is just a test, but having a memcpy here just so that the previous > line can be done with a std template instead of memset seems like setting a bad > example. Done. https://codereview.chromium.org/861963002/diff/80001/net/udp/udp_socket_perft... net/udp/udp_socket_perftest.cc:46: while (num_of_packets) { On 2015/01/23 03:05:34, rvargas wrote: > for ? I need a counter to track how many packets are sent. If i use a for i need a separate counter. This seems to be cleaner to me. https://codereview.chromium.org/861963002/diff/80001/net/udp/udp_socket_perft... net/udp/udp_socket_perftest.cc:87: server->AllowAddressReuse(); On 2015/01/23 03:05:35, rvargas wrote: > This generates unreliable behavior on Windows. Done. https://codereview.chromium.org/861963002/diff/80001/net/udp/udp_socket_perft... net/udp/udp_socket_perftest.cc:114: server.reset(); On 2015/01/23 03:05:35, rvargas wrote: > Don't really need these, do you? Done.
https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... File net/udp/udp_socket_win.cc (right): https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:126: WSAEVENT read_event_; On 2015/01/23 02:29:47, Alpha wrote: > On 2015/01/22 23:45:14, rvargas wrote: > > Don't do manual handle management. Use a version of ScopedHandle > > I don't see one for WSAEvent. I also think it's strange that we'll use a > ScopedHandle for read_event_ and write_event_ but have to manually manage it for > read_overlapped_.hEvent. I seems to me coherence (i.e. manage manually just like > read_overlapped_.hEvent) is important here. You can use a ScopedHandle directly, or define typedef GenericScopedHandle<HandleTraits, VerifierTraits> ScopedWSAEvent; or use a specific WSAEventTraits class. or use CreateEvent directly to create a manual-reset, non-signaled, non-inheritable event There should be no manual handle management anywhere... but rewriting all our code is complicated. The least we can do is not write new code with manual handle management. I agree with the code consistency argument... but the async code is to be deleted soon so I would be fine living with inconsistent code for a little while. Or you could do xx_overlapped_.hEvent = event_.Handle() and remove manual closing from the async code. https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:179: AddRef(); On 2015/01/23 02:29:47, Alpha wrote: > On 2015/01/22 23:45:14, rvargas wrote: > > I don't think this is needed for non-blocking. In fact, the whole core object > is > > not needed if there are no async operations. > > Core contains a large amount of member variables that are shared between > overlapped mode and non-blocking mode. I seems cleaner to retain this structure > until we decided to get rid of the overlapped mode. That's true, but it makes the post-experiment cleanup more complicated because it requires a real code refactoring (as opposed to simply deleting unused code and some renaming). Regardless of keeping Core at this point, the comment, and grabbing a reference, is not needed for the non-blocking case. If we keep Core, there should be a todo referencing the proper bug, to make sure the cleanup is thorough. https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:653: void UDPSocketWin::DidCompleteReadNonBlocking() { On 2015/01/23 02:29:47, Alpha wrote: > On 2015/01/22 23:45:14, rvargas wrote: > > DCHECK(buffer)? And we should dcheck(!buffer) on RecvFrom > > Done for the first one. > > InternalRecvFromNonBlocking is called by OnReadSiangledNonBlocking and > core_->read_iobuffer() is already defined. Right. That's why I suggested adding a dcehck on RecvFrom which is only called by the user. https://chromiumcodereview.appspot.com/861963002/diff/60001/net/udp/udp_socke... net/udp/udp_socket_win.cc:665: // If network_events.iErrorCode[FD_READ_BIT] is nonzero, still call On 2015/01/23 02:29:47, Alpha wrote: > On 2015/01/22 23:45:14, rvargas wrote: > > I don't really follow this comment. > > This is taken from tcp_socket_win but I updated the comment to make it clearer. ok. It still seems somewhat red-herringish as the "natural" thing to do is to read after receiving the notification, but I guess someone could think about "optimizing" the code if there was an error at some point (which really doesn't predict what recv will do) https://chromiumcodereview.appspot.com/861963002/diff/140001/net/udp/udp_sock... File net/udp/udp_socket_win.cc (right): https://chromiumcodereview.appspot.com/861963002/diff/140001/net/udp/udp_sock... net/udp/udp_socket_win.cc:171: memset(&read_overlapped_, 0, sizeof(read_overlapped_)); Sorry, I meant clearing the object at destruction makes crash dumps harder to debug, and if there is no bug it should not be needed. https://chromiumcodereview.appspot.com/861963002/diff/140001/net/udp/udp_sock... net/udp/udp_socket_win.cc:863: // completed.g typo https://chromiumcodereview.appspot.com/861963002/diff/140001/net/udp/udp_sock... net/udp/udp_socket_win.cc:879: } else { no else after return https://chromiumcodereview.appspot.com/861963002/diff/140001/net/udp/udp_sock... net/udp/udp_socket_win.cc:884: } else { this else also goes away https://chromiumcodereview.appspot.com/861963002/diff/140001/net/udp/udp_sock... net/udp/udp_socket_win.cc:1230: NOTREACHED() << "Cannot change to non-blocking mode after socket is used."; nit: The DCHECK (from ps4) was cleaner IMO. (and text with DCHECKS is usually not useful other than to make the binary bigger)
spotted a typo. https://codereview.chromium.org/861963002/diff/140001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/861963002/diff/140001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:50: // In non-blocking mode watch the signals to continure read/write. typo. continue
Patchset #9 (id:160001) has been deleted
this patchset pretty much changed the entire patch: (1) I moved out all the code that performs non blocking IO out of Core. This make removal of Core and overlapped IO code path easier. All changes to Core was reverted. (2) In order to do (1) I need to rewrite all the code that calls LogRead() because it assumes the recvfrom address is stored in |core_|. (3) There was a bug in my code that resulted in only read or write. I found out that WSAEventSelect can apply only one event object to one socket. There is now only one event, one object watcher and I need to call WSAEnumNetworkEvents to determine if it is a read or write event. Updated the test case to cover this. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:126: WSAEVENT read_event_; On 2015/01/23 20:05:24, rvargas wrote: > On 2015/01/23 02:29:47, Alpha wrote: > > On 2015/01/22 23:45:14, rvargas wrote: > > > Don't do manual handle management. Use a version of ScopedHandle > > > > I don't see one for WSAEvent. I also think it's strange that we'll use a > > ScopedHandle for read_event_ and write_event_ but have to manually manage it > for > > read_overlapped_.hEvent. I seems to me coherence (i.e. manage manually just > like > > read_overlapped_.hEvent) is important here. > > You can use a ScopedHandle directly, or define > > typedef GenericScopedHandle<HandleTraits, VerifierTraits> ScopedWSAEvent; > > or use a specific WSAEventTraits class. > > or use CreateEvent directly to create a manual-reset, non-signaled, > non-inheritable event > > There should be no manual handle management anywhere... but rewriting all our > code is complicated. The least we can do is not write new code with manual > handle management. > > I agree with the code consistency argument... but the async code is to be > deleted soon so I would be fine living with inconsistent code for a little > while. > > Or you could do xx_overlapped_.hEvent = event_.Handle() and remove manual > closing from the async code. In this case I'll leave the handle management in Core untouched. But read_event_ and write_event_ will be moved to UDPSocketWin and use ScopedHandle. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:179: AddRef(); On 2015/01/23 20:05:24, rvargas wrote: > On 2015/01/23 02:29:47, Alpha wrote: > > On 2015/01/22 23:45:14, rvargas wrote: > > > I don't think this is needed for non-blocking. In fact, the whole core > object > > is > > > not needed if there are no async operations. > > > > Core contains a large amount of member variables that are shared between > > overlapped mode and non-blocking mode. I seems cleaner to retain this > structure > > until we decided to get rid of the overlapped mode. > > That's true, but it makes the post-experiment cleanup more complicated because > it requires a real code refactoring (as opposed to simply deleting unused code > and some renaming). > > Regardless of keeping Core at this point, the comment, and grabbing a reference, > is not needed for the non-blocking case. > > If we keep Core, there should be a todo referencing the proper bug, to make sure > the cleanup is thorough. Okay I'll keep Core untouched but duplicate neededs bits to UDPSocketWin, it will make removing the overlapped IO easier to do. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:653: void UDPSocketWin::DidCompleteReadNonBlocking() { On 2015/01/23 20:05:24, rvargas wrote: > On 2015/01/23 02:29:47, Alpha wrote: > > On 2015/01/22 23:45:14, rvargas wrote: > > > DCHECK(buffer)? And we should dcheck(!buffer) on RecvFrom > > > > Done for the first one. > > > > InternalRecvFromNonBlocking is called by OnReadSiangledNonBlocking and > > core_->read_iobuffer() is already defined. > > Right. That's why I suggested adding a dcehck on RecvFrom which is only called > by the user. Acknowledged. https://codereview.chromium.org/861963002/diff/60001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:665: // If network_events.iErrorCode[FD_READ_BIT] is nonzero, still call On 2015/01/23 20:05:24, rvargas wrote: > On 2015/01/23 02:29:47, Alpha wrote: > > On 2015/01/22 23:45:14, rvargas wrote: > > > I don't really follow this comment. > > > > This is taken from tcp_socket_win but I updated the comment to make it > clearer. > > ok. It still seems somewhat red-herringish as the "natural" thing to do is to > read after receiving the notification, but I guess someone could think about > "optimizing" the code if there was an error at some point (which really doesn't > predict what recv will do) It sounds to me you think the comment is bogus. In that case I'll just remove that. Reading after the notification is the natural thing to do anyway. https://codereview.chromium.org/861963002/diff/140001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/861963002/diff/140001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:50: // In non-blocking mode watch the signals to continure read/write. On 2015/01/26 20:53:29, guoweis_chromium wrote: > typo. continue Removed this code already. https://codereview.chromium.org/861963002/diff/140001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:171: memset(&read_overlapped_, 0, sizeof(read_overlapped_)); On 2015/01/23 20:05:25, rvargas wrote: > Sorry, I meant clearing the object at destruction makes crash dumps harder to > debug, and if there is no bug it should not be needed. Done. https://codereview.chromium.org/861963002/diff/140001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:863: // completed.g On 2015/01/23 20:05:25, rvargas wrote: > typo Done. https://codereview.chromium.org/861963002/diff/140001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:879: } else { On 2015/01/23 20:05:25, rvargas wrote: > no else after return Done. https://codereview.chromium.org/861963002/diff/140001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:884: } else { On 2015/01/23 20:05:25, rvargas wrote: > this else also goes away Done. https://codereview.chromium.org/861963002/diff/140001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:1230: NOTREACHED() << "Cannot change to non-blocking mode after socket is used."; On 2015/01/23 20:05:25, rvargas wrote: > nit: The DCHECK (from ps4) was cleaner IMO. (and text with DCHECKS is usually > not useful other than to make the binary bigger) In that case I'll just remove the DCHECK or NOTREACHED.
rch: Since rvargas is OOO for the whole week. Would you like suggest another reviewer to look at this change?
Sorry for the delay. I was not really expecting a rewrite after the previous round of comments. Looks basically fine. https://chromiumcodereview.appspot.com/861963002/diff/240001/net/udp/udp_sock... File net/udp/udp_socket_win.cc (right): https://chromiumcodereview.appspot.com/861963002/diff/240001/net/udp/udp_sock... net/udp/udp_socket_win.cc:264: use_non_blocking_io_(true), false https://chromiumcodereview.appspot.com/861963002/diff/240001/net/udp/udp_sock... net/udp/udp_socket_win.cc:291: if (!use_non_blocking_io_ || SetNonBlocking(socket_)) { Failure to set non blocking must be accounted for the experiment. https://chromiumcodereview.appspot.com/861963002/diff/240001/net/udp/udp_sock... net/udp/udp_socket_win.cc:316: read_write_event_.Close(); must be called after closesocket (or it may race to use an invalid handle) https://chromiumcodereview.appspot.com/861963002/diff/240001/net/udp/udp_sock... net/udp/udp_socket_win.cc:660: DCHECK(read_iobuffer_); Either both methods dcheck the buffer or not. (I have no preference about the dcheck here, given that the condition is verified at line 646) However, I must insist in verifying that the buffer is not set when we are being called by the user, specially now that we ignore subtle mismatches between the buffer and the event being notified. https://chromiumcodereview.appspot.com/861963002/diff/240001/net/udp/udp_sock... net/udp/udp_socket_win.cc:683: if (read_write_watcher_.GetWatchedObject() != NULL) { When would this be the case? (red flag: checking a raw handle vs null) https://chromiumcodereview.appspot.com/861963002/diff/240001/net/udp/udp_sock... net/udp/udp_socket_win.cc:1184: return; This looks like a serious issue. The data from the experiment will be wrong.
https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:596: if (recv_from_address_) { nit: no {}
https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:264: use_non_blocking_io_(true), On 2015/02/03 03:11:41, rvargas wrote: > false Done. https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:291: if (!use_non_blocking_io_ || SetNonBlocking(socket_)) { On 2015/02/03 03:11:41, rvargas wrote: > Failure to set non blocking must be accounted for the experiment. guiwei pointed out that WSAEventSelect already sets the socket to non blocking. I moved this call. https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:316: read_write_event_.Close(); On 2015/02/03 03:11:41, rvargas wrote: > must be called after closesocket (or it may race to use an invalid handle) Done. https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:596: if (recv_from_address_) { On 2015/02/03 20:55:30, rvargas wrote: > nit: no {} Done. https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:660: DCHECK(read_iobuffer_); On 2015/02/03 03:11:41, rvargas wrote: > Either both methods dcheck the buffer or not. (I have no preference about the > dcheck here, given that the condition is verified at line 646) > > However, I must insist in verifying that the buffer is not set when we are being > called by the user, specially now that we ignore subtle mismatches between the > buffer and the event being notified. Yeah this DCHECK should be removed since line 646 already does the check. I'll move the DCHECK to a method that is hit earlier. https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:683: if (read_write_watcher_.GetWatchedObject() != NULL) { On 2015/02/03 03:11:41, rvargas wrote: > When would this be the case? (red flag: checking a raw handle vs null) For example both write and read got WSA_IO_PENDING there will be two calls to WatchForReadWrite(). The check for NULL comes from this comment: https://code.google.com/p/chromium/codesearch#chromium/src/base/win/object_wa... https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:1184: return; On 2015/02/03 03:11:41, rvargas wrote: > This looks like a serious issue. The data from the experiment will be wrong. I'll return a boolean with this call.
https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:683: if (read_write_watcher_.GetWatchedObject() != NULL) { On 2015/02/03 23:24:46, Alpha wrote: > On 2015/02/03 03:11:41, rvargas wrote: > > When would this be the case? (red flag: checking a raw handle vs null) > > For example both write and read got WSA_IO_PENDING there will be two calls to > WatchForReadWrite(). > > The check for NULL comes from this comment: > https://code.google.com/p/chromium/codesearch#chromium/src/base/win/object_wa... The code from the object watcher is quite old... and technically it may be ok to indicate if there is no watch in progress by returning a null handle, but these days checking for a null handle is a code smell (raw handle... there should be an object somewhere with an explicit method to check validity). If we are waiting for read and the user writes we would be waiting... but I'd prefer if this code figures out if it is waiting or not without having to look at the handle tracked by the watcher. Maybe looking at the write buffer when reading?
On 2015/02/03 23:54:34, rvargas wrote: > https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.cc > File net/udp/udp_socket_win.cc (right): > > https://codereview.chromium.org/861963002/diff/240001/net/udp/udp_socket_win.... > net/udp/udp_socket_win.cc:683: if (read_write_watcher_.GetWatchedObject() != > NULL) { > On 2015/02/03 23:24:46, Alpha wrote: > > On 2015/02/03 03:11:41, rvargas wrote: > > > When would this be the case? (red flag: checking a raw handle vs null) > > > > For example both write and read got WSA_IO_PENDING there will be two calls to > > WatchForReadWrite(). > > > > The check for NULL comes from this comment: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/win/object_wa... > > The code from the object watcher is quite old... and technically it may be ok to > indicate if there is no watch in progress by returning a null handle, but these > days checking for a null handle is a code smell (raw handle... there should be > an object somewhere with an explicit method to check validity). > > If we are waiting for read and the user writes we would be waiting... but I'd > prefer if this code figures out if it is waiting or not without having to look > at the handle tracked by the watcher. Maybe looking at the write buffer when > reading? I've added boolean to replace checking the handle. There's one more condition that WatchForReadWrite can be called twice: 1. OnObjectSignaled called. 2. OnReadSignaled called. Got WSA_IO_PENDING. Calls WatchForReadWrite. 3. WatchForReadWrite called again in OnObjectSignaled. Having a boolean make it simpler. I also added a DCHECK to verify that the event is watched.
ping.
Updated by having a DCHECK in UseNonBlockingIO().
lgtm
https://codereview.chromium.org/861963002/diff/300001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/861963002/diff/300001/net/udp/udp_socket_win.... net/udp/udp_socket_win.cc:683: if (watching_read_write_event_) { BTW, you _could_ change this after https://codereview.chromium.org/868143004/ lands
New patchsets have been uploaded after l-g-t-m from rvargas@chromium.org
On 2015/02/05 00:44:28, rvargas wrote: > https://codereview.chromium.org/861963002/diff/300001/net/udp/udp_socket_win.cc > File net/udp/udp_socket_win.cc (right): > > https://codereview.chromium.org/861963002/diff/300001/net/udp/udp_socket_win.... > net/udp/udp_socket_win.cc:683: if (watching_read_write_event_) { > BTW, you _could_ change this after https://codereview.chromium.org/868143004/ > lands Updated to use IsWatching().
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/861963002/340001
Message was sent while issue was closed.
Committed patchset #17 (id:340001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/5a5ee680f9fc9647e3b7228d181fc7e2bb7ddecf Cr-Commit-Position: refs/heads/master@{#314752} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
