[PATCH nft v2 1/1] mergesort: avoid cloning value in expr_msort_cmp()

[PATCH nft v2 1/1] mergesort: avoid cloning value in expr_msort_cmp()

[Thomas Haller]

If we have a plain EXPR_VALUE value, there is no need to copy
it via mpz_set().

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
v2: drop goto (again) and restore recursion.

 src/mergesort.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/src/mergesort.c b/src/mergesort.c
index 5965236af6b7..4d0e280fdc5e 100644
--- a/src/mergesort.c
+++ b/src/mergesort.c
@@ -12,8 +12,6 @@
 #include <gmputil.h>
 #include <list.h>
 
-static void expr_msort_value(const struct expr *expr, mpz_t value);
-
 static void concat_expr_msort_value(const struct expr *expr, mpz_t value)
 {
 	unsigned int len = 0, ilen;
@@ -29,20 +27,17 @@ static void concat_expr_msort_value(const struct expr *expr, mpz_t value)
 	mpz_import_data(value, data, BYTEORDER_HOST_ENDIAN, len);
 }
 
-static void expr_msort_value(const struct expr *expr, mpz_t value)
+static mpz_srcptr expr_msort_value(const struct expr *expr, mpz_t value)
 {
 	switch (expr->etype) {
 	case EXPR_SET_ELEM:
-		expr_msort_value(expr->key, value);
-		break;
+		return expr_msort_value(expr->key, value);
 	case EXPR_BINOP:
 	case EXPR_MAPPING:
 	case EXPR_RANGE:
-		expr_msort_value(expr->left, value);
-		break;
+		return expr_msort_value(expr->left, value);
 	case EXPR_VALUE:
-		mpz_set(value, expr->value);
-		break;
+		return expr->value;
 	case EXPR_CONCAT:
 		concat_expr_msort_value(expr, value);
 		break;
@@ -53,20 +48,24 @@ static void expr_msort_value(const struct expr *expr, mpz_t value)
 	default:
 		BUG("Unknown expression %s\n", expr_name(expr));
 	}
+	return value;
 }
 
 static int expr_msort_cmp(const struct expr *e1, const struct expr *e2)
 {
-	mpz_t value1, value2;
+	mpz_srcptr value1;
+	mpz_srcptr value2;
+	mpz_t value1_tmp;
+	mpz_t value2_tmp;
 	int ret;
 
-	mpz_init(value1);
-	mpz_init(value2);
-	expr_msort_value(e1, value1);
-	expr_msort_value(e2, value2);
+	mpz_init(value1_tmp);
+	mpz_init(value2_tmp);
+	value1 = expr_msort_value(e1, value1_tmp);
+	value2 = expr_msort_value(e2, value2_tmp);
 	ret = mpz_cmp(value1, value2);
-	mpz_clear(value1);
-	mpz_clear(value2);
+	mpz_clear(value1_tmp);
+	mpz_clear(value2_tmp);
 
 	return ret;
 }
-- 
2.41.0

Re: [PATCH nft v2 1/1] mergesort: avoid cloning value in expr_msort_cmp()

[Pablo Neira Ayuso]

On Wed, Sep 27, 2023 at 11:20:24AM +0200, Thomas Haller wrote:
> If we have a plain EXPR_VALUE value, there is no need to copy
> it via mpz_set().

Applied, thanks