Bug 15

Summary: Tags do not preserve constructor/destructor semantics
Product: ns-3 Reporter: Gustavo J. A. M. Carneiro <gjcarneiro>
Component: coreAssignee: Mathieu Lacage <mathieu.lacage>
Status: RESOLVED FIXED    
Severity: blocker    
Priority: P1    
Version: pre-release   
Hardware: PC   
OS: Linux   
Attachments: implement constructor/destructor management for tags.
a patch which works (I think)

Description Gustavo J. A. M. Carneiro 2007-04-06 09:18:25 UTC 
Because tag copying uses memcpy(), C++ class constructors/destructors are not being called when appropriate.  This makes it impossible to use tags that contain references to reference counted large objects, and is generally a surprise to the programmer that tags are not treated like regular C++ objects.

The branch [1] fixes the problem, with no performance penalty (I think, haven't benchmarked it), by using a specialized new allocator.

[1] http://telecom.inescporto.pt/~gjc/ns3/hgwebdir.cgi/ns-3-tags-nomemcpy/
Comment 1 Gustavo J. A. M. Carneiro 2007-04-13 12:32:18 UTC 
Hmm.. I think I've been sloppy with this patch; it is incomplete.
Comment 2 Gustavo J. A. M. Carneiro 2007-04-13 13:31:08 UTC 
Bah.. I give up; with the current tags architecture it is basically impossible to call the destructor for tags.  C++ templates are too limiting :|
Comment 3 Mathieu Lacage 2007-04-18 02:18:20 UTC 
It is actually possible to call the destructor for tags. You could add a void TagRegistry::Destruct (uint32_t uid, uint8_t buf[TAGS::SIZE]); method and invoke it from RemoveAll and Remove. Something which works just like PrettyPrint.

However, You also need to be careful to make sure that the tag data is properly aligned. i.e., you need to move it to the start of the struct TagData: on many platforms, it is not possible to invoke a constructor or destructor on a data structure which is not aligned properly.

Let me take a step back. I think that the following is what you want to get:
  - on Add, invoke a copy constructor once to copy the object into struct TagData
  - on Remove, invoke the copy constructor once to copy the object into the user's instance. Invoke the destructor if needed on struct TagData.
  - on RemoveAll, invoke all destructors if needed.

Is this really all you need to make your use-case work ? If so, I think that the only issue is that it could potentially make ::RemoveAll very costly in terms of CPU useage because you have to lookup the destructor rather than simply release the data.

Would you care enough to produce a patch to allow us to benchmark it ?
Comment 4 Mathieu Lacage 2007-04-18 03:04:36 UTC 
Created attachment 12 [details]
implement constructor/destructor management for tags.
Comment 5 Mathieu Lacage 2007-04-18 03:07:08 UTC 
would you care do some testing and benchmarking on this patch ? I suspect that the static void Destruct (uint32_t uid, uint8_t buf[Tags::SIZE]); method might need to be made inline to avoid any runtime cost but this code might actually work and do what you intend in a not-too-unportable way.

I am really worried about the portability implications of this patch: it will need testing on a larger set of platforms than usual before being considered for inclusion but I see this patch as being extremly useful potentially.
Comment 6 Mathieu Lacage 2007-04-19 04:30:36 UTC 
Created attachment 13 [details]
a patch which works (I think)
Comment 7 Mathieu Lacage 2007-04-19 04:31:48 UTC 
I have benchmarked this patch on a few testcases and did not see any impact on cpu usage. It would be nice to see a bit more functional testing of this patch.
Comment 8 Mathieu Lacage 2007-04-19 05:00:02 UTC 
I pushed a version of the last patch. Further testing to know if it works for you would be nice.