Bug 1352 - Map container reimplementation for attribute system
Map container reimplementation for attribute system
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: core
ns-3-dev
All All
: P5 normal
Assigned To: Mathieu Lacage
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-30 13:44 EST by Jaume Nin
Modified: 2012-08-30 11:38 EDT (History)
3 users (show)

See Also:


Attachments
Patch that enables maps into the attribute system (53.79 KB, patch)
2012-01-30 13:44 EST, Jaume Nin
Details | Diff
Patch without the unneded whitespaces (26.28 KB, patch)
2012-03-13 06:37 EDT, Jaume Nin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaume Nin 2012-01-30 13:44:15 EST
Created attachment 1317 [details]
Patch that enables maps into the attribute system

In the context of LENA project, we submitted some time ago in [1] a code extension to enable usage of maps instead of vectors in the attribute system. 

A simpler and more elegant solution was proposed and finally accepted, accepting a integer map as an attribute but storing it in a vector. The workaround seemed fine at the moment but it turned out that it is not, since the map keys (non consecutive) were replaced by the vector index. 

Since the current implementation does not fit our needs, we are submitting again a patch that enables this feature (attached to the report)

The patch affects core and config-store components.

Any feedback or suggestion on how to improve the proposed solution will be more than welcomed.

BR,
Jaume Nin

[1] http://codereview.appspot.com/4644042/
Comment 1 Mathieu Lacage 2012-01-30 15:03:14 EST
patch is full of un-needed whitespace changes. Please, resubmit for review.
Comment 2 Jaume Nin 2012-03-13 06:37:04 EDT
Created attachment 1356 [details]
Patch without the unneded whitespaces

Sorry for the long delay, the comment just slept through my inbox and I completely forgot.

Extra whitespaces where added by check-style.py, hope this one suits better.

Regards,
Jaume
Comment 3 Mathieu Lacage 2012-03-13 09:28:55 EDT
I am afraid that:

  - the latest patch does not include the new files you created. I tried to extract them from the old patch but had to edit it by hand to do so: neither hg import nor patch understood it (how on earth did you generate that first patch ??)

  - the latest patch still has some whitespace stuff which makes reading this a painful exercise.
 
  - you have failed to provide any hints as to why you have made these changes and/or provide a high-level description of your changes.

I would suggest that before you post a new patch for review, you try to review the code yourself and put yourself in the shoes of someone who works on that code about 30mins each week (are you into role playing games ?): what can you do to make your reader happy and willing to go further ? I am sure it will make a huge difference.

Anyhow, I think I reverse engineered your original problem from the patch (I still have no clue as to what you use this map support for: followup patches will need to provide some background on this and if you did provide it in your first review request ages ago, I would suggest you think really hard about context, and the lack of it) and no, I am not happy with the need to support two kinds of containers to iterate the object tree. 

