[Freeipa-devel] [PATCH] 811 performance: faster DN implementation

[Petr Viktorin]

On 04/09/2015 01:56 PM, Petr Vobornik wrote:
> On 04/02/2015 11:54 AM, Petr Viktorin wrote:
>> On 03/31/2015 12:11 PM, Petr Vobornik wrote:
>>> The only different thing is a lack of utf-8 encoded str support(as
>>> input). I don't know how much important the support is.
>>
>> I don't think that support is too important (assuming IPA doesn't use
>> it!). However, the behavior with this patch is dangerous.
>> It allows unicode and ASCII strings, but fails on non-ASCII strings.
>> That means things will usually work, but as soon as a non-ASCII
>> component is introduced at the wrong place, you get an error.
>>
>> Restoring support for utf-8 encoded str looks easy to do; here's a patch
>> you can squash in. Or did I miss something?
>
> I also had to fix creation of AVAs to support utf-8 encoded str as input
> for attr and value (separately).
>
>>
>>> maybe it could be attached to ticket
>>> https://fedorahosted.org/freeipa/ticket/4947
>>> -----
>>> DN code was optimized to be faster if DNs are created from string. This
>>> is the major use case, since most DNs come from LDAP.
>>>
>>> With this patch, DN creation is almost 8-10x faster (with 30K-100K DNs).
>>>
>>> Second mojor use case - deepcopy in LDAPEntry is about 20x faster - done
>>> by custom __deepcopy__ function.
>>>
>>> The major change is that DN is no longer internally composed  of RDNs
>>> and AVAs but it rather keeps the data in open ldap format - the same as
>>> output of str2dn function. Therefore, for immutable DNs, no other
>>> transformations are required on instantiation.
>>>
>>> The format is:
>>>
>>> DN: [RDN, RDN,...]
>>> RDN: [AVA, AVA,...]
>>> AVA: ['utf-8 encoded str - attr', 'utf-8 encode str -value', FLAG]
>>> FLAG: int
>>>
>>> Further indexing of DN object constructs an RDN which is just an
>>> encapsulation of the RDN part of open ldap representation. Indexing of
>>> RDN constructs AVA in the same fashion.
>>>
>>> Obtained EditableAVA, EditableRDN from EditableDN shares the respected
>>> lists of the open ldap repr. so that the change of value or attr is
>>> reflected in parent object.
>>
>>
>> Looks good. A couple of comments:
>>
>> RDN.to_openldap: _avas always has 3 components, right? I'd prefer
>> `list(a)` over `[a[0], a[1], a[2]]`.  Similarly for tuple in in __add__
>> and RDN._avas_from_sequence.
>
> Fixed
>
>>
>> DN._rdns_from_value: the error message at the end is wrong, RDN is also
>> accepted. (And, `type(value)` would be more informative than
>> `value.__class__.__name__`.)
>
> Fixed
>
>>
>> You can optimize __deepcopy__ for immutable DNs even further: just
>> return self!
>
> Fixed, but kept part for EditableDN
>
>>
>> In DN.find & rfind, RDNs are not accepted but the error message says
>> they are.
>
> messages fixed
>
>>
>> You removed the newline at end of file.
>>
>
> line readded

ACK

-- 
Petr Viktorin