Bugzilla – Bug 2214
TCP ScheduleNow() for data transfer
Last modified: 2017-02-03 08:04:16 EST
Created attachment 2181 [details] change Schedule (TimeStep(1)) to ScheduleNow() This patch is carry over from bug 2111; it moves all Schedule(TimeStep(1)...) events to ScheduleNow (). All tests pass with this change. Was it strictly needed for bug 2067?
Hi, with this patch I think we will drop performance on TCP. Why? Let's analyze it, picking one little sketch: @@ -2313,7 +2307,10 @@ m_retxEvent.Cancel (); } // Try to send more data - SendPendingData (m_connected); + if (!m_sendPendingDataEvent.IsRunning ()) + { + m_sendPendingDataEvent = Simulator::Schedule ( TimeStep (1), &TcpSocketBase::SendPendingData, this, m_connected); + } } If we replace SendPendingData scheduled at now () + Timestep (1) with now (), we are basically reverting the situation (since, after these line, there is nothing and the function returns). Why Tommaso added these Timestep in first place? Well, it is clearly explained here: http://mailman.isi.edu/pipermail/ns-developers/2015-February/012577.html and in the following mail.
(In reply to natale.patriciello from comment #1) > Hi, with this patch I think we will drop performance on TCP. Why? Let's > analyze it, picking one little sketch: > > @@ -2313,7 +2307,10 @@ > m_retxEvent.Cancel (); > } > // Try to send more data > - SendPendingData (m_connected); > + if (!m_sendPendingDataEvent.IsRunning ()) > + { > + m_sendPendingDataEvent = Simulator::Schedule ( TimeStep (1), > &TcpSocketBase::SendPendingData, this, m_connected); > + } > } > > If we replace SendPendingData scheduled at now () + Timestep (1) with now > (), we are basically reverting the situation (since, after these line, there > is nothing and the function returns). > > Why Tommaso added these Timestep in first place? Well, it is clearly > explained here: > > http://mailman.isi.edu/pipermail/ns-developers/2015-February/012577.html > > and in the following mail. I think we should audit these (should we add a TimeStep to each of these three cases?) and add a test to prevent regressions; our current tests are not catching the subtle issue that Tommaso pointed out. Aleksandr pointed out the possible weakness of this approach in bug 2111.
I have conducted some experiment, and I believe that the only points where we need this event is when the application is pushing down some data. As a result, I removed the sendpendingdata event from parts that, inside the TCP socket, call SendPendingData. Patch is in queue with TCP SACK.
Patched added
Created attachment 2762 [details] Patch to have the step only when application is pushing down the data
fixed in 12648:2200388888c5