git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 3/4] cat-file: use a single strbuf for all output

From
René Scharfe <l.s.r@web.de>
Date
Aug 14, 2018, 19:30 UTC
Message-ID
<567deabf-cdd3-c4e3-bb6a-b593c1f87c6e@web.de>
In-Reply-To
<20180814182022.GC26919@sigill.intra.peff.net>
Am 14.08.2018 um 20:20 schrieb Jeff King:
Show 32 quoted lines
> When we're in batch mode, we end up in batch_object_write()
> for each object, which allocates its own strbuf for each
> call. Instead, we can provide a single "scratch" buffer that
> gets reused for each output. When running:
> 
>   git cat-file --batch-all-objects --batch-check='%(objectname)'
> 
> on git.git, my best-of-five time drops from:
> 
>   real	0m0.171s
>   user	0m0.159s
>   sys	0m0.012s
> 
> to:
> 
>   real	0m0.133s
>   user	0m0.121s
>   sys	0m0.012s
> 
> Note that we could do this just by putting the "scratch"
> pointer into "struct expand_data", but I chose instead to
> add an extra parameter to the callstack. That's more
> verbose, but it makes it a bit more obvious what is going
> on, which in turn makes it easy to see where we need to be
> releasing the string in the caller (right after the loop
> which uses it in each case).
> 
> Based-on-a-patch-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> It also made it easy to see that without the prior patch,
> we'd have been using "buf" for two parameters. :)
Good catch.
Show 19 quoted lines
> 
>  builtin/cat-file.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 3ed1d0be80..08dced2614 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -338,11 +338,11 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  	}
>  }
>  
> -static void batch_object_write(const char *obj_name, struct batch_options *opt,
> +static void batch_object_write(const char *obj_name,
> +			       struct strbuf *scratch,
> +			       struct batch_options *opt,
>  			       struct expand_data *data)
>  {
> -	struct strbuf buf = STRBUF_INIT;

We could also avoid passing that buffer around by making it static. I shy away from adding static variables because the resulting code won't be thread-safe, but that fear might be irrational, especially with cat-file.

Show 84 quoted lines
> -
>  	if (!data->skip_object_info &&
>  	    oid_object_info_extended(the_repository, &data->oid, &data->info,
>  				     OBJECT_INFO_LOOKUP_REPLACE) < 0) {
> @@ -352,10 +352,10 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
>  		return;
>  	}
>  
> -	strbuf_expand(&buf, opt->format, expand_format, data);
> -	strbuf_addch(&buf, '\n');
> -	batch_write(opt, buf.buf, buf.len);
> -	strbuf_release(&buf);
> +	strbuf_reset(scratch);
> +	strbuf_expand(scratch, opt->format, expand_format, data);
> +	strbuf_addch(scratch, '\n');
> +	batch_write(opt, scratch->buf, scratch->len);
>  
>  	if (opt->print_contents) {
>  		print_object_or_die(opt, data);
> @@ -363,7 +363,9 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
>  	}
>  }
>  
> -static void batch_one_object(const char *obj_name, struct batch_options *opt,
> +static void batch_one_object(const char *obj_name,
> +			     struct strbuf *scratch,
> +			     struct batch_options *opt,
>  			     struct expand_data *data)
>  {
>  	struct object_context ctx;
> @@ -405,20 +407,21 @@ static void batch_one_object(const char *obj_name, struct batch_options *opt,
>  		return;
>  	}
>  
> -	batch_object_write(obj_name, opt, data);
> +	batch_object_write(obj_name, scratch, opt, data);
>  }
>  
>  struct object_cb_data {
>  	struct batch_options *opt;
>  	struct expand_data *expand;
>  	struct oidset *seen;
> +	struct strbuf *scratch;
>  };
>  
>  static int batch_object_cb(const struct object_id *oid, void *vdata)
>  {
>  	struct object_cb_data *data = vdata;
>  	oidcpy(&data->expand->oid, oid);
> -	batch_object_write(NULL, data->opt, data->expand);
> +	batch_object_write(NULL, data->scratch, data->opt, data->expand);
>  	return 0;
>  }
>  
> @@ -509,6 +512,7 @@ static int batch_objects(struct batch_options *opt)
>  
>  		cb.opt = opt;
>  		cb.expand = &data;
> +		cb.scratch = &output;
>  
>  		if (opt->unordered) {
>  			struct oidset seen = OIDSET_INIT;
> @@ -531,6 +535,7 @@ static int batch_objects(struct batch_options *opt)
>  			oid_array_clear(&sa);
>  		}
>  
> +		strbuf_release(&output);
>  		return 0;
>  	}
>  
> @@ -559,10 +564,11 @@ static int batch_objects(struct batch_options *opt)
>  			data.rest = p;
>  		}
>  
> -		batch_one_object(input.buf, opt, &data);
> +		batch_one_object(input.buf, &output, opt, &data);
>  	}
>  
>  	strbuf_release(&input);
> +	strbuf_release(&output);
>  	warn_on_object_refname_ambiguity = save_warning;
>  	return retval;
>  }
> 
Previous: Jeff KingNext: Jeff King
Message 21 of 27 in “speeding up cat-file by reordering object access”
  1. 0/7 speeding up cat-file by reordering object accessJeff King, Aug 10, 2018
  2. 1/7 for_each_*_object: store flag definitions in a single locationJeff King, Aug 10, 2018
  3. Stefan BellerAug 10, 2018
  4. Jeff KingAug 10, 2018
  5. Jeff KingAug 10, 2018
  6. Stefan BellerAug 10, 2018
  7. Jeff KingAug 11, 2018
  8. 2/7 for_each_*_object: take flag arguments as enumJeff King, Aug 10, 2018
  9. 3/7 for_each_*_object: give more comprehensive docstringsJeff King, Aug 10, 2018
  10. 4/7 for_each_packed_object: support iterating in pack-orderJeff King, Aug 10, 2018
  11. Derrick StoleeAug 15, 2018
  12. Jeff KingAug 16, 2018
  13. 5/7 t1006: test cat-file --batch-all-objects with duplicatesJeff King, Aug 10, 2018
  14. 6/7 cat-file: rename batch_{loose,packed}_object callbacksJeff King, Aug 10, 2018
  15. 7/7 cat-file: support "unordered" output for --batch-all-objectsJeff King, Aug 10, 2018
  16. Jonathan TanAug 13, 2018
  17. 0/4 finishing touches on jk/for-each-object-iterationJeff King, Aug 14, 2018
  18. 1/4 cat-file: use oidset check-and-insertJeff King, Aug 14, 2018
  19. 2/4 cat-file: split batch "buf" into two variablesJeff King, Aug 14, 2018
  20. 3/4 cat-file: use a single strbuf for all outputJeff King, Aug 14, 2018
  21. René ScharfeAug 14, 2018
  22. Jeff KingAug 14, 2018
  23. 4/4 for_each_*_object: move declarations to object-store.hJeff King, Aug 14, 2018
  24. Derrick StoleeAug 15, 2018
  25. Jeff KingAug 16, 2018
  26. Junio C HamanoAug 16, 2018
  27. Jeff KingAug 16, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.