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.
git patch file (master) 0001-Trac-Ticket-310-Avoid-calling-escape_string-for-logg.patch
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
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.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.