With the above disclaimers about my lack of understanding of the big picture, I would suggest you merge these two types of objects (the map & the vector) into one that supports a map-like API instead of a vector-like API. Namely, maybe just keep the ObjectPtrMap* classes and kill the other one by making sure that MakeObjectVectorAccessor and MakeObjectMapAccessor both create a single type of ObjectPtrMapValue that has the following API (basically, the one you used originally but tweaked to return the pair key/value:

class ObjectPtrMapValue 
{
public:
  std::pair<uint32_t, Ptr<Object> > Get (uint32_t i) const;
};

I have no idea what would be the impact of such a change on the existing Config/ConfigStore code. Before you embark on this, I would suggest you try to provide (again maybe, sorry) some background information on what you are trying to do at the model level.
Comment 4 Jaume Nin 2012-03-14 12:58:16 EDT
Hi Mathieu,

First of all, apologies for the inconveniences and the little dedication I'd been able to put in this issue and the missing files on the last submitted. Patch generation is a brand new topic for me and it seems there is a lot of room for improvement here.

Back to the basics. The main motivation for this feature is that in LENA we have several map containers that need to be made available through the attribute system in order to access the traces. The containers need to be maps since their keys are not consecutive. A sample path of what we need follows:
/NodeList/#nodeId/DeviceList/#deviceId/LteUeRrc/RadioBearerMap/#RNTI/LteRlc/TxPDU

#RNTI is simply a user ID unique to the LTE network, hence we have things like the following:
/NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/1/LteRlc/TxPDU
/NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/2/LteRlc/TxPDU
/NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/3/LteRlc/TxPDU
/NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/4/LteRlc/TxPDU

The current implementation, translates the map to a vector, hence the keys are missed and the paths generated are:
/NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/0/LteRlc/TxPDU
/NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/1/LteRlc/TxPDU
/NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/0/LteRlc/TxPDU
/NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/1/LteRlc/TxPDU

The original idea was to avoid touching already working stuff, hence the duplicity. However, from the maintainer point of view, it makes much more sense to have a single ObjectPtrMapValue for both vectors and maps and probably the solution will me more seamless, just the API and the container.

If you agree with this direction I'll try to provide the patch (clean and with just the right number of whitespaces)  and extend the testing to cover maps as well within a couple of weeks.

Regards,
Jaume
Comment 5 Mathieu Lacage 2012-03-23 04:21:31 EDT
(In reply to comment #4)

> #RNTI is simply a user ID unique to the LTE network, hence we have things like
> the following:
> /NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/1/LteRlc/TxPDU
> /NodeList/0/DeviceList/0/LteUeRrc/RadioBearerMap/2/LteRlc/TxPDU
> /NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/3/LteRlc/TxPDU
> /NodeList/1/DeviceList/0/LteUeRrc/RadioBearerMap/4/LteRlc/TxPDU

how do you allocate the user ids ?
Comment 6 Jaume Nin 2012-03-23 09:45:23 EDT
(In reply to comment #5)
> (In reply to comment #4)
> how do you allocate the user ids ?
We have two different IDs, radio IDs are sequentially allocated in the system (first UE id 1)  but on the eNodeB you can have a non sequentiall collection of radio IDs. Logical channel IDs are sequentially allocated within the eNodeB, but again, there is no direct mapping since for instance one UE can use more than one logical channel.
Comment 7 Nicola Baldo 2012-03-29 05:58:31 EDT
Mathieu and I discussed this issue while attending WNS3 2012, here is the summary:

1) the use case for having maps of objects is agreed upon, since there are several cases (in addition to the example posted by Jaume) where it is needed to provide to the user a list of objects with potentially non-consecutive IDs;

2) that considered, Mathieu suggested to refactor the patch so as to always use maps under the hood, in order to avoid most of the code duplication and make maintenance easier. If I understood correctly, this is the same technical solution previously discussed at the end of comment #3 and at the end of comment #4.
Comment 8 Jaume Nin 2012-03-30 11:35:53 EDT
Ok, I was waiting for the green light to follow this path. I'll try to provide a rigorously formatted patch within 1 / 2 weeks.

(In reply to comment #7)
> Mathieu and I discussed this issue while attending WNS3 2012, here is the
> summary:
> 
> 1) the use case for having maps of objects is agreed upon, since there are
> several cases (in addition to the example posted by Jaume) where it is needed
> to provide to the user a list of objects with potentially non-consecutive IDs;
> 
> 2) that considered, Mathieu suggested to refactor the patch so as to always use
> maps under the hood, in order to avoid most of the code duplication and make
> maintenance easier. If I understood correctly, this is the same technical
> solution previously discussed at the end of comment #3 and at the end of
> comment #4.

(In reply to comment #7)
> Mathieu and I discussed this issue while attending WNS3 2012, here is the
> summary:
> 
> 1) the use case for having maps of objects is agreed upon, since there are
> several cases (in addition to the example posted by Jaume) where it is needed
> to provide to the user a list of objects with potentially non-consecutive IDs;
> 
> 2) that considered, Mathieu suggested to refactor the patch so as to always use
> maps under the hood, in order to avoid most of the code duplication and make
> maintenance easier. If I understood correctly, this is the same technical
> solution previously discussed at the end of comment #3 and at the end of
> comment #4.
Comment 9 Mathieu Lacage 2012-04-01 13:22:57 EDT
bonus points go to a map implementation that handles strings as keys
Comment 10 Jaume Nin 2012-04-17 04:30:20 EDT
Hi,

