pack-objects: Create an alternative name hash algorithm (recreated) #1823
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickstolee
force-pushed
the
full-name
branch
2 times, most recently
from
d7b67b7 to
ab341dd
Compare
|
/submit |
|
Submitted as pull.1823.git.1730775907.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@a4c5ed7. |
|
This patch series was integrated into seen via git@4f1ca50. |
|
This patch series was integrated into seen via git@38d672e. |
|
This patch series was integrated into seen via git@e81b3ef. |
|
This patch series was integrated into seen via git@27dd7da. |
|
This patch series was integrated into seen via git@3feb04a. |
|
This patch series was integrated into seen via git@f70d861. |
|
This patch series was integrated into seen via git@960de39. |
|
This patch series was integrated into seen via git@db680f6. |
|
This patch series was integrated into seen via git@d9378c7. |
|
This patch series was integrated into seen via git@27b487d. |
|
This patch series was integrated into seen via git@becc917. |
|
This patch series was integrated into seen via git@6ac0572. |
|
This patch series was integrated into seen via git@f6a46ca. |
|
This patch series was integrated into seen via git@e8ae937. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -15,7 +15,8 @@ SYNOPSIS | |||
| [--revs [--unpacked | --all]] [--keep-pack=<pack-name>] | |||
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Tue, Nov 05, 2024 at 03:05:01AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> The pack_name_hash() method has not been materially changed since it was
> introduced in ce0bd64299a (pack-objects: improve path grouping
> heuristics., 2006-06-05). The intention here is to group objects by path
> name, but also attempt to group similar file types together by making
> the most-significant digits of the hash be focused on the final
> characters.
>
> Here's the crux of the implementation:
>
> /*
> * This effectively just creates a sortable number from the
> * last sixteen non-whitespace characters. Last characters
> * count "most", so things that end in ".c" sort together.
> */
> while ((c = *name++) != 0) {
> if (isspace(c))
> continue;
> hash = (hash >> 2) + (c << 24);
> }
Hah. I like that the existing implementation is small enough to fit (in
its entirety!) into the commit message!
> As the comment mentions, this only cares about the last sixteen
> non-whitespace characters. This cause some filenames to collide more
> than others. Here are some examples that I've seen while investigating
> repositories that are growing more than they should be:
>
> * "/CHANGELOG.json" is 15 characters, and is created by the beachball
> [1] tool. Only the final character of the parent directory can
> differntiate different versions of this file, but also only the two
s/differntiate/differentiate ;-).
> most-significant digits. If that character is a letter, then this is
> always a collision. Similar issues occur with the similar
> "/CHANGELOG.md" path, though there is more opportunity for
> differences in the parent directory.
>
> * Localization files frequently have common filenames but differentiate
> via parent directories. In C#, the name "/strings.resx.lcl" is used
> for these localization files and they will all collide in name-hash.
>
> [1] https://github.com/microsoft/beachball
>
> I've come across many other examples where some internal tool uses a
> common name across multiple directories and is causing Git to repack
> poorly due to name-hash collisions.
>
> It is clear that the existing name-hash algorithm is optimized for
> repositories with short path names, but also is optimized for packing a
> single snapshot of a repository, not a repository with many versions of
> the same file. In my testing, this has proven out where the name-hash
> algorithm does a good job of finding peer files as delta bases when
> unable to use a historical version of that exact file.
I'm not sure I entirely agree with the suggestion that the existing hash
function is only about packing repositories with short pathnames. I
think an important part of the existing implementation is that tries to
group similar files together, regardless of whether or not they appear
in the same tree.
As you have shown, this can be a problem when the fact two files that
happen to end in "CHANGELOG.json" end up in vastly different trees and
*aren't* related. I don't think that nailing all of these details in the
commit message is necessary, but I do think it's worth adjusting what
the original commit message says in terms of what the existing algorithm
is optimized for.
> However, for repositories that have many versions of most files and
> directories, it is more important that the objects that appear at the
> same path are grouped together.
>
> Create a new pack_full_name_hash() method and a new --full-name-hash
> option for 'git pack-objects' to call that method instead. Add a simple
> pass-through for 'git repack --full-name-hash' for additional testing in
> the context of a full repack, where I expect this will be most
> effective.
>
> The hash algorithm is as simple as possible to be reasonably effective:
> for each character of the path string, add a multiple of that character
> and a large prime number (chosen arbitrarily, but intended to be large
> relative to the size of a uint32_t). Then, shift the current hash value
> to the right by 5, with overlap. The addition and shift parameters are
> standard mechanisms for creating hard-to-predict behaviors in the bits
> of the resulting hash.
>
> This is not meant to be cryptographic at all, but uniformly distributed
> across the possible hash values. This creates a hash that appears
> pseudorandom. There is no ability to consider similar file types as
> being close to each other.
I think you hint at this in the series' cover letter, but I suspect that
this pseduorandom behavior hurts in some small number of cases and that
the full-name hash idea isn't a pure win, e.g., when we really do want
to delta two paths that both end in CHAGNELOG.json despite being in
different parts of the tree.
You have some tables here below that demonstrate a significant
improvement with the full-name hash in use, which I think is good worth
keeping in my own opinion. It may be worth updating those to include the
new examples you highlighted in your revised cover letter as well.
> In a later change, a test-tool will be added so the effectiveness of
> this hash can be demonstrated directly.
>
> For now, let's consider how effective this mechanism is when repacking a
> repository with and without the --full-name-hash option. Specifically,
Is this repository publicly available? If so, it may be worth mentioning
here.
> let's use 'git repack -adf [--full-name-hash]' as our test.
>
> On the Git repository, we do not expect much difference. All path names
> are short. This is backed by our results:
>
> | Stage | Pack Size | Repack Time |
> |-----------------------|-----------|-------------|
> | After clone | 260 MB | N/A |
> | Standard Repack | 127MB | 106s |
> | With --full-name-hash | 126 MB | 99s |
Ahh. Here's a great example of it helping to a smaller extent. Thanks
for including this as part of demonstrating the full picture (both the
benefits and drawbacks).
> This example demonstrates how there is some natural overhead coming from
> the cloned copy because the server is hosting many forks and has not
> optimized for exactly this set of reachable objects. But the full repack
> has similar characteristics with and without --full-name-hash.
Good.
> However, we can test this in a repository that uses one of the
> problematic naming conventions above. The fluentui [2] repo uses
> beachball to generate CHANGELOG.json and CHANGELOG.md files, and these
> files have very poor delta characteristics when comparing against
> versions across parent directories.
>
> | Stage | Pack Size | Repack Time |
> |-----------------------|-----------|-------------|
> | After clone | 694 MB | N/A |
> | Standard Repack | 438 MB | 728s |
> | With --full-name-hash | 168 MB | 142s |
>
> [2] https://github.com/microsoft/fluentui
>
> In this example, we see significant gains in the compressed packfile
> size as well as the time taken to compute the packfile.
Amazing!
> Using a collection of repositories that use the beachball tool, I was
> able to make similar comparisions with dramatic results. While the
> fluentui repo is public, the others are private so cannot be shared for
> reproduction. The results are so significant that I find it important to
> share here:
>
> | Repo | Standard Repack | With --full-name-hash |
> |----------|-----------------|-----------------------|
> | fluentui | 438 MB | 168 MB |
> | Repo B | 6,255 MB | 829 MB |
> | Repo C | 37,737 MB | 7,125 MB |
> | Repo D | 130,049 MB | 6,190 MB |
>
> Future changes could include making --full-name-hash implied by a config
> value or even implied by default during a full repack.
>
> It is important to point out that the name hash value is stored in the
> .bitmap file format, so we must disable the --full-name-hash option when
> bitmaps are being read or written. Later, the bitmap format could be
> updated to be aware of the name hash version so deltas can be quickly
> computed across the bitmapped/not-bitmapped boundary.
Agreed.
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
> Documentation/git-pack-objects.txt | 3 ++-
> builtin/pack-objects.c | 25 ++++++++++++++++++++-----
> builtin/repack.c | 5 +++++
> pack-objects.h | 21 +++++++++++++++++++++
> t/t5300-pack-object.sh | 15 +++++++++++++++
> 5 files changed, 63 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
> index e32404c6aae..93861d9f85b 100644
> --- a/Documentation/git-pack-objects.txt
> +++ b/Documentation/git-pack-objects.txt
> @@ -15,7 +15,8 @@ SYNOPSIS
> [--revs [--unpacked | --all]] [--keep-pack=<pack-name>]
> [--cruft] [--cruft-expiration=<time>]
> [--stdout [--filter=<filter-spec>] | <base-name>]
> - [--shallow] [--keep-true-parents] [--[no-]sparse] < <object-list>
> + [--shallow] [--keep-true-parents] [--[no-]sparse]
> + [--full-name-hash] < <object-list>
OK, I see that --full-name-hash is now listed in the synopsis, but I
don't see a corresponding description of what the option does later on
in this file. I took a look through the remaining patches in this series
and couldn't find any further changes to git-pack-objects(1) either.
> DESCRIPTION
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 08007142671..85595dfcd88 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -266,6 +266,14 @@ struct configured_exclusion {
> static struct oidmap configured_exclusions;
>
> static struct oidset excluded_by_config;
> +static int use_full_name_hash;
> +
> +static inline uint32_t pack_name_hash_fn(const char *name)
> +{
> + if (use_full_name_hash)
> + return pack_full_name_hash(name);
> + return pack_name_hash(name);
> +}
>
> /*
> * stats
> @@ -1698,7 +1706,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
> return 0;
> }
>
> - create_object_entry(oid, type, pack_name_hash(name),
> + create_object_entry(oid, type, pack_name_hash_fn(name),
> exclude, name && no_try_delta(name),
> found_pack, found_offset);
> return 1;
> @@ -1912,7 +1920,7 @@ static void add_preferred_base_object(const char *name)
> {
> struct pbase_tree *it;
> size_t cmplen;
> - unsigned hash = pack_name_hash(name);
> + unsigned hash = pack_name_hash_fn(name);
>
> if (!num_preferred_base || check_pbase_path(hash))
> return;
> @@ -3422,7 +3430,7 @@ static void show_object_pack_hint(struct object *object, const char *name,
> * here using a now in order to perhaps improve the delta selection
> * process.
> */
> - oe->hash = pack_name_hash(name);
> + oe->hash = pack_name_hash_fn(name);
> oe->no_try_delta = name && no_try_delta(name);
>
> stdin_packs_hints_nr++;
> @@ -3572,7 +3580,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type
> entry = packlist_find(&to_pack, oid);
> if (entry) {
> if (name) {
> - entry->hash = pack_name_hash(name);
> + entry->hash = pack_name_hash_fn(name);
> entry->no_try_delta = no_try_delta(name);
> }
> } else {
> @@ -3595,7 +3603,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type
> return;
> }
>
> - entry = create_object_entry(oid, type, pack_name_hash(name),
> + entry = create_object_entry(oid, type, pack_name_hash_fn(name),
> 0, name && no_try_delta(name),
> pack, offset);
> }
> @@ -4429,6 +4437,8 @@ int cmd_pack_objects(int argc,
> OPT_STRING_LIST(0, "uri-protocol", &uri_protocols,
> N_("protocol"),
> N_("exclude any configured uploadpack.blobpackfileuri with this protocol")),
> + OPT_BOOL(0, "full-name-hash", &use_full_name_hash,
> + N_("optimize delta compression across identical path names over time")),
> OPT_END(),
> };
>
> @@ -4576,6 +4586,11 @@ int cmd_pack_objects(int argc,
> if (pack_to_stdout || !rev_list_all)
> write_bitmap_index = 0;
>
> + if (write_bitmap_index && use_full_name_hash) {
> + warning(_("currently, the --full-name-hash option is incompatible with --write-bitmap-index"));
> + use_full_name_hash = 0;
> + }
> +
Good, we determine this early on in the command, so we don't risk
computing different hash functions within the same process.
I wonder if it's worth guarding against mixing the hash functions within
the pack_name_hash() and pack_full_name_hash() functions themselves. I'm
thinking something like:
static inline uint32_t pack_name_hash(const char *name)
{
if (use_full_name_hash)
BUG("called pack_name_hash() with --full-name-hash")
/* ... */
}
and the inverse in pack_full_name_hash(). I don't think it's strictly
necessary, but it would be a nice guard against someone calling, e.g.,
pack_full_name_hash() directly instead of pack_name_hash_fn().
The other small thought I had here is that we should use the convenience
function die_for_incompatible_opt3() here, since it uses an existing
translation string for pairs of incompatible options.
(As an aside, though that function is actually implemented in the
_opt4() variant, and it knows how to handle a pair, trio, and quartet of
mutually incompatible options, there is no die_for_incompatible_opt2()
function. It may be worth adding one here since I'm sure there are other
spots which would benefit from such a function).
> diff --git a/builtin/repack.c b/builtin/repack.c
> index d6bb37e84ae..ab2a2e46b20 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
I'm surprised to see the new option plumbed into repack in this commit.
I would have thought that it'd appear in the subsequent commit instead.
The implementation below looks good, I just imagined it would be placed
in the next commit instead of this one.
The remaining parts of this change look good to me.
Thanks,
TaylorThere was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Thu, Nov 21, 2024 at 03:08:09PM -0500, Taylor Blau wrote:
> The remaining parts of this change look good to me.
Oops, one thing I forgot (which reading Peff's message in [1] reminded
me of) is that I think we need to disable full-name hashing when we're
reusing existing packfiles as is the case with try_partial_reuse().
There we're always looking at classic name hash values, so mixing the
two would be a mistake. I think that amounts to:
--- 8< ---
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 762949e4c8..7e370bcfc9 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4070,6 +4070,8 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
if (!(bitmap_git = prepare_bitmap_walk(revs, 0)))
return -1;
+ use_full_name_hash = 0;
+
if (pack_options_allow_reuse())
reuse_partial_packfile_from_bitmap(bitmap_git,
&reuse_packfiles,
--- >8 ---
Thanks,
Taylor
[1]: https://lore.kernel.org/git/20241104172533.GA2985568@coredump.intra.peff.net/There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Taylor Blau <me@ttaylorr.com> writes:
> On Thu, Nov 21, 2024 at 03:08:09PM -0500, Taylor Blau wrote:
>> The remaining parts of this change look good to me.
>
> Oops, one thing I forgot (which reading Peff's message in [1] reminded
> me of) is that I think we need to disable full-name hashing when we're
> reusing existing packfiles as is the case with try_partial_reuse().
>
> There we're always looking at classic name hash values, so mixing the
> two would be a mistake. I think that amounts to:
>
> --- 8< ---
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 762949e4c8..7e370bcfc9 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4070,6 +4070,8 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
> if (!(bitmap_git = prepare_bitmap_walk(revs, 0)))
> return -1;
>
> + use_full_name_hash = 0;
Hmph, is this early enough, or has some other code path already
computed the name hashes for the paths for the files to be packed?
... Goes and looks ...
This is called from get_object_list() which
- is not called under --stdin-packs,
- is not called in cruft mode,
- is not called when reading object list from --stdin
so we are looking at the bog-standard "objects to be packed are
given in the form of rev-list command line options from our command
line". And in the function, we walk the history near the end, which
makes show_object calls that adds object-entry with the name-hash.
So the call to get_object_list_from_bitmap() happens way before the
first use of the name-hash function, so this is probably safe.
And obviously get_object_list_from_bitmap() is the only place we
select objects to be packed from an existing pack and a bitmap file,
so even if we gain new callers in the future, it is very likely that
the new callers would benefit from this change.
OK. Nicely done.
> if (pack_options_allow_reuse())
> reuse_partial_packfile_from_bitmap(bitmap_git,
> &reuse_packfiles,
> --- >8 ---
>
> Thanks,
> Taylor
>
> [1]: https://lore.kernel.org/git/20241104172533.GA2985568@coredump.intra.peff.net/There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/21/24 4:35 PM, Taylor Blau wrote:
> On Thu, Nov 21, 2024 at 03:08:09PM -0500, Taylor Blau wrote:
>> The remaining parts of this change look good to me.
> > Oops, one thing I forgot (which reading Peff's message in [1] reminded
> me of) is that I think we need to disable full-name hashing when we're
> reusing existing packfiles as is the case with try_partial_reuse().
> > There we're always looking at classic name hash values, so mixing the
> two would be a mistake. I think that amounts to:
> > --- 8< ---
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 762949e4c8..7e370bcfc9 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4070,6 +4070,8 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
> if (!(bitmap_git = prepare_bitmap_walk(revs, 0)))
> return -1;
> > + use_full_name_hash = 0;
> +
Thanks. I have applied this code change with a comment detailing
the context around the bitmap file storing only the default name-hash
(for now) but that it can change in the future.
Thanks,
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/21/24 3:08 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:01AM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>> It is clear that the existing name-hash algorithm is optimized for
>> repositories with short path names, but also is optimized for packing a
>> single snapshot of a repository, not a repository with many versions of
>> the same file. In my testing, this has proven out where the name-hash
>> algorithm does a good job of finding peer files as delta bases when
>> unable to use a historical version of that exact file.
> > I'm not sure I entirely agree with the suggestion that the existing hash
> function is only about packing repositories with short pathnames. I
> think an important part of the existing implementation is that tries to
> group similar files together, regardless of whether or not they appear
> in the same tree.
I'll be more explicit about the design for "hash locality" earlier in
the message, but also pointing out that the locality only makes sense as
a benefit when there are not enough versions of a file in history, since
it's nearly always better to choose a previous version of the same file
instead of a different path with a name-hash collision. Directory renames
are on place where this is a positive decision, but those are typically
rare compared to the full history of a large repo.
>> This is not meant to be cryptographic at all, but uniformly distributed
>> across the possible hash values. This creates a hash that appears
>> pseudorandom. There is no ability to consider similar file types as
>> being close to each other.
> > I think you hint at this in the series' cover letter, but I suspect that
> this pseduorandom behavior hurts in some small number of cases and that
> the full-name hash idea isn't a pure win, e.g., when we really do want
> to delta two paths that both end in CHAGNELOG.json despite being in
> different parts of the tree.
I mention that this doesn't work well in all cases when operating under
a 'git push' or in a shallow clone. Shallow clones are disabled in a later
commit and we don't have the necessary implementation to make this hash
function be selected within 'git push'.
> You have some tables here below that demonstrate a significant
> improvement with the full-name hash in use, which I think is good worth
> keeping in my own opinion. It may be worth updating those to include the
> new examples you highlighted in your revised cover letter as well.
I'll try to remember to move the newer examples to the cover letter.
>> In a later change, a test-tool will be added so the effectiveness of
>> this hash can be demonstrated directly.
>>
>> For now, let's consider how effective this mechanism is when repacking a
>> repository with and without the --full-name-hash option. Specifically,
> > Is this repository publicly available? If so, it may be worth mentioning
> here.
Here, by "when repacking a repository" I mean "we are going to test
repacking a number of example repositories, that will be listed in detail
in the coming tables".
>> Using a collection of repositories that use the beachball tool, I was
>> able to make similar comparisions with dramatic results. While the
>> fluentui repo is public, the others are private so cannot be shared for
>> reproduction. The results are so significant that I find it important to
>> share here:
>>
>> | Repo | Standard Repack | With --full-name-hash |
>> |----------|-----------------|-----------------------|
>> | fluentui | 438 MB | 168 MB |
>> | Repo B | 6,255 MB | 829 MB |
>> | Repo C | 37,737 MB | 7,125 MB |
>> | Repo D | 130,049 MB | 6,190 MB |
These repos B, C, and D are _not_ publicly available, though.
>> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
>> index e32404c6aae..93861d9f85b 100644
>> --- a/Documentation/git-pack-objects.txt
>> +++ b/Documentation/git-pack-objects.txt
>> @@ -15,7 +15,8 @@ SYNOPSIS
>> [--revs [--unpacked | --all]] [--keep-pack=<pack-name>]
>> [--cruft] [--cruft-expiration=<time>]
>> [--stdout [--filter=<filter-spec>] | <base-name>]
>> - [--shallow] [--keep-true-parents] [--[no-]sparse] < <object-list>
>> + [--shallow] [--keep-true-parents] [--[no-]sparse]
>> + [--full-name-hash] < <object-list>
> > OK, I see that --full-name-hash is now listed in the synopsis, but I
> don't see a corresponding description of what the option does later on
> in this file. I took a look through the remaining patches in this series
> and couldn't find any further changes to git-pack-objects(1) either.
I'll fix that. Thanks. As well as moving the 'git repack' changes out
of this patch. I'll adjust the commit message to say "packing all objects'
instead of 'git repack' to be clear that this can be done with a direct
call to 'git pack-objects' instead of needing 'git repack'.
>> + if (write_bitmap_index && use_full_name_hash) {
>> + warning(_("currently, the --full-name-hash option is incompatible with --write-bitmap-index"));
>> + use_full_name_hash = 0;
>> + }
>> +
> > Good, we determine this early on in the command, so we don't risk
> computing different hash functions within the same process.
> > I wonder if it's worth guarding against mixing the hash functions within
> the pack_name_hash() and pack_full_name_hash() functions themselves. I'm
> thinking something like:
> > static inline uint32_t pack_name_hash(const char *name)
> {
> if (use_full_name_hash)
> BUG("called pack_name_hash() with --full-name-hash")
> /* ... */
> }
> > and the inverse in pack_full_name_hash(). I don't think it's strictly
> necessary, but it would be a nice guard against someone calling, e.g.,
> pack_full_name_hash() directly instead of pack_name_hash_fn().
I think this is interesting defensive programming for future contributions.
We essentially want the methods to only be called by pack_name_hash_fn()
and don't have method privacy. We could extract it to its own header file
but then would need to modify the prototype to include the signal for
which hash type to use, but that would cause us to lose our ability to
check for a bug like this.
It may be even better to store a static value for the value of
use_full_name_hash when it first executes, so it can exit if it notices
a different value. (This is becoming large enough for its own patch.)
> The other small thought I had here is that we should use the convenience
> function die_for_incompatible_opt3() here, since it uses an existing
> translation string for pairs of incompatible options.
> > (As an aside, though that function is actually implemented in the
> _opt4() variant, and it knows how to handle a pair, trio, and quartet of
> mutually incompatible options, there is no die_for_incompatible_opt2()
> function. It may be worth adding one here since I'm sure there are other
> spots which would benefit from such a function).
Interesting. I've not considered these functions before.
>> diff --git a/builtin/repack.c b/builtin/repack.c
>> index d6bb37e84ae..ab2a2e46b20 100644
>> --- a/builtin/repack.c
>> +++ b/builtin/repack.c
> > I'm surprised to see the new option plumbed into repack in this commit.
> I would have thought that it'd appear in the subsequent commit instead.
> The implementation below looks good, I just imagined it would be placed
> in the next commit instead of this one.
Yes, I should delay that to patch 2.
Thanks,
-Stole
| @@ -777,6 +777,13 @@ test_expect_success 'repack -ad cleans up old .tmp-* packs' ' | |||
| test_must_be_empty tmpfiles | |||
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Tue, Nov 05, 2024 at 03:05:02AM +0000, Derrick Stolee via GitGitGadget wrote:
> ---
> t/t7700-repack.sh | 7 +++++++
> t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++
> 2 files changed, 33 insertions(+)
OK, I stand by my thinking in the previous patch that this one is where
the changes to builtin/repack.c belong.
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index c4c3d1a15d9..fc2cc9d37be 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -777,6 +777,13 @@ test_expect_success 'repack -ad cleans up old .tmp-* packs' '
> test_must_be_empty tmpfiles
> '
>
> +test_expect_success '--full-name-hash option passes through to pack-objects' '
> + GIT_TRACE2_EVENT="$(pwd)/full-trace.txt" \
> + git repack -a --full-name-hash &&
> + test_subcommand_flex git pack-objects --full-name-hash <full-trace.txt
OK. To be honest, I am not sure I would have written the same test to
test trivially correct behavior, but I am not opposed to having such a
test either.
I do think that test_subcommand_flex may be unnecessary though, since
you could instead write this as:
test_subcommand "git pack-objects.*--full-name-hash" <full-trace.txt
and get the same behavior.
> +'
> +
> +
Nit: extra newline here.
Thanks,
TaylorThere was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/21/24 3:12 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:02AM +0000, Derrick Stolee via GitGitGadget wrote:
>> ---
>> t/t7700-repack.sh | 7 +++++++
>> t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++
>> 2 files changed, 33 insertions(+)
> > OK, I stand by my thinking in the previous patch that this one is where
> the changes to builtin/repack.c belong.
Yes. I should have done this already.
> I do think that test_subcommand_flex may be unnecessary though, since
> you could instead write this as:
> > test_subcommand "git pack-objects.*--full-name-hash" <full-trace.txt
> > and get the same behavior.
This does require knowing a bit about the internals of test_subcommand
that may be too much of a burden for future contributors.
Thanks,
-Stolee
| @@ -266,6 +266,14 @@ struct configured_exclusion { | |||
| static struct oidmap configured_exclusions; | |||
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Tue, Nov 05, 2024 at 03:05:03AM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index 2e28d02b20f..75b40f07bbd 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -30,6 +30,7 @@ linux-TEST-vars)
> export GIT_TEST_NO_WRITE_REV_INDEX=1
> export GIT_TEST_CHECKOUT_WORKERS=2
> export GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1
> + export GIT_TEST_FULL_NAME_HASH=1
> ;;
> linux-clang)
> export GIT_TEST_DEFAULT_HASH=sha1
Hmm. I appreciate what this new GIT_TEST_ variable is trying to do, but
I am somewhat saddened to see this list in linux-TEST-vars growing
rather than shrinking.
I'm most definitely part of the problem here, but I think too often we
add new entries to this list and let them languish without ever removing
them after they have served their intended purpose.
So I think the question is: what do we hope to get out of running the
test suite in a mode where we use the full-name hash all of the time? I
can't imagine any interesting breakage (other than individual tests'
sensitivity to specific delta/base pairs) that would be caught by merely
changing the hash function here.
I dunno. Maybe there is some exotic behavior that this shook out for you
during development which I'm not aware of. If that were the case, I
think that keeping this variable around makes sense, since the appearance
of that exotic behavior proves that the variable is useful at shaking
out bugs.
But assuming not, I think that I would just as soon avoid this test
variable entirely, which I think in this case amounts to dropping this
patch from the series.
Thanks,
TaylorThere was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/21/24 3:15 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:03AM +0000, Derrick Stolee via GitGitGadget wrote:
>> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>> index 2e28d02b20f..75b40f07bbd 100755
>> --- a/ci/run-build-and-tests.sh
>> +++ b/ci/run-build-and-tests.sh
>> @@ -30,6 +30,7 @@ linux-TEST-vars)
>> export GIT_TEST_NO_WRITE_REV_INDEX=1
>> export GIT_TEST_CHECKOUT_WORKERS=2
>> export GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1
>> + export GIT_TEST_FULL_NAME_HASH=1
>> ;;
>> linux-clang)
>> export GIT_TEST_DEFAULT_HASH=sha1
> > Hmm. I appreciate what this new GIT_TEST_ variable is trying to do, but
> I am somewhat saddened to see this list in linux-TEST-vars growing
> rather than shrinking.
You make good points that this does not need to be here.
It's enough that someone could manually check the test suite
with this test variable to make sure that enough of the other
options are tested with this feature.
Thanks,
-Stolee
| @@ -9,7 +9,9 @@ git-repack - Pack unpacked objects in a repository | |||
| SYNOPSIS | |||
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Tue, Nov 05, 2024 at 03:05:04AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> This also adds the '--full-name-hash' option introduced in the previous
> change and adds newlines to the synopsis.
I think "the previous change" is not quite accurate here, even if
you move the implementation to pass through '--full-name-hash' via
repack into the second patch.
It would be nice to have the option added in 'repack' in the same commit
as adjusts the documentation instead of splitting them apart.
Thanks,
TaylorThere was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/21/24 3:17 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:04AM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> This also adds the '--full-name-hash' option introduced in the previous
>> change and adds newlines to the synopsis.
> > I think "the previous change" is not quite accurate here, even if
> you move the implementation to pass through '--full-name-hash' via
> repack into the second patch.
Ah, I should definitely rearrange the commits.
> It would be nice to have the option added in 'repack' in the same commit
> as adjusts the documentation instead of splitting them apart.
Part of the point of the split was that the synopsis in builtin/repack.c
needs more than just the addition of the --full-name-hash option in order
to make it match the Documentation synopsis.
But you're right, the code change is small enough that these things can
be combined.
Thanks,
-Stolee
| @@ -0,0 +1,94 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Tue, Nov 05, 2024 at 03:05:05AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> As custom options are added to 'git pack-objects' and 'git repack' to
> adjust how compression is done, use this new performance test script to
> demonstrate their effectiveness in performance and size.
Nicely done, thank you for adding a perf test to allow readers to easily
verify these changes themselves.
> In the case of the Git repository, these numbers show some of the issues
> with this approach:
>
> [...]
>
> The thin pack that simulates a push is much worse with --full-name-hash
> in this case. The name hash values are doing a lot to assist with delta
> bases, it seems. The big pack and shallow clone cases are slightly worse
> with the --full-name-hash option. Only the full repack gains some
> benefits in size.
Not a problem with your patch, but just thinking aloud: do you think
there is an easy/straightforward way to suggest when to use
--full-name-hash or not?
> ---
> t/perf/p5313-pack-objects.sh | 94 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 94 insertions(+)
> create mode 100755 t/perf/p5313-pack-objects.sh
>
> diff --git a/t/perf/p5313-pack-objects.sh b/t/perf/p5313-pack-objects.sh
> new file mode 100755
> index 00000000000..dfa29695315
> --- /dev/null
> +++ b/t/perf/p5313-pack-objects.sh
> @@ -0,0 +1,94 @@
> +#!/bin/sh
> +
> +test_description='Tests pack performance using bitmaps'
> +. ./perf-lib.sh
> +
> +GIT_TEST_PASSING_SANITIZE_LEAK=0
> +export GIT_TEST_PASSING_SANITIZE_LEAK
> +
> +test_perf_large_repo
> +
> +test_expect_success 'create rev input' '
> + cat >in-thin <<-EOF &&
> + $(git rev-parse HEAD)
> + ^$(git rev-parse HEAD~1)
> + EOF
> +
> + cat >in-big <<-EOF &&
> + $(git rev-parse HEAD)
> + ^$(git rev-parse HEAD~1000)
> + EOF
> +
> + cat >in-shallow <<-EOF
> + $(git rev-parse HEAD)
> + --shallow $(git rev-parse HEAD)
> + EOF
> +'
I was going to comment that these could probably be moved into the
individual perf test that cares about reading each of these inputs. But
having them shared here makes sense since we are naturally comparing
generating two packs with the same input (with and without
--full-name-hash). So the shared setup here makes sense to me.
> +
> +test_perf 'thin pack' '
> + git pack-objects --thin --stdout --revs --sparse <in-thin >out
> +'
> +
> +test_size 'thin pack size' '
> + test_file_size out
> +'
Nice. I always forget about this and end up writing 'wc -c <out'.
> +test_perf 'thin pack with --full-name-hash' '
> + git pack-objects --thin --stdout --revs --sparse --full-name-hash <in-thin >out
> +'
> +
> +test_size 'thin pack size with --full-name-hash' '
> + test_file_size out
> +'
> +
> +test_perf 'big pack' '
> + git pack-objects --stdout --revs --sparse <in-big >out
> +'
> +
> +test_size 'big pack size' '
> + test_file_size out
> +'
> +
> +test_perf 'big pack with --full-name-hash' '
> + git pack-objects --stdout --revs --sparse --full-name-hash <in-big >out
> +'
> +
> +test_size 'big pack size with --full-name-hash' '
> + test_file_size out
> +'
> +
> +test_perf 'shallow fetch pack' '
> + git pack-objects --stdout --revs --sparse --shallow <in-shallow >out
> +'
> +
> +test_size 'shallow pack size' '
> + test_file_size out
> +'
> +
> +test_perf 'shallow pack with --full-name-hash' '
> + git pack-objects --stdout --revs --sparse --shallow --full-name-hash <in-shallow >out
> +'
> +
> +test_size 'shallow pack size with --full-name-hash' '
> + test_file_size out
> +'
> +
> +test_perf 'repack' '
> + git repack -adf
> +'
> +
> +test_size 'repack size' '
> + pack=$(ls .git/objects/pack/pack-*.pack) &&
> + test_file_size "$pack"
Here and below, I think it's fine to inline this as in:
test_file_size "$(ls .git/objects/pack/pack-*.pack)"
...but I wonder: will using ".git" break this test in bare repositories?
Should we write instead:
pack="$(ls $(git rev-parse --git-dir)/objects/pack/pack-*.pack)" &&
test_file_size
?
Thanks,
TaylorThere was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/21/24 3:31 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:05AM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>> The thin pack that simulates a push is much worse with --full-name-hash
>> in this case. The name hash values are doing a lot to assist with delta
>> bases, it seems. The big pack and shallow clone cases are slightly worse
>> with the --full-name-hash option. Only the full repack gains some
>> benefits in size.
> > Not a problem with your patch, but just thinking aloud: do you think
> there is an easy/straightforward way to suggest when to use
> --full-name-hash or not?
The kinds of heuristics I would use are:
1. Are there enough commits that enough files have enough versions
across history that it's very important to keep deltas within a path?
2. Is the repository at least 500MB such that there is actually room for
a "meaningful" change in size?
3. Are there a lot of name-hash collisions? (The last patch in the series
helps do this through a test-helper, but isn't something we can expect
end users to check themselves.)
>> + cat >in-shallow <<-EOF
>> + $(git rev-parse HEAD)
>> + --shallow $(git rev-parse HEAD)
>> + EOF
>> +'
> > I was going to comment that these could probably be moved into the
> individual perf test that cares about reading each of these inputs. But
> having them shared here makes sense since we are naturally comparing
> generating two packs with the same input (with and without
> --full-name-hash). So the shared setup here makes sense to me.
I also wanted to avoid having these commands be part of the time
measurement, even if they are extremely small.
>> +
>> +test_perf 'thin pack' '
>> + git pack-objects --thin --stdout --revs --sparse <in-thin >out
>> +'
>> +
>> +test_size 'thin pack size' '
>> + test_file_size out
>> +'
> > Nice. I always forget about this and end up writing 'wc -c <out'.
I believe this is a Junio recommendation from an earlier version.
>> +test_size 'repack size' '
>> + pack=$(ls .git/objects/pack/pack-*.pack) &&
>> + test_file_size "$pack"
> > Here and below, I think it's fine to inline this as in:
> > test_file_size "$(ls .git/objects/pack/pack-*.pack)"
Generally I prefer to split things into stages so the verbose output
provides a clear definition of the value when calling the Git command.
> ...but I wonder: will using ".git" break this test in bare repositories?
> Should we write instead:
> > pack="$(ls $(git rev-parse --git-dir)/objects/pack/pack-*.pack)" &&
> test_file_size
> > ?
While this would break a bare repo, the perf lib makes a bare repo be
copied into a non-bare repo as follows:
test_perf_copy_repo_contents () {
for stuff in "$1"/*
do
case "$stuff" in
*/objects|*/hooks|*/config|*/commondir|*/gitdir|*/worktrees|*/fsmonitor--daemon*)
;;
*)
cp -R "$stuff" "$repo/.git/" || exit 1
;;
esac
done
}
I'll still add the `git rev-parse` suggestion because it's safest.
Thanks,
-Stolee
builtin/pack-objects.c
Outdated
| @@ -4576,6 +4586,20 @@ int cmd_pack_objects(int argc, | |||
| if (pack_to_stdout || !rev_list_all) | |||
| write_bitmap_index = 0; | |||
|
|
|||
| if (use_full_name_hash < 0) | |||
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Tue, Nov 05, 2024 at 03:05:06AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> As demonstrated in the previous change, the --full-name-hash option of
> 'git pack-objects' is less effective in a trunctated history. Thus, even
> when the option is selected via a command-line option or config, disable
> this option when the '--shallow' option is specified. This will help
> performance in servers that choose to enable the --full-name-hash option
> by default for a repository while not regressing their ability to serve
> shallow clones.
>
> This will not present a compatibility issue in the future when the full
> name hash values are stored in the reachability bitmaps, since shallow
> clones disable bitmaps.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
> builtin/pack-objects.c | 6 ++++++
> t/perf/p5313-pack-objects.sh | 1 +
> 2 files changed, 7 insertions(+)
I appreciate demonstrating the value of declaring --shallow and
--full-name-hash incompatible by showing the performance numbers in the
previous patch.
But TBH I think that it would be equally fine or slightly better to say
up front "when combined with --shallow, this option produces larger
packs during testing, so the two are incompatible for now". You could
include some performance numbers there to illustrate that difference in
the commit log too if you wanted.
But I don't think it's worth introducing the pair as compatible only to
mark them incompatible later on in the same series.
Thanks,
TaylorThere was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/21/24 3:33 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:06AM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> As demonstrated in the previous change, the --full-name-hash option of
>> 'git pack-objects' is less effective in a trunctated history. Thus, even
>> when the option is selected via a command-line option or config, disable
>> this option when the '--shallow' option is specified. This will help
>> performance in servers that choose to enable the --full-name-hash option
>> by default for a repository while not regressing their ability to serve
>> shallow clones.
>>
>> This will not present a compatibility issue in the future when the full
>> name hash values are stored in the reachability bitmaps, since shallow
>> clones disable bitmaps.
>>
>> Signed-off-by: Derrick Stolee <stolee@gmail.com>
>> ---
>> builtin/pack-objects.c | 6 ++++++
>> t/perf/p5313-pack-objects.sh | 1 +
>> 2 files changed, 7 insertions(+)
> > I appreciate demonstrating the value of declaring --shallow and
> --full-name-hash incompatible by showing the performance numbers in the
> previous patch.
> > But TBH I think that it would be equally fine or slightly better to say
> up front "when combined with --shallow, this option produces larger
> packs during testing, so the two are incompatible for now". You could
> include some performance numbers there to illustrate that difference in
> the commit log too if you wanted.
> > But I don't think it's worth introducing the pair as compatible only to
> mark them incompatible later on in the same series.
I disagree and here's why: they are not functionally incompatible. This
performance-focused change is worth justifying with performance test data
_and_ isolating from the initial implementation with its own reasoning
for future history spelunkers. Having these warning lines blame to this
patch instead of the initial implementation will make it much easier to
understand the justification of this change.
But maybe this patch can be removed if we use Jonathan's function. I'll
check the performance tests to see if this continues to be justified.
Thanks,
-Stolee
| @@ -816,6 +816,7 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o | |||
| TEST_BUILTINS_OBJS += test-match-trees.o | |||
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Tue, Nov 05, 2024 at 03:05:07AM +0000, Derrick Stolee via GitGitGadget wrote:
> Test this tree
> -----------------------------------------------------------------
> 5314.1: paths at head 4.5K
> 5314.2: number of distinct name-hashes 4.1K
> 5314.3: number of distinct full-name-hashes 4.5K
> 5314.4: maximum multiplicity of name-hashes 13
> 5314.5: maximum multiplicity of fullname-hashes 1
>
> Here, the maximum collision multiplicity is 13, but around 10% of paths
> have a collision with another path.
Neat.
> diff --git a/t/helper/test-name-hash.c b/t/helper/test-name-hash.c
> new file mode 100644
> index 00000000000..e4ecd159b76
> --- /dev/null
> +++ b/t/helper/test-name-hash.c
> @@ -0,0 +1,24 @@
> +/*
> + * test-name-hash.c: Read a list of paths over stdin and report on their
> + * name-hash and full name-hash.
> + */
> +
> +#include "test-tool.h"
> +#include "git-compat-util.h"
> +#include "pack-objects.h"
> +#include "strbuf.h"
> +
> +int cmd__name_hash(int argc UNUSED, const char **argv UNUSED)
> +{
> + struct strbuf line = STRBUF_INIT;
> +
> + while (!strbuf_getline(&line, stdin)) {
> + uint32_t name_hash = pack_name_hash(line.buf);
> + uint32_t full_hash = pack_full_name_hash(line.buf);
> +
> + printf("%10"PRIu32"\t%10"PRIu32"\t%s\n", name_hash, full_hash, line.buf);
I'm definitely nitpicking, but having a tab to separate these two 32-bit
values feels odd when we know already that they will be at most
10-characters wide.
I probably would have written:
printf("%10"PRIu32" %10"PRIu32"\t%s\n", name_hash, full_hash, line.buf);
instead, but this is obviously not a big deal either way ;-).
> diff --git a/t/perf/p5314-name-hash.sh b/t/perf/p5314-name-hash.sh
> new file mode 100755
> index 00000000000..9fe26612fac
> --- /dev/null
> +++ b/t/perf/p5314-name-hash.sh
> @@ -0,0 +1,41 @@
> +#!/bin/sh
> +
> +test_description='Tests pack performance using bitmaps'
> +. ./perf-lib.sh
> +
> +GIT_TEST_PASSING_SANITIZE_LEAK=0
> +export GIT_TEST_PASSING_SANITIZE_LEAK
Does this conflict with Patrick's series to remove these leak checking
annotations? I think it might, which is not unexpected given this series
was written before that one (and it's my fault for not reviewing it
earlier).
> +test_perf_large_repo
> +
> +test_size 'paths at head' '
> + git ls-tree -r --name-only HEAD >path-list &&
> + wc -l <path-list
> +'
> +
> +test_size 'number of distinct name-hashes' '
> + cat path-list | test-tool name-hash >name-hashes &&
> + cat name-hashes | awk "{ print \$1; }" | sort -n | uniq -c >name-hash-count &&
In these two (and a handful of others lower down in this same script)
the "cat ... |" is unnecessary. I think this one should be written as:
test-tool name-hash <path-list >name-hashes &&
awk "{ print \$1; }" <name-hashes | sort | uniq -c >name-hash-count &&
(sort -n is unnecessary, since we just care about getting the list in
sorted order so that "uniq -c" can count the number of unique values).
> + wc -l <name-hash-count
> +'
> +
> +test_size 'number of distinct full-name-hashes' '
> + cat name-hashes | awk "{ print \$2; }" | sort -n | uniq -c >full-name-hash-count &&
> + wc -l <full-name-hash-count
> +'
> +
> +test_size 'maximum multiplicity of name-hashes' '
> + cat name-hash-count | \
> + sort -nr | \
> + head -n 1 | \
> + awk "{ print \$1; }"
> +'
> +
> +test_size 'maximum multiplicity of fullname-hashes' '
> + cat full-name-hash-count | \
> + sort -nr | \
> + head -n 1 | \
> + awk "{ print \$1; }"
Nitpicking again, but you could extract the "sort | head | awk" pipeline
into a function.
Thanks,
Taylor
|
On the Git mailing list, Jonathan Tan wrote (reply to this): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This series introduces a new name-hash algorithm, but does not replace the
> existing one. There are cases, such as packing a single snapshot of a
> repository, where the existing algorithm outperforms the new one.
I came up with a hash function that both uses information from a lot
more of the path (not the full name, though) and preserves the sortable
property (diff at the end of this email). It also contains fixes to the
existing algorithm: not wasting the most significant bits of the hash
if files in the repo mostly end in a lowercase alphabetic character, and
the cast from a possibly-signed-possibly-unsigned char to a uint32_t.
The results look quite good. In summary, the pack sizes are comparable
to Stolee's results in the case of fluentui, and better than Stolee's
results in the case of git.
Here's one run on the fluentui repo (git clone https://
github.com/microsoft/fluentui; cd fluentui; git checkout
a637a06df05360ce5ff21420803f64608226a875^ following the instructions
in [1]:
(before my change)
Test this tree
---------------------------------------------------------------------
5313.2: thin pack 0.03(0.01+0.01)
5313.3: thin pack size 1.1K
5313.4: thin pack with --full-name-hash 0.03(0.00+0.02)
5313.5: thin pack size with --full-name-hash 3.0K
5313.6: big pack 1.60(2.87+0.32)
5313.7: big pack size 57.9M
5313.8: big pack with --full-name-hash 1.41(1.94+0.37)
5313.9: big pack size with --full-name-hash 57.8M
5313.10: shallow fetch pack 1.69(2.70+0.22)
5313.11: shallow pack size 33.0M
5313.12: shallow pack with --full-name-hash 1.49(1.84+0.34)
5313.13: shallow pack size with --full-name-hash 33.6M
5313.14: repack 75.10(537.66+5.47)
5313.15: repack size 454.2M
5313.16: repack with --full-name-hash 18.10(92.50+5.14)
5313.17: repack size with --full-name-hash 174.8M
(after my change)
Test this tree
---------------------------------------------------------------------
5313.2: thin pack 0.03(0.01+0.02)
5313.3: thin pack size 1.1K
5313.4: thin pack with --full-name-hash 0.03(0.01+0.02)
5313.5: thin pack size with --full-name-hash 1.1K
5313.6: big pack 1.62(2.94+0.28)
5313.7: big pack size 57.9M
5313.8: big pack with --full-name-hash 1.35(2.07+0.37)
5313.9: big pack size with --full-name-hash 57.6M
5313.10: shallow fetch pack 1.63(2.52+0.29)
5313.11: shallow pack size 33.0M
5313.12: shallow pack with --full-name-hash 1.50(2.10+0.23)
5313.13: shallow pack size with --full-name-hash 33.1M
5313.14: repack 74.86(531.39+5.49)
5313.15: repack size 454.7M
5313.16: repack with --full-name-hash 19.71(111.39+5.12)
5313.17: repack size with --full-name-hash 165.6M
The tests were run by:
GENERATE_COMPILATION_DATABASE=yes make CC=clang && (cd t/perf && env GIT_PERF_LARGE_REPO=~/tmp/fluentui ./run -- p5313*.sh)
The similarity in sizes looked suspicious, so I replaced the contents
of the hash function with "return 0;" and indeed the sizes significantly
increased, so hopefully there is nothing wrong with my setup.
The git repo was called out in [1] as demonstrating "some of the issues
with this approach", but here are the results, run by:
GENERATE_COMPILATION_DATABASE=yes make CC=clang && (cd t/perf && ./run -- p5313*.sh)
Test this tree
--------------------------------------------------------------------
5313.2: thin pack 0.03(0.00+0.02)
5313.3: thin pack size 2.9K
5313.4: thin pack with --full-name-hash 0.03(0.00+0.02)
5313.5: thin pack size with --full-name-hash 2.9K
5313.6: big pack 1.69(2.80+0.28)
5313.7: big pack size 18.7M
5313.8: big pack with --full-name-hash 1.68(2.82+0.31)
5313.9: big pack size with --full-name-hash 18.8M
5313.10: shallow fetch pack 0.96(1.47+0.16)
5313.11: shallow pack size 12.1M
5313.12: shallow pack with --full-name-hash 1.01(1.51+0.14)
5313.13: shallow pack size with --full-name-hash 12.1M
5313.14: repack 17.05(69.99+4.33)
5313.15: repack size 116.5M
5313.16: repack with --full-name-hash 15.74(67.03+4.18)
5313.17: repack size with --full-name-hash 116.1M
[1] https://lore.kernel.org/git/c14ef6879e451401381ebbdb8f30d33c8f56c25b.1730775908.git.gitgitgadget@gmail.com/
> | Repo | Standard Repack | With --full-name-hash |
> |----------|-----------------|-----------------------|
> | fluentui | 438 MB | 168 MB |
> | Repo B | 6,255 MB | 829 MB |
> | Repo C | 37,737 MB | 7,125 MB |
> | Repo D | 130,049 MB | 6,190 MB |
> | Repo E | 100,957 MB | 22,979 MB |
> | Repo F | 8,308 MB | 746 MB |
> | Repo G | 4,329 MB | 3,643 MB |
If the results are similar for some of the above repos (I do not have
access to them), maybe it's worth considering using my hash function (or
a variation of it).
I'll also take a look at the rest of the patch set.
---
diff --git a/pack-objects.h b/pack-objects.h
index 88360aa3e8..c4f35eafa0 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -209,23 +209,24 @@ static inline uint32_t pack_name_hash(const char *name)
static inline uint32_t pack_full_name_hash(const char *name)
{
- const uint32_t bigp = 1234572167U;
- uint32_t c, hash = bigp;
+ uint32_t hash = 0, base = 0;
+ uint8_t c;
if (!name)
return 0;
- /*
- * Do the simplest thing that will resemble pseudo-randomness: add
- * random multiples of a large prime number with a binary shift.
- * The goal is not to be cryptographic, but to be generally
- * uniformly distributed.
- */
- while ((c = *name++) != 0) {
- hash += c * bigp;
- hash = (hash >> 5) | (hash << 27);
+ while ((c = (uint8_t) *name++) != 0) {
+ if (isspace(c))
+ continue;
+ if (c == '/') {
+ base = (base >> 6) ^ hash;
+ hash = 0;
+ } else {
+ uint8_t nybble_swapped = (c >> 4) + ((c & 15) << 4);
+ hash = (hash >> 2) + (nybble_swapped << 24);
+ }
}
- return hash;
+ return (base >> 6) ^ hash;
} |
|
User |
|
This patch series was integrated into seen via git@fd23820. |
|
This patch series was integrated into seen via git@e5f4def. |
|
This patch series was integrated into seen via git@a691732. |
|
This patch series was integrated into seen via git@8b7facf. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@11dc68f. |
|
This patch series was integrated into seen via git@e53a59b. |
|
This patch series was integrated into seen via git@065a629. |
|
This patch series was integrated into seen via git@15afe74. |
|
There was a status update in the "Cooking" section about the branch "git pack-objects" and its wrapper "git repack" learned an option to use an alternative path-hash function to improve delta-base selection to produce a packfile with deeper history than window size. source: <pull.1823.v2.git.1733181682.gitgitgadget@gmail.com> |
As we will explore in later changes, the default name-hash function used in 'git pack-objects' has a tendency to cause collisions and cause poor delta selection. This change creates an alternative that avoids some collisions while preserving some amount of hash locality. The pack_name_hash() method has not been materially changed since it was introduced in ce0bd64 (pack-objects: improve path grouping heuristics., 2006-06-05). The intention here is to group objects by path name, but also attempt to group similar file types together by making the most-significant digits of the hash be focused on the final characters. Here's the crux of the implementation: /* * This effectively just creates a sortable number from the * last sixteen non-whitespace characters. Last characters * count "most", so things that end in ".c" sort together. */ while ((c = *name++) != 0) { if (isspace(c)) continue; hash = (hash >> 2) + (c << 24); } As the comment mentions, this only cares about the last sixteen non-whitespace characters. This cause some filenames to collide more than others. This collision is somewhat by design in order to promote hash locality for files that have similar types (.c, .h, .json) or could be the same file across a directory rename (a/foo.txt to b/foo.txt). This leads to decent cross-path deltas in cases like shallow clones or packing a repository with very few historical versions of files that share common data with other similarly-named files. However, when the name-hash instead leads to a large number of name-hash collisions for otherwise unrelated files, this can lead to confusing the delta calculation to prefer cross-path deltas over previous versions of the same file. The new pack_name_hash_v2() function attempts to fix this issue by taking more of the directory path into account through its hash function. Its naming implies that we will later wire up details for choosing a name-hash function by version. The first change is to be more careful about paths using non-ASCII characters. With these characters in mind, reverse the bits in the byte as the least-significant bits have the highest entropy and we want to maximize their influence. This is done with some bit manipulation that swaps the two halves, then the quarters within those halves, and then the bits within those quarters. The second change is to perform hash composition operations at every level of the path. This is done by storing a 'base' hash value that contains the hash of the parent directory. When reaching a directory boundary, we XOR the current level's name-hash value with a downshift of the previous level's hash. This perturbation intends to create low-bit distinctions for paths with the same final 16 bytes but distinct parent directory structures. The collision rate and effectiveness of this hash function will be explored in later changes as the function is integrated with 'git pack-objects' and 'git repack'. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Derrick Stolee <stolee@gmail.com>
The previous change introduced a new pack_name_hash_v2() function that intends to satisfy much of the hash locality features of the existing pack_name_hash() function while also distinguishing paths with similar final components of their paths. This change adds a new --name-hash-version option for 'git pack-objects' to allow users to select their preferred function version. This use of an integer version allows for future expansion and a direct way to later store a name hash version in the .bitmap format. For now, let's consider how effective this mechanism is when repacking a repository with different name hash versions. Specifically, we will execute 'git pack-objects' the same way a 'git repack -adf' process would, except we include --name-hash-version=<n> for testing. On the Git repository, we do not expect much difference. All path names are short. This is backed by our results: | Stage | Pack Size | Repack Time | |-----------------------|-----------|-------------| | After clone | 260 MB | N/A | | --name-hash-version=1 | 127 MB | 129s | | --name-hash-version=2 | 127 MB | 112s | This example demonstrates how there is some natural overhead coming from the cloned copy because the server is hosting many forks and has not optimized for exactly this set of reachable objects. But the full repack has similar characteristics for both versions. Let's consider some repositories that are hitting too many collisions with version 1. First, let's explore the kinds of paths that are commonly causing these collisions: * "/CHANGELOG.json" is 15 characters, and is created by the beachball [1] tool. Only the final character of the parent directory can differentiate different versions of this file, but also only the two most-significant digits. If that character is a letter, then this is always a collision. Similar issues occur with the similar "/CHANGELOG.md" path, though there is more opportunity for differences In the parent directory. * Localization files frequently have common filenames but differentiates via parent directories. In C#, the name "/strings.resx.lcl" is used for these localization files and they will all collide in name-hash. [1] https://github.com/microsoft/beachball I've come across many other examples where some internal tool uses a common name across multiple directories and is causing Git to repack poorly due to name-hash collisions. One open-source example is the fluentui [2] repo, which uses beachball to generate CHANGELOG.json and CHANGELOG.md files, and these files have very poor delta characteristics when comparing against versions across parent directories. | Stage | Pack Size | Repack Time | |-----------------------|-----------|-------------| | After clone | 694 MB | N/A | | --name-hash-version=1 | 438 MB | 728s | | --name-hash-version=2 | 168 MB | 142s | [2] https://github.com/microsoft/fluentui In this example, we see significant gains in the compressed packfile size as well as the time taken to compute the packfile. Using a collection of repositories that use the beachball tool, I was able to make similar comparisions with dramatic results. While the fluentui repo is public, the others are private so cannot be shared for reproduction. The results are so significant that I find it important to share here: | Repo | --name-hash-version=1 | --name-hash-version=2 | |----------|-----------------------|-----------------------| | fluentui | 440 MB | 161 MB | | Repo B | 6,248 MB | 856 MB | | Repo C | 37,278 MB | 6,755 MB | | Repo D | 131,204 MB | 7,463 MB | Future changes could include making --name-hash-version implied by a config value or even implied by default during a full repack. It is important to point out that the name hash value is stored in the .bitmap file format, so we must force --name-hash-version=1 when bitmaps are being read or written. Later, the bitmap format could be updated to be aware of the name hash version so deltas can be quickly computed across the bitmapped/not-bitmapped boundary. Signed-off-by: Derrick Stolee <stolee@gmail.com>
The new '--name-hash-version' option for 'git repack' is a simple pass-through to the underlying 'git pack-objects' subcommand. However, this subcommand may have other options and a temporary filename as part of the subcommand execution that may not be predictable or could change over time. The existing test_subcommand method requires an exact list of arguments for the subcommand. This is too rigid for our needs here, so create a new method, test_subcommand_flex. Use it to check that the --name-hash-version option is passing through. Since we are modifying the 'git repack' command, let's bring its usage in line with the Documentation's synopsis. This removes it from the allow list in t0450 so it will remain in sync in the future. Signed-off-by: Derrick Stolee <stolee@gmail.com>
Add a new environment variable to opt-in to different values of the --name-hash-version=<n> option in 'git pack-objects'. This allows for extra testing of the feature without repeating all of the test scenarios. Unlike many GIT_TEST_* variables, we are choosing to not add this to the linux-TEST-vars CI build as that test run is already overloaded. The behavior exposed by this test variable is of low risk and should be sufficient to allow manual testing when an issue arises. But this option isn't free. There are a few tests that change behavior with the variable enabled. First, there are a few tests that are very sensitive to certain delta bases being picked. These are both involving the generation of thin bundles and then counting their objects via 'git index-pack --fix-thin' which pulls the delta base into the new packfile. For these tests, disable the option as a decent long-term option. Second, there are some tests that compare the exact output of a 'git pack-objects' process when using bitmaps. The warning that ignores the --name-hash-version=2 and forces version 1 causes these tests to fail. Disable the environment variable to get around this issue. Signed-off-by: Derrick Stolee <stolee@gmail.com>
As custom options are added to 'git pack-objects' and 'git repack' to adjust how compression is done, use this new performance test script to demonstrate their effectiveness in performance and size. The recently-added --name-hash-version option allows for testing different name hash functions. Version 2 intends to preserve some of the locality of version 1 while more often breaking collisions due to long filenames. Distinguishing objects by more of the path is critical when there are many name hash collisions and several versions of the same path in the full history, giving a significant boost to the full repack case. The locality of the hash function is critical to compressing something like a shallow clone or a thin pack representing a push of a single commit. This can be seen by running pt5313 on the open source fluentui repository [1]. Most commits will have this kind of output for the thin and big pack cases, though certain commits (such as [2]) will have problematic thin pack size for other reasons. [1] https://github.com/microsoft/fluentui [2] a637a06df05360ce5ff21420803f64608226a875 Checked out at the parent of [2], I see the following statistics: Test HEAD --------------------------------------------------------------- 5313.2: thin pack with version 1 0.37(0.44+0.02) 5313.3: thin pack size with version 1 1.2M 5313.4: big pack with version 1 2.04(7.77+0.23) 5313.5: big pack size with version 1 20.4M 5313.6: shallow fetch pack with version 1 1.41(2.94+0.11) 5313.7: shallow pack size with version 1 34.4M 5313.8: repack with version 1 95.70(676.41+2.87) 5313.9: repack size with version 1 439.3M 5313.10: thin pack with version 2 0.12(0.12+0.06) 5313.11: thin pack size with version 2 22.0K 5313.12: big pack with version 2 2.80(5.43+0.34) 5313.13: big pack size with version 2 25.9M 5313.14: shallow fetch pack with version 2 1.77(2.80+0.19) 5313.15: shallow pack size with version 2 33.7M 5313.16: repack with version 2 33.68(139.52+2.58) 5313.17: repack size with version 2 160.5M To make comparisons easier, I will reformat this output into a different table style: | Test | V1 Time | V2 Time | V1 Size | V2 Size | |--------------|---------|---------|---------|---------| | Thin Pack | 0.37 s | 0.12 s | 1.2 M | 22.0 K | | Big Pack | 2.04 s | 2.80 s | 20.4 M | 25.9 M | | Shallow Pack | 1.41 s | 1.77 s | 34.4 M | 33.7 M | | Repack | 95.70 s | 33.68 s | 439.3 M | 160.5 M | The v2 hash function successfully differentiates the CHANGELOG.md files from each other, which leads to significant improvements in the thin pack (simulating a push of this commit) and the full repack. There is some bloat in the "big pack" scenario and essentially the same results for the shallow pack. In the case of the Git repository, these numbers show some of the issues with this approach: | Test | V1 Time | V2 Time | V1 Size | V2 Size | |--------------|---------|---------|---------|---------| | Thin Pack | 0.02 s | 0.02 s | 1.1 K | 1.1 K | | Big Pack | 1.69 s | 1.95 s | 13.5 M | 14.5 M | | Shallow Pack | 1.26 s | 1.29 s | 12.0 M | 12.2 M | | Repack | 29.51 s | 29.01 s | 237.7 M | 238.2 M | Here, the attempts to remove conflicts in the v2 function seem to cause slight bloat to these sizes. This shows that the Git repository benefits a lot from cross-path delta pairs. The results are similar with the nodejs/node repo: | Test | V1 Time | V2 Time | V1 Size | V2 Size | |--------------|---------|---------|---------|---------| | Thin Pack | 0.02 s | 0.02 s | 1.6 K | 1.6 K | | Big Pack | 4.61 s | 3.26 s | 56.0 M | 52.8 M | | Shallow Pack | 7.82 s | 7.51 s | 104.6 M | 107.0 M | | Repack | 88.90 s | 73.75 s | 740.1 M | 764.5 M | Here, the v2 name-hash causes some size bloat more often than it reduces the size, but it also universally improves performance time, which is an interesting reversal. This must mean that it is helping to short-circuit some delta computations even if it is not finding the most efficient ones. The performance improvement cannot be explained only due to the I/O cost of writing the resulting packfile. The Linux kernel repository was the initial target of the default name hash value, and its naming conventions are practically build to take the most advantage of the default name hash values: | Test | V1 Time | V2 Time | V1 Size | V2 Size | |--------------|----------|----------|---------|---------| | Thin Pack | 0.17 s | 0.07 s | 4.6 K | 4.6 K | | Big Pack | 17.88 s | 12.35 s | 201.1 M | 159.1 M | | Shallow Pack | 11.05 s | 22.94 s | 269.2 M | 273.8 M | | Repack | 727.39 s | 566.95 s | 2.5 G | 2.5 G | Here, the thin and big packs gain some performance boosts in time, with a modest gain in the size of the big pack. The shallow pack, however, is more expensive to compute, likely because similarly-named files across different directories are farther apart in the name hash ordering in v2. The repack also gains benefits in computation time but no meaningful change to the full size. Finally, an internal Javascript repo of moderate size shows significant gains when repacking with --name-hash-version=2 due to it having many name hash collisions. However, it's worth noting that only the full repack case has significant differences from the v1 name hash: | Test | V1 Time | V2 Time | V1 Size | V2 Size | |-----------|-----------|----------|---------|---------| | Thin Pack | 8.28 s | 7.28 s | 16.8 K | 16.8 K | | Big Pack | 12.81 s | 11.66 s | 29.1 M | 29.1 M | | Shallow | 4.86 s | 4.06 s | 42.5 M | 44.1 M | | Repack | 3126.50 s | 496.33 s | 6.2 G | 855.6 M | Signed-off-by: Derrick Stolee <stolee@gmail.com>
Add a new test-tool helper, name-hash, to output the value of the name-hash algorithms for the input list of strings, one per line. Since the name-hash values can be stored in the .bitmap files, it is important that these hash functions do not change across Git versions. Add a simple test to t5310-pack-bitmaps.sh to provide some testing of the current values. Due to how these functions are implemented, it would be difficult to change them without disturbing these values. The paths used for this test are carefully selected to demonstrate some of the behavior differences of the two current name hash versions, including which conditions will cause them to collide. Create a performance test that uses test_size to demonstrate how collisions occur for these hash algorithms. This test helps inform someone as to the behavior of the name-hash algorithms for their repo based on the paths at HEAD. My copy of the Git repository shows modest statistics around the collisions of the default name-hash algorithm: Test this tree -------------------------------------------------- 5314.1: paths at head 4.5K 5314.2: distinct hash value: v1 4.1K 5314.3: maximum multiplicity: v1 13 5314.4: distinct hash value: v2 4.2K 5314.5: maximum multiplicity: v2 9 Here, the maximum collision multiplicity is 13, but around 10% of paths have a collision with another path. In a more interesting example, the microsoft/fluentui [1] repo had these statistics at time of committing: Test this tree -------------------------------------------------- 5314.1: paths at head 19.5K 5314.2: distinct hash value: v1 8.2K 5314.3: maximum multiplicity: v1 279 5314.4: distinct hash value: v2 17.8K 5314.5: maximum multiplicity: v2 44 [1] https://github.com/microsoft/fluentui That demonstrates that of the nearly twenty thousand path names, they are assigned around eight thousand distinct values. 279 paths are assigned to a single value, leading the packing algorithm to sort objects from those paths together, by size. With the v2 name hash function, the maximum multiplicity lowers to 44, leaving some room for further improvement. In a more extreme example, an internal monorepo had a much worse collision rate: Test this tree -------------------------------------------------- 5314.1: paths at head 227.3K 5314.2: distinct hash value: v1 72.3K 5314.3: maximum multiplicity: v1 14.4K 5314.4: distinct hash value: v2 166.5K 5314.5: maximum multiplicity: v2 138 Here, we can see that the v2 name hash function provides somem improvements, but there are still a number of collisions that could lead to repacking problems at this scale. Signed-off-by: Derrick Stolee <stolee@gmail.com>
When the --name-hash-version option is used in 'git pack-objects', it can change from the initial assignment to when it is used based on interactions with other arguments. Specifically, when writing or reading bitmaps, we must force version 1 for now. This could change in the future when the bitmap format can store a name hash version value, indicating which was used during the writing of the packfile. Protect the 'git pack-objects' process from getting confused by failing with a BUG() statement if the value of the name hash version changes between calls to pack_name_hash_fn(). Signed-off-by: Derrick Stolee <stolee@gmail.com>
The '--name-hash-version=<n>' option in 'git pack-objects' was introduced to allow for specifying an alternative name hash function when organizing objects for delta compression. The pack_name_hash_v2() function was designed to break some collisions while also preserving some amount of locality for cross-path deltas. However, in some repositories, that effort to preserve locality results in enough collisions that it causes issues with full repacks. Create a third name hash function and extend the '--name-hash-version' option in 'git pack-objects' and 'git repack' to understand it. This hash version abandons all efforts for locality and focuses on creating a somewhat uniformly-distributed hash function to minimize collisions. We can observe the effect of this collision avoidance in a large internal monorepo that suffered from collisions in the previous versions. The updates to p5314-name-hash.sh show these results: Test this tree -------------------------------------------------- 5314.1: paths at head 227.3K 5314.2: distinct hash value: v1 72.3K 5314.3: maximum multiplicity: v1 14.4K 5314.4: distinct hash value: v2 166.5K 5314.5: maximum multiplicity: v2 138 5314.6: distinct hash value: v3 227.3K 5314.7: maximum multiplicity: v3 2 These results demonstrate that of the 227,000+ paths, nearly all of them find distinct hash values. The maximum multiplicity is 2, improved from 138 in the v2 hash function. The v2 hash function also had only 166K distinct values, so it had a wide spread of collisions. A more modest improvement is available in the open source fluentui repo [1] with these results: Test this tree -------------------------------------------------- 5314.1: paths at head 19.5K 5314.2: distinct hash value: v1 8.2K 5314.3: maximum multiplicity: v1 279 5314.4: distinct hash value: v2 17.8K 5314.5: maximum multiplicity: v2 44 5314.6: distinct hash value: v3 19.5K 5314.7: maximum multiplicity: v3 1 [1] https://github.com/microsoft/fluentui However, it is important to demonstrate the effectiveness of this function in the context of compressing a repository. We can use p5313-pack-objects.sh to measure these changes. I will use a simplified table summarizing the output of that performance test. | Test | V1 Time | V2 Time | V3 Time | V1 Size | V2 Size | V3 Size | |-----------|---------|---------|---------|---------|---------|---------| | Thin Pack | 0.37 s | 0.12 s | 0.07 s | 1.2 M | 22.0 K | 20.4 K | | Big Pack | 2.04 s | 2.80 s | 1.40 s | 20.4 M | 25.9 M | 19.2 M | | Shallow | 1.41 s | 1.77 s | 1.27 s | 34.4 M | 33.7 M | 34.8 M | | Repack | 95.70 s | 33.68 s | 20.88 s | 439.3 M | 160.5 M | 169.1 M | Here, there are some performance improvements on a time basis, and the thin and big packs are somewhat smaller in v3. The shallow and repacked packs are somewhat bigger, though, compared to v2. Two repositories that have very few collisions in the v1 name hash are the Git and Linux repositories. Here are their stats for p5313: Git: | Test | V1 Time | V2 Time | V3 Time | V1 Size | V2 Size | V3 Size | |-----------|---------|---------|---------|---------|---------|---------| | Thin Pack | 0.02 s | 0.02 s | 0.02 s | 1.1 K | 1.1 K | 15.3 K | | Big Pack | 1.69 s | 1.95 s | 1.67 s | 13.5 M | 14.5 M | 14.9 M | | Shallow | 1.26 s | 1.29 s | 1.16 s | 12.0 M | 12.2 M | 12.5 M | | Repack | 29.51 s | 29.01 s | 29.08 s | 237.7 M | 238.2 M | 237.7 M | Linux: | Test | V1 Time | V2 Time | V3 Time | V1 Size | V2 Size | V3 Size | |-----------|----------|----------|----------|---------|---------|---------| | Thin Pack | 0.17 s | 0.07 s | 0.07 s | 4.6 K | 4.6 K | 6.8 K | | Big Pack | 17.88 s | 12.35 s | 12.14 s | 201.1 M | 149.1 M | 160.4 M | | Shallow | 11.05 s | 22.94 s | 22.16 s | 269.2 M | 273.8 M | 271.8 M | | Repack | 727.39 s | 566.95 s | 539.33 s | 2.5 G | 2.5 G | 2.6 G | These repositories make good use of the cross-path deltas that come about from the v1 name hash function, so they already had mixed results with the v2 function. The v3 function is generally worse for these repositories. An internal Javascript-based repository with name hash collisions similar to the fluentui repo has these results: | Test | V1 Time | V2 Time | V3 Time | V1 Size | V2 Size | V3 Size | |-----------|-----------|----------|----------|---------|---------|---------| | Thin Pack | 8.28 s | 7.28 s | 0.04 s | 16.8 K | 16.8 K | 3.2 K | | Big Pack | 12.81 s | 11.66 s | 2.52 s | 29.1 M | 29.1 M | 30.6 M | | Shallow | 4.86 s | 4.06 s | 3.77 s | 42.5 M | 44.1 M | 45.7 M | | Repack | 3126.50 s | 496.33 s | 306.86 s | 6.2 G | 855.6 M | 838.2 M | This repository is also listed as "Repo B" in the repacking size table below, along with other Javascript repos that have many name hash collisions with the v1 name hash: | Repo | V1 Size | V2 Size | V3 Size | |----------|-----------|---------|---------| | fluentui | 440 M | 161 M | 170 M | | Repo B | 6,248 M | 856 M | 840 M | | Repo C | 37,278 M | 6,921 M | 6,755 M | | Repo D | 131,204 M | 7,463 M | 7,124 M | While the fluentui repo had an increase in size using the v3 name hash, the others had modest improvements over the v2 name hash. But those modest improvements are dwarfed by the difference from v1 to v2, so it is unlikely that the regression seen in the other scenarios (packfiles that are not from full repacks) will be worth using v3 over v2. That is, unless there are enough collisions even with v2 that the full repack scenario has larger improvements than these. When using GIT_TEST_NAME_HASH_VERSION=3, there are some necessary changes to t5616-partial-clone.sh since the server now picks different delta bases that the client does not have (and does not then fetch dynamically). These changes are a minimal patch and the functionality should be fixed in other changes. Signed-off-by: Derrick Stolee <stolee@gmail.com>
derrickstolee
force-pushed
the
full-name
branch
from
64fd7b3 to
3d63954
Compare
|
/submit |
|
Submitted as pull.1823.v3.git.1734715194.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@827fd92. |
|
This patch series was integrated into seen via git@d200f8e. |
|
This patch series was integrated into seen via git@fb15845. |
|
This patch series was integrated into seen via git@3fa1f6c. |
|
This patch series was integrated into seen via git@0cfdd20. |
|
There was a status update in the "Cooking" section about the branch "git pack-objects" and its wrapper "git repack" learned an option to use an alternative path-hash function to improve delta-base selection to produce a packfile with deeper history than window size. Comments? source: <pull.1823.v3.git.1734715194.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@3fcf565. |
|
There was a status update in the "Cooking" section about the branch "git pack-objects" and its wrapper "git repack" learned an option to use an alternative path-hash function to improve delta-base selection to produce a packfile with deeper history than window size. Comments? source: <pull.1823.v3.git.1734715194.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@9cc6d34. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a recreation of the topic in [1] that was closed. (I force-pushed my branch and GitHub won't let me reopen the PR for GitGitGadget to create this as v3.)
[1] https://lore.kernel.org/git/pull.1785.v2.git.1726692381.gitgitgadget@gmail.com/
I've been focused recently on understanding and mitigating the growth of a few internal repositories. Some of these are growing much larger than expected for the number of contributors, and there are multiple aspects to why this growth is so large.
This is part of the RFC I submitted [2] involving the path-walk API, though this doesn't use the path-walk API directly. In full repack cases, it seems that the --full-name-hash option gets nearly as good compression as the --path-walk option introduced in that series. I continue to work on that feature as well, so we can review it after this series is complete.
[2] https://lore.kernel.org/git/pull.1786.git.1725935335.gitgitgadget@gmail.com/
The main issue plaguing these repositories is that deltas are not being computed against objects that appear at the same path. While the size of these files at tip is one aspect of growth that would prevent this issue, the changes to these files are reasonable and should result in good delta compression. However, Git is not discovering the connections across different versions of the same file.
One way to find some improvement in these repositories is to increase the window size, which was an initial indicator that the delta compression could be improved, but was not a clear indicator. After some digging (and prototyping some analysis tools) the main discovery was that the current name-hash algorithm only considers the last 16 characters in the path name and has some naturally-occurring collisions within that scope.
This series creates a mechanism to select alternative name hashes using a new
--name-hash-version=<n>option. The versions are:Version 1 is the default name hash that already exists. This option focuses on the final bytes of the path to maximize locality for cross-path deltas.
Version 2 is the new path-component hash function suggested by Jonathan Tan in the previous version (with some modifications). This hash function essentially computes the v1 name hash of each path component and then overlays those hashes with a shift to make the parent directories contribute less to the final hash, but enough to break many collisions that exist in v1.
Version 3 is the hash function that I submitted under the
--full-name-hashfeature in the previous versions. This uses a pseudorandom hash procedure to minimize collisions but at the expense of losing on locality. This version is implemented in the final patch of the series mostly for comparison purposes, as it is unlikely to be selected as a valuable hash function over v2. The final patch could be omitted from the merged version.See the patches themselves for detailed results in the p5313-pack-objects.sh performance test and the p5314-name-hash.sh test that demonstrates how many collisions occur with each hash function.
In general, the v2 name hash function gets very close to the compression results of v3 in the full repack case, even in the repositories that feature many name hash collisions. These benefits come as well without downsides to other kinds of packfiles, including small pushed packs, larger incremental fetch packs, and shallow clones.
I should point out that there is still a significant jump in compression effectiveness between these name hash version options and the
--path-walkfeature I suggested in my RFC [2] and has review underway in [3] (along with changes togit pack-objectsandgit repackin [4]).[3] https://lore.kernel.org/git/pull.1818.v2.git.1731181272.gitgitgadget@gmail.com/
[4] #1819
To compare these options in a set of Javascript repositories that have different levels of name hash collisions, see the following table that lists the size of the packfile after
git repack -adf [--name-hash-version=<n>|--path-walk]:As we can see, v2 nearly reaches the effectiveness of v3 (and outperforms it once!) but there is still a significant change between the
--name-hash-versionfeature and the--path-walkfeature.The main reason we are considering this
--name-hash-versionfeature is that it has the least amount of stretch required in order for it to be integrated with reachability bitmaps, required for server environments. In fact, the change in this version to use a numerical version makes it more obvious how to connect the version number to a value in the.bitmapfile format. Tests are added to guarantee that the hash functions preserve their behavior over time, since data files depend on that.Thanks, -Stolee
UPDATES SINCE V1
BIG CHANGE:
--full-name-hashis replaced with--name-hash-version=<n>.--name-hash-version=2uses Jonathan Tan's hash function (with some adjustments). See the first patch for this implementation, credited to him.--name-hash-version=3uses the hash function I wrote for the previous version's--full-name-hash. This is left as the final patch so it could be easily removed from the series if not considered worth having since it has some pain points that are resolved from v2 without significant issues to overall repo size.Commit messaes are updated with these changes, as well as a better attempt to indicate the benefit of cross-path delta pairs, such as renames or similar content based on file extension.
Performance numbers are regenerated for the same set of repositories. Size data is somewhat nondeterministic due to concurrent threads competing over delta computations.
The
--name-hash-versionoption is not added togit repackuntil its own patch.The patch that updates
git repack's synopsis match its docs is squashed into the patch that adds the option togit repack.Documentation is expanded for
git pack-objectsand reused forgit repack.GIT_TEST_FULL_NAME_HASHis nowGIT_TEST_NAME_HASH_VERSIONwith similar caveats required for tests. It is removed from the linux-TEST-vars CI job.The performance test p5313-pack-objects.sh is now organized via a loop over the different versions. This separates the scenarios, which makes things harder to compare directly, but makes it trivial to add new versions.
The patch that disabled
--full-name-hashwhen performing a shallow clone is no longer present, as it is not necessary when using--name-hash-version=2. Perhaps it would be valuable for repo using v3, if that is kept in the series.We force name hash version 1 when writing or reading bitmaps.
A small patch is added to cause a BUG() failure if the name hash version global changes between calls to pack_name_hash_fn(). This is solely defensive programming.
Several typos, style issues, or suggested comments are resolved.
UPDATES SINCE v2
For extra safety, the new name-hash algorithm uses unsigned characters.
A stray 'full_name_hash' variable is removed.
Commit messages are improved.
'git repack' documentation now points to 'git pack-objects' docs for the
--name-hash-version option.
The changes to t5616 are delayed until the introduction of version 3,
since the v2 name hash does not demonstrate the behavior.
cc: gitster@pobox.com
cc: johannes.schindelin@gmx.de
cc: peff@peff.net
cc: ps@pks.im
cc: me@ttaylorr.com
cc: johncai86@gmail.com
cc: newren@gmail.com
cc: jonathantanmy@google.com
cc: karthik nayak karthik.188@gmail.com