[PATCH] ldb-modules: avoid reallocating the attributes array if we don't have ranged attributes in the request

[Jeremy Allison]

On Mon, May 25, 2015 at 09:46:59PM -0700, Matthieu Patou wrote:
> Hello All,
> 
> Please have a look at the following patch, it avoids calling
> talloc_realloc when not needed, it's a small performance gain (~3%)
> but still interesting to have.
> 
> Please review and push.

Review below, just a few fixes - please make the changes
and re-submit. I promise I'll re-review in a timely fashion !

> >From 552d41c17cf983ea2f243025dc6ef4ba14f18a8d Mon Sep 17 00:00:00 2001
> From: Matthieu Patou <mat at matws.net>
> Date: Mon, 25 May 2015 09:21:58 -0700
> Subject: [PATCH 3/4] ldb-modules: avoid reallocating the attributes array if
>  we don't have ranged attributes in the request
> 
> Instead of reallocating the array every time, just do it when needed.
> It's accounting for 3% of runtime on dbcheck (talloc alone, there is
> some other gains due to the lack of talloc_free to do).
> 
> Change-Id: I6f44d3582c9b496f78c0b0a42d61bf54dd4cbdbe
> Signed-off-by: Matthieu Patou <mat at matws.net>
> ---
>  source4/dsdb/samdb/ldb_modules/ranged_results.c | 31 +++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/source4/dsdb/samdb/ldb_modules/ranged_results.c b/source4/dsdb/samdb/ldb_modules/ranged_results.c
> index 60d7503..b19ed44 100644
> --- a/source4/dsdb/samdb/ldb_modules/ranged_results.c
> +++ b/source4/dsdb/samdb/ldb_modules/ranged_results.c
> @@ -91,6 +91,12 @@ static int rr_search_callback(struct ldb_request *req, struct ldb_reply *ares)
>  				       LDB_ERR_OPERATIONS_ERROR);
>  	}
>  
> +	/*
> +	 * FIXME / IMPROVEME
> +	 * Instead of re-iterating on the list we should just remember the
> +	 * list of attribute with range values and deal with them
> +	 * then.
> +	 */
>  	/* Find those that are range requests from the attribute list */
>  	for (i = 0; ac->req->op.search.attrs[i]; i++) {
>  		char *p, *new_attr;
> @@ -201,16 +207,29 @@ static int rr_search(struct ldb_module *module, struct ldb_request *req)
>  	/* Strip the range request from the attribute */
>  	for (i = 0; req->op.search.attrs && req->op.search.attrs[i]; i++) {
>  		char *p;
> -		new_attrs = talloc_realloc(req, new_attrs, const char *, i+2);
> -		new_attrs[i] = req->op.search.attrs[i];
> -		new_attrs[i+1] = NULL;
> -		p = strchr(new_attrs[i], ';');
> +		p = strchr(req->op.search.attrs[i], ';');
>  		if (!p) {
>  			continue;
>  		}
>  		if (strncasecmp(p, ";range=", strlen(";range=")) != 0) {
>  			continue;
>  		}
> +		if (!new_attrs) {
> +			/*
> +			 * First time that we are constructing new_attrs
> +			 * so let's copy all the previous attributes
> +			 */
> +			int j;
> +			new_attrs = talloc_realloc(req, new_attrs, const char *, i + 2);

Check needed for talloc_realloc() returning NULL. I know the original
code didn't have that, but we're trying to improve it :-).

> +			for (j = 0; j < i; j++) {
> +				new_attrs[j] = req->op.search.attrs[j];
> +			}
> +
> +		} else {
> +			new_attrs = talloc_realloc(req, new_attrs, const char *, i + 2);

and the same here.

> +		}
> +		new_attrs[i] = req->op.search.attrs[i];
> +		new_attrs[i+1] = NULL;
>  		end = (unsigned int)-1;
>  		if (sscanf(p, ";range=%u-*", &start) != 1) {
>  			if (sscanf(p, ";range=%u-%u", &start, &end) != 2) {
> @@ -256,7 +275,9 @@ static int rr_search(struct ldb_module *module, struct ldb_request *req)
>  	}
>  
>  	/* No change, just run the original request as if we were never here */
> -	talloc_free(new_attrs);
> +	if (new_attrs) {
> +		talloc_free(new_attrs);
> +	}

The above change not needed - talloc_free() copes with being
passed NULL.

>  	return ldb_next_request(module, req);
>  }
>  
> -- 
> 2.1.4
>