Define option for toggling implicit decoding / encoding

[datakurre]

Needs

• review
• some tests with unicode flag disabled
• benchark / profiling example with on and off

Fixes #16

[coveralls]

Coverage decreased (-2.4%) to 95.664% when pulling 4faf13f on datakurre:datakurre-use-unicode-option into 570e5ec on bluedynamics:master.

[coveralls]

Coverage decreased (-0.003%) to 98.059% when pulling b277183 on datakurre:datakurre-use-unicode-option into 570e5ec on bluedynamics:master.

[coveralls]

Coverage decreased (-0.003%) to 98.059% when pulling 344147d on datakurre:datakurre-use-unicode-option into 570e5ec on bluedynamics:master.

[coveralls]

Coverage increased (+0.05%) to 98.115% when pulling 0a2c548 on datakurre:datakurre-use-unicode-option into 570e5ec on bluedynamics:master.

[datakurre]

Before

After with use_unicode = False

Not as dramatic change as it was in my first round after I had normalized the search queries (which I haven't done yet in these upstream pulls), but still significant.

The difference from my original issue #16 is probably that because of normalization of attrlist in queries, even simple queries need to decode the same large result blob from cached as the bigger queries. So, once I manage to also add normalization as an option, it will probably slow down node.ext.ldap when enabled without disabling implicit unicode deoding.

[jensens]

Overall @rnixx and I both think that repeating of complete code blocks via copy paste is not an option.
I would see this as a first draft, but not as a solution we want to merge.

Overall we gain 1.75% performance according to your profiling. This might be more in a scenario with many attributes and so is a good feature we would like to merge if the repeated/ copy-pasted code style is refactored.

[jensens]

This kind of code is repeating and blows up several lines.

In order to follow a DRY paradigm, I propose to pass the current node to decodes and encodes and handle the if in there.

[jensens]

here copy pasting the whole logic section is not ok. When passing the current node to decode this is not needed any more. This may need some millisecs more, but I doubt it has a real impact.

[jensens]

same here

[jensens]

this block it is a good example why the repeating of code is bad.

[jensens]

Further ideas to improve:

• on nodes __setitem__ copy the _use_unicode from root/parent to the node. A bool should not waste much RAM, but it saves a lot traversing up the tree. Dependent on tree depth this should reduce costs to get the value.

• pass node over and handle condition centrally

  def decodes(value, node):
      if not node._use_unicode:
          return value
      .....
  

[datakurre]

Thanks for the review. I postpone this until I manage to make another pull with query normalizations (may take a couple of weeks).

For our current version, decode removal was the last optimization I did, so I was surprised, how much my normalizations added decode calls (compared to only 1,75% here). So, I need to do the other optimizations properly at first so that I can compare the version without any decode the version with _use_unicode-checking decode.

The version without decoding at will remains the prettiest one :)

[rnixx]

The version without decoding at will remains the prettiest one

Agreed. I also think we should remove auto encoding / decoding in the end, but since this is an API break this should be postponed to 1.1

[datakurre]

👍 So, I should definitely wait for that the. :) (And I would not be able to do this too soon anyways ;)

…

On 17. joulukuuta 2016 klo 14.07 +0200, Robert Niederreiter ***@***.***>, wrote: > > The version without decoding at will remains the prettiest one > Agreed. I also think we should remove auto encoding / decoding in the end, but since this is an API break this should be postponed to 1.1 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub (#25 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAJyv3kMWsPyuDfhspVZSr9wXkZMtvhBks5rI9BfgaJpZM4Kp9AN).

[rnixx]

Hi. Since python-ldap in py 3 handles everything except attribute values as unicode out of the box, this PR is obsolete. For py 2 there's a compatibility mode (vontrolled by bytes_mode setting). Merge of #44 takes care of py3 and bytes_mode.