Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(341)

Issue 122010043: SOCIS2014 - Tcp variants (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 3 months ago by n.p
Modified:
6 years ago
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

SOCIS2014 - Tcp variants This patchset includes: - Tcp Cubic - Tcp BIC - Tcp Highspeed - Tcp Hybla - Tcp Noordwijk Commits: https://github.com/kronat/ns-3-dev-git/tree/tcp-versions-for-socis

Patch Set 1 : Initial patch set #

Total comments: 12

Patch Set 2 : Updated patch set wrt comments #

Total comments: 42

Patch Set 3 : Final version (from paper accepted to WNS3) #

Patch Set 4 : Update minor things on NS_LOG_FUNCTION and added missing documentation #

Patch Set 5 : Added example in tcp-variants-comparison #

Patch Set 6 : Rebased patch against master #

Patch Set 7 : Corrected typo on TcpHybla #

Total comments: 15

Patch Set 8 : BIC updated wrt Tom review #

Total comments: 9

Patch Set 9 : Fixing private/protected membership and fixed documentation #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3488 lines, -26 lines) Patch
M examples/tcp/tcp-variants-comparison.cc View 1 2 3 4 5 6 7 8 3 chunks +31 lines, -25 lines 0 comments Download
A src/internet/model/tcp-bic.h View 1 2 3 4 5 6 7 8 1 chunk +166 lines, -0 lines 0 comments Download
A src/internet/model/tcp-bic.cc View 1 2 3 4 5 6 7 8 1 chunk +402 lines, -0 lines 0 comments Download
A src/internet/model/tcp-cubic.h View 1 2 3 4 5 6 7 8 1 chunk +215 lines, -0 lines 0 comments Download
A src/internet/model/tcp-cubic.cc View 1 2 3 4 5 6 7 8 1 chunk +545 lines, -0 lines 0 comments Download
A src/internet/model/tcp-highspeed.h View 1 2 3 4 5 6 7 8 1 chunk +103 lines, -0 lines 0 comments Download
A src/internet/model/tcp-highspeed.cc View 1 2 3 4 5 6 7 8 1 chunk +786 lines, -0 lines 0 comments Download
A src/internet/model/tcp-hybla.h View 1 2 3 4 5 6 7 8 1 chunk +74 lines, -0 lines 0 comments Download
A src/internet/model/tcp-hybla.cc View 1 2 3 4 5 6 7 8 1 chunk +204 lines, -0 lines 0 comments Download
M src/internet/model/tcp-newreno.h View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
A src/internet/model/tcp-noordwijk.h View 1 2 3 4 5 6 1 chunk +137 lines, -0 lines 0 comments Download
A src/internet/model/tcp-noordwijk.cc View 1 2 3 4 5 6 1 chunk +815 lines, -0 lines 0 comments Download
M src/internet/wscript View 1 2 7 2 chunks +10 lines, -0 lines 0 comments Download

Messages

Total messages: 10
Tommaso Pecorella
1st block of comments. Noordwijk not checked. https://codereview.appspot.com/122010043/diff/1/src/internet/model/tcp-cubic.cc File src/internet/model/tcp-cubic.cc (right): https://codereview.appspot.com/122010043/diff/1/src/internet/model/tcp-cubic.cc#newcode46 src/internet/model/tcp-cubic.cc:46: .AddAttribute ("HyStart", ...
7 years, 3 months ago (2014-08-12 19:20:05 UTC) #1
n.p
Updated patchset https://codereview.appspot.com/122010043/diff/1/src/internet/model/tcp-highspeed.h File src/internet/model/tcp-highspeed.h (right): https://codereview.appspot.com/122010043/diff/1/src/internet/model/tcp-highspeed.h#newcode64 src/internet/model/tcp-highspeed.h:64: float TableLookupB (uint32_t w); On 2014/08/12 19:20:04, ...
7 years, 3 months ago (2014-08-26 12:44:44 UTC) #2
Tom Henderson
This code looks good (only minor comments). However, there are no tests and examples provided. ...
6 years, 10 months ago (2015-01-05 15:36:18 UTC) #3
n.p
Updated code wrt the paper submitted to Wns3.
6 years, 8 months ago (2015-03-24 10:39:18 UTC) #4
Tom Henderson
On 2015/03/24 10:39:18, n.p wrote: > Updated code wrt the paper submitted to Wns3. Are ...
6 years, 8 months ago (2015-03-24 17:09:23 UTC) #5
n.p
On 2015/03/24 17:09:23, Tom Henderson wrote: > On 2015/03/24 10:39:18, n.p wrote: > > Updated ...
6 years, 8 months ago (2015-03-24 18:29:20 UTC) #6
Tom Henderson
comments on BIC variant only. Please add ns-3-reviews@googlegroups.com as a cc' on this codereview issue. ...
6 years, 5 months ago (2015-06-08 22:31:51 UTC) #7
n.p
On 2015/06/08 22:31:51, Tom Henderson wrote: > comments on BIC variant only. > > Please ...
6 years, 5 months ago (2015-06-09 16:22:05 UTC) #8
Tom Henderson
On 2015/06/09 16:22:05, n.p wrote: > > Reply of the questions above. Some parameters (like ...
6 years, 5 months ago (2015-06-09 17:57:57 UTC) #9
Tom Henderson
6 years, 5 months ago (2015-06-09 18:42:58 UTC) #10
I've gone through the rest of these files.  I would normally say "not ready for
commit" due to lack of tests, but given we have the GSoC program focused on
this, I will let this point slide.

In general, I recommend:

1) take a pass through all files to fix logging macros as suggested

2) For each new file, please run the check-style.py program on it.  Run as
"utils/check-style.py -i -l 3 -f src/internet/model/tcp-hybla.h"  (for example)

