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

[PATCH nft] 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>
---
 src/mergesort.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/src/mergesort.c b/src/mergesort.c
index 5965236af6b7..af7f163b2779 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,20 @@ 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)
 {
+recursive_again:
 	switch (expr->etype) {
 	case EXPR_SET_ELEM:
-		expr_msort_value(expr->key, value);
-		break;
+		expr = expr->key;
+		goto recursive_again;
 	case EXPR_BINOP:
 	case EXPR_MAPPING:
 	case EXPR_RANGE:
-		expr_msort_value(expr->left, value);
-		break;
+		expr = expr->left;
+		goto recursive_again;
 	case EXPR_VALUE:
-		mpz_set(value, expr->value);
-		break;
+		return expr->value;
 	case EXPR_CONCAT:
 		concat_expr_msort_value(expr, value);
 		break;
@@ -53,20 +51,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] mergesort: avoid cloning value in expr_msort_cmp()

[Pablo Neira Ayuso]

Hi Thomas,

Thanks for your patch.

On Wed, Sep 27, 2023 at 08:59:34AM +0200, Thomas Haller wrote:
> 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>
> ---
>  src/mergesort.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/src/mergesort.c b/src/mergesort.c
> index 5965236af6b7..af7f163b2779 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,20 @@ 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)
>  {
> +recursive_again:

We are in userspace, recursion is possible. Any chance to avoid the
copy without this goto approach?

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

[Thomas Haller]

On Wed, 2023-09-27 at 10:46 +0200, Pablo Neira Ayuso wrote:
> Hi Thomas,
> 
> We are in userspace, recursion is possible. Any chance to avoid the
> copy without this goto approach?

Hi Pablo,

I will drop in v2.

Thomas