I've upload to codereview [1] the promised patch for the map attributes refactoring. I keept it quite minimal. Short list of the changes follows:
- Replaced the std::vector< Ptr<Object> > m_objects container by a std::map<uint32_t, Ptr<Object> > one as agreed. It could be extended to support strings as well but I did not have much more time to test it, it goes to my to do buffer.
- Changed the virtual Ptr<Object> DoGet (const ObjectBase *object, uint32_t i) prototype to Ptr<Object> DoGet (const ObjectBase *object, uint32_t i, uint32_t *index) to get the index of the maps. For vectors, index and i are just the same but I could not find any other elegant solution to this.
- Changed the attribute iteration to support the new map container and the new Get API
- Extended the attribute testing to do some minor testing of this feature (actually it is a copy&paste&replace of the AttributeVector test, that was not extremely extensive).

On my local copy I tried it out with a couple of simulation programs and the patch did exactly what we needed.

If there is something else needed to be done, please, let me know.

Best regards,
Jaume

[http://codereview.appspot.com/6047044/]
Comment 11 Mathieu Lacage 2012-04-17 15:47:07 EDT
(In reply to comment #10)

> - Replaced the std::vector< Ptr<Object> > m_objects container by a
> std::map<uint32_t, Ptr<Object> > one as agreed. It could be extended to support
> strings as well but I did not have much more time to test it, it goes to my to
> do buffer.

If you feel confident that this can be done later, I am fine with it. The really nice thing to do would be to replace the Names implementation with this.

> [http://codereview.appspot.com/6047044/]

the patch looks really good. I will look at it again tomorrow before commit.

thanks for the hard work
Comment 12 Tom Henderson 2012-05-08 01:49:13 EDT
(In reply to comment #11)
> (In reply to comment #10)
> 
> > - Replaced the std::vector< Ptr<Object> > m_objects container by a
> > std::map<uint32_t, Ptr<Object> > one as agreed. It could be extended to support
> > strings as well but I did not have much more time to test it, it goes to my to
> > do buffer.
> 
> If you feel confident that this can be done later, I am fine with it. The
> really nice thing to do would be to replace the Names implementation with this.
> 
> > [http://codereview.appspot.com/6047044/]
> 
> the patch looks really good. I will look at it again tomorrow before commit.
> 
> thanks for the hard work


I'm recommending (from the code review comments) to:
- delete TestMap2 test attribute (unused and not understood)
- keep bug 1352 open for handling strings and also for resolving the question on attribute tests in the code review
- otherwise merge the latest patch so LENA merge can proceed
Comment 13 Nicola Baldo 2012-05-09 06:09:25 EDT
Jaume prepared a patch following Tom's indication, I just pushed it to ns-3-dev (changeset:   7884:963d5bfe9c52)
Comment 14 Jaume Nin 2012-05-09 11:20:51 EDT
As a final comment, work pending to be done:
- String support
- Test getter and setter for vector and maps

I am sorry but was not able to work on this lately. I will try my best for the next release.
Comment 15 Tom Henderson 2012-05-13 00:17:19 EDT
(In reply to comment #13)
> Jaume prepared a patch following Tom's indication, I just pushed it to ns-3-dev
> (changeset:   7884:963d5bfe9c52)

This is crashing under valgrind; to reproduce:
./test.py -g -s attributes
Comment 16 Nicola Baldo 2012-05-16 11:44:31 EDT
changeset:   8751:b89660102b63
parent:      8749:4462ac63d4cf
user:        Jaume Nin
date:        Mon May 14 18:15:22 2012 +0200
summary:     Fixed valgrind errors in attribute system. Fixed typo in AttributeMap test. Fixed Get() method for elements not present in the map
Comment 17 Jaume Nin 2012-05-18 10:37:20 EDT
With
changeset:   8751:b89660102b63
parent:      8749:4462ac63d4cf
The bug should have been addressed

Let me know if it is not the case.
Comment 18 Tom Henderson 2012-08-30 11:38:03 EDT
closing because no other issues have been raised since the commits from May 2012