3) My understanding is that Noordwijk is being held back because it is not
ready, correct?

4) the only major issue to resolve, IMO, is the private/protected data and
methods.  I don't want to be pedantic about this issue, leading to tedious
implementation of dozens of setters and getters.  My main comment was that it
seemed that a lot of members and data were being made protected, but not all,
and I wondered what drove the decisions, and also whether too much was assumed
about the need to provide everything to future subclasses.

I also realize that some of the issues arise because of the legacy design of
these classes and use of inheritance instead of composition for congestion
control behavior.

So, what I propose that you do is as follows.  On a class by class basis, please
review what you have made protected and private (including data members). 
Include a general comment in each class that much of the internal implementation
is exposed to facilitate future subclassing.  Then, think about what reasonably
needs to be exposed as protected, and what would never need to be exposed (and
should be private)-- if anything -- and make any such changes.  I'm not going to
ask you for implementation of lots of setters and getters on data members if you
think that these state variables should be accessible.  Maybe you have already
done this type of review of your code, in which case you don't make any changes;
I'm just asking you to revisit in light of my comments, and I'll let you make
the call.

https://codereview.appspot.com/122010043/diff/120001/examples/tcp/tcp-variant...
File examples/tcp/tcp-variants-comparison.cc (right):

https://codereview.appspot.com/122010043/diff/120001/examples/tcp/tcp-variant...
examples/tcp/tcp-variants-comparison.cc:250: { // the default protocol type in
ns3::TcpWestwood is Bic
these comments about the default protocol type (there are four such new ones
proposed in this file) do not apply to protocols other than Westwood.  remove
these comments.

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
File src/internet/model/tcp-cubic.cc (right):

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
src/internet/model/tcp-cubic.cc:37: .AddConstructor<TcpCubic> ()
Please add ".SetGroupName ("Internet")"  in all of the TypeId methods of your
final patch.

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
src/internet/model/tcp-cubic.cc:104: NS_LOG_FUNCTION_NOARGS ();
use "NS_LOG_FUNCTION (this);"

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
src/internet/model/tcp-cubic.cc:180: NS_LOG_FUNCTION (this);
this << threshold

(I'll stop leaving such comments in the code review-- just do a pass through all
of your files for these).

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
File src/internet/model/tcp-cubic.h (right):

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
src/internet/model/tcp-cubic.h:123: };
(question also applies to BIC) Do you think that Cubic state might want to be
exposed as a traced value, for someone interested in tracking the state of the
cong. control machine?  If so, this enum could go public.

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
src/internet/model/tcp-cubic.h:181: void HystartUpdate (const Time& delay);
for API consistency, pass Time in as value (you do so in the next method)

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-h...
File src/internet/model/tcp-highspeed.cc (right):

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-h...
src/internet/model/tcp-highspeed.cc:69: NS_LOG_LOGIC ("TcpHighSpeed receieved
ACK for seq " << seq <<
received

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-h...
File src/internet/model/tcp-highspeed.h (right):

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-h...
src/internet/model/tcp-highspeed.h:77: * \brief Lookup table for a (from RFC
3649)
suggest:  'Lookup table for the coefficient a (from RFC 3649)'

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-n...
File src/internet/model/tcp-noordwijk.cc (right):

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-n...
src/internet/model/tcp-noordwijk.cc:35: // Assumption: defBurstWnd <= m_rWnd ,
so SETUP RIGHT Timer and InitialCwnd
please elaborate this comment

https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-n...
src/internet/model/tcp-noordwijk.cc:176: //                " m_bWnd: " <<
m_bWnd);
remove dead code
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b