[Freeipa-devel] [PATCH] 809 speed up convert_attribute_members

[Jan Cholasta]

Dne 9.4.2015 v 13:56 Petr Vobornik napsal(a):
> On 04/02/2015 09:47 AM, Jan Cholasta wrote:
>> Hi,
>>
>> Dne 31.3.2015 v 12:11 Petr Vobornik napsal(a):
>>> A workaround to avoid usage of slow LDAPEntry._sync_attr #4946.
>>>
>>> I originally wanted to avoid DN processing as well but we can't do that
>>> because of DNs which are encoded - e.g. contains '+' or ','. Therefore
>>> patch 811 - faster DN implementation is very useful. Also patch 809 is
>>> useful to avoid high load of 389.
>>>
>>> https://fedorahosted.org/freeipa/ticket/4965
>>
>>
>> 1)
>>
>> +            dn = container_dns.get(ldap_obj_name, None)
>> +            if not dn:
>> +                ldap_obj = self.api.Object[ldap_obj_name]
>> +                dn = DN(ldap_obj.container_dn, api.env.basedn)
>> +                container_dns[ldap_obj_name] = dn
>> +            return dn
>>
>>      a) The second argument of .get() is None by default
>>
>>      b) "not dn" matches None as well as empty DNs, use "dn is not None"
>> (it's not that there could be empty DNs here, but let's not give a
>> potential reader the wrong idea)
>>
>>      c) It would be better to catch KeyError rather than call .get() and
>> check the result:
>>
>>              try:
>>                  dn = container_dns[ldap_obj_name]
>>              except KeyError:
>>                  dn = ...
>>                  container_dns[ldap_obj_name] = dn
>
> Changed
>
>>
>>
>> 2) Does get_new_attr() actually provide any speed up? Unless I'm missing
>> something, it just mirrors the virtual member attributes already readily
>> available from entry_attrs in new_attrs.
>
> Yes, a bit. With 30K members and my vm get_new_attr takes ~ 0.114s.
> setdefault takes ~ 0.686s which is about 7-10% of the entire
> convert_attribute_members. Pure dict is faster.
>
>>
>>
>> 3) get_container_dn() and get_new_attr() do not need to be functions,
>> since each is called just from a single spot.
>
> Changed
>
>>
>>
>> 4) "memberdn = DN(member)" could be one for loop up.
>>
>
> Changed
>
>>
>> Here's what I ended up with trying to fix the above (untested):
>>
>>          for attr in self.attribute_members:
>>              try:
>>                  value = entry_attrs.raw[attr]
>>              except KeyError:
>>                  continue
>>              del entry_attrs[attr]
>>
>>              ldap_objs = {}
>>              for ldap_obj_name in self.attribute_members[attr]:
>>                  ldap_obj = self.api.Object[ldap_obj_name]
>>                  container_dn = DN(ldap_obj.container_dn, api.env.basedn)
>>                  ldap_objs[container_dn] = ldap_obj
>>
>>              for member in value:
>>                  memberdn = DN(member)
>>                  try:
>>                      ldap_obj = ldap_objs[DN(*memberdn[1:])]
>>                  except KeyError:
>>                      continue
>>
>>                  new_attr = '%s_%s' % (attr, ldap_obj.name)
>>                  new_value = ldap_obj.get_primary_key_from_dn(memberdn)
>>                  entry_attrs.setdefault(new_attr, []).append(new_value)
>
> Without any modifications the code is ~ 2.3x slower than mine. In patch
> 811 DN's slice, __hash__ and __eq__ functions are optimized.

Thanks, ACK.

Pushed to master: e4930b3235e5d61d227a7e43d30a8feb7f35664d

-- 
Jan Cholasta