From 26fea25025f08bb5285cd69fc8f0e6904f6fe955 Mon Sep 17 00:00:00 2001 From: "Thierry bordaz (tbordaz)" Date: Tue, 11 Jun 2013 11:18:15 +0200 Subject: [PATCH] Ticket 512 - improve performance of vattr code Bug Description: The first part of the fix was incompleted, breaking fix #490. The problem was that when an entry had no cached virtual attribute, we continued to call the SP. In fact the cache mechanism supported only one virtual attribute (nsroledn) and used the 'watermark' (in the entry) to know if the virtual attribute was already evaluated. Now the virtual cache contains several attributes and we can not rely on only watermark to know if the attribute was already evaluated. Fix Description: The fix implements a cache containing all the virtual attributes. It uses a hash table for lookup performance reason. If a virtual attribute is evaluated, its value is store in the cache. This even if the present value of the attribute is NULL (no Service Provider gave a result). The meaning of the watermark returns to its origin: it says if the cached virtual values are or not valid (a change in SP triggers invalidation of the cache). https://fedorahosted.org/389/ticket/512 Reviewed by: ? Platforms tested: F18 cos/roles acceptance #490 test case #512 test case Flag Day: no Doc impact: no --- ldap/servers/slapd/entry.c | 327 +++++++++++++++++++++++++++++++++++----- ldap/servers/slapd/proto-slap.h | 4 - ldap/servers/slapd/slap.h | 8 +- ldap/servers/slapd/vattr.c | 16 -- 4 files changed, 293 insertions(+), 62 deletions(-) diff --git a/ldap/servers/slapd/entry.c b/ldap/servers/slapd/entry.c index ea86fcc..3343975 100644 --- a/ldap/servers/slapd/entry.c +++ b/ldap/servers/slapd/entry.c @@ -64,6 +64,22 @@ /* a helper function to set special rdn to a tombstone entry */ static int _entry_set_tombstone_rdn(Slapi_Entry *e, const char *normdn); +/* computation of the size of the vattr in the entry */ +static void entry_vattr_READ_LOCK(const Slapi_Entry *e); +static void entry_vattr_READ_UNLOCK(const Slapi_Entry *e); +static void entry_vattr_WRITE_LOCK(const Slapi_Entry *e); +static void entry_vattr_WRITE_UNLOCK(const Slapi_Entry *e); +static PRIntn entry_vattr_hash_compare_keys(const void *v1, const void *v2); +static PRIntn entry_vattr_hash_compare_values(const void *v1, const void *v2); +static PLHashNumber entry_vattr_hash_fn(const void *type_name); +static PRIntn entry_vattr_size_hash_entry(PLHashEntry *he, PRIntn index, void *arg); +static size_t entry_vattr_size(Slapi_Entry *e); +static struct _entry_vattr *entry_vattr_lookup_nolock(Slapi_Entry *e, char *attr_name); +static void entry_vattr_add_nolock(Slapi_Entry *e, struct _entry_vattr *value); +static PRIntn entry_vattr_destroy_hash_entry(PLHashEntry *he, PRIntn index, void *arg); +static void entry_vattr_free_nolock(Slapi_Entry *e); +static void entry_vattr_alloc_nolock(Slapi_Entry *e); + /* protected attributes which are not included in the flattened entry, * which will be stored in the db. */ static char *protected_attrs_all [] = {PSEUDO_ATTR_UNHASHEDUSERPASSWORD, @@ -83,6 +99,14 @@ struct attrs_in_extension attrs_in_extension[] = {NULL, NULL, NULL} }; + +struct _entry_vattr { + char *key; /* key of the attribute (attribute name) in the virtual + * attribute hash table + */ + Slapi_Attr *vattr; /* attribute computed by a SP */ +}; + /* * An attribute name is of the form 'basename[;option]'. * The state informaion is encoded in options. For example: @@ -2055,7 +2079,9 @@ slapi_entry_free( Slapi_Entry *e ) /* JCM - Should be ** so that we can NULL the slapi_ch_free((void **)&e->e_uniqueid); attrlist_free(e->e_attrs); attrlist_free(e->e_deleted_attrs); - attrlist_free(e->e_virtual_attrs); + entry_vattr_WRITE_LOCK(e); + entry_vattr_free_nolock(e); + entry_vattr_WRITE_UNLOCK(e); if(e->e_virtual_lock) slapi_destroy_rwlock(e->e_virtual_lock); slapi_ch_free((void**)&e); @@ -2118,7 +2144,7 @@ slapi_entry_size(Slapi_Entry *e) size += slapi_rdn_get_size(&e->e_srdn); size += slapi_attrlist_size(e->e_attrs); if (e->e_deleted_attrs) size += slapi_attrlist_size(e->e_deleted_attrs); - if (e->e_virtual_attrs) size += slapi_attrlist_size(e->e_virtual_attrs); + size += entry_vattr_size(e); size += sizeof(Slapi_Entry); return size; @@ -2430,6 +2456,189 @@ void slapi_entrycache_vattrcache_watermark_invalidate() } } +/* The following functions control the virtual attribute cache + * stored in each entry (e_virtual_attrs). Access to that cache + * requires holding a lock (e_virtual_lock) + * + */ + +static void entry_vattr_READ_LOCK(const Slapi_Entry *e){ + slapi_rwlock_rdlock(e->e_virtual_lock); +} + +static void entry_vattr_READ_UNLOCK(const Slapi_Entry *e) { + slapi_rwlock_unlock(e->e_virtual_lock); + +} +static void entry_vattr_WRITE_LOCK(const Slapi_Entry *e){ + slapi_rwlock_wrlock(e->e_virtual_lock); + +} +static void entry_vattr_WRITE_UNLOCK(const Slapi_Entry *e){ + slapi_rwlock_unlock(e->e_virtual_lock); +} + +/* those 3 functions are used by the hash table mechanism */ +static PRIntn entry_vattr_hash_compare_keys(const void *v1, const void *v2) +{ + /* Should this be subtype aware, etc ? */ + return ( (0 == strcasecmp(v1,v2)) ? 1 : 0) ; +} + +static PRIntn entry_vattr_hash_compare_values(const void *v1, const void *v2) +{ + return( ((char *)v1 == (char *)v2 ) ? 1 : 0); +} + +static PLHashNumber entry_vattr_hash_fn(const void *type_name) +{ + /* need a hash function here */ + /* Sum the bytes for now */ + PLHashNumber result = 0; + char * current_position = NULL; + char current_char = 0; + for (current_position = (char*) type_name; *current_position; current_position++) { + current_char = tolower(*current_position); + result += current_char; + } + return result; +} + +/* This callback is used to compute the size of each virtual attribute + * cached in e_virtual_attrs + */ +static PRIntn +entry_vattr_size_hash_entry(PLHashEntry *he, PRIntn index, void *arg) +{ + struct _entry_vattr *value; + int *size; + size = (int *) arg; + + if (he == NULL) + return HT_ENUMERATE_NEXT; + + value = (struct _entry_vattr *) he->value; + if (value->key) { + *size += strlen(value->key); + } else { + LDAPDebug(LDAP_DEBUG_ANY, "Warning: virtual attribute entry (in the hash table) has a NULL key", 0, 0, 0); + } + PR_ASSERT(value); + /* let's use this function to compute the size, although here + * this is only one attribute + */ + *size += slapi_attrlist_size(value->vattr); + *size += sizeof(PLHashEntry); + + return HT_ENUMERATE_NEXT; +} + +/* enumerate all the vattr attributes and compute their cumul size */ +static size_t entry_vattr_size(Slapi_Entry *e) +{ + int size = 0; + + entry_vattr_READ_LOCK(e); + + if (e->e_virtual_attrs == NULL) { + entry_vattr_READ_UNLOCK(e); + return 0; + } + + PL_HashTableEnumerateEntries(e->e_virtual_attrs, entry_vattr_size_hash_entry, &size); + size += sizeof(struct PLHashTable); + + entry_vattr_READ_UNLOCK(e); + return (size); +} + +/* if attr_name has already been evaluated (and cached) then returns it + * Else it returns NULL + * The caller must hold e_virtual_lock in read or write + */ +static struct _entry_vattr *entry_vattr_lookup_nolock(Slapi_Entry *e, char *attr_name) +{ + struct _entry_vattr *vattr; + + if (e->e_virtual_attrs == NULL) { + return NULL; + } + + vattr = PL_HashTableLookupConst(e->e_virtual_attrs, (void*) attr_name); + + return (vattr); +} + +/* It adds an attribute in the virtual attribute cache + * The caller must hold e_virtual_lock in write + */ +static void entry_vattr_add_nolock(Slapi_Entry *e, struct _entry_vattr *value) +{ + if (e->e_virtual_attrs == NULL) { + LDAPDebug(LDAP_DEBUG_ANY, "Warning: entry %s no vattr hash table to store %s", + slapi_entry_get_ndn(e), value->key, 0); + return; + } + + if (entry_vattr_lookup_nolock(e, value->key)) { + /* sorry we keep only the first value (SP) */ + return; + } else { + PL_HashTableAdd(e->e_virtual_attrs, value->key, value); + } +} + +/* This call back is used when an entry get out of the entry cache + * It frees for each entry: + * - the key (attribute name) + * - the slapi_attr + * - the structure stored in the hash table + */ +static PRIntn +entry_vattr_destroy_hash_entry(PLHashEntry *he, PRIntn index, void *arg) +{ + struct _entry_vattr *value = NULL; + + if (he == NULL) + return HT_ENUMERATE_NEXT; + + value = (struct _entry_vattr *)he->value; + PR_ASSERT (value); + slapi_ch_free((void *) &value->key); + slapi_attr_free( &value->vattr ); + slapi_ch_free((void *) &value); + + return HT_ENUMERATE_NEXT; +} + +/* The caller must hold e_virtual_lock in write mode */ +static void entry_vattr_free_nolock(Slapi_Entry *e) +{ + /* destroy the content */ + PLHashTable *hash; + + hash = e->e_virtual_attrs; + if (hash == NULL) { + return; + } + + PL_HashTableEnumerateEntries(hash, entry_vattr_destroy_hash_entry, NULL); + + if (hash) + PL_HashTableDestroy(hash); + e->e_virtual_attrs = NULL; + +} + +/* Allocated the hash table used for the virtual attribute cache + * The caller must hold e_virtual_lock in write mode + */ +static void entry_vattr_alloc_nolock(Slapi_Entry *e) +{ + e->e_virtual_attrs = PL_NewHashTable(10, + entry_vattr_hash_fn, entry_vattr_hash_compare_keys, + entry_vattr_hash_compare_values, NULL, NULL); +} /* * slapi_entry_vattrcache_findAndTest() * @@ -2453,12 +2662,12 @@ slapi_entry_vattrcache_findAndTest( const Slapi_Entry *e, const char *type, filter_type_t filter_type, int *rc ) { - Slapi_Attr *tmp_attr = NULL; + struct _entry_vattr *vattr; int r= SLAPI_ENTRY_VATTR_NOT_RESOLVED; /* assume not resolved yet */ *rc = -1; - if ((e->e_virtual_attrs == NULL) || ! slapi_entry_vattrcache_watermark_isvalid(e)) { + if (! slapi_entry_vattrcache_watermark_isvalid(e)) { /* there is not virtual attribute cached or they are all invalid * just return */ @@ -2466,11 +2675,14 @@ slapi_entry_vattrcache_findAndTest( const Slapi_Entry *e, const char *type, } /* Check if the attribute is already cached */ - vattrcache_entry_READ_LOCK(e); - if (e->e_virtual_attrs) { - tmp_attr = attrlist_find(e->e_virtual_attrs, type); - if (tmp_attr != NULL) { - if (valueset_isempty(&(tmp_attr->a_present_values))) { + entry_vattr_READ_LOCK(e); + + if (vattr = entry_vattr_lookup_nolock(e, type)) { + /* That means this 'type' vattr was already evaluated */ + if (vattr->vattr != NULL) { + + /* that means it is a virtual attribute */ + if (valueset_isempty(&(vattr->vattr->a_present_values))) { /* * this is a vattr that has been * cached already but does not exist @@ -2483,20 +2695,20 @@ slapi_entry_vattrcache_findAndTest( const Slapi_Entry *e, const char *type, */ r = SLAPI_ENTRY_VATTR_RESOLVED_EXISTS; if (filter_type == FILTER_TYPE_AVA) { - *rc = plugin_call_syntax_filter_ava(tmp_attr, + *rc = plugin_call_syntax_filter_ava(vattr->vattr, f->f_choice, &f->f_ava); } else if (filter_type == FILTER_TYPE_SUBSTRING) { - *rc = plugin_call_syntax_filter_sub(NULL, tmp_attr, + *rc = plugin_call_syntax_filter_sub(NULL, vattr->vattr, &f->f_sub); } else if (filter_type == FILTER_TYPE_PRES) { /* type is there, that's all we need to know. */ *rc = 0; } } - } /* tmp_attr != NULL */ + } /* vattr->vattr != NULL */ } - vattrcache_entry_READ_UNLOCK(e); + entry_vattr_READ_UNLOCK(e); return r; } @@ -2523,11 +2735,11 @@ slapi_entry_vattrcache_find_values_and_type_ex( const Slapi_Entry *e, Slapi_ValueSet ***results, char ***actual_type_name) { - Slapi_Attr *tmp_attr = NULL; + struct _entry_vattr *vattr; int r = SLAPI_ENTRY_VATTR_NOT_RESOLVED; /* assume not resolved yet */ - if ((e->e_virtual_attrs == NULL) || ! slapi_entry_vattrcache_watermark_isvalid(e)) { + if (! slapi_entry_vattrcache_watermark_isvalid(e)) { /* there is not virtual attribute cached or they are all invalid * just return */ @@ -2535,11 +2747,14 @@ slapi_entry_vattrcache_find_values_and_type_ex( const Slapi_Entry *e, } /* check if the attribute is not already cached */ - vattrcache_entry_READ_LOCK(e); - if (e->e_virtual_attrs) { - tmp_attr = attrlist_find(e->e_virtual_attrs, type); - if (tmp_attr != NULL) { - if (valueset_isempty(&(tmp_attr->a_present_values))) { + entry_vattr_READ_LOCK(e); + if (vattr = entry_vattr_lookup_nolock(e, type)) { + /* That means this 'type' vattr was already evaluated */ + + if (vattr->vattr != NULL) { + + /* that means it is a virtual attribute */ + if (valueset_isempty(&(vattr->vattr->a_present_values))) { /* * this is a vattr that has been * cached already but does not exist @@ -2554,17 +2769,17 @@ slapi_entry_vattrcache_find_values_and_type_ex( const Slapi_Entry *e, r = SLAPI_ENTRY_VATTR_RESOLVED_EXISTS; *results = (Slapi_ValueSet**) slapi_ch_calloc(1, sizeof (**results)); - **results = valueset_dup(&(tmp_attr->a_present_values)); + **results = valueset_dup(&(vattr->vattr->a_present_values)); *actual_type_name = (char**) slapi_ch_malloc(sizeof (**actual_type_name)); - slapi_attr_get_type(tmp_attr, &vattr_type); + slapi_attr_get_type(vattr->vattr, &vattr_type); **actual_type_name = slapi_ch_strdup(vattr_type); } - } /* tmp_attr != NULL */ + } /* vattr->vattr != NULL */ } - vattrcache_entry_READ_UNLOCK(e); + entry_vattr_READ_UNLOCK(e); return r; } @@ -2579,11 +2794,11 @@ slapi_entry_vattrcache_find_values_and_type( const Slapi_Entry *e, Slapi_ValueSet **results, char **actual_type_name) { - Slapi_Attr *tmp_attr = NULL; + struct _entry_vattr *vattr; int r= SLAPI_ENTRY_VATTR_NOT_RESOLVED; /* assume not resolved yet */ - if ((e->e_virtual_attrs == NULL) || ! slapi_entry_vattrcache_watermark_isvalid(e)) { + if (! slapi_entry_vattrcache_watermark_isvalid(e)) { /* there is not virtual attribute cached or they are all invalid * just return */ @@ -2591,11 +2806,14 @@ slapi_entry_vattrcache_find_values_and_type( const Slapi_Entry *e, } /* Check if the attribute is already cached */ - vattrcache_entry_READ_LOCK(e); - if (e->e_virtual_attrs) { - tmp_attr = attrlist_find(e->e_virtual_attrs, type); - if (tmp_attr != NULL) { - if (valueset_isempty(&(tmp_attr->a_present_values))) { + entry_vattr_READ_LOCK(e); + if (vattr = entry_vattr_lookup_nolock(e, type)) { + /* That means this 'type' vattr was already evaluated */ + + if (vattr->vattr != NULL) { + /* that means it is a virtual attribute */ + + if (valueset_isempty(&(vattr->vattr->a_present_values))) { /* * this is a vattr that has been * cached already but does not exist @@ -2609,15 +2827,15 @@ slapi_entry_vattrcache_find_values_and_type( const Slapi_Entry *e, char *vattr_type = NULL; r = SLAPI_ENTRY_VATTR_RESOLVED_EXISTS; - *results = valueset_dup(&(tmp_attr->a_present_values)); + *results = valueset_dup(&(vattr->vattr->a_present_values)); - slapi_attr_get_type(tmp_attr, &vattr_type); + slapi_attr_get_type(vattr->vattr, &vattr_type); *actual_type_name = slapi_ch_strdup(vattr_type); } } } - vattrcache_entry_READ_UNLOCK(e); + entry_vattr_READ_UNLOCK(e); return r; } @@ -2652,26 +2870,53 @@ slapi_entry_vattrcache_merge_sv(Slapi_Entry *e, const char *type, Slapi_ValueSet *valset, int buffer_flags) { Slapi_Value **vals = NULL; + struct _entry_vattr *vattr; /* only attempt to merge if it's a cacheable attribute */ if ( slapi_vattrcache_iscacheable(type) || (buffer_flags & SLAPI_VIRTUALATTRS_VALUES_CACHEABLE)) { - vattrcache_entry_WRITE_LOCK(e); + entry_vattr_WRITE_LOCK(e); - if(!slapi_entry_vattrcache_watermark_isvalid(e) && e->e_virtual_attrs) + if(!slapi_entry_vattrcache_watermark_isvalid(e)) { - attrlist_free(e->e_virtual_attrs); - e->e_virtual_attrs = NULL; + /* free the previous set of vattrs */ + entry_vattr_free_nolock(e); + } + /* allocate a new vattr hash table if required */ + if (e->e_virtual_attrs == NULL) { + entry_vattr_alloc_nolock(e); + } if(valset) vals = valueset_get_valuearray(valset); - /* dups the type (if necessary) and vals */ - attrlist_merge_valuearray( &e->e_virtual_attrs, type, vals); + /* Add the vals in the hashtable */ + vattr = entry_vattr_lookup_nolock(e, type); + if (vattr) { + /* virtual attribute already cached, add the value */ + valueset_add_valuearray( &vattr->vattr->a_present_values, vals ); + } else { + Slapi_Attr *attr = NULL; + + /* Create the new virtual attribute */ + attr = slapi_attr_new(); + slapi_attr_init(attr, type); + + /* then the slot to add it in the hash table */ + vattr = (struct _entry_vattr *)slapi_ch_calloc( 1, sizeof(struct _entry_vattr)); + vattr->key = slapi_ch_strdup(type); + vattr->vattr = attr; + + /* now add the value */ + valueset_add_valuearray( &vattr->vattr->a_present_values, vals ); + + /* finally put it into the hash table */ + entry_vattr_add_nolock(e, vattr); + } slapi_entry_vattrcache_watermark_set(e); - vattrcache_entry_WRITE_UNLOCK(e); + entry_vattr_WRITE_UNLOCK(e); } diff --git a/ldap/servers/slapd/proto-slap.h b/ldap/servers/slapd/proto-slap.h index 9fe26ff..914793f 100644 --- a/ldap/servers/slapd/proto-slap.h +++ b/ldap/servers/slapd/proto-slap.h @@ -1359,10 +1359,6 @@ void subentry_create_filter(Slapi_Filter** filter); */ void vattr_init(); void vattr_cleanup(); -void vattrcache_entry_READ_LOCK(const Slapi_Entry *e); -void vattrcache_entry_READ_UNLOCK(const Slapi_Entry *e); -void vattrcache_entry_WRITE_LOCK(const Slapi_Entry *e); -void vattrcache_entry_WRITE_UNLOCK(const Slapi_Entry *e); /* * slapd_plhash.c - supplement to NSPR plhash diff --git a/ldap/servers/slapd/slap.h b/ldap/servers/slapd/slap.h index 1039b0a..06399eb 100644 --- a/ldap/servers/slapd/slap.h +++ b/ldap/servers/slapd/slap.h @@ -614,6 +614,12 @@ struct slapi_dn int ndn_len; /* normalized dn length */ }; +struct vattr_evaluated +{ + char *attr_name; + struct vattr_evaluated *next; +}; + /* * Represents a Relative Distinguished Name. */ @@ -656,7 +662,7 @@ struct slapi_entry { CSN *e_maxcsn; /* maximum CSN of the entry */ Slapi_Attr *e_attrs; /* list of attributes and values */ Slapi_Attr *e_deleted_attrs; /* deleted list of attributes and values */ - Slapi_Attr *e_virtual_attrs; /* list of virtual attributes */ + PLHashTable *e_virtual_attrs; /* cache of virtual attributes */ time_t e_virtual_watermark; /* indicates cache consistency when compared to global watermark */ Slapi_RWLock *e_virtual_lock; /* for access to cached vattrs */ diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c index b04fca7..d7753ee 100644 --- a/ldap/servers/slapd/vattr.c +++ b/ldap/servers/slapd/vattr.c @@ -2348,22 +2348,6 @@ void slapi_vattrcache_cache_none() cache_all = 0; } -void vattrcache_entry_READ_LOCK(const Slapi_Entry *e){ - slapi_rwlock_rdlock(e->e_virtual_lock); -} - -void vattrcache_entry_READ_UNLOCK(const Slapi_Entry *e) { - slapi_rwlock_unlock(e->e_virtual_lock); - -} -void vattrcache_entry_WRITE_LOCK(const Slapi_Entry *e){ - slapi_rwlock_wrlock(e->e_virtual_lock); - -} -void vattrcache_entry_WRITE_UNLOCK(const Slapi_Entry *e){ - slapi_rwlock_unlock(e->e_virtual_lock); -} - Slapi_PBlock * slapi_vattr_get_pblock_from_context(vattr_context *c) { -- 1.7.11.7