From: Arran Cudbard-Bell Date: Thu, 24 May 2018 10:39:32 +0000 (+0600) Subject: Minor memory efficiency improvements in dictionaries X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=031bf4f717fdaa63db9c68dbd4b94d0d2f96a96b;p=thirdparty%2Ffreeradius-server.git Minor memory efficiency improvements in dictionaries There's no advantage to using individual pools for attribute names as all attributes are allocated within a pool which ensures the name buffer allocation is contiguous. --- diff --git a/src/lib/util/dict.c b/src/lib/util/dict.c index 6042f49b62e..93bc9fab01f 100644 --- a/src/lib/util/dict.c +++ b/src/lib/util/dict.c @@ -98,7 +98,10 @@ struct fr_dict { fr_hash_table_t *values_by_alias; //!< Lookup an attribute enum by its alias name. fr_dict_attr_t *root; //!< Root attribute of this dictionary. + TALLOC_CTX *pool; //!< Talloc memory pool to reduce allocs. + TALLOC_CTX *fixup_pool; //!< Temporary pool for fixups, reduces holes + ///< in the dictionary. }; /** Map data types to names representing those types @@ -1135,7 +1138,7 @@ static bool dict_attr_fields_valid(fr_dict_t *dict, fr_dict_attr_t const *parent return true; } -/** Allocate a dictionary attribute as a pool, and assign a name buffer +/** Allocate a dictionary attribute and assign a name * * @param[in] ctx to allocate attribute in. * @param[in] name to set. @@ -1152,13 +1155,7 @@ static fr_dict_attr_t *dict_attr_alloc_name(TALLOC_CTX *ctx, char const *name) return NULL; } -#ifdef HAVE_TALLOC_POOLED_OBJECT - da = talloc_pooled_object(ctx, fr_dict_attr_t, 1, strlen(name) + 1); - memset(da, 0, sizeof(*da)); -#else da = talloc_zero(ctx, fr_dict_attr_t); -#endif - da->name = talloc_typed_strdup(da, name); if (!da->name) { talloc_free(da); @@ -1290,13 +1287,7 @@ static fr_dict_attr_t *dict_attr_ref_alloc(fr_dict_t *dict, fr_dict_attr_t const return NULL; } -#ifdef HAVE_TALLOC_POOLED_OBJECT - ref_n = talloc_pooled_object(dict->pool, fr_dict_attr_ref_t, 1, strlen(name) + 1); - memset(ref_n, 0, sizeof(*ref_n)); -#else ref_n = talloc_zero(dict->pool, fr_dict_attr_ref_t); -#endif - ref_n->tlv.name = talloc_typed_strdup(ref, name); if (!ref_n->tlv.name) { talloc_free(ref_n); @@ -1376,13 +1367,7 @@ static int dict_vendor_add(fr_dict_t *dict, char const *name, unsigned int num) return -1; } -#ifdef HAVE_TALLOC_POOLED_OBJECT - vendor = talloc_pooled_object(dict, fr_dict_vendor_t, 1, strlen(name) + 1); - memset(vendor, 0, sizeof(*vendor)); -#else vendor = talloc_zero(dict, fr_dict_vendor_t); -#endif - vendor->name = talloc_typed_strdup(vendor, name); if (!vendor->name) { talloc_free(vendor); @@ -1455,8 +1440,10 @@ static inline int dict_attr_child_add(fr_dict_attr_t *parent, fr_dict_attr_t *ch * We only allocate the pointer array *if* the parent has children. */ if (!parent->children) parent->children = talloc_zero_array(parent, fr_dict_attr_t const *, UINT8_MAX + 1); - if (!parent->children) return -1; - + if (!parent->children) { + fr_strerror_printf("Out of memory"); + return -1; + } /* * Treat the array as a hash of 255 bins, with attributes * sorted into bins using num % 255. @@ -3679,14 +3666,16 @@ static fr_dict_t *dict_alloc(TALLOC_CTX *ctx) } /* - * Pre-Allocate 2MB of pool memory for rapid startup - * This is about what's reported for all current dictionary - * attributes in the monolithic dictionary, and will go - * down once the dictionaries are split. + * Pre-Allocate 6MB of pool memory for rapid startup + * As that's the working memory required during + * dictionary initialisation. */ - dict->pool = talloc_pool(dict, (1024 * 1024 * 2)); + dict->pool = talloc_pool(dict, (1024 * 1024 * 6)); if (!dict->pool) goto error; + dict->fixup_pool = talloc_pool(dict, (1024 * 1024 * 1)); + if (!dict->fixup_pool) goto error; + /* * Create the table of vendor by name. There MAY NOT * be multiple vendors of the same name. @@ -4112,7 +4101,7 @@ static int dict_read_process_value(fr_dict_t *dict, char **argv, int argc) if (!da) { dict_enum_fixup_t *fixup; - fixup = talloc_zero(dict->pool, dict_enum_fixup_t); + fixup = talloc_zero(dict->fixup_pool, dict_enum_fixup_t); if (!fixup) { oom: talloc_free(fixup); @@ -4969,7 +4958,7 @@ int fr_dict_from_file(fr_dict_t **out, char const *fn) if (!defined_cast_types) { FR_NAME_NUMBER const *p; fr_dict_attr_flags_t flags; - char *type_name; + char *type_name = NULL; memset(&flags, 0, sizeof(flags)); @@ -4978,7 +4967,10 @@ int fr_dict_from_file(fr_dict_t **out, char const *fn) for (p = dict_attr_types; p->name; p++) { fr_dict_attr_t *n; - type_name = talloc_typed_asprintf(dict->pool, "Tmp-Cast-%s", p->name); + /* + * Reduce holes in the main pool by using the fixup pool + */ + type_name = talloc_typed_asprintf(dict->fixup_pool, "Tmp-Cast-%s", p->name); n = dict_attr_alloc(dict->pool, dict->root, type_name, FR_CAST_BASE + p->number, p->number, &flags); @@ -4987,6 +4979,7 @@ int fr_dict_from_file(fr_dict_t **out, char const *fn) talloc_free(dict); return -1; } + talloc_free(type_name); if (!fr_hash_table_insert(dict->attributes_by_name, n)) goto error; @@ -4994,8 +4987,6 @@ int fr_dict_from_file(fr_dict_t **out, char const *fn) * Set up parenting for the attribute. */ if (dict_attr_child_add(dict->root, n) < 0) goto error; - - talloc_free(type_name); } defined_cast_types = true; } @@ -5037,6 +5028,7 @@ int fr_dict_from_file(fr_dict_t **out, char const *fn) dict->enum_fixup = next; } } + TALLOC_FREE(dict->fixup_pool); /* * Walk over all of the hash tables to ensure they're @@ -5112,23 +5104,31 @@ int fr_dict_internal_afrom_file(fr_dict_t **out, char const *dict_subdir) for (p = dict_attr_types; p->name; p++) { fr_dict_attr_t *n; - type_name = talloc_typed_asprintf(dict->pool, "Tmp-Cast-%s", p->name); + /* + * Reduce holes in the main pool by using the fixup pool + */ + type_name = talloc_typed_asprintf(dict->fixup_pool, "Tmp-Cast-%s", p->name); n = dict_attr_alloc(dict->pool, dict->root, type_name, FR_CAST_BASE + p->number, p->number, &flags); - if (!n) goto error; + if (!n) { + talloc_free(type_name); + goto error; + } if (!fr_hash_table_insert(dict->attributes_by_name, n)) { fr_strerror_printf("Failed inserting \"%s\" into internal dictionary", type_name); + talloc_free(type_name); goto error; } + talloc_free(type_name); + /* * Set up parenting for the attribute. */ if (dict_attr_child_add(dict->root, n) < 0) goto error; - talloc_free(type_name); } if (dict_path && dict_from_file(dict, dict_path, FR_DICTIONARY_FILE, NULL, 0) < 0) goto error; @@ -5212,6 +5212,7 @@ int fr_dict_protocol_afrom_file(fr_dict_t **out, char const *proto_name) for (this = dict->enum_fixup; this != NULL; this = next) { fr_value_box_t value; fr_type_t type; + int ret; next = this->next; da = fr_dict_attr_by_name(dict, this->attribute); @@ -5228,7 +5229,10 @@ int fr_dict_protocol_afrom_file(fr_dict_t **out, char const *proto_name) goto error; } - if (fr_dict_enum_add_alias(da, this->alias, &value, false, false) < 0) goto error; + ret = fr_dict_enum_add_alias(da, this->alias, &value, false, false); + fr_value_box_clear(&value); + + if (ret < 0) goto error; /* * Just so we don't lose track of things. @@ -5236,6 +5240,7 @@ int fr_dict_protocol_afrom_file(fr_dict_t **out, char const *proto_name) dict->enum_fixup = next; } } + TALLOC_FREE(dict->fixup_pool); /* * Walk over all of the hash tables to ensure they're diff --git a/src/main/radict.c b/src/main/radict.c index d1992d18b90..617bc11c772 100644 --- a/src/main/radict.c +++ b/src/main/radict.c @@ -132,11 +132,11 @@ static void da_print_info_td(fr_dict_t const *dict, fr_dict_attr_t const *da) fr_dict_snprint_flags(flags, sizeof(flags), &da->flags); /* Protocol Name Type */ - printf("%s\t%s\t%s\t%s\t%s\n", fr_dict_root(dict)->name, oid_str, da->name, + printf("%p, %s\t%s\t%s\t%s\t%s\n", da, fr_dict_root(dict)->name, oid_str, da->name, fr_int2str(dict_attr_types, da->type, "?Unknown?"), flags); } -static void _fr_dict_export(uint64_t *count, fr_dict_attr_t const *da, unsigned int lvl) +static void _fr_dict_export(uint64_t *count, uintptr_t *low, uintptr_t *high, fr_dict_attr_t const *da, unsigned int lvl) { unsigned int i; size_t len; @@ -145,24 +145,38 @@ static void _fr_dict_export(uint64_t *count, fr_dict_attr_t const *da, unsigned fr_dict_snprint_flags(flags, sizeof(flags), &da->flags); - if (!da->flags.is_root) da_print_info_td(fr_dict_by_da(da), da); - (*count)++; + /* + * Root attributes are allocated outside of the pool + * so it's not helpful to include them in the calculation. + */ + if (!da->flags.is_root) { + if (low && ((uintptr_t)da < *low)) { + *low = da; + } + if (high && ((uintptr_t)da > *high)) { + *high = da; + } + + da_print_info_td(fr_dict_by_da(da), da); + } + + if (count) (*count)++; len = talloc_array_length(da->children); for (i = 0; i < len; i++) { for (p = da->children[i]; p; p = p->next) { - _fr_dict_export(count, p, lvl + 1); + _fr_dict_export(count, low, high, p, lvl + 1); } } } -static uint64_t fr_dict_export(fr_dict_t *dict) +static void fr_dict_export(uint64_t *count, uintptr_t *low, uintptr_t *high, fr_dict_t *dict) { - uint64_t count = 0; + if (count) *count = 0; + if (low) *low = UINTPTR_MAX; + if (high) *high = 0; - _fr_dict_export(&count, fr_dict_root(dict), 0); - - return count; + _fr_dict_export(count, low, high, fr_dict_root(dict), 0); } int main(int argc, char *argv[]) @@ -245,11 +259,14 @@ int main(int argc, char *argv[]) fr_dict_t **dict_p = dicts; do { - uint64_t count; - - count = fr_dict_export(*dict_p); - DEBUG2("Total attr %" PRIu64, count); - DEBUG2("Total size %zu (bytes)", talloc_total_size(*dict_p)); + uint64_t count; + uintptr_t high; + uintptr_t low; + + fr_dict_export(&count, &low, &high, *dict_p); + DEBUG2("Attribute count %" PRIu64, count); + DEBUG2("Memory alloc %zu (bytes)", talloc_total_size(*dict_p)); + DEBUG2("Memory spread %zu (bytes)", (high - low)); } while (++dict_p < dict_end); }