Re: [PATCH 3/4] ls-files: use strbuf_add_uint()

[Jeff King <peff@peff.net>]

On Tue, May 12, 2026 at 01:56:02PM +0200, René Scharfe wrote:

> Speed up printing of objectsize values by using the specialized function
> strbuf_add_uint() as well as strbuf_insert() for padding instead of the
> general-purpose function strbuf_addf().  Here are the numbers I get when
> listing files in the Linux kernel repo:
> 
> Benchmark 1: ./git_main -C ../linux ls-files --format='%(objectsize)'
>   Time (mean ± σ):     257.3 ms ±   0.4 ms    [User: 197.4 ms, System: 56.7 ms]
>   Range (min … max):   256.7 ms … 258.1 ms    11 runs
> 
> Benchmark 2: ./git -C ../linux ls-files --format='%(objectsize)'
>   Time (mean ± σ):     253.4 ms ±   0.3 ms    [User: 193.6 ms, System: 56.6 ms]
>   Range (min … max):   253.0 ms … 253.8 ms    11 runs

OK, so here the improvement is less impressive than the previous commit.
And the code is...

>  {
> +	static const char padding[] = "       ";
> +	size_t min_len = padded ? strlen(padding) : 0;
> +	size_t orig_len = line->len;
> +	size_t len;
> +
>  	if (type == OBJ_BLOB) {
>  		unsigned long size;
>  		if (odb_read_object_info(repo->objects, oid, &size) < 0)
>  			die(_("could not get object info about '%s'"),
>  			    oid_to_hex(oid));
> -		if (padded)
> -			strbuf_addf(line, "%7"PRIuMAX, (uintmax_t)size);
> -		else
> -			strbuf_addf(line, "%"PRIuMAX, (uintmax_t)size);
> -	} else if (padded) {
> -		strbuf_addf(line, "%7s", "-");
> +		strbuf_add_uint(line, size);
>  	} else {
>  		strbuf_addstr(line, "-");
>  	}
> +	len = line->len - orig_len;
> +	if (len < min_len)
> +		strbuf_insert(line, orig_len, padding, min_len - len);
>  }

...also less nice. We are formatting into the strbuf, and then maybe
memmove()-ing the result to accommodate padding. I wonder how much that
affects the timing. It's extra shuffling, but memmove() etc is often
surprisingly fast.

I wonder how bad it would be to handle the padding ahead of time.
Obviously strbuf_add_uint() knows the size of the result right before it
calls memcpy(), and it could insert the padding then. But adding a
padding length parameter (let alone the space vs "0" decision) to that
function feels kind of gross.

In the earlier patch I raised the notion of pre-computing the output
length. If we had a helper to do that, it would be pretty easy to do:

  /* noop if third parameter is negative */
  strbuf_pad(line, ' ', 7 - decimal_digits(size));
  strbuf_add_uint(line, size);

You could also imagine a world where we had some stateful formatting
system, and you could say:

  strbuf_pad_next(line, ' ', 7);
  strbuf_add_uint(line, size);

but somebody has to store that state between the calls, and I don't love
the idea of bloating strbuf with it. So probably you have some
"formatter" struct, and it operates on a strbuf. And now we have all of
the OO boilerplate hassles like initializing and tearing down our
formatter object. ;) So probably not worth it for this triviality.
Having it all in one string ("%7d") is nice and concise.

I have often wondered how hard it would be to implement our own
vsnprintf(), and whether we could do better than the libc ones. It would
be nice to be able to add shorthands for common types (instead of the
unreadable PRIuMAX mess), as well as custom ones (e.g., hex oids).

Probably also not worth the headache. ;)

-Peff