[Freeipa-devel] [PATCH] 0007 Performace optimization for ldap_parse_configentry

[Petr Spacek]

On 02/28/2012 02:29 PM, Adam Tkac wrote:
> On 02/23/2012 04:31 PM, Petr Spacek wrote:
>> Hello,
>>
>> this patch is performance optimization of yesterday's fix
>> https://fedorahosted.org/bind-dyndb-ldap/ticket/43 - hold bind and
>> plugin global settings in LDAP.
> Thanks for the patch, Petr, please check my comment below and then push
> the patch to master.
>
> Regards, Adam
Fixed and pushed to master.

https://fedorahosted.org/bind-dyndb-ldap/changeset/08b311d5baf992f415cba46783325c2564e9be4a/

-- 
Petr^2 Spacek
>>
>>
>> bind-dyndb-ldap-pspacek-0007-Performace-optimization-for-ldap_parse_configentry.patch
>>
>>
>>
>> From 61c4da9c39d3b42594dab39779da2495970d34f4 Mon Sep 17 00:00:00 2001
>> From: Petr Spacek<pspacek at redhat.com>
>> Date: Thu, 23 Feb 2012 16:14:13 +0100
>> Subject: [PATCH] Performace optimization for ldap_parse_configentry().
>> Signed-off-by: Petr Spacek<pspacek at redhat.com>
>>
>> ---
>> src/ldap_helper.c | 24 ++++++++++++++----------
>> 1 files changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
>> index bb1166d..42dbd74 100644
>> --- a/src/ldap_helper.c
>> +++ b/src/ldap_helper.c
>> @@ -966,17 +966,13 @@ ldap_parse_configentry(ldap_entry_t *entry,
>> ldap_instance_t *inst)
>> isc_result_t result;
>> ldap_valuelist_t values;
>> isc_boolean_t unlock_required;
> If I read code correctly the "unlock_required" variable is no longer
> needed, is it? Please remove it's declaration.
>> + isc_boolean_t sync_ptr_new;
>> isc_timer_t *timer_inst;
>> isc_interval_t timer_interval;
>> isc_uint32_t interval_sec;
>> isc_timertype_t timer_type;
>>
>> - /* Before changing parameters transit to single-thread operation. */
>> - result = isc_task_beginexclusive(inst->task);
>> - RUNTIME_CHECK(result == ISC_R_SUCCESS ||
>> - result == ISC_R_LOCKBUSY);
>> - if (result == ISC_R_SUCCESS)
>> - unlock_required = ISC_TRUE;
>> + /* BIND functions are thread safe, lock only ldap instance 'inst'. */
>>
>> log_debug(3, "Parsing configuration object");
>>
>> @@ -993,8 +989,18 @@ ldap_parse_configentry(ldap_entry_t *entry,
>> ldap_instance_t *inst)
>> result = ldap_entry_getvalues(entry, "idnsAllowSyncPTR",&values);
>> if (result == ISC_R_SUCCESS) {
>> log_debug(2, "Setting global AllowSyncPTR = %s", HEAD(values)->value);
>> - inst->sync_ptr = (strcmp(HEAD(values)->value, "TRUE") == 0)
>> + sync_ptr_new = (strcmp(HEAD(values)->value, "TRUE") == 0)
>> ? ISC_TRUE : ISC_FALSE;
>> +
>> + if (inst->sync_ptr != sync_ptr_new) { /* lock BIND only if necessary */
>> + result = isc_task_beginexclusive(inst->task);
>> + RUNTIME_CHECK(result == ISC_R_SUCCESS ||
>> + result == ISC_R_LOCKBUSY);
>> + inst->sync_ptr = sync_ptr_new;
>> + if (result == ISC_R_SUCCESS) {
>> + isc_task_endexclusive(inst->task);
>> + }
>> + }
>> }
>>
>> result = ldap_entry_getvalues(entry, "idnsZoneRefresh",&values);
>> @@ -1019,9 +1025,7 @@ ldap_parse_configentry(ldap_entry_t *entry,
>> ldap_instance_t *inst)
>> }
>>
>> cleanup:
>> - if (unlock_required == ISC_TRUE)
>> - isc_task_endexclusive(inst->task);
>> -
>> + /* Configuration errors are not fatal. */
>> return ISC_R_SUCCESS;
>> }
>>
>