Bug 1383

Summary:	Proposal to change PacketTag API to more C++-like style
Product:	ns-3	Reporter:	Alex Afanasyev <alexander.afanasyev>
Component:	network	Assignee:	Mathieu Lacage <mathieu.lacage>
Status:	PATCH PENDING ---
Severity:	normal	CC:	tomh
Priority:	P5
Version:	ns-3-dev
Hardware:	All
OS:	All
See Also:	https://www.nsnam.org/bugzilla/show_bug.cgi?id=2361
Attachments:	patch Patch changing PacketTag API Patch changing usage of PacketTag API for all sources, examples, and tests Patch changing usage of PacketTag API for some tests

Description Alex Afanasyev 2012-02-29 21:21:00 EST

I'm proposing a modification to PacketTag API to be more of C++ style.

Instead of current practice:

 Tag tag;
 tag.SetParameter (bla);
 packet->AddPacketTag (tag);

...

 Tag tag;
 bool ok = packet->(Remove|Peek)PacketTag (tag);
 if (!ok) doSomething ();

a new way:

 Ptr<Tag> tag = CreateObject<Tag> ();
 tag->SetParameter (bla);
 packet->AddPacketTag (tag);

...

  Ptr<const Tag> tagReadOnly = packet->PeekPacketTag<Tag> ();
  Ptr<Tag> tag = packet->RemovePacketTag<Tag> ();

  if (tagReadOnly == 0) doSomething ();


Other comments.
===============


I wasn't quite sure why data for tags were stored in a buffer, instead of just having data inside the Tag class.  In the following patch I have changed this behavior, which automatically removed limit on packet tag size (and eliminated memory wasting empty and small tags).

I tried to find use of Serialize/Deserialze function on Tags in the code.  As I understand, the only place where such functionality be needed is MpiInterface (so, I'm not sure if it is used there).  In all other cases, simulations will work much more efficiently without unnecessary serialization/deserialization. 

The patch also changes every usage of PacketTag API throughout the code.  Changes are minimal, so if I missed something, it is easy to fix.

Comment 1 Alex Afanasyev 2012-02-29 21:22:03 EST

Created attachment 1347 [details]
patch

Comment 2 Tommaso Pecorella 2012-03-17 19:31:54 EDT

Ptr<> == dynamic memory == slowdown.

On the other hand, it might also happen that your approach minimize some other 
thing, so we could have a positive balance.

Can you check the performace before and after the patch ?

As a general thing, tho, this would be a quite API change in a ns-3 core part 
(even if it's not heavily used), so unless there are very good resons to do so 
(huge performace improvements, real cases where tag bufers are too short, etc). 
I'm a bit conservative.

Cheers,

T.

Comment 3 Alex Afanasyev 2012-04-17 23:55:29 EDT

Hi. I have done a simple performance evaluation using current PacketTag API and the one I'm proposing with the patch.

Here are some results in debug and optimized NS-3 builds, including source that was tested.  I hope my test case is somewhat representative.

// proposed PacketTag API, 100,000,000 iterations of adding and removing tag

// Run 1 (debug):
// real    0m47.055s
// user    0m46.106s
// sys     0m0.024s
// Run 2 (debug):
// real    0m43.307s
// user    0m43.280s
// sys     0m0.026s
// Run 3 (debug):
// real    0m43.195s
// user    0m43.172s
// sys     0m0.022s
  
// Run 1 (optimized):
// real    0m9.144s
// user    0m9.125s
// sys     0m0.017s
// Run 2 (optimized):
// real    0m9.083s
// user    0m9.067s
// sys     0m0.014s
// Run 3 (optimized):
// real    0m9.049s
// user    0m9.034s
// sys     0m0.014s
  
Ptr<Ipv4PacketInfoTag> tag = Create<Ipv4PacketInfoTag> ();
tag->SetAddress (Ipv4Address::GetAny ());
tag->SetTtl (255);
tag->SetLocalAddress (Ipv4Address::GetAny ());
tag->SetRecvIf (1);

Ptr<Packet> packet = Create<Packet> ();

for (uint32_t i = 0; i < 100000000; i++)
  {
    packet->AddPacketTag (tag);
    Ptr<const Ipv4PacketInfoTag> tag = packet->RemovePacketTag<Ipv4PacketInfoTag> ();

    tag->GetAddress ();
    tag->GetTtl ();
    tag->GetLocalAddress ();
    tag->GetRecvIf ();
  }

  
// the current API, 100,000,000 iterations iterations of adding and removing tag
// Run 1 (debug):
// real    0m51.303s
// user    0m51.279s
// sys     0m0.023s
// Run 2 (debug):
// real    0m42.992s
// user    0m42.968s
// sys     0m0.023s
// Run 3 (debug):
// real    0m44.505s
// user    0m44.485s
// sys     0m0.020s

// Run 1 (optimized):
// real    0m14.612s
// user    0m14.594s
// sys     0m0.016s
// Run 2 (optimized):
// real    0m14.670s
// user    0m14.656s
// sys     0m0.013s
// Run 3 (optimized):
// real    0m14.680s
// user    0m14.660s
// sys     0m0.019s
  
Ipv4PacketInfoTag tag;
tag.SetAddress (Ipv4Address::GetAny ());
tag.SetTtl (255);
tag.SetLocalAddress (Ipv4Address::GetAny ());
tag.SetRecvIf (1);
  
Ptr<Packet> packet = Create<Packet> ();

for (uint32_t i = 0; i < 100000000; i++)
  {
    packet->AddPacketTag (tag);

    Ipv4PacketInfoTag tag2;
    packet->RemovePacketTag (tag);

    tag.GetAddress ();
    tag.GetTtl ();
    tag.GetLocalAddress ();
    tag.GetRecvIf ();
  }

----

In short, in optimized build with current packet tag API we have ~14nanoseconds per AddPacketTag/RemovePacketTag.  With dynamic memory it is about 9nanosec per the same operation.  The biggest difference, i think, comes from the fact the right we do Serialization/Deserialization every time we add/remove tag.  With dynamic memory, only pointer is copied.

----

The general thing.  I was trying to track packet path (in terms of visited nodes).  With the current packet tag API (taking into account packet tag limit), I was not able to implement such tagging.  With pointers, I don't have problems any more.

Comment 4 Alex Afanasyev 2012-04-18 00:13:38 EDT

Created attachment 1378 [details]
Patch changing PacketTag API

Comment 5 Alex Afanasyev 2012-04-18 00:14:25 EDT

Created attachment 1379 [details]
Patch changing usage of PacketTag API for all sources, examples, and tests

Comment 6 Alex Afanasyev 2012-04-18 00:15:08 EDT

Created attachment 1380 [details]
Patch changing usage of PacketTag API for some tests