Poor performance when removing dynamic attributes

[mliszcz]

I have a server which allows for adding and removing dynamic attributes:

• CreateDynamicAttributes(500, 100) - create 500x double spectrum with 100 elements
• ClearDynamicAttributes - remove all dynamic attributes

Removing 500 attributes takes more than a minute. Attribute creation is relatively fast.

I have had a look into the code. The problem is in MultiAttribute::remove_attribute which does MultiAttribute::check_associated for all attributes. The overall cost of a single remove_attribute call is thus proportional to N^2 string comparisons (N is the number of attributes). With 1000 attributes configured, removing a single attribute takes ~1 second.

Improvement idea: can we use Attribute* instead of string determine associated attribute?

Callgrind profile:

The helper code was generated by Pogo.
The server is located here

[andygotz]

@mliszcz well spotted! This sounds very much like issue #424 with string compare taking too much time. It was resolved by introducing a map. I wonder if the map cannot be used to delete attributes?

[bourtemb]

Indeed, this is very similar to #424 and I think the code could be improved to use directly get_attr_ind_by_name method, which is using the map:

void MultiAttribute::check_associated(long index,string &dev_name)
{
	if ((attr_list[index]->get_writable() == Tango::READ_WITH_WRITE) || 
	    (attr_list[index]->get_writable() == Tango::READ_WRITE))
	{
		unsigned long assoc_index;
		string &assoc_name = attr_list[index]->get_assoc_name();
		try
		{
			unsigned long assoc_index = MultiAttribute::get_attr_ind_by_name(assoc_name);
//
// Check that the associated attribute is writable
//
			if ((attr_list[assoc_index]->get_writable() != Tango::WRITE) &&
			    (attr_list[assoc_index]->get_writable() != Tango::READ_WRITE))
			{
				TangoSys_OMemStream o;
				o << "Device --> " << dev_name;
				o << "\nProperty writable_attr_name for attribute " << attr_list[index]->get_name();
				o << " is set to " << assoc_name;
				o << ", but this attribute is not writable" << ends;
				Except::throw_exception((const char *)API_AttrOptProp,
				                                     o.str(),
				                                     (const char *)"MultiAttribute::check_associated");
			}
//
// Check that the two associated attributes have the same data type
//
			if (attr_list[writable_attr_list[assoc_index]]->get_data_type() != attr_list[index]->get_data_type())
			{
				TangoSys_OMemStream o;
				o << "Device --> " << dev_name;
				o << "\nProperty writable_attr_name for attribute " << attr_list[index]->get_name();
				o << " is set to " << assoc_name;
				o << ", but these two attributes do not support the same data type" << ends;
				Except::throw_exception((const char *)API_AttrOptProp,
				                                     o.str(),
				                                     (const char *)"MultiAttribute::check_associated");
			}

			attr_list[index]->set_assoc_ind(assoc_index);
		}
		catch (const Tango::DevFailed &e)
		{
			TangoSys_OMemStream o;
			o << "Device --> " << dev_name;
			o << "\nProperty writable_attr_name for attribute " << attr_list[index]->get_name();
			o << " is set to " << assoc_name;
			o << ", but this attribute does not exists" << ends;
			Except::throw_exception((const char *)API_AttrOptProp,
			                                     o.str(),
			                                     (const char *)"MultiAttribute::check_associated");
		}
	}
}

[mliszcz]

Thanks Andy and Reynald. Unfortunately get_attr_ind_by_name does not help. The execution time is still the same.

Diff for relevant part (I've moved the catch few lines up to have smaller diff):

        if ((attr_list[index]->get_writable() == Tango::READ_WITH_WRITE) ||
            (attr_list[index]->get_writable() == Tango::READ_WRITE))
        {
-               unsigned long j;
                string &assoc_name = attr_list[index]->get_assoc_name();
-               transform(assoc_name.begin(),assoc_name.end(),assoc_name.begin(),::tolower);
-               for (j = 0;j < writable_attr_list.size();j++)
+
+               long assoc_index = 0;
+
+               try
                {
-                       if (attr_list[writable_attr_list[j]]->get_name_lower() == assoc_name)
-                               break;
+                       assoc_index = get_attr_ind_by_name(assoc_name.c_str());
                }
-               if (j == writable_attr_list.size())
+               catch (const Tango::DevFailed&)
                {
                        TangoSys_OMemStream o;

                        o << "Device --> " << dev_name;
                        o << "\nProperty writable_attr_name for attribute " << attr_list[index]->get_name();
                        o << " is set to " << assoc_name;

Callgrind:

[bourtemb]

Thanks Andy and Reynald. Unfortunately get_attr_ind_by_name does not help. The execution time is still the same.

But you removed 700 attributes instead of 500, according to your callgrind results?
The callgrind results seems to indicate some improvements too.

[mliszcz]

Sorry for the confusion. Yesterday I've bumped the attribute number from 500 to 700.

• original post is for 9.3.3 built from source distribution with 500 attributes configured - remove_attribute cost is 83%.
• previous post is for 9.3.3 built from cppTango, with proposed correction, with 700 attributes configured - remove_attribute cost is 72%.
• below is 9.3.3 built from cppTango, without any changes, with 700 attributes configured - remove_attribute cost is 82%.
  

So yes, there is some improvement (~80% vs ~70%), but with a lot of attributes it does not make noticeable difference, because both options need quite a long time to complete.

[mliszcz]

We discussed this during Tango meeting and the conclusion is that if we need better performance here, we'd need to rework the way how attributes are managed (i.e. get rid of list of indices). It would be risky and would require lot of effort.

So, I have opened a PR with the simple solution proposed by @bourtemb.

@bourtemb, @andygotz you may want to check PR #564 (I do not push for it, the issue is not critical/blocking, we may discard the PR without a merge as well).