Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23875019:
Add SyncSocket::ReceiveWithTimeout() and SyncSocket unit tests. (Closed)
|
Created:
7 years, 3 months ago by DaleCurtis Modified:
7 years, 2 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, awong, Nico Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd SyncSocket::ReceiveWithTimeout() and SyncSocket unit tests.
Method does as advertised using select() on POSIX and the existing
CancelableFileOperation() wrapper on Windows since there is no
select() equivalent on XP.
It was also high time we had some unit tests for this class, so I
added a small set which exercise almost the full functionality.
BUG=289124
TEST=base_unittests!
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229621
Patch Set 1 #
Total comments: 9
Patch Set 2 : Remove DCHECK. #Patch Set 3 : Add comments. #Patch Set 4 : Only read Peek() bytes. #
Total comments: 4
Patch Set 5 : Fixes. #
Total comments: 23
Patch Set 6 : Nits, nits everywhere! #Patch Set 7 : Handle EINTR. Reverse expectations. #
Total comments: 15
Patch Set 8 : Fix timeout and naming. #
Total comments: 12
Patch Set 9 : Explicitly check microseconds. #
Total comments: 2
Patch Set 10 : Revert NaCl changes. #Patch Set 11 : Consts. #
Total comments: 2
Patch Set 12 : Nits. #Patch Set 13 : static_cast<suseconds_t> #Patch Set 14 : Only AssertIOAllowed() on blocking Send(). #
Total comments: 3
Messages
Total messages: 50 (0 generated)
Split off of https://codereview.chromium.org/22886005/
lgtm
https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc#new... base/sync_socket_posix.cc:118: DCHECK(timeout.InMicroseconds() < Time::kMicrosecondsPerSecond); nit: DCHECK_LT? https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc#new... base/sync_socket_posix.cc:126: DCHECK_GE(Peek(), length); Why do we think Peek() can't be less than length? I thought select could return with fewer bytes than requested...
https://codereview.chromium.org/23875019/diff/1/base/sync_socket_unittest.cc File base/sync_socket_unittest.cc (right): https://codereview.chromium.org/23875019/diff/1/base/sync_socket_unittest.cc#... base/sync_socket_unittest.cc:53: ASSERT_EQ(a->Peek(), 0u); ASSERT_EQ just exists the current function. Is that the expected behavior? If so, please comment.
https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc#new... base/sync_socket_posix.cc:118: DCHECK(timeout.InMicroseconds() < Time::kMicrosecondsPerSecond); On 2013/09/17 18:54:12, awong wrote: > nit: DCHECK_LT? Doesn't work for some reason, ends up with a link time error indicating that kMicrosecondsPerSecond is not defined... Spent some not insignificant time trying to resolve, but no luck. Ideas? https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc#new... base/sync_socket_posix.cc:126: DCHECK_GE(Peek(), length); On 2013/09/17 18:54:12, awong wrote: > Why do we think Peek() can't be less than length? I thought select could return > with fewer bytes than requested... Correct, I'll remove the condition. For its intended use case it seems practically true since we're only sending 4 bytes around at a time.
https://codereview.chromium.org/23875019/diff/1/base/sync_socket_unittest.cc File base/sync_socket_unittest.cc (right): https://codereview.chromium.org/23875019/diff/1/base/sync_socket_unittest.cc#... base/sync_socket_unittest.cc:53: ASSERT_EQ(a->Peek(), 0u); On 2013/09/17 18:57:00, awong wrote: > ASSERT_EQ just exists the current function. Is that the expected behavior? > > If so, please comment. Done.
Latest patchset seems corrupted. Could you reupload? On Tue, Sep 17, 2013 at 12:00 PM, <dalecurtis@chromium.org> wrote: > > https://codereview.chromium.**org/23875019/diff/1/base/sync_** > socket_posix.cc<https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc> > File base/sync_socket_posix.cc (right): > > https://codereview.chromium.**org/23875019/diff/1/base/sync_** > socket_posix.cc#newcode118<https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc#newcode118> > base/sync_socket_posix.cc:118: DCHECK(timeout.InMicroseconds(**) < > Time::kMicrosecondsPerSecond); > On 2013/09/17 18:54:12, awong wrote: > >> nit: DCHECK_LT? >> > > Doesn't work for some reason, ends up with a link time error indicating > that kMicrosecondsPerSecond is not defined... Spent some not > insignificant time trying to resolve, but no luck. Ideas? > > > https://codereview.chromium.**org/23875019/diff/1/base/sync_** > socket_posix.cc#newcode126<https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc#newcode126> > base/sync_socket_posix.cc:126: DCHECK_GE(Peek(), length); > On 2013/09/17 18:54:12, awong wrote: > >> Why do we think Peek() can't be less than length? I thought select >> > could return > >> with fewer bytes than requested... >> > > Correct, I'll remove the condition. For its intended use case it seems > practically true since we're only sending 4 bytes around at a time. > > https://codereview.chromium.**org/23875019/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Whoops, reuploaded.
https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc#new... base/sync_socket_posix.cc:118: DCHECK(timeout.InMicroseconds() < Time::kMicrosecondsPerSecond); On 2013/09/17 19:00:25, DaleCurtis wrote: > On 2013/09/17 18:54:12, awong wrote: > > nit: DCHECK_LT? > > Doesn't work for some reason, ends up with a link time error indicating that > kMicrosecondsPerSecond is not defined... Spent some not insignificant time > trying to resolve, but no luck. Ideas? Oh right. It's cause there isn't a definition for kMicrosecondsPerSecond...just a declaration (which happens to have a constant-propagatable value). We *could* fix this, but why bother. https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc#new... base/sync_socket_posix.cc:126: DCHECK_GE(Peek(), length); On 2013/09/17 19:00:25, DaleCurtis wrote: > On 2013/09/17 18:54:12, awong wrote: > > Why do we think Peek() can't be less than length? I thought select could > return > > with fewer bytes than requested... > > Correct, I'll remove the condition. For its intended use case it seems > practically true since we're only sending 4 bytes around at a time. Do you want to read the amount from Peek() or the amount from length?
https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/1/base/sync_socket_posix.cc#new... base/sync_socket_posix.cc:126: DCHECK_GE(Peek(), length); On 2013/09/17 19:57:11, awong wrote: > On 2013/09/17 19:00:25, DaleCurtis wrote: > > On 2013/09/17 18:54:12, awong wrote: > > > Why do we think Peek() can't be less than length? I thought select could > > return > > > with fewer bytes than requested... > > > > Correct, I'll remove the condition. For its intended use case it seems > > practically true since we're only sending 4 bytes around at a time. > > Do you want to read the amount from Peek() or the amount from length? To be perfect this should be a while loop which runs a select() with a decreasing timeout and reads only Peek() bytes at a time. Doing this means adding Peek() and TimeTicks::Now() calls. Potentially it also breaks down into multiple Receive calls unnecessarily if the bytes available change between Peek() and Receive. What's written works correctly for our simple use case, but may block longer than expected for future use cases. I think the current code is sufficient, but am willing to polish to "perfect" if you prefer.
PTAL. This version only reads the amount returned by Peek() per our offline conversation. https://codereview.chromium.org/23875019/diff/25001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/25001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:123: TimeTicks start_time = base::TimeTicks::Now(); Is this worth doing? TimeTicks::Now() has a resolution of 1 to 15ms, yet the timeouts we'll be using are 1 to 20ms. https://codereview.chromium.org/23875019/diff/25001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:176: long flags = 0; While coding I began to wonder if this is necessary to do for every Send() call. Or should it be a part of the constructor and CreatePair function. tommi: WDYT?
https://codereview.chromium.org/23875019/diff/25001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/25001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:176: long flags = 0; On 2013/09/25 22:48:25, DaleCurtis wrote: > While coding I began to wonder if this is necessary to do for every Send() call. > Or should it be a part of the constructor and CreatePair function. tommi: > WDYT? you mean set the socket to non-blocking inside CreatePair? Sgtm... wonder why that wasn't done - do we ever accept sockets created outside of this class?
https://codereview.chromium.org/23875019/diff/25001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/25001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:176: long flags = 0; On 2013/09/29 09:15:54, tommi wrote: > On 2013/09/25 22:48:25, DaleCurtis wrote: > > While coding I began to wonder if this is necessary to do for every Send() > call. > > Or should it be a part of the constructor and CreatePair function. tommi: > > WDYT? > > you mean set the socket to non-blocking inside CreatePair? Sgtm... wonder why > that wasn't done - do we ever accept sockets created outside of this class? Not that I know of or can find. After this lands I'll create a follow up CL to move the async send settings to CreatePair().
Oh and hah, I found the sync socket unit tests... in ipc/ https://code.google.com/p/chromium/codesearch#chromium/src/ipc/sync_socket_un... Which does some multiprocess ipc testing which seems a bit overkill, but probably doesn't hurt. I think it's worth keeping the new tests I added for base/
Oh, sorry I should have told you that :-/ I think we shouldn't throw the multiprocess tests away just yet since the class is intented for that scenario (copying/sharing socket handles over etc), but +1 to keeping the new tests. On Mon, Sep 30, 2013 at 8:03 PM, <dalecurtis@chromium.org> wrote: > Oh and hah, I found the sync socket unit tests... in ipc/ > > https://code.google.com/p/**chromium/codesearch#chromium/** > src/ipc/sync_socket_unittest.**cc<https://code.google.com/p/chromium/codesearch#chromium/src/ipc/sync_socket_unittest.cc> > > Which does some multiprocess ipc testing which seems a bit overkill, but > probably doesn't hurt. I think it's worth keeping the new tests I added > for > base/ > > https://codereview.chromium.**org/23875019/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
awong is heading for vacation, so cc:ajwong, +thakis for OWNERS review.
I think willchan (posix) and jar (windows) are better reviewers for this than I. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:130: FD_SET(handle_, &rfds); You need to check that handle_ is < FD_SETSIZE, else this will clobber memory
https://codereview.chromium.org/23875019/diff/36001/base/sync_socket.h File base/sync_socket.h (right): https://codereview.chromium.org/23875019/diff/36001/base/sync_socket.h#newcode58 base/sync_socket.h:58: // length is the number of bytes of data to receive (must be non-zero). In retrospect, this API is a bit short-sighted. But since it seems not to cause any issues now, this is fine. Typically these APIs usually will have |length| refer to the size of the buffer, but partial reads are allowed. This is because low latency is important. Yet, if you want to reduce API entry costs (and possible context switches), you can add a ReadUntil() API that will loop within the implementation and return only when enough has been read. That's really what should be happening here. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:114: TimeDelta timeout) { Please use ThreadRestrictions::AssertIOAllowed(); All these functions really should be using this. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:123: TimeTicks start_time = base::TimeTicks::Now(); drop the base:: https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:125: fd_set rfds; Please choose a more descriptive name. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:134: const int select_result = HANDLE_EINTR( I think there's a bug here, right? HANDLE_EINTR() is going to bail out each time there's an interrupt, which is not going to update timeout_struct. Can you write a test for this? You can probably use a separate thread and tkill() this thread to deliver the EINTR. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:140: // must Peek() for the amount ready for reading to avoid blocking. Why don't you just use read() instead and do the loop here rather than in Receive()'s file_util::ReadFromFD()? https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:149: (timeout -= (base::TimeTicks::Now() - start_time)) > drop the base:: https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_unittest.cc File base/sync_socket_unittest.cc (right): https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_unittest... base/sync_socket_unittest.cc:27: ASSERT_EQ(socket_->Peek(), 0u); ASSERT_EQ(expected, actual), you've got them reversed as (actual, expected). This is important for the gTest macro messages.
(Just comments) https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:114: TimeDelta timeout) { On 2013/10/04 21:01:35, willchan wrote: > Please use ThreadRestrictions::AssertIOAllowed(); > > All these functions really should be using this. I'll check if that can be done. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:134: const int select_result = HANDLE_EINTR( On 2013/10/04 21:01:35, willchan wrote: > I think there's a bug here, right? HANDLE_EINTR() is going to bail out each time > there's an interrupt, which is not going to update timeout_struct. > > Can you write a test for this? You can probably use a separate thread and > tkill() this thread to deliver the EINTR. select() will update the timeout struct on EINTR: http://linux.die.net/man/2/select https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:140: // must Peek() for the amount ready for reading to avoid blocking. On 2013/10/04 21:01:35, willchan wrote: > Why don't you just use read() instead and do the loop here rather than in > Receive()'s file_util::ReadFromFD()? Code reuse? What's the benefit of reimplementing the EINTR read loop? https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_unittest.cc File base/sync_socket_unittest.cc (right): https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_unittest... base/sync_socket_unittest.cc:27: ASSERT_EQ(socket_->Peek(), 0u); On 2013/10/04 21:01:35, willchan wrote: > ASSERT_EQ(expected, actual), you've got them reversed as (actual, expected). > This is important for the gTest macro messages. If you're talking about the message format, that isn't true for ASSERT, only EXPECT. Assert will say "x vs y" where as EXPECT says "expected y, got x"
This patch set also fixes a number of style nits and potential bugs that we never hit since we're only sending / receiving 4 bytes at a time. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:114: TimeDelta timeout) { On 2013/10/04 21:08:39, DaleCurtis wrote: > On 2013/10/04 21:01:35, willchan wrote: > > Please use ThreadRestrictions::AssertIOAllowed(); > > > > All these functions really should be using this. > > I'll check if that can be done. Done. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:123: TimeTicks start_time = base::TimeTicks::Now(); On 2013/10/04 21:01:35, willchan wrote: > drop the base:: Done. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:125: fd_set rfds; On 2013/10/04 21:01:35, willchan wrote: > Please choose a more descriptive name. Done. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:130: FD_SET(handle_, &rfds); On 2013/10/04 05:35:12, Nico wrote: > You need to check that handle_ is < FD_SETSIZE, else this will clobber memory Done. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:149: (timeout -= (base::TimeTicks::Now() - start_time)) > On 2013/10/04 21:01:35, willchan wrote: > drop the base:: Done.
willchan: ping?
Part-time + conference => very little review bandwidth. I'll try to wrap this up today though. Sorry for the delays :( On Tue, Oct 8, 2013 at 11:09 AM, <dalecurtis@chromium.org> wrote: > willchan: ping? > > https://codereview.chromium.**org/23875019/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm going to hand off the approval to jar@ since he needs to review the windows code anyway. But other than the comments I've already listed, this CL seems fine to me. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:134: const int select_result = HANDLE_EINTR( On 2013/10/04 21:08:39, DaleCurtis wrote: > On 2013/10/04 21:01:35, willchan wrote: > > I think there's a bug here, right? HANDLE_EINTR() is going to bail out each > time > > there's an interrupt, which is not going to update timeout_struct. > > > > Can you write a test for this? You can probably use a separate thread and > > tkill() this thread to deliver the EINTR. > > select() will update the timeout struct on EINTR: > > http://linux.die.net/man/2/select How about the non-linux posix platforms? What happens on OS X? https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:140: // must Peek() for the amount ready for reading to avoid blocking. On 2013/10/04 21:08:39, DaleCurtis wrote: > On 2013/10/04 21:01:35, willchan wrote: > > Why don't you just use read() instead and do the loop here rather than in > > Receive()'s file_util::ReadFromFD()? > > Code reuse? What's the benefit of reimplementing the EINTR read loop? Sorry, I was hung up on disliking the API that always reads the full buffer length rather than returning ASAP. This is fine as is. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_unittest.cc File base/sync_socket_unittest.cc (right): https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_unittest... base/sync_socket_unittest.cc:27: ASSERT_EQ(socket_->Peek(), 0u); On 2013/10/04 21:08:39, DaleCurtis wrote: > On 2013/10/04 21:01:35, willchan wrote: > > ASSERT_EQ(expected, actual), you've got them reversed as (actual, expected). > > This is important for the gTest macro messages. > > If you're talking about the message format, that isn't true for ASSERT, only > EXPECT. Assert will say "x vs y" where as EXPECT says "expected y, got x" Oh, news to me :) But I think we should still make this change for consistency (no one expects assertions and expectations to vary in this manner). And it's what gTest docs themselves recommend: https://code.google.com/p/googletest/wiki/Primer#Binary_Comparison.
Thanks for the review. Patch updated. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:134: const int select_result = HANDLE_EINTR( On 2013/10/09 20:06:52, willchan wrote: > On 2013/10/04 21:08:39, DaleCurtis wrote: > > On 2013/10/04 21:01:35, willchan wrote: > > > I think there's a bug here, right? HANDLE_EINTR() is going to bail out each > > time > > > there's an interrupt, which is not going to update timeout_struct. > > > > > > Can you write a test for this? You can probably use a separate thread and > > > tkill() this thread to deliver the EINTR. > > > > select() will update the timeout struct on EINTR: > > > > http://linux.die.net/man/2/select > > How about the non-linux posix platforms? What happens on OS X? Man page is sadly conflicting, saying both that it's not updated and not to rely on it remaining static. After digging through the darwin open source code, it looks like all usages end up reinitializing the timeout value on EINTR, so I'll break out the EINTR handling. https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_unittest.cc File base/sync_socket_unittest.cc (right): https://codereview.chromium.org/23875019/diff/36001/base/sync_socket_unittest... base/sync_socket_unittest.cc:27: ASSERT_EQ(socket_->Peek(), 0u); On 2013/10/09 20:06:52, willchan wrote: > On 2013/10/04 21:08:39, DaleCurtis wrote: > > On 2013/10/04 21:01:35, willchan wrote: > > > ASSERT_EQ(expected, actual), you've got them reversed as (actual, expected). > > > This is important for the gTest macro messages. > > > > If you're talking about the message format, that isn't true for ASSERT, only > > EXPECT. Assert will say "x vs y" where as EXPECT says "expected y, got x" > > Oh, news to me :) But I think we should still make this change for consistency > (no one expects assertions and expectations to vary in this manner). And it's > what gTest docs themselves recommend: > https://code.google.com/p/googletest/wiki/Primer#Binary_Comparison. In media, we decided the "no yoda conditions" style rule supersedes for ASSERT_EQ. However, base/ style seems the opposite, so I've changed to expected, actual for consistency.
I probably need to re-read this more carefully, but the loop constraints for the timeout seem broken on both posix and Windows. https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:86: return true; Why has this changed to false? https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:125: DCHECK(timeout.InMicroseconds() < Time::kMicrosecondsPerSecond); nit: Much easier to read is something like: timeout < TimeDelta::FromSeconds(1) The constants were really meant for internal use, and you should let that class handle all such arithmetic whenever possible. I don't know if it would work, but best would be: DCHECK_LT(timeout, TimeDelta::FromSeconds(1)); https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:162: (timeout -= (TimeTicks::Now() - start_time)) > TimeDelta()); This looks wrong. It appears that you're decrementing |timeout| repeatedly. If you cycle through this loop a few times, you can easily exhaust |timeout|, without spending a full |timeout| period. I think the correct test to continue is: TimeTicks::Now() - start_time < timeout https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_unittest.cc File base/sync_socket_unittest.cc (right): https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_unittest... base/sync_socket_unittest.cc:49: void SendReceivePeek(base::SyncSocket* a, base::SyncSocket* b) { nit: avoid one letter variable names througout file. Better might be: socket1 socket2 or socket_a socket_b (the latter is used in other files). https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_win.cc File base/sync_socket_win.cc (right): https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_win.cc#n... base/sync_socket_win.cc:178: timeout_in_ms -= (TimeTicks::Now() - start_time).InMilliseconds(); This also looks wrong. Here again, timeout_in_ms is decremented with much too often with much too large a value, which means you commonly won't wait the requested time (at least when you cycle through this loop a few times). Also, you shouldn't keep changing the timeout_in_ms value (which might also risk hitting INFINITE by accident). The while loop should just be: while (count < length && (timeout_in_ms == INFINITE || (TimeTicks::Now() - start_time).InMilliseconds() < timeout_in_ms))
(no updates, just comments) https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:86: return true; On 2013/10/10 02:01:28, jar wrote: > Why has this changed to false? CancelableSyncSocket::Shutdown() will result in the handle being closed and set to kInvalidHandle. As such I thought it made sense for Close() to not fail if called after Shutdown(). https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:162: (timeout -= (TimeTicks::Now() - start_time)) > TimeDelta()); On 2013/10/10 02:01:28, jar wrote: > This looks wrong. > > It appears that you're decrementing |timeout| repeatedly. If you cycle through > this loop a few times, you can easily exhaust |timeout|, without spending a full > |timeout| period. > > I think the correct test to continue is: > > TimeTicks::Now() - start_time < timeout Can you elaborate? We need to update the timeout value we give to select() so that the time spent both in select() and the loop doesn't exceed timeout. Are you proposing a different update mechanism than repeated subtraction of the delta? https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_win.cc File base/sync_socket_win.cc (right): https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_win.cc#n... base/sync_socket_win.cc:178: timeout_in_ms -= (TimeTicks::Now() - start_time).InMilliseconds(); On 2013/10/10 02:01:28, jar wrote: > This also looks wrong. > > Here again, timeout_in_ms is decremented with much too often with much too large > a value, which means you commonly won't wait the requested time (at least when > you cycle through this loop a few times). > > Also, you shouldn't keep changing the timeout_in_ms value (which might also risk > hitting INFINITE by accident). > > The while loop should just be: > while (count < length && (timeout_in_ms == INFINITE || > (TimeTicks::Now() - start_time).InMilliseconds() < timeout_in_ms)) Same question as the POSIX variant.
https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:162: (timeout -= (TimeTicks::Now() - start_time)) > TimeDelta()); On 2013/10/10 17:37:34, DaleCurtis wrote: > On 2013/10/10 02:01:28, jar wrote: > > This looks wrong. > > > > It appears that you're decrementing |timeout| repeatedly. If you cycle > through > > this loop a few times, you can easily exhaust |timeout|, without spending a > full > > |timeout| period. > > > > I think the correct test to continue is: > > > > TimeTicks::Now() - start_time < timeout > > Can you elaborate? We need to update the timeout value we give to select() so > that the time spent both in select() and the loop doesn't exceed timeout. Are > you proposing a different update mechanism than repeated subtraction of the > delta? The following is an explanation of how your code appears to go wrong. Example: You are asked to get all the data you can for the next 500ms (i.e., |timeout| == 500ms). After only waiting for 50ms, select() returns and indicates that data is ready. You read the data. You realize that 50ms (Now() - start_time) has passed since being called, and you reduce |timeout| to 450ms. Things are looking good as you loop back to line 133. You enter select with a delay of 450ms to wait, which is dead on. Select returns with more data ready after only 10ms. You are now 60ms into your attempted wait of "no more than 500ms." You read the data... and now the bug surfaces... Line 162 calculates that you are indeed 60ms into your wait, and further reduces |timeout| by 60ms <yikes!?!>. The value of timeout is now a mere 390ms, as it was reduced by 50ms the first time through line 162, and then by 60ms this time. Sadly, we have only waited a total of 60ms since we were called. We *should* next call select() with a full 440ms remaining, but we instead call it with a mere 390ms. :-( Another few passes like this (no more than 10?) and we'll diminish |timeout| to zero (and return), even though we won't have spent much time in this function :-(. To say it another way: You shouldn't diminish |timeout| by the duration you have run since start_time each pass through the loop. You should reset |timeout| to the remnant duration. Perhaps you were assuming that you reset |start_time| every pass in the loop. Please note that it is never updated (that I could find). As a matter of style, pushing a "-=" assignment into the already complex conditional of a while loop adds a lot of readability issues, and may be a part of why this code caused confusion (and made it harder to see the error). To get rid of this, you should probably (in this case) avoid using "continue" on line 143, and instead use an indented block to hold the code from lines 144 to 160. My original comment focused on the error in the while loop condition shown above. The following is the skeletal code that you could use to have a correct value of |timeout| to feed to the timeout_struct on line 138 (and still make a minimal number of calls to Now()). TimeTicks finish_time = TimeTicks::Now() + timeout; // new line 129 ... do { // Current line 133. if (timeout < TimeDelta()) break; ... etc. Lines 134 onwards } // Close new block around lines 144-160. if (bytes_read_total >= length) break; timeout = finish_time - TimeTicks::Now(); } while (1);
Thanks for your review. Patch updated. https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:121: DCHECK_LT(handle_, FD_SETSIZE); I made this a DCHECK(), but I'm wondering if it should be a CHECK or handled error instead. https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:125: DCHECK(timeout.InMicroseconds() < Time::kMicrosecondsPerSecond); On 2013/10/10 02:01:28, jar wrote: > nit: Much easier to read is something like: > > timeout < TimeDelta::FromSeconds(1) > > The constants were really meant for internal use, and you should let that class > handle all such arithmetic whenever possible. > > I don't know if it would work, but best would be: > DCHECK_LT(timeout, TimeDelta::FromSeconds(1)); I like the second one, but it requires adding an ostream<< operator to TimeDelta a la: #include <ostream> ... inline std::ostream& operator<<(std::ostream& os, const TimeDelta& delta) { os << delta.InMicroseconds() << "μs"; return os; } This was raised back in '09 and saw some disagreement apparently. Time to reconsider? https://groups.google.com/forum/#!topic/chromium-dev/swpKeYfcCOc https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:157: Receive(charbuffer + bytes_read_total, bytes_to_read); I just realized that Receive might hit and handle an EINTR internally. Since we should be guaranteed not to block, it seems like this is okay in terms of the timeout though. What do you think? https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:162: (timeout -= (TimeTicks::Now() - start_time)) > TimeDelta()); On 2013/10/11 02:36:12, jar wrote: > On 2013/10/10 17:37:34, DaleCurtis wrote: > > On 2013/10/10 02:01:28, jar wrote: > > > This looks wrong. > > > > > > It appears that you're decrementing |timeout| repeatedly. If you cycle > > through > > > this loop a few times, you can easily exhaust |timeout|, without spending a > > full > > > |timeout| period. > > > > > > I think the correct test to continue is: > > > > > > TimeTicks::Now() - start_time < timeout > > > > Can you elaborate? We need to update the timeout value we give to select() so > > that the time spent both in select() and the loop doesn't exceed timeout. Are > > you proposing a different update mechanism than repeated subtraction of the > > delta? > > The following is an explanation of how your code appears to go wrong. > > Example: You are asked to get all the data you can for the next 500ms (i.e., > |timeout| == 500ms). > > After only waiting for 50ms, select() returns and indicates that data is ready. > You read the data. > You realize that 50ms (Now() - start_time) has passed since being called, and > you reduce |timeout| to 450ms. > Things are looking good as you loop back to line 133. > You enter select with a delay of 450ms to wait, which is dead on. > Select returns with more data ready after only 10ms. > You are now 60ms into your attempted wait of "no more than 500ms." > You read the data... and now the bug surfaces... > Line 162 calculates that you are indeed 60ms into your wait, and further reduces > |timeout| by 60ms <yikes!?!>. > The value of timeout is now a mere 390ms, as it was reduced by 50ms the first > time through line 162, and then by 60ms this time. > Sadly, we have only waited a total of 60ms since we were called. > > We *should* next call select() with a full 440ms remaining, but we instead call > it with a mere 390ms. :-( > > Another few passes like this (no more than 10?) and we'll diminish |timeout| to > zero (and return), even though we won't have spent much time in this function > :-(. > > To say it another way: You shouldn't diminish |timeout| by the duration you have > run since start_time each pass through the loop. You should reset |timeout| to > the remnant duration. > > Perhaps you were assuming that you reset |start_time| every pass in the loop. > Please note that it is never updated (that I could find). > > As a matter of style, pushing a "-=" assignment into the already complex > conditional of a while loop adds a lot of readability issues, and may be a part > of why this code caused confusion (and made it harder to see the error). To get > rid of this, you should probably (in this case) avoid using "continue" on line > 143, and instead use an indented block to hold the code from lines 144 to 160. > > My original comment focused on the error in the while loop condition shown > above. The following is the skeletal code that you could use to have a correct > value of |timeout| to feed to the timeout_struct on line 138 (and still make a > minimal number of calls to Now()). > > TimeTicks finish_time = TimeTicks::Now() + timeout; // new line 129 > ... > do { // Current line 133. > if (timeout < TimeDelta()) > break; > > ... etc. Lines 134 onwards > > } // Close new block around lines 144-160. > if (bytes_read_total >= length) > break; > timeout = finish_time - TimeTicks::Now(); > } while (1); > > Ack, I shouldn't have missed that. :( Thanks for your patient and detailed explanation. I started writing up your suggestion, but didn't like the extra conditionals, indentation, and while(1) it introduced. I've switched it over to a for() loop which allows the EINTR and exit conditionals to be simplified, while using a construct more appropriate for containing assignments. https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_unittest.cc File base/sync_socket_unittest.cc (right): https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_unittest... base/sync_socket_unittest.cc:49: void SendReceivePeek(base::SyncSocket* a, base::SyncSocket* b) { On 2013/10/10 02:01:28, jar wrote: > nit: avoid one letter variable names througout file. > > Better might be: > > socket1 > socket2 > > or > socket_a > socket_b > > (the latter is used in other files). > Done. https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_win.cc File base/sync_socket_win.cc (right): https://codereview.chromium.org/23875019/diff/69001/base/sync_socket_win.cc#n... base/sync_socket_win.cc:178: timeout_in_ms -= (TimeTicks::Now() - start_time).InMilliseconds(); On 2013/10/10 17:37:34, DaleCurtis wrote: > On 2013/10/10 02:01:28, jar wrote: > > This also looks wrong. > > > > Here again, timeout_in_ms is decremented with much too often with much too > large > > a value, which means you commonly won't wait the requested time (at least when > > you cycle through this loop a few times). > > > > Also, you shouldn't keep changing the timeout_in_ms value (which might also > risk > > hitting INFINITE by accident). > > > > The while loop should just be: > > while (count < length && (timeout_in_ms == INFINITE || > > (TimeTicks::Now() - start_time).InMilliseconds() < timeout_in_ms)) > > Same question as the POSIX variant. I'm now tracking a current_time value to avoid updated timeout_in_ms. Thanks for the suggestion.
https://codereview.chromium.org/23875019/diff/83001/base/sync_socket.h File base/sync_socket.h (right): https://codereview.chromium.org/23875019/diff/83001/base/sync_socket.h#newcode62 base/sync_socket.h:62: // Same as Receive() but only blocks for data until |timeout| has elapsed. nit: "... or |buffer| |length| is exhausted." https://codereview.chromium.org/23875019/diff/83001/base/sync_socket.h#newcode64 base/sync_socket.h:64: // return the amount of data read prior to timeout. nit: Remove leading words "Timeout will" and trailing words "prior to timeout." It should just be: Returns the amount of data read. https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_nacl.cc File base/sync_socket_nacl.cc (right): https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_nacl.cc#... base/sync_socket_nacl.cc:49: return -1; Shouldn't this be return 0? https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:134: for (bytes_read_total = 0; bytes_read_total < length && timeout > TimeDelta(); nit: This seems close to safe... but I'm wary that with TimeDelta's underlying resolution of microseconds you could have: timeout > TimeDelta() while timeout.Microseconds() == 0 Note that today, TimeTicks on at least Windows is in milliseconds... but that might not always be so true (no need to code that dependency here). Are you fine with calling select with a 0 timeout? https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:140: struct timeval timeout_struct = { 0, timeout.InMicroseconds() }; nit: (really just a comment) I'm wary that with TimeDelta's underlying resolution of microseconds you could have: timeout > TimeDelta() // So the loop runs. while timeout.Microseconds() == 0 select() can deal with this nicely, but will return immediately. https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:161: if (bytes_received != bytes_to_read) When does this happen?
PTAL. https://codereview.chromium.org/23875019/diff/83001/base/sync_socket.h File base/sync_socket.h (right): https://codereview.chromium.org/23875019/diff/83001/base/sync_socket.h#newcode62 base/sync_socket.h:62: // Same as Receive() but only blocks for data until |timeout| has elapsed. On 2013/10/14 15:31:07, jar wrote: > nit: "... or |buffer| |length| is exhausted." Done. https://codereview.chromium.org/23875019/diff/83001/base/sync_socket.h#newcode64 base/sync_socket.h:64: // return the amount of data read prior to timeout. On 2013/10/14 15:31:07, jar wrote: > nit: Remove leading words "Timeout will" and trailing words "prior to timeout." > It should just be: > > Returns the amount of data read. Done. https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_nacl.cc File base/sync_socket_nacl.cc (right): https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_nacl.cc#... base/sync_socket_nacl.cc:49: return -1; On 2013/10/14 15:31:07, jar wrote: > Shouldn't this be return 0? I just copied the pattern, but probably all of these should return 0 since they're size_t and also call NOTIMPLEMENTED(). I've changed them all. https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:134: for (bytes_read_total = 0; bytes_read_total < length && timeout > TimeDelta(); On 2013/10/14 15:31:07, jar wrote: > nit: This seems close to safe... but I'm wary that with TimeDelta's underlying > resolution of microseconds you could have: > > timeout > TimeDelta() > > while > > timeout.Microseconds() == 0 > > Note that today, TimeTicks on at least Windows is in milliseconds... but that > might not always be so true (no need to code that dependency here). > > Are you fine with calling select with a 0 timeout? Should be fine, but I'll change it to .InMicroseconds() > 0 for clarity. https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:140: struct timeval timeout_struct = { 0, timeout.InMicroseconds() }; On 2013/10/14 15:31:07, jar wrote: > nit: (really just a comment) I'm wary that with TimeDelta's underlying > resolution of microseconds you could have: > > timeout > TimeDelta() // So the loop runs. > > while > > timeout.Microseconds() == 0 > > select() can deal with this nicely, but will return immediately. Done. https://codereview.chromium.org/23875019/diff/83001/base/sync_socket_posix.cc... base/sync_socket_posix.cc:161: if (bytes_received != bytes_to_read) On 2013/10/14 15:31:07, jar wrote: > When does this happen? If the read() call underlying file_util::ReadFromFD() fails it'll return false and Receive() will return zero.
https://codereview.chromium.org/23875019/diff/98001/base/sync_socket_nacl.cc File base/sync_socket_nacl.cc (right): https://codereview.chromium.org/23875019/diff/98001/base/sync_socket_nacl.cc#... base/sync_socket_nacl.cc:51: return 0; When I argued for returning 0 (not -1), I was more sure about your function.. which didn't define an error return value. It seems compliant to return 0, since you "read nothing" (during this notably poor, in fact unimplemented, method). For the other methods, they *might* have error semantics, and hence they *might* have a return value of -1 considered as meaningful. As a result, unless you investigate, I'd restrict this change to your function.
https://codereview.chromium.org/23875019/diff/98001/base/sync_socket_nacl.cc File base/sync_socket_nacl.cc (right): https://codereview.chromium.org/23875019/diff/98001/base/sync_socket_nacl.cc#... base/sync_socket_nacl.cc:51: return 0; On 2013/10/14 22:49:18, jar wrote: > When I argued for returning 0 (not -1), I was more sure about your function.. > which didn't define an error return value. > > It seems compliant to return 0, since you "read nothing" (during this notably > poor, in fact unimplemented, method). > > For the other methods, they *might* have error semantics, and hence they *might* > have a return value of -1 considered as meaningful. > > As a result, unless you investigate, I'd restrict this change to your function. Only Receive(), Shutdown() are currently called by untrusted code: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/audio_... https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/... I think there's actually a bug since Close() is never called from the untrusted side. I'll actually be adding a Send() implementation in a follow up patch set shortly. All of this work on SyncSocket is to facilitate this audio change: https://codereview.chromium.org/22886005/ If you prefer, I can save these changes for that patch set and just return 0 for the new method in this one.
On 2013/10/14 23:12:33, DaleCurtis wrote: > https://codereview.chromium.org/23875019/diff/98001/base/sync_socket_nacl.cc > File base/sync_socket_nacl.cc (right): > > https://codereview.chromium.org/23875019/diff/98001/base/sync_socket_nacl.cc#... > base/sync_socket_nacl.cc:51: return 0; > On 2013/10/14 22:49:18, jar wrote: > > When I argued for returning 0 (not -1), I was more sure about your function.. > > which didn't define an error return value. > > > > It seems compliant to return 0, since you "read nothing" (during this notably > > poor, in fact unimplemented, method). > > > > For the other methods, they *might* have error semantics, and hence they > *might* > > have a return value of -1 considered as meaningful. > > > > As a result, unless you investigate, I'd restrict this change to your > function. > > Only Receive(), Shutdown() are currently called by untrusted code: > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/audio_... > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/... > > I think there's actually a bug since Close() is never called from the untrusted > side. > > I'll actually be adding a Send() implementation in a follow up patch set > shortly. All of this work on SyncSocket is to facilitate this audio change: > https://codereview.chromium.org/22886005/ > > If you prefer, I can save these changes for that patch set and just return 0 for > the new method in this one. Just the new method returning 0 SGTM
Done.
jar: Any other comments for this CL? The NaCl changes depend on this.
LGTM % nit https://codereview.chromium.org/23875019/diff/113001/base/sync_socket_win.cc File base/sync_socket_win.cc (right): https://codereview.chromium.org/23875019/diff/113001/base/sync_socket_win.cc#... base/sync_socket_win.cc:114: size_t CancelableFileOperation(Function operation, HANDLE file, nit: Since you're cleaning this.... one arg per line in declations and definitions.
https://codereview.chromium.org/23875019/diff/113001/base/sync_socket_win.cc File base/sync_socket_win.cc (right): https://codereview.chromium.org/23875019/diff/113001/base/sync_socket_win.cc#... base/sync_socket_win.cc:114: size_t CancelableFileOperation(Function operation, HANDLE file, On 2013/10/15 21:14:00, jar wrote: > nit: Since you're cleaning this.... one arg per line in declations and > definitions. Done.
Thanks for the review!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/23875019/118001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_aosp. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
The ThreadRestrictions::AssertIORequired() changes require this bug fix to land first: https://codereview.chromium.org/26687007/
Actually, a small change was necessary to keep the AssertIOAllowed calls. I've moved the SyncSocket::Send() code into a helper function used by SyncSocket::Send() (which calls AssertIOAllowed) and CancellableSyncSocket::Send() (which is non-blocking, so it does not AssertIOAllowed). On OSX, we must use the main UI thread to execute CoreAudio calls instead of our normal audio worker thread. In a couple spots in AudioOutputController we end up issuing a non-blocking CancellableSyncSocket::Send() from the UI thread: https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... It's non-trivial to pass these tasks to the worker thread and maintain thread safety across AudioSyncReader and AudioOutputController, so I hope my workaround is sufficient. PTAL.
Patch set 14 still LGTM. One small thought below, that you can take or leave. https://codereview.chromium.org/23875019/diff/140007/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/140007/base/sync_socket_posix.c... base/sync_socket_posix.cc:143: bytes_read_total < length && timeout.InMicroseconds() > 0; nit: I don't know if this is time sensitive code (and I don't know how often it has to cycle around and chew cycles)... but... InMicroseconds() is a 64bit divide, which on Windows is done via a library call, and is slow. (I recall being surprised when I single stepped through assembly code a few years ago). I don't know how this plays on Linux. If you tracked timeout_ms as a separate variable (declare on line 139, update on line 144), then you wouldn't have to do this conversion twice per iteration (second time is on line 150). Clarity seems very comparable.. code a tad shorter.... YMMV. Your call.
https://codereview.chromium.org/23875019/diff/140007/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/140007/base/sync_socket_posix.c... base/sync_socket_posix.cc:143: bytes_read_total < length && timeout.InMicroseconds() > 0; On 2013/10/18 22:02:52, jar wrote: > nit: I don't know if this is time sensitive code (and I don't know how often it > has to cycle around and chew cycles)... but... > > InMicroseconds() is a 64bit divide, which on Windows is done via a library call, > and is slow. (I recall being surprised when I single stepped through assembly > code a few years ago). I don't know how this plays on Linux. > > If you tracked timeout_ms as a separate variable (declare on line 139, update on > line 144), then you wouldn't have to do this conversion twice per iteration > (second time is on line 150). > > Clarity seems very comparable.. code a tad shorter.... YMMV. > > Your call. I don't see where it maps to a divide? https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.cc&...
LGTM https://codereview.chromium.org/23875019/diff/140007/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://codereview.chromium.org/23875019/diff/140007/base/sync_socket_posix.c... base/sync_socket_posix.cc:143: bytes_read_total < length && timeout.InMicroseconds() > 0; On 2013/10/18 22:10:50, DaleCurtis wrote: > On 2013/10/18 22:02:52, jar wrote: > > nit: I don't know if this is time sensitive code (and I don't know how often > it > > has to cycle around and chew cycles)... but... > > > > InMicroseconds() is a 64bit divide, which on Windows is done via a library > call, > > and is slow. (I recall being surprised when I single stepped through assembly > > code a few years ago). I don't know how this plays on Linux. > > > > If you tracked timeout_ms as a separate variable (declare on line 139, update > on > > line 144), then you wouldn't have to do this conversion twice per iteration > > (second time is on line 150). > > > > Clarity seems very comparable.. code a tad shorter.... YMMV. > > > > Your call. > > I don't see where it maps to a divide? > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.cc&... <DOH!> Nevermind. I'm so used to looking at ToMilliseconds(). As you noted, ToMicroseconds() uses the internal format, does no math (beyond the cast you need to discard high-order bits... which is NBD). Nevermind. Sorry.
Thanks again for the reviews!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/23875019/140007
Retried try job too often on win_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/23875019/140007
Message was sent while issue was closed.
Change committed as 229621 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
