Re: [PATCH 1/3] sha1_name: Create perf test for find_unique_abbrev()

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 18, 2017, 00:51 UTC
Message-ID: <xmqqa81su8v9.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170915165750.198201-2-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> writes:

> +int cmd_main(int ac, const char **av)
> +{
> +	setup_git_directory();

As far as I recall, we do not (yet) allow declaration after statement in our codebase. Move this down to make it after all decls.

Show 7 quoted lines

> +
> +	unsigned int hash_delt = 0x13579BDF;
> +	unsigned int hash_base = 0x01020304;
> +	struct object_id oid;
> +
> +	int i, count = 0;
> +	int n = sizeof(struct object_id) / sizeof(int);

It probably is technically OK to assume sizeof(int) always equals to sizeof(unsigned), but because you use 'n' _only_ to work with uint and never with int, it would make more sense to match.

But I do not think we want this "clever" optimization that involves 'n' in the first place.

> +	while (count++ < 100000) {
> +		for (i = 0; i < n; i++)
> +			((unsigned int*)oid.hash)[i] = hash_base;

Does it make sense to assume that uint is always 4-byte (so this code won't work if it is 8-byte on your platform) and doing this is faster than using platform-optimized memcpy()?

> +		find_unique_abbrev(oid.hash, MINIMUM_ABBREV);
> +
> +		hash_base += hash_delt;
> +	}

I also wonder if this is measuring the right thing. I am guessing that by making many queries for a unique abbreviation of "random" (well, it is deterministic, but my point is these queries are not based on the names of objects that exist in the repository) hashes, this test measures how much time it takes for us to decide that such a random hash does not exist. In the real life use, we make the opposite query far more frequently: we have an object that we _know_ exists in the repository and we try to find a sufficient length to disambiguate it from others, and I suspect that these two use different logic. Don't you need to be measuring the time it takes to compute the shortest abbreviation of an object that exists instead?

Show 20 quoted lines

> +	exit(0);
> +}
> diff --git a/t/perf/p0008-abbrev.sh b/t/perf/p0008-abbrev.sh
> new file mode 100755
> index 000000000..7c3fad807
> --- /dev/null
> +++ b/t/perf/p0008-abbrev.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +
> +test_description='Test object disambiguation through abbreviations'
> +. ./perf-lib.sh
> +
> +test_perf_large_repo
> +
> +test_perf 'find_unique_abbrev()' '
> +	test-abbrev
> +'
> +
> +test_done