#310 Avoid calling escape_string() for logged DNs
Closed: wontfix None Opened 9 years ago by nkinder.

There are many areas in the server where we have a normalized DN that we then pass to escape_string() before logging. One of the things that escape_string() function does is to convert non-ascii characters to hex escape codes. I'm not sure that this is necessary, as I don't know that there is any problem logging UTF-8 characters to the logfiles (it would likely be welcomed by many people using different locales).

There are some areas where we might get a good performance boost by avoiding the escape_string() function. For example, every DN during a BIND is passed to escape_string() just prior to writing to the access log. This is expensive, as escape_string loops through the DN character by character, performs a function call for each character, then does a memcpy() of each character to a buffer. All of this is done even if no escaping is needed. I expect that we are also doing this escaping in many other logging areas, since as search bases, etc. We should eliminate use o this function anywhere possible. It would also be nice to see what effect this has on performance by running SLAMD before and after the change.


Fix description: removed unnecessary escape_string calls and the
static buffer used by escape_string.

Ran slamd repeatedly (BIND+SEARCH+UNBIND from 4 threads in 10 min.), but I could not get the good evidence that No escape_string improves the performance. Please note that the bind dn contains ascii characters and digits only. The following is the average of 5 repeated attempts each.

{{{
[With escape_string]
Total_Duration Total_Count Avg_Duration AVG_Count/Interval
--------------+-------------+-------------+-------------------
2395787.2 2404987.0 0.998 40083.117
--------------+-------------+-------------+-------------------
[No escape_string]
Total_Duration Total_Count Avg_Duration AVG_Count/Interval
--------------+-------------+-------------+-------------------
2395570.8 2314081.2 1.045 38568.020
--------------+-------------+-------------+-------------------
}}}

Reviewed by Rich (Thank you!!)

Pushed to master.

$ git merge trac310
Updating 178fe6a..e689473
Fast-forward
ldap/servers/plugins/acl/acl.c | 38 +++-----
ldap/servers/plugins/acl/aclanom.c | 11 +-
ldap/servers/plugins/acl/acllas.c | 2 +-
ldap/servers/plugins/acl/aclutil.c | 7 +-
ldap/servers/plugins/chainingdb/cb_config.c | 7 +-
ldap/servers/plugins/cos/cos_cache.c | 5 +-
ldap/servers/plugins/pam_passthru/pam_ptimpl.c | 35 +++----
ldap/servers/plugins/replication/repl5_replica.c | 104 ++++++++------------
.../plugins/replication/repl5_replica_config.c | 9 +-
ldap/servers/plugins/replication/replutil.c | 3 +-
ldap/servers/plugins/replication/urp.c | 5 +-
ldap/servers/plugins/replication/urp_glue.c | 5 +-
.../plugins/replication/windows_connection.c | 3 +-
.../plugins/replication/windows_protocol_util.c | 14 +--
ldap/servers/plugins/views/views.c | 9 +-
ldap/servers/slapd/add.c | 9 +-
ldap/servers/slapd/attr.c | 3 +-
ldap/servers/slapd/auth.c | 38 ++++----
ldap/servers/slapd/back-ldbm/import-threads.c | 21 ++---
ldap/servers/slapd/back-ldbm/ldbm_modrdn.c | 15 +--
ldap/servers/slapd/back-ldbm/ldbm_search.c | 11 +--
ldap/servers/slapd/back-ldbm/vlv.c | 3 +-
ldap/servers/slapd/bind.c | 13 +--
ldap/servers/slapd/compare.c | 4 +-
ldap/servers/slapd/configdse.c | 4 +-
ldap/servers/slapd/delete.c | 5 +-
ldap/servers/slapd/entry.c | 15 ++--
ldap/servers/slapd/modify.c | 22 ++---
ldap/servers/slapd/modrdn.c | 19 ++--
ldap/servers/slapd/opshared.c | 15 +--
ldap/servers/slapd/psearch.c | 3 +-
ldap/servers/slapd/pw.c | 6 +-
ldap/servers/slapd/resourcelimit.c | 4 +-
ldap/servers/slapd/result.c | 5 +-
ldap/servers/slapd/sasl_map.c | 2 +-
ldap/servers/slapd/schema.c | 29 ++----
ldap/servers/slapd/search.c | 3 +-
37 files changed, 199 insertions(+), 307 deletions(-)

$ git push
Counting objects: 179, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (69/69), done.
Writing objects: 100% (69/69), 8.92 KiB, done.
Total 69 (delta 64), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
178fe6a..e689473 master -> master

Added initial screened field value.

Metadata Update from @nkinder:
- Issue assigned to nhosoi
- Issue set to the milestone: 1.2.11.rc1

4 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/310

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Fixed)

a year ago

Login to comment on this ticket.

Metadata