PseudoTcp to expose settings of nagling and ACK delay

PseudoTcp to expose settings of nagling and ACK delay

Changes are as follows:
1. PseudoTcp to expose whether Nagle's algo is turned on.
2. PseudoTcp to expose setting of ACK delay.
3. JingleSession to disable Nagle's algo.
4. Set ACK delay to 10 milliseconds.
5. Unit tests for testing jingle session latency.

BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91057

[Sergey Ulanov, 2011-06-28 18:34:26]

http://codereview.chromium.org/7275024/diff/1/jingle/glue/pseudotcp_adapter.h
File jingle/glue/pseudotcp_adapter.h (right):

http://codereview.chromium.org/7275024/diff/1/jingle/glue/pseudotcp_adapter.h...
jingle/glue/pseudotcp_adapter.h:59: void SetNagling(bool nagling);
nit: SetNoDelay or SetNagleEnabled? I like SetNoDelay better because it is
consistent with sockets API.

http://codereview.chromium.org/7275024/diff/1/remoting/protocol/jingle_sessio...
File remoting/protocol/jingle_session_unittest.cc (right):

http://codereview.chromium.org/7275024/diff/1/remoting/protocol/jingle_sessio...
remoting/protocol/jingle_session_unittest.cc:699: TEST_F(JingleSessionTest,
TestLatency) {
Add comment that explain what this test verifies.
Doest this really need to be a unittest? Maybe make a separate benchmarking
tool?

[Alpha Left Google, 2011-06-28 18:47:39]

On 2011/06/28 18:34:26, sergeyu wrote:
> http://codereview.chromium.org/7275024/diff/1/jingle/glue/pseudotcp_adapter.h
> File jingle/glue/pseudotcp_adapter.h (right):
> 
>
http://codereview.chromium.org/7275024/diff/1/jingle/glue/pseudotcp_adapter.h...
> jingle/glue/pseudotcp_adapter.h:59: void SetNagling(bool nagling);
> nit: SetNoDelay or SetNagleEnabled? I like SetNoDelay better because it is
> consistent with sockets API.
> 
>
http://codereview.chromium.org/7275024/diff/1/remoting/protocol/jingle_sessio...
> File remoting/protocol/jingle_session_unittest.cc (right):
> 
>
http://codereview.chromium.org/7275024/diff/1/remoting/protocol/jingle_sessio...
> remoting/protocol/jingle_session_unittest.cc:699: TEST_F(JingleSessionTest,
> TestLatency) {
> Add comment that explain what this test verifies.
> Doest this really need to be a unittest? Maybe make a separate benchmarking
> tool?

Making it as a unit test make it easier to run and to catch regression. We could
still have a more comprehensive testing tool.

[Wez, 2011-06-28 19:27:17]

Drive by!

http://codereview.chromium.org/7275024/diff/5001/jingle/glue/pseudotcp_adapter.h
File jingle/glue/pseudotcp_adapter.h (right):

http://codereview.chromium.org/7275024/diff/5001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.h:59: void SetNoDelay(bool nagling);
These should use the names that PseudoTcp uses, unless StreamSocket has its own
names for them, which I don't think it does.

Alternatively, provide a function exposing PseudoTcp::SetOption() instead?

http://codereview.chromium.org/7275024/diff/5001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/7275024/diff/5001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:49: // and don't overwhelm the network with
ACK packets.
nit: It's chosen to balance the extra latency against the reduced load due to
ACK traffic.

http://codereview.chromium.org/7275024/diff/5001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_unittest.cc (right):

http://codereview.chromium.org/7275024/diff/5001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:454: return base::Time::Now() -
start_time_;
This isn't latency, it's elapsed time.

http://codereview.chromium.org/7275024/diff/5001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:702: TEST_F(JingleSessionTest,
TestLatency) {
This isn't testing latency, it's testing elapsed time (i.e. latency plus
throughput time for the given data size).

[Sergey Ulanov, 2011-06-28 23:18:39]

LGTM, if Wez's comments are addressed: you are measuring time it takes to send
block of data and it's not the same as latency. Please update comments/names to
make it clear that what you are measuring is not latency.