From b89a279be96dd60b2b33fdae9821d17e3a9b74cb Mon Sep 17 00:00:00 2001 From: Ondrej Kusnirik Date: Thu, 15 Aug 2024 09:09:13 +0200 Subject: [PATCH 1/2] hash table BUGFIX remove unnecessary variable --- src/hash_table.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/hash_table.c b/src/hash_table.c index f951e6aa1..88531a982 100644 --- a/src/hash_table.c +++ b/src/hash_table.c @@ -247,7 +247,6 @@ lyht_resize(struct ly_ht *ht, int operation, int check) * @param[in] hash Hash to find. * @param[in] mod Whether the operation modifies the hash table (insert or remove) or not (find). * @param[in] val_equal Callback for checking value equivalence. - * @param[out] crec_p Optional found first record. * @param[out] col Optional collision number of @p rec_p, 0 for no collision. * @param[out] rec_p Found exact matching record, may be a collision of @p crec_p. * @return LY_ENOTFOUND if no record found, @@ -255,15 +254,12 @@ lyht_resize(struct ly_ht *ht, int operation, int check) */ static LY_ERR lyht_find_rec(const struct ly_ht *ht, void *val_p, uint32_t hash, ly_bool mod, lyht_value_equal_cb val_equal, - struct ly_ht_rec **crec_p, uint32_t *col, struct ly_ht_rec **rec_p) + uint32_t *col, struct ly_ht_rec **rec_p) { uint32_t hlist_idx = hash & (ht->size - 1); struct ly_ht_rec *rec; uint32_t rec_idx; - if (crec_p) { - *crec_p = NULL; - } if (col) { *col = 0; } @@ -271,9 +267,6 @@ lyht_find_rec(const struct ly_ht *ht, void *val_p, uint32_t hash, ly_bool mod, l LYHT_ITER_HLIST_RECS(ht, hlist_idx, rec_idx, rec) { if ((rec->hash == hash) && val_equal(val_p, &rec->val, mod, ht->cb_data)) { - if (crec_p) { - *crec_p = rec; - } *rec_p = rec; return LY_SUCCESS; } @@ -292,7 +285,7 @@ lyht_find(const struct ly_ht *ht, void *val_p, uint32_t hash, void **match_p) { struct ly_ht_rec *rec; - lyht_find_rec(ht, val_p, hash, 0, ht->val_equal, NULL, NULL, &rec); + lyht_find_rec(ht, val_p, hash, 0, ht->val_equal, NULL, &rec); if (rec && match_p) { *match_p = rec->val; @@ -305,7 +298,7 @@ lyht_find_with_val_cb(const struct ly_ht *ht, void *val_p, uint32_t hash, lyht_v { struct ly_ht_rec *rec; - lyht_find_rec(ht, val_p, hash, 0, val_equal ? val_equal : ht->val_equal, NULL, NULL, &rec); + lyht_find_rec(ht, val_p, hash, 0, val_equal ? val_equal : ht->val_equal, NULL, &rec); if (rec && match_p) { *match_p = rec->val; @@ -317,12 +310,12 @@ LIBYANG_API_DEF LY_ERR lyht_find_next_with_collision_cb(const struct ly_ht *ht, void *val_p, uint32_t hash, lyht_value_equal_cb collision_val_equal, void **match_p) { - struct ly_ht_rec *rec, *crec; + struct ly_ht_rec *rec; uint32_t rec_idx; uint32_t i; /* find the record of the previously found value */ - if (lyht_find_rec(ht, val_p, hash, 1, ht->val_equal, &crec, &i, &rec)) { + if (lyht_find_rec(ht, val_p, hash, 1, ht->val_equal, &i, &rec)) { /* not found, cannot happen */ LOGINT_RET(NULL); } @@ -373,7 +366,7 @@ _lyht_insert_with_resize_cb(struct ly_ht *ht, void *val_p, uint32_t hash, lyht_v uint32_t rec_idx; if (check) { - if (lyht_find_rec(ht, val_p, hash, 1, ht->val_equal, NULL, NULL, &rec) == LY_SUCCESS) { + if (lyht_find_rec(ht, val_p, hash, 1, ht->val_equal, NULL, &rec) == LY_SUCCESS) { if (rec && match_p) { *match_p = rec->val; } @@ -459,7 +452,7 @@ lyht_remove_with_resize_cb(struct ly_ht *ht, void *val_p, uint32_t hash, lyht_va uint32_t prev_rec_idx; uint32_t rec_idx; - if (lyht_find_rec(ht, val_p, hash, 1, ht->val_equal, NULL, NULL, &found_rec)) { + if (lyht_find_rec(ht, val_p, hash, 1, ht->val_equal, NULL, &found_rec)) { LOGARG(NULL, hash); return LY_ENOTFOUND; } From ae8dd311c35b54244f0dd6678e0534cb5d087898 Mon Sep 17 00:00:00 2001 From: Ondrej Kusnirik Date: Thu, 15 Aug 2024 10:14:11 +0200 Subject: [PATCH 2/2] dict OPTIMIZE duplicating values in dictionary --- src/dict.c | 55 ++++++++++++++++++++++++++---- src/dict.h | 12 +++++++ src/path.c | 26 +++++++------- src/path.h | 3 +- src/plugins_types.c | 18 ++++++---- src/plugins_types/instanceid.c | 2 +- src/schema_compile.c | 4 +-- src/tree_data.c | 2 +- src/tree_data_new.c | 2 +- src/tree_schema.c | 4 +-- src/tree_schema_common.c | 2 +- src/xpath.c | 6 ++-- tests/utests/data/test_tree_data.c | 2 +- 13 files changed, 98 insertions(+), 40 deletions(-) diff --git a/src/dict.c b/src/dict.c index 9f741a22a..ba574c8ca 100644 --- a/src/dict.c +++ b/src/dict.c @@ -104,9 +104,6 @@ lydict_resize_val_eq(void *val1_p, void *val2_p, ly_bool mod, void *UNUSED(cb_da str1 = ((struct ly_dict_rec *)val1_p)->value; str2 = ((struct ly_dict_rec *)val2_p)->value; - LY_CHECK_ERR_RET(!str1, LOGARG(NULL, val1_p), 0); - LY_CHECK_ERR_RET(!str2, LOGARG(NULL, val2_p), 0); - if (mod) { /* used when inserting new values */ if (strcmp(str1, str2) == 0) { @@ -177,7 +174,7 @@ lydict_remove(const struct ly_ctx *ctx, const char *value) return ret; } -LY_ERR +static LY_ERR dict_insert(const struct ly_ctx *ctx, char *value, size_t len, ly_bool zerocopy, const char **str_p) { LY_ERR ret = LY_SUCCESS; @@ -221,9 +218,7 @@ dict_insert(const struct ly_ctx *ctx, char *value, size_t len, ly_bool zerocopy, return ret; } - if (str_p) { - *str_p = match->value; - } + *str_p = match->value; return ret; } @@ -269,3 +264,49 @@ lydict_insert_zc(const struct ly_ctx *ctx, char *value, const char **str_p) return result; } + +static LY_ERR +dict_dup(const struct ly_ctx *ctx, char *value, const char **str_p) +{ + LY_ERR ret = LY_SUCCESS; + struct ly_dict_rec *match = NULL, rec; + uint32_t hash; + + /* set new callback to only compare memory addresses */ + lyht_value_equal_cb prev = lyht_set_cb(ctx->dict.hash_tab, lydict_resize_val_eq); + + LOGDBG(LY_LDGDICT, "duplicating %s", value); + hash = lyht_hash(value, strlen(value)); + rec.value = value; + + ret = lyht_find(ctx->dict.hash_tab, (void *)&rec, hash, (void **)&match); + if (ret == LY_SUCCESS) { + /* record found, increase refcount */ + match->refcount++; + *str_p = match->value; + } + + /* restore callback */ + lyht_set_cb(ctx->dict.hash_tab, prev); + + return ret; +} + +LIBYANG_API_DEF LY_ERR +lydict_dup(const struct ly_ctx *ctx, const char *value, const char **str_p) +{ + LY_ERR result; + + LY_CHECK_ARG_RET(ctx, ctx, str_p, LY_EINVAL); + + if (!value) { + *str_p = NULL; + return LY_SUCCESS; + } + + pthread_mutex_lock((pthread_mutex_t *)&ctx->dict.lock); + result = dict_dup(ctx, (char *)value, str_p); + pthread_mutex_unlock((pthread_mutex_t *)&ctx->dict.lock); + + return result; +} diff --git a/src/dict.h b/src/dict.h index cf897e70f..bc8cac1fe 100644 --- a/src/dict.h +++ b/src/dict.h @@ -113,6 +113,18 @@ LIBYANG_API_DECL LY_ERR lydict_insert_zc(const struct ly_ctx *ctx, char *value, */ LIBYANG_API_DECL LY_ERR lydict_remove(const struct ly_ctx *ctx, const char *value); +/** + * @brief Duplicate string in dictionary. Only a reference counter is incremented. + * + * @param[in] ctx libyang context handler + * @param[in] value NULL-terminated string to be duplicated in the dictionary (reference counter is incremented). + * @param[out] str_p Optional parameter to get pointer to the string corresponding to the @p value and stored in dictionary. + * @return LY_SUCCESS in case the string already exists in the dictionary. + * @return LY_ENOTFOUND in case the string was not found. + * @return LY_ERR on other errors + */ +LIBYANG_API_DECL LY_ERR lydict_dup(const struct ly_ctx *ctx, const char *value, const char **str_p); + /** @} dict */ #ifdef __cplusplus diff --git a/src/path.c b/src/path.c index 9a774141b..387676269 100644 --- a/src/path.c +++ b/src/path.c @@ -708,7 +708,7 @@ ly_path_compile_predicate(const struct ly_ctx *ctx, const struct lysc_node *cur_ /* store the value */ LOG_LOCSET(key, NULL); - ret = lyd_value_store(ctx, &p->value, ((struct lysc_node_leaf *)key)->type, val, val_len, 0, 0, + ret = lyd_value_store(ctx_node->module->ctx, &p->value, ((struct lysc_node_leaf *)key)->type, val, val_len, 0, 0, NULL, format, prefix_data, LYD_HINT_DATA, key, NULL); LOG_LOCBACK(1, 0); LY_CHECK_ERR_GOTO(ret, p->value.realtype = NULL, cleanup); @@ -736,7 +736,7 @@ ly_path_compile_predicate(const struct ly_ctx *ctx, const struct lysc_node *cur_ /* names (keys) are unique - it was checked when parsing */ LOGVAL(ctx, LYVE_XPATH, "Predicate missing for a key of %s \"%s\" in path.", lys_nodetype2str(ctx_node->nodetype), ctx_node->name); - ly_path_predicates_free(ctx, *predicates); + ly_path_predicates_free(ctx_node->module->ctx, *predicates); *predicates = NULL; ret = LY_EVALID; goto cleanup; @@ -771,12 +771,10 @@ ly_path_compile_predicate(const struct ly_ctx *ctx, const struct lysc_node *cur_ } /* store the value */ - if (ctx_node) { - LOG_LOCSET(ctx_node, NULL); - } - ret = lyd_value_store(ctx, &p->value, ((struct lysc_node_leaflist *)ctx_node)->type, val, val_len, 0, 0, + LOG_LOCSET(ctx_node, NULL); + ret = lyd_value_store(ctx_node->module->ctx, &p->value, ((struct lysc_node_leaflist *)ctx_node)->type, val, val_len, 0, 0, NULL, format, prefix_data, LYD_HINT_DATA, ctx_node, NULL); - LOG_LOCBACK(ctx_node ? 1 : 0, 0); + LOG_LOCBACK(1, 0); LY_CHECK_ERR_GOTO(ret, p->value.realtype = NULL, cleanup); ++(*tok_idx); @@ -1093,7 +1091,7 @@ ly_path_compile_deref(const struct ly_ctx *ctx, const struct lysc_node *ctx_node } lref = (const struct lysc_type_leafref *)deref_leaf_node->type; LY_CHECK_GOTO(ret = ly_path_append(ctx, path2, path), cleanup); - ly_path_free(ctx, path2); + ly_path_free(path2); path2 = NULL; /* compile dereferenced leafref expression and append it to the path */ @@ -1101,7 +1099,7 @@ ly_path_compile_deref(const struct ly_ctx *ctx, const struct lysc_node *ctx_node &path2), cleanup); node2 = path2[LY_ARRAY_COUNT(path2) - 1].node; LY_CHECK_GOTO(ret = ly_path_append(ctx, path2, path), cleanup); - ly_path_free(ctx, path2); + ly_path_free(path2); path2 = NULL; /* properly parsed path must always continue with ')' and '/' */ @@ -1125,9 +1123,9 @@ ly_path_compile_deref(const struct ly_ctx *ctx, const struct lysc_node *ctx_node LY_CHECK_GOTO(ret = ly_path_append(ctx, path2, path), cleanup); cleanup: - ly_path_free(ctx, path2); + ly_path_free(path2); if (ret) { - ly_path_free(ctx, *path); + ly_path_free(*path); *path = NULL; } return ret; @@ -1283,7 +1281,7 @@ _ly_path_compile(const struct ly_ctx *ctx, const struct lys_module *cur_mod, con cleanup: if (ret) { - ly_path_free(ctx, *path); + ly_path_free(*path); *path = NULL; } LOG_LOCBACK(cur_node ? 1 : 0, 0); @@ -1488,7 +1486,7 @@ ly_path_predicates_free(const struct ly_ctx *ctx, struct ly_path_predicate *pred } void -ly_path_free(const struct ly_ctx *ctx, struct ly_path *path) +ly_path_free(struct ly_path *path) { LY_ARRAY_COUNT_TYPE u; @@ -1497,7 +1495,7 @@ ly_path_free(const struct ly_ctx *ctx, struct ly_path *path) } LY_ARRAY_FOR(path, u) { - ly_path_predicates_free(ctx, path[u].predicates); + ly_path_predicates_free(path[u].node->module->ctx, path[u].predicates); } LY_ARRAY_FREE(path); } diff --git a/src/path.h b/src/path.h index 9a9e342bb..68cf76e6a 100644 --- a/src/path.h +++ b/src/path.h @@ -257,9 +257,8 @@ void ly_path_predicates_free(const struct ly_ctx *ctx, struct ly_path_predicate /** * @brief Free ly_path structure. * - * @param[in] ctx libyang context. * @param[in] path The structure ([sized array](@ref sizedarrays)) to free. */ -void ly_path_free(const struct ly_ctx *ctx, struct ly_path *path); +void ly_path_free(struct ly_path *path); #endif /* LY_PATH_H_ */ diff --git a/src/plugins_types.c b/src/plugins_types.c index 830569154..921dd6b30 100644 --- a/src/plugins_types.c +++ b/src/plugins_types.c @@ -352,10 +352,16 @@ lyplg_type_print_simple(const struct ly_ctx *UNUSED(ctx), const struct lyd_value LIBYANG_API_DEF LY_ERR lyplg_type_dup_simple(const struct ly_ctx *ctx, const struct lyd_value *original, struct lyd_value *dup) { - memset(dup, 0, sizeof *dup); - LY_CHECK_RET(lydict_insert(ctx, original->_canonical, 0, &dup->_canonical)); + LY_ERR r; + + if ((r = lydict_dup(ctx, original->_canonical, &dup->_canonical))) { + /* in case of error NULL the values so that freeing does not fail */ + memset(dup, 0, sizeof *dup); + return r; + } memcpy(dup->fixed_mem, original->fixed_mem, sizeof dup->fixed_mem); dup->realtype = original->realtype; + return LY_SUCCESS; } @@ -888,7 +894,7 @@ lyplg_type_lypath_new(const struct ly_ctx *ctx, const char *value, size_t value_ ly_err_clean((struct ly_ctx *)ctx, e); } - ly_path_free(ctx, *path); + ly_path_free(*path); *path = NULL; } @@ -896,9 +902,9 @@ lyplg_type_lypath_new(const struct ly_ctx *ctx, const char *value, size_t value_ } LIBYANG_API_DEF void -lyplg_type_lypath_free(const struct ly_ctx *ctx, struct ly_path *path) +lyplg_type_lypath_free(const struct ly_ctx *UNUSED(ctx), struct ly_path *path) { - ly_path_free(ctx, path); + ly_path_free(path); } LIBYANG_API_DEF LY_ERR @@ -1007,7 +1013,7 @@ lyplg_type_resolve_leafref_get_target_path(const struct lyxp_expr *path, const s LY_CHECK_GOTO(lyxp_expr_parse(ctx_node->module->ctx, str_path, 0, 1, target_path), cleanup); cleanup: - ly_path_free(ctx_node->module->ctx, p); + ly_path_free(p); free(str_path); return rc; } diff --git a/src/plugins_types/instanceid.c b/src/plugins_types/instanceid.c index 00ad45a98..6c1629c7e 100644 --- a/src/plugins_types/instanceid.c +++ b/src/plugins_types/instanceid.c @@ -319,7 +319,7 @@ lyplg_type_free_instanceid(const struct ly_ctx *ctx, struct lyd_value *value) { lydict_remove(ctx, value->_canonical); value->_canonical = NULL; - ly_path_free(ctx, value->target); + ly_path_free(value->target); } /** diff --git a/src/schema_compile.c b/src/schema_compile.c index bda517cfc..fde312839 100644 --- a/src/schema_compile.c +++ b/src/schema_compile.c @@ -866,7 +866,7 @@ lys_compile_unres_leafref(struct lysc_ctx *ctx, const struct lysc_node *node, st /* get the target node */ target = p[LY_ARRAY_COUNT(p) - 1].node; - ly_path_free(node->module->ctx, p); + ly_path_free(p); if (!(target->nodetype & (LYS_LEAF | LYS_LEAFLIST))) { LOGVAL(ctx->ctx, LYVE_REFERENCE, "Invalid leafref path \"%s\" - target node is %s instead of leaf or leaf-list.", @@ -1456,7 +1456,7 @@ lys_compile_unres_depset(struct ly_ctx *ctx, struct lys_glob_unres *unres) ret = ly_path_compile_leafref(cctx.ctx, l->node, cctx.ext, lref->path, (l->node->flags & LYS_IS_OUTPUT) ? LY_PATH_OPER_OUTPUT : LY_PATH_OPER_INPUT, LY_PATH_TARGET_MANY, LY_VALUE_SCHEMA_RESOLVED, lref->prefixes, &path); - ly_path_free(l->node->module->ctx, path); + ly_path_free(path); assert(ret != LY_ERECOMPILE); if (ret) { diff --git a/src/tree_data.c b/src/tree_data.c index e1463ea25..cc33341a3 100644 --- a/src/tree_data.c +++ b/src/tree_data.c @@ -3624,7 +3624,7 @@ lyd_find_path(const struct lyd_node *ctx_node, const char *path, ly_bool output, cleanup: lyxp_expr_free(LYD_CTX(ctx_node), expr); - ly_path_free(LYD_CTX(ctx_node), lypath); + ly_path_free(lypath); return ret; } diff --git a/src/tree_data_new.c b/src/tree_data_new.c index f3bd9e7f7..2b27be6ae 100644 --- a/src/tree_data_new.c +++ b/src/tree_data_new.c @@ -1811,7 +1811,7 @@ lyd_new_path_(struct lyd_node *parent, const struct ly_ctx *ctx, const struct ly LY_ARRAY_INCREMENT(p); } } - ly_path_free(ctx, p); + ly_path_free(p); if (!ret) { /* set out params only on success */ if (new_parent) { diff --git a/src/tree_schema.c b/src/tree_schema.c index d7ac8d88d..81bda1bb6 100644 --- a/src/tree_schema.c +++ b/src/tree_schema.c @@ -644,7 +644,7 @@ lys_find_path_atoms(const struct ly_ctx *ctx, const struct lysc_node *ctx_node, ret = lys_find_lypath_atoms(p, set); cleanup: - ly_path_free(ctx, p); + ly_path_free(p); lyxp_expr_free(ctx, expr); return ret; } @@ -679,7 +679,7 @@ lys_find_path(const struct ly_ctx *ctx, const struct lysc_node *ctx_node, const snode = p[LY_ARRAY_COUNT(p) - 1].node; cleanup: - ly_path_free(ctx, p); + ly_path_free(p); lyxp_expr_free(ctx, expr); return snode; } diff --git a/src/tree_schema_common.c b/src/tree_schema_common.c index 803009a62..720bd1d40 100644 --- a/src/tree_schema_common.c +++ b/src/tree_schema_common.c @@ -1829,7 +1829,7 @@ lysc_node_lref_target(const struct lysc_node *node) /* get the target node */ target = p[LY_ARRAY_COUNT(p) - 1].node; - ly_path_free(node->module->ctx, p); + ly_path_free(p); return target; } diff --git a/src/xpath.c b/src/xpath.c index 0b3062f1f..2251eab6c 100644 --- a/src/xpath.c +++ b/src/xpath.c @@ -4042,7 +4042,7 @@ xpath_deref(struct lyxp_set **args, uint32_t UNUSED(arg_count), struct lyxp_set if (!r) { /* get the target node */ target = p[LY_ARRAY_COUNT(p) - 1].node; - ly_path_free(set->ctx, p); + ly_path_free(p); LY_CHECK_RET(lyxp_set_scnode_insert_node(set, target, LYXP_NODE_ELEM, LYXP_AXIS_SELF, NULL)); } /* else the target was found before but is disabled so it was removed */ @@ -8272,7 +8272,9 @@ eval_name_test_with_predicate(const struct lyxp_expr *exp, uint32_t *tok_idx, en options &= ~LYXP_SKIP_EXPR; } lydict_remove(set->ctx, ncname_dict); - ly_path_predicates_free(set->ctx, predicates); + if (predicates) { + ly_path_predicates_free(scnode->module->ctx, predicates); + } return rc; } diff --git a/tests/utests/data/test_tree_data.c b/tests/utests/data/test_tree_data.c index fabd170b2..b3dde5a6a 100644 --- a/tests/utests/data/test_tree_data.c +++ b/tests/utests/data/test_tree_data.c @@ -409,7 +409,7 @@ test_target(void **state) assert_string_equal(lyd_get_value(term->prev), "b"); lyd_free_all(tree); - ly_path_free(UTEST_LYCTX, path); + ly_path_free(path); lyxp_expr_free(UTEST_LYCTX, exp); }