From 6d5848df3092f826e25b5103f5e6480f236aa43c Mon Sep 17 00:00:00 2001 From: Fredrik Persson Date: Thu, 24 Sep 2026 18:32:21 +0200 Subject: [PATCH 1/3] tests UPDATE perf cases for validation producing a diff perf.yang declares no defaults and no when conditions, and test_validate passes NULL for the diff, so lyd_val_diff_add() is never reached and the cost of building a validation diff is not measured anywhere. Add perf_dflt.yang, where every list instance offers four alternative sets of defaults gated on the same leaf, so only one set can ever apply. Two cases validate that data, one asking for a diff and one not. --- tests/perf/perf.c | 78 +++++++ tests/perf/perf_dflt.yang | 450 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 528 insertions(+) create mode 100644 tests/perf/perf_dflt.yang diff --git a/tests/perf/perf.c b/tests/perf/perf.c index c3c43085e..52fa70368 100644 --- a/tests/perf/perf.c +++ b/tests/perf/perf.c @@ -342,6 +342,78 @@ setup_data_offset_tree(const struct lys_module *mod, uint32_t count, struct test } /* TEST CB */ +static LY_ERR +setup_data_dflt_tree(const struct lys_module *mod, uint32_t count, struct test_state *state) +{ + const struct lys_module *dflt_mod; + struct lyd_node *parent; + char buf[64]; + uint32_t i; + LY_ERR r; + + state->count = count; + + dflt_mod = ly_ctx_get_module_implemented(mod->ctx, "perf_dflt"); + if (!dflt_mod) { + return LY_ENOTFOUND; + } + state->mod = dflt_mod; + + for (i = 0; i < count; ++i) { + sprintf(buf, "/perf_dflt:cont/lst[k='%" PRIu32 "']", i); + if ((r = lyd_new_path(state->data1, mod->ctx, buf, NULL, 0, &parent))) { + return r; + } + if (!state->data1) { + state->data1 = parent; + } + + sprintf(buf, "/perf_dflt:cont/lst[k='%" PRIu32 "']/type", i); + if ((r = lyd_new_path(state->data1, mod->ctx, buf, "a", 0, NULL))) { + return r; + } + } + + return LY_SUCCESS; +} + +static LY_ERR +test_validate_dflt(struct test_state *state, struct timespec *ts_start, struct timespec *ts_end, uint32_t *size) +{ + LY_ERR r; + + *size = 0; + TEST_START(ts_start); + + if ((r = lyd_validate_all(&state->data1, NULL, LYD_VALIDATE_PRESENT, NULL))) { + return r; + } + + TEST_END(ts_end); + + return LY_SUCCESS; +} + +static LY_ERR +test_validate_dflt_diff(struct test_state *state, struct timespec *ts_start, struct timespec *ts_end, uint32_t *size) +{ + struct lyd_node *diff = NULL; + LY_ERR r; + + *size = 0; + TEST_START(ts_start); + + if ((r = lyd_validate_all(&state->data1, NULL, LYD_VALIDATE_PRESENT, &diff))) { + return r; + } + + TEST_END(ts_end); + + lyd_free_siblings(diff); + + return LY_SUCCESS; +} + static LY_ERR test_create_new_text(struct test_state *state, struct timespec *ts_start, struct timespec *ts_end, uint32_t *size) { @@ -820,6 +892,8 @@ struct test tests[] = { {"create new text", setup_basic, test_create_new_text}, {"create path", setup_basic, test_create_path}, {"validate", setup_data_single_tree, test_validate}, + {"validate defaults", setup_data_dflt_tree, test_validate_dflt}, + {"validate defaults diff", setup_data_dflt_tree, test_validate_dflt_diff}, {"parse xml mem validate", setup_data_single_tree, test_parse_xml_mem_validate}, {"parse xml mem no validate", setup_data_single_tree, test_parse_xml_mem_no_validate}, {"parse xml file no validate format", setup_data_single_tree, test_parse_xml_file_no_validate_format}, @@ -887,6 +961,10 @@ main(int argc, char **argv) ret = LY_ENOTFOUND; goto cleanup; } + if (!ly_ctx_load_module(ctx, "perf_dflt", NULL, NULL)) { + ret = LY_ENOTFOUND; + goto cleanup; + } /* tests */ name_len = 0; diff --git a/tests/perf/perf_dflt.yang b/tests/perf/perf_dflt.yang new file mode 100644 index 000000000..57a60dd7a --- /dev/null +++ b/tests/perf/perf_dflt.yang @@ -0,0 +1,450 @@ +module perf_dflt { + yang-version 1.1; + namespace "urn:sysrepo:tests:perf-dflt"; + prefix pd; + + description + "Every list instance offers four alternative sets of defaults gated on the same leaf, + so only one set can ever apply. Validation materialises the defaults of all four + before resolving the when conditions, then deletes the three that do not apply."; + + container cont { + list lst { + key "k"; + + leaf k { + type uint32; + } + + leaf type { + type enumeration { + enum "a"; + enum "b"; + enum "c"; + enum "d"; + } + } + + container set-a { + when "../type = 'a'"; + + leaf d1 { + type uint32; + default 1; + } + + leaf d2 { + type uint32; + default 2; + } + + leaf d3 { + type uint32; + default 3; + } + + leaf d4 { + type uint32; + default 4; + } + + leaf d5 { + type uint32; + default 5; + } + + leaf d6 { + type uint32; + default 6; + } + + leaf d7 { + type uint32; + default 7; + } + + leaf d8 { + type uint32; + default 8; + } + + leaf d9 { + type uint32; + default 9; + } + + leaf d10 { + type uint32; + default 10; + } + + leaf d11 { + type uint32; + default 11; + } + + leaf d12 { + type uint32; + default 12; + } + + leaf d13 { + type uint32; + default 13; + } + + leaf d14 { + type uint32; + default 14; + } + + leaf d15 { + type uint32; + default 15; + } + + leaf d16 { + type uint32; + default 16; + } + + leaf d17 { + type uint32; + default 17; + } + + leaf d18 { + type uint32; + default 18; + } + + leaf d19 { + type uint32; + default 19; + } + + leaf d20 { + type uint32; + default 20; + } + + } + + container set-b { + when "../type = 'b'"; + + leaf d1 { + type uint32; + default 1; + } + + leaf d2 { + type uint32; + default 2; + } + + leaf d3 { + type uint32; + default 3; + } + + leaf d4 { + type uint32; + default 4; + } + + leaf d5 { + type uint32; + default 5; + } + + leaf d6 { + type uint32; + default 6; + } + + leaf d7 { + type uint32; + default 7; + } + + leaf d8 { + type uint32; + default 8; + } + + leaf d9 { + type uint32; + default 9; + } + + leaf d10 { + type uint32; + default 10; + } + + leaf d11 { + type uint32; + default 11; + } + + leaf d12 { + type uint32; + default 12; + } + + leaf d13 { + type uint32; + default 13; + } + + leaf d14 { + type uint32; + default 14; + } + + leaf d15 { + type uint32; + default 15; + } + + leaf d16 { + type uint32; + default 16; + } + + leaf d17 { + type uint32; + default 17; + } + + leaf d18 { + type uint32; + default 18; + } + + leaf d19 { + type uint32; + default 19; + } + + leaf d20 { + type uint32; + default 20; + } + + } + + container set-c { + when "../type = 'c'"; + + leaf d1 { + type uint32; + default 1; + } + + leaf d2 { + type uint32; + default 2; + } + + leaf d3 { + type uint32; + default 3; + } + + leaf d4 { + type uint32; + default 4; + } + + leaf d5 { + type uint32; + default 5; + } + + leaf d6 { + type uint32; + default 6; + } + + leaf d7 { + type uint32; + default 7; + } + + leaf d8 { + type uint32; + default 8; + } + + leaf d9 { + type uint32; + default 9; + } + + leaf d10 { + type uint32; + default 10; + } + + leaf d11 { + type uint32; + default 11; + } + + leaf d12 { + type uint32; + default 12; + } + + leaf d13 { + type uint32; + default 13; + } + + leaf d14 { + type uint32; + default 14; + } + + leaf d15 { + type uint32; + default 15; + } + + leaf d16 { + type uint32; + default 16; + } + + leaf d17 { + type uint32; + default 17; + } + + leaf d18 { + type uint32; + default 18; + } + + leaf d19 { + type uint32; + default 19; + } + + leaf d20 { + type uint32; + default 20; + } + + } + + container set-d { + when "../type = 'd'"; + + leaf d1 { + type uint32; + default 1; + } + + leaf d2 { + type uint32; + default 2; + } + + leaf d3 { + type uint32; + default 3; + } + + leaf d4 { + type uint32; + default 4; + } + + leaf d5 { + type uint32; + default 5; + } + + leaf d6 { + type uint32; + default 6; + } + + leaf d7 { + type uint32; + default 7; + } + + leaf d8 { + type uint32; + default 8; + } + + leaf d9 { + type uint32; + default 9; + } + + leaf d10 { + type uint32; + default 10; + } + + leaf d11 { + type uint32; + default 11; + } + + leaf d12 { + type uint32; + default 12; + } + + leaf d13 { + type uint32; + default 13; + } + + leaf d14 { + type uint32; + default 14; + } + + leaf d15 { + type uint32; + default 15; + } + + leaf d16 { + type uint32; + default 16; + } + + leaf d17 { + type uint32; + default 17; + } + + leaf d18 { + type uint32; + default 18; + } + + leaf d19 { + type uint32; + default 19; + } + + leaf d20 { + type uint32; + default 20; + } + + } + + } + } +} From a10ddcfccd3b75891106b6a65ba54ee36cdd34bb Mon Sep 17 00:00:00 2001 From: Fredrik Persson Date: Thu, 24 Sep 2026 18:32:48 +0200 Subject: [PATCH 2/3] validation OPTIMIZE append to the diff instead of merging a one-node diff lyd_val_diff_add() is called once per node created or deleted by validation. It built a standalone one-node diff and merged that into the accumulating diff, which duplicated the node's whole ancestor chain on every call. lyd_diff_add() already reuses the ancestors present in the target diff, so append straight into it when the node has no entry there yet. The merge is only needed when an operation on that very node already exists, to reconcile the two. lyd_diff_add() leaves out an operation that a parent already states, so add it back to keep the diff shape unchanged. --- src/diff.c | 90 ++++++++++++++++++++++++++++++++---------------- src/diff.h | 27 +++++++++++++++ src/validation.c | 10 +++++- 3 files changed, 97 insertions(+), 30 deletions(-) diff --git a/src/diff.c b/src/diff.c index e22f7c949..7e7b7c77d 100644 --- a/src/diff.c +++ b/src/diff.c @@ -377,13 +377,49 @@ lyd_diff_dup(const struct lyd_node *node, enum lyd_diff_op op, struct lyd_node * return LY_SUCCESS; } +void +lyd_diff_find_node(struct lyd_node *diff, const struct lyd_node *node, struct lyd_node **diff_parent, + struct lyd_node **match) +{ + struct lyd_node *siblings, *m = NULL, *dparent = NULL; + const struct lyd_node *parent = NULL; + + siblings = diff; + do { + /* find next node parent */ + parent = node; + while (parent->parent && (!dparent || (parent->parent->schema != dparent->schema))) { + parent = parent->parent; + } + + if (lysc_is_dup_inst_list(parent->schema)) { + /* assume it never exists, we are not able to distinguish whether it does or not */ + m = NULL; + break; + } + + /* check whether it exists in the diff */ + if (lyd_find_sibling_first(siblings, parent, &m)) { + break; + } + + /* another parent found */ + dparent = m; + + /* move down in the diff */ + siblings = lyd_child_no_keys(m); + } while (parent != node); + + *diff_parent = dparent; + *match = (m && (parent == node)) ? m : NULL; +} + LY_ERR lyd_diff_add(const struct lyd_node *node, enum lyd_diff_op op, const char *orig_default, const char *orig_value, const char *key, const char *value, const char *position, const char *orig_key, const char *orig_position, struct lyd_node **diff, struct lyd_node **diff_node) { - struct lyd_node *dup, *siblings, *match = NULL, *diff_parent = NULL, *elem; - const struct lyd_node *parent = NULL; + struct lyd_node *dup, *match = NULL, *diff_parent = NULL, *elem; enum lyd_diff_op cur_op; struct lyd_meta *meta; ly_bool found; @@ -410,35 +446,11 @@ lyd_diff_add(const struct lyd_node *node, enum lyd_diff_op op, const char *orig_ *diff_node = NULL; } - /* find the first existing parent */ - siblings = *diff; - do { - /* find next node parent */ - parent = node; - while (parent->parent && (!diff_parent || (parent->parent->schema != diff_parent->schema))) { - parent = parent->parent; - } - - if (lysc_is_dup_inst_list(parent->schema)) { - /* assume it never exists, we are not able to distinguish whether it does or not */ - match = NULL; - break; - } - - /* check whether it exists in the diff */ - if (lyd_find_sibling_first(siblings, parent, &match)) { - break; - } - - /* another parent found */ - diff_parent = match; + lyd_diff_find_node(*diff, node, &diff_parent, &match); - /* move down in the diff */ - siblings = lyd_child_no_keys(match); - } while (parent != node); - - if (match && (parent == node)) { + if (match) { /* special case when there is already an operation on our descendant */ + diff_parent = match; assert(!lyd_diff_get_op(diff_parent, &cur_op, NULL)); /* move it to the end where it is expected (matters for user-ordered lists) */ @@ -525,6 +537,26 @@ lyd_diff_add(const struct lyd_node *node, enum lyd_diff_op op, const char *orig_ return LY_SUCCESS; } +LY_ERR +lyd_diff_add_explicit_op(const struct lyd_node *node, enum lyd_diff_op op, const char *key, const char *value, + const char *position, struct lyd_node **diff) +{ + struct lyd_node *dup = NULL; + struct lyd_meta *meta; + + LY_CHECK_RET(lyd_diff_add(node, op, NULL, NULL, key, value, position, NULL, NULL, diff, &dup)); + + /* ::lyd_diff_add() omits an operation a parent already states, a validation diff states it + * on every node */ + lyd_diff_find_meta(dup, "operation", &meta, NULL); + if (!meta) { + LY_CHECK_RET(lyd_new_meta(NULL, dup, NULL, "yang:operation", lyd_diff_op2str(op), + LYD_NEW_VAL_STORE_ONLY, NULL)); + } + + return LY_SUCCESS; +} + /** * @brief Get a userord entry for a specific user-ordered list/leaf-list. Create if does not exist yet. * diff --git a/src/diff.h b/src/diff.h index b7f360496..176536a82 100644 --- a/src/diff.h +++ b/src/diff.h @@ -40,6 +40,17 @@ enum lyd_diff_op { LYD_DIFF_OP_NONE /**< No change of an existing inner node or default flag change of a term node. */ }; +/** + * @brief Find a node and its deepest existing ancestor in a diff. + * + * @param[in] diff Diff to search (first sibling). + * @param[in] node Data node to look for. + * @param[out] diff_parent Deepest ancestor of @p node in @p diff, NULL if none. + * @param[out] match Diff node of @p node, NULL if not present. + */ +void lyd_diff_find_node(struct lyd_node *diff, const struct lyd_node *node, struct lyd_node **diff_parent, + struct lyd_node **match); + /** * @brief Add a new change into diff. * @@ -60,4 +71,20 @@ LIBYANG_API_DECL LY_ERR lyd_diff_add(const struct lyd_node *node, enum lyd_diff_ const char *orig_value, const char *key, const char *value, const char *position, const char *orig_key, const char *orig_position, struct lyd_node **diff, struct lyd_node **diff_node); +/** + * @brief Add a new change into diff, always stating the operation on the added node. + * + * Unlike ::lyd_diff_add(), sets the operation metadata even when a parent already states it. + * + * @param[in] node Node (subtree) to add into diff. + * @param[in] op Operation to set. + * @param[in] key Key metadata to set. + * @param[in] value Value metadata to set. + * @param[in] position Position metadata to set. + * @param[in,out] diff Diff to append to. + * @return LY_ERR value. + */ +LY_ERR lyd_diff_add_explicit_op(const struct lyd_node *node, enum lyd_diff_op op, const char *key, + const char *value, const char *position, struct lyd_node **diff); + #endif /* LY_DIFF_H_ */ diff --git a/src/validation.c b/src/validation.c index d43176443..f4039cfff 100644 --- a/src/validation.c +++ b/src/validation.c @@ -188,7 +188,7 @@ LY_ERR lyd_val_diff_add(const struct lyd_node *node, enum lyd_diff_op op, struct lyd_node **diff) { LY_ERR ret = LY_SUCCESS; - struct lyd_node *new_diff = NULL; + struct lyd_node *new_diff = NULL, *diff_parent, *match; const struct lyd_node *prev_inst; char *key = NULL, *value = NULL, *position = NULL; size_t buflen = 0, bufused = 0; @@ -240,6 +240,14 @@ lyd_val_diff_add(const struct lyd_node *node, enum lyd_diff_op op, struct lyd_no } } + /* appending reuses the ancestors already in the diff, unlike building a one-node diff and + * merging it; merge only to reconcile an operation already recorded on this node */ + lyd_diff_find_node(*diff, node, &diff_parent, &match); + if (!match) { + ret = lyd_diff_add_explicit_op(node, op, key, value, position, diff); + goto cleanup; + } + /* create new diff tree */ LY_CHECK_GOTO(ret = lyd_diff_add(node, op, NULL, NULL, key, value, position, NULL, NULL, &new_diff, NULL), cleanup); From a5cc933eea16fc69199afcf7c5f678f00b9ae659 Mon Sep 17 00:00:00 2001 From: Fredrik Persson Date: Thu, 24 Sep 2026 18:36:47 +0200 Subject: [PATCH 3/3] validation OPTIMIZE drop diff nodes created and deleted in one run When a when condition turns out false, the default nodes created speculatively before it was resolved are auto-deleted again. Recording that deletion went through lyd_diff_merge_all(), which duplicated the deleted subtree and reconciled it node by node, only for the entries to end up as "none" and be dropped as redundant. Recognise the case directly: if the diff already holds a create for the node, and nothing in that subtree was set explicitly, the create and the delete cancel out, so remove the subtree together with any ancestor left without a change. --- src/diff.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++ src/diff.h | 10 ++++++++ src/validation.c | 10 ++++++++ 3 files changed, 83 insertions(+) diff --git a/src/diff.c b/src/diff.c index 7e7b7c77d..e14f808da 100644 --- a/src/diff.c +++ b/src/diff.c @@ -2732,6 +2732,69 @@ lyd_diff_is_redundant(struct lyd_node *diff) return 0; } +/** + * @brief Check whether a diff subtree was created by validation only, so that deleting the + * corresponding data nodes cancels it out. + * + * @param[in] diff_node Diff subtree to check. + * @return Whether it can be dropped. + */ +static ly_bool +lyd_diff_val_subtree_created(const struct lyd_node *diff_node) +{ + const struct lyd_node *elem; + struct lyd_meta *meta; + struct lyd_attr *attr; + + LYD_TREE_DFS_BEGIN(diff_node, elem) { + if (!elem->schema) { + /* cannot reason about opaque nodes */ + return 0; + } + + lyd_diff_find_meta(elem, "operation", &meta, &attr); + if (attr) { + return 0; + } + if (meta && (lyd_diff_str2op(lyd_get_meta_value(meta)) != LYD_DIFF_OP_CREATE)) { + return 0; + } + + /* a non-default term was set explicitly, deleting it is a real change */ + if ((elem->schema->nodetype & LYD_NODE_TERM) && !(elem->flags & LYD_DEFAULT)) { + return 0; + } + + LYD_TREE_DFS_END(diff_node, elem); + } + + return 1; +} + +LY_ERR +lyd_diff_val_del_created(struct lyd_node *diff_node, struct lyd_node **diff) +{ + struct lyd_node *parent; + enum lyd_diff_op op; + + LY_CHECK_RET(lyd_diff_get_op(diff_node, &op, NULL)); + if ((op != LYD_DIFF_OP_CREATE) || !lyd_diff_val_subtree_created(diff_node)) { + return LY_ENOT; + } + + /* drop it, then any ancestor left without a change */ + do { + parent = lyd_parent(diff_node); + if (diff_node == *diff) { + *diff = (*diff)->next; + } + lyd_free_tree(diff_node); + diff_node = parent; + } while (diff_node && lyd_diff_is_redundant(diff_node)); + + return LY_SUCCESS; +} + /** * @brief Merge all diff metadata found on a source diff node. * diff --git a/src/diff.h b/src/diff.h index 176536a82..85545f1b7 100644 --- a/src/diff.h +++ b/src/diff.h @@ -87,4 +87,14 @@ LIBYANG_API_DECL LY_ERR lyd_diff_add(const struct lyd_node *node, enum lyd_diff_ LY_ERR lyd_diff_add_explicit_op(const struct lyd_node *node, enum lyd_diff_op op, const char *key, const char *value, const char *position, struct lyd_node **diff); +/** + * @brief Drop a diff subtree that validation created, when its data nodes are being deleted again. + * + * @param[in] diff_node Diff node of the data node being deleted. + * @param[in,out] diff Diff @p diff_node belongs to. + * @return LY_SUCCESS if the subtree was dropped. + * @return LY_ENOT if it has to be merged instead. + */ +LY_ERR lyd_diff_val_del_created(struct lyd_node *diff_node, struct lyd_node **diff); + #endif /* LY_DIFF_H_ */ diff --git a/src/validation.c b/src/validation.c index f4039cfff..eb03b1818 100644 --- a/src/validation.c +++ b/src/validation.c @@ -248,6 +248,16 @@ lyd_val_diff_add(const struct lyd_node *node, enum lyd_diff_op op, struct lyd_no goto cleanup; } + /* a create followed by a delete cancels out; merging would reconcile the whole subtree to + * "none" only to drop it as redundant */ + if (op == LYD_DIFF_OP_DELETE) { + ret = lyd_diff_val_del_created(match, diff); + if (ret != LY_ENOT) { + goto cleanup; + } + ret = LY_SUCCESS; + } + /* create new diff tree */ LY_CHECK_GOTO(ret = lyd_diff_add(node, op, NULL, NULL, key, value, position, NULL, NULL, &new_diff, NULL), cleanup);