Re: [PATCH 6/7] xdiff: conditionally use Rust's implementation of xxhash

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 17, 2025, 23:29 UTC
Message-ID: <aHmHb99q/if6KTTw@nand.local>
In-Reply-To: <5a959c9bdad79cf972b95dcf4324135dd7c94dac.1752784344.git.gitgitgadget@gmail.com>

On Thu, Jul 17, 2025 at 08:32:23PM +0000, Ezekiel Newren via GitGitGadget wrote:

Show 14 quoted lines

>     // desktop
>     // CPU: 8-core Intel Core i7-9700 (-MCP-) speed/min/max: 800/800/4700 MHz
>     $ hyperfine --warmup 3 -L exe $BASE/build_release/git,$BASE/build_v2.49.0/git '{exe} log --oneline --shortstat v6.8..v6.9 >/dev/null'
>     Benchmark 1: /home/steamuser/dev/git/build_release/git log --oneline --shortstat v6.8..v6.9 >/dev/null
>       Time (mean ± σ):      6.823 s ±  0.020 s    [User: 6.624 s, System: 0.180 s]
>       Range (min … max):    6.801 s …  6.858 s    10 runs
>
>     Benchmark 2: /home/steamuser/dev/git/build_v2.49.0/git log --oneline --shortstat v6.8..v6.9 >/dev/null
>       Time (mean ± σ):      8.151 s ±  0.024 s    [User: 7.928 s, System: 0.198 s]
>       Range (min … max):    8.105 s …  8.184 s    10 runs
>
>     Summary
>       /home/steamuser/dev/git/build_release/git log --oneline --shortstat v6.8..v6.9 >/dev/null ran
>         1.19 ± 0.01 times faster than /home/steamuser/dev/git/build_v2.49.0/git log --oneline --shortstat v6.8..v6.9 >/dev/null

Very cool!

Show 7 quoted lines

> Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
> ---
>  rust/Cargo.lock       |  7 +++++++
>  rust/xdiff/Cargo.toml |  1 +
>  rust/xdiff/src/lib.rs |  7 +++++++
>  xdiff/xprepare.c      | 19 +++++++++++++++++--
>  4 files changed, 32 insertions(+), 2 deletions(-)

This patch is delightfully simple. Thank you for carefully preparing the previous five patches to make this one as tiny as it is.

Show 5 quoted lines

> @@ -175,14 +178,26 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_
>
>  	xdl_parse_lines(mf, narec, xdf);
>
> +	if ((xpp->flags & XDF_WHITESPACE_FLAGS) == 0) {

It may be worth adding a comment here to explain why we aren't using xdl_hash_record() when xpp->flags lacks XDF_WHITESPACE_FLAGS.

(As a meta-note for reviewing this series, there are a handful of style nits that I haven't mentioned, e.g., if (... == 0) instead of if (!...). But since the xdiff code doesn't match the project's style conventions, I have avoided mentioning it in my review.)

Show 17 quoted lines

> +		for (usize i = 0; i < (usize) xdf->nrec; i++) {
> +			xrecord_t *rec = xdf->recs[i];
> +			rec->ha = xxh3_64(rec->ptr, rec->size);
> +		}
> +	} else {
> +		for (usize i = 0; i < (usize) xdf->nrec; i++) {
> +			xrecord_t *rec = xdf->recs[i];
> +			char const* dump = (char const*) rec->ptr;
> +			rec->ha = xdl_hash_record(&dump, (char const*) (rec->ptr + rec->size), xpp->flags);
> +		}
> +	}
> +
>  	for (usize i = 0; i < (usize) xdf->nrec; i++) {
>  		xrecord_t *rec = xdf->recs[i];
> -		char const* dump = (char const*) rec->ptr;
> -		rec->ha = xdl_hash_record(&dump, (char const*) (rec->ptr + rec->size), xpp->flags);
>  		xdl_classify_record(pass, cf, rec);

I am curious why you are calling xdl_classify_record() here as a post-processing step rather than inline with the hash calculation above.

Thanks, Taylor