[Ns-developers] merge of rts/wallclock simulator code

[Mathieu Lacage]

On Thu, 2008-10-16 at 17:30 -0700, craigdo at ee.washington.edu wrote:

> The basic architecture is pimpl.  There is an interface class called
> Simulator.  There are two implementations.  The implementation for the
> real-time simulator is called realtime-simulator-impl.  In order to switch
> to the realtime-simulator-impl you bind ns3::RealtimeSimulatorImpl to the
> SimulatorImplementationType global variable.  It's clear that there are two
> choices throughout the architecture, the default simulator implementation or
> the real-time simulator implementation.  There's not a wall-clock thing in
> there anywhere, so why should the extension class be called
> WallclockSimulator?  It seems off in left field to me.

Because, as I said in a previous email, I intend to implement
WallclockSimulator correctly in both the non-realtime and the realtime
case. Lots of comments below are related to this issue: It seems I had
not made that intent clear. 

> There is another module that lives down at the bottom of the
hierarchy 
> called the WallClockSynchronizer.  This establishes the actual relationship
> with some real-time clock.  I expect that this may actually eventually be
> replaced by something that looks at a time-stamp counter instead of making
> system calls for the time-of-day clock.  In that case, you might have a
> TscSynchronizer and no WallClock anything at all.  It seems to me that

Having a Tsc-based simulation backend does not necessarily exclude the
possibility of implementing meaningfully the WallclockSimulator API. It
just requires a careful measurement of the Tsc clock against the
wall-clock time.

> putting the real-time scheduling methods into a class called
> WallclockSimulator makes the wrong connection and stands out as, well, oddly
> different than the rest of the conceptual model.  I decided it would be
> better to leave the RealtimeSimulator name.
> 
> Regarding the RealtimeSimulator class, you changed the method names from
> ScheduleRealtime(), etc., to just Schedule().  I thought this would be a
> mistake because there are already Simulator::Schedule methods.  Now there
> are parallel RealtimeSimulator::Schedule methods (or Wallclock or whatever).
> It seems to me that it is very easy to think that if you start using the
> real-time simulator, you should just begin using the corresponding
> RealtimeSimulator::Schedule() methods in your models.  This would be
> disastrous!  They are absolutely not the same methods and they will
> absolutely not do the same thing and if you follow the naming lead, your
> code will begin to fail in very hard to debug ways.  Not good.

I removed the prefix because the prefix is already present in the class
name: there is no confusion possible unless you move the methods back in
the Simulator class which I proposed earlier but which you did not seem
too excited about.

> I decided it would be better to keep the name of the methods in the new
> class different (e.g., RealtimeSimulator::ScheduleRealtime, to reinforce
> that they are different methods, with different semantics to be used in
> different places for different reasons.  Perhaps it looked odd to you at
> first glance, but it was there for IMO an important reason.  Maybe if you
> don't like the at-first-glance redundancy of the name, we could reinforce
> that these are the multithread safe-methods and call it
> MultithreadSimulator::ScheduleRealtime or something like that.  I cringed
> when I saw RealtimeSimulator::Schedule().
>
> 
> You also removed the GetRealtimeNow method.  I think this is a useful method
> and I'm not sure why you wanted to get rid of it.  It is useful, for

I got rid of it because I see no reason to export and implement an API
we have no use-case for. Should a use-case come up, I would be happy to
add it back. [more below]

> example, in a scenario you wanted to support -- RunOneEvent.  Since there is
> no real-time synchronization if an external event loop is in charge, it
> seems useful to be able to give that external loop a real-time base to work
> with.  It would be incredibly convenient to provide a real-time that is
> nicely represented as Time units.  If it's not there, the client gets to
> convert from Unix native time structures back and forth to ns-3 times
> through doubles and seconds or some such conversion path.
> GetCurrentRealtime seems extremely convenient for this and useful for
> timestamping events or quick performance measurements.

There is already SystemWallClockMs for this but if you feel that this is
really much more helpful, then, I would be fine with adding it back.

> Regarding the event impl stuff, you did basically what I was in the process
> of doing.  Seems like it will work just fine, but I wanted to do some
> serious testing before I tossed it out.  I've changed the main-test-sync
> program to reflect the new multithread, non-multithread distinction and have
> let it beat on your code for a while and it seems solid.

I spotted one bug/typo while testing locally: 

diff -r 096de171a476 -r 26869487fef3 src/simulator/realtime-simulator-impl.cc
--- a/src/simulator/realtime-simulator-impl.cc	Thu Oct 16 12:29:11 2008 +0200
+++ b/src/simulator/realtime-simulator-impl.cc	Fri Oct 17 12:49:24 2008 +0200
@@ -525,7 +525,7 @@
 
     NS_LOG_LOGIC ("handle " << next.key.m_ts);
     m_currentTs = next.key.m_ts;
-    m_currentUid = next.key.m_ts;
+    m_currentUid = next.key.m_uid;
     event = next.impl;
   }
   event->Invoke ();


> 
> I am curious about why you decided to change the relationship between the
> RealtimeSimulator/WallclockSimulator and Simulator from the "friend"ly
> relation I had implemented.

Because friends are evil and if I can avoid them, I try to. This is why
I wanted to move the RealtimeSimulator implementation in the
simulator.cc file: I was hoping that this would be enough to get rid of
the friend. I was wrong: I had to split the MakeEvent methods too which
is something I had been planning to do at some point anyway.

> We have a central interface class Simulator with a replaceable
> implementation.  One of the implementations has some new methods that I
> don't think are really meaningful in the other (like ScheduleRealtimeNow or
> whatever you decided to call it).  It made good sense to me to have a friend

I thought that they were meaningful in both cases and that they could
meaningfully be implemented in both cases.

> class of Simulator that used all of the nifty templates and such in the
> central Simulator class and provided access to the new methods by vectoring
> them to the real-time implementation where they were meaningful.  If you
> weren't using the real-time implementation I asserted something to the
> effect of "you can't do that unless you have the real-time simulator
> implementation selected".  This seemed to me to fit nicely with the
> architecture and conceptual model, and provided a meaningful error message.
> 
> You pulled the common templated stuff out of Simulator and made Simulator
> and RealtimeSimulator peers.  You added real-time multithread-semantics
> scheduling methods to the default implementation (where I really don't they
> are meaningful) and then plumbed the real-time calls to both
> implementations.  The real-time implementation does what it should, but you
> errored out if the new methods are called in the default implementation.
> The end result is that the user just sees "not implemented".

I would be happy to see a patch to implement them efficiently in the
non-realtime case: the key is that we do not need/want to enforce a
strict event ordering between non-wall-clock and wall-clock events. It
is on my TODO: feel free to beat me to it.

> This all seems a bit less straightforward to me.  You obviously spent some
> considerable amount of effort to make all of these changes, and I'm
> interested in why you felt all of that effort was worthwhile.

Because, as I said above, I intend to implement the 'wall-clock' API in
the non-realtime case.

To summarize, I would appreciate that you keep the name
WallclockSimulator because that makes it clear that the relevant API is
not specific to your realtime implementation and that you don't add the
prefixes to the Schedule methods because the context is sufficiently
clear.

regards,
Mathieu