Add a map in MultiAttribute object to improve performances (#424)#430
Conversation
Ingvord
left a comment
Ingvord
left a comment
There was a problem hiding this comment.
Some improvements can be applied IMHO
| // - att : The newly configured attribute | ||
| // | ||
| //------------------------------------------------------------------------------------------------------------------- | ||
| void MultiAttribute::add_attr(Attribute *att) |
There was a problem hiding this comment.
I think this can be overloaded with add_attr(Attribute *att, size_t index) and used to prevent code duplications above.
There was a problem hiding this comment.
After a little bit more thinking I think it would be better to split this method into two:
size_t add_attr(Attribute), where size_t is an index of the newly added attribute, may be passed via overloaded method aka size_t add_attr(Attribute, index)
void put_attr_into_map(Attribute, index)
And finally aggregate these two in a third one:
size_t ndx = add_attr(attr);
put_attr_into_map(attr, ndx)
There was a problem hiding this comment.
I added put_attribute_in_map() method to reduce code duplication...
add_attr() method is used in some other parts of the code and already existed before this PR so I think we should stick with the same interface for this method.
I could rename put_attribute_in_map in put_attr_into_map if you prefer...
| ite = attr_list.end() - 2; | ||
|
|
||
| attr_list.insert(ite,new FwdAttribute(prop_list,*new_attr,dev_name,index)); | ||
| Attribute * new_fwd_attr = new FwdAttribute(prop_list,*new_attr,dev_name,index); |
There was a problem hiding this comment.
Seems to me as a code duplication. Must be extracted into a dedicated method overloaded from add_attr, see below
| else | ||
| { | ||
| attr_list.insert(ite,new Attribute(prop_list,attr,dev_name,index)); | ||
| Attribute * new_attr = new Attribute(prop_list,attr,dev_name,index); |
There was a problem hiding this comment.
Seems to me as a code duplication. Must be extracted into a dedicated method overloaded from add_attr, see below
| else | ||
| { | ||
| attr_list.insert(ite,new WAttribute(prop_list,attr,dev_name,index)); | ||
| Attribute * new_attr = new WAttribute(prop_list,attr,dev_name,index); |
There was a problem hiding this comment.
Seems to me as a code duplication. Must be extracted into a dedicated method overloaded from add_attr, see below
…vector. Indexes are now correctly updated taking this into account (tango-controls#458). This should fix a bug introduced in tango-controls#430 which was impacting servers using dynamic attributes.
…vector. Indexes are now correctly updated taking this into account (tango-controls#458). This should fix a bug introduced in tango-controls#430 which was impacting servers using dynamic attributes.
… map (tango-controls#459) State and Status attributes should always be at the end of attributes list vector. Indexes are now correctly updated taking this into account (tango-controls#458). This should fix a bug introduced in tango-controls#430 which was impacting servers using dynamic attributes.
2 participants
This solution is using an std::map.
Using an unordered_map if the compiler supports it could slightly improve the performances even more.
The gain is already big using an std::map.