pack-objects: create new name-hash algorithm

[derrickstolee]

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 [1] [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.

[1] https://github.com/gitgitgadget/git/pull/1786

[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 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.

However, my findings show that when a repository has many versions of files at the same path (and especially when there are many name-hash collisions) then there are significant gains to be made using the new algorithm.

(This table is updated in v2 with even more private examples that were found while communicating findings internally.)

| 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  |

I include Repo G here as an example where the improvement is less drastic, since this repo does not demonstrate a very high rate of name-hash collisions; the collisions that exist seem to be in paths that are not changed very often. Thus, the standard name-hash algorithm is nearly as effective in these full repacks.

The main change in this series is in patch 1, which adds the algorithm and the option to 'git pack-objects' and 'git repack'. The remaining patches are focused on creating more evidence around the value of the new name-hash algorithm and its effects on the packfiles created with it.

I will also try to make clear that I've been focused on client-side performance and size concerns. Based on discussions in v1, it appears that the following is true:

• This feature is completely orthogonal to delta islands.
• Changing the name-hash function can lead to compatibility issues with .bitmap files, as they store a name-hash value. Without augmenting the data structure to indicate which name-hash value was used at write time, the full-name-hash values should not be stored in the .bitmap files or used when reading .bitmap files and other objects. Thus, the full-name-hash is marked as incompatible with bitmaps for now.
• The --path-walk option from the RFC ([1] and [2]) is likely incompatible with the delta-islands feature without significant work, so this suggests that the --full-name-hash is a better long-term solution for servers. This would still require the .bitmap modifications to make it work, but it's a smaller leap to get there.

Thanks, -Stolee

UPDATES IN V2

Thank you for all of the discussion on v1. Here are the things I've learned and how they have changed this patch series:

• The test-tool name-hash helper change collides with the removal of test-tool oid-array, so I have rebased this series onto the latest master branch.
• I learned (thanks, Taylor and Peff) that the .bitmap files store name-hash values. This means that the --full-name-hash option risks having different name-hash functions across bitmap reads and dynamic computations from the object walk. For this reason, the option is now made explicit to not work with bitmap walks. This could be corrected in the future with a modification to the .bitmap data structure to store a "name-hash version" value. This behavior is confirmed with a test.
• A new test explicitly tests that git repack --full-name-hash passes the option to git pack-objects. This uses a new helper method, test_subcommand_flex that is more flexible than the existing test_subcommand.
• To get more testing across different cases, add a new GIT_TEST_FULL_NAME_HASH environment variable. I point out which tests need modification when this is enabled.
• The size-comparison tests were not portable with their use of du. Instead, use wc -c for the single pack-file that remains after a git repack -adf ... command.
• The final patch still updates the test-tool name-hash helper, which was previously only used by a performance test. Now, use that test in a regular test to help guarantee that the functions do not change over time. This is directly related to the fact that these values can be stored in the .bitmap files and we need stable hash functions to keep compatibility with files written by previous versions of Git.

Other things that have happened include investigations into ways to adapt the full-name hash to improve upon the name-hash. I did some experimenting with increasing the size of 'struct object_entry' by using a 64-bit hash value (name-hash, then full-name-hash) for a single-pass compression or two 32-bit hash values for a two-pass compression process. I include my WIP branch at [3] to show what I tried, though the single-pass option did not present any improvements and the two-pass option seems to be broken to the point that the compression is substantially worse. (I'll try to elaborate on this in a reply to this cover letter.)

[3] https://github.com/derrickstolee/git/compare/full-name...derrickstolee:git:full-name-wip

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

[derrickstolee]

/submit

[gitgitgadget[bot]]

Submitted as pull.1785.git.1725890210.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1785/derrickstolee/full-name-v1

To fetch this version to local tag pr-1785/derrickstolee/full-name-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1785/derrickstolee/full-name-v1

[gitgitgadget[bot]]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 9/9/24 9:56 AM, Derrick Stolee via GitGitGadget wrote:

> However, my findings show that when a repository has many versions of files
> at the same path (and especially when there are many name-hash collisions)
> then there are significant gains to be made using the new algorithm.

Of course this table didn't render correctly. Here's a readable version:

| 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  |

Thanks,
-Stolee

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

Yes, as I explained in the other message, this "collision" is an
integral part of the design to allow us gather candidates together
that may yield good deltas among them.  In addition, header files
whose names end with ".h" tend to share a bit comment at the
beginning of them in many projects, and the proximity (not
"collision") of the hash value is used to make them delta candidates
with each other.

I do agree that considering files at the same path from different
(but close-by) revisions as the prime candidates is very important,
but if you spread the "collissions" very thin by using "uniform
distribution", I am afraid that you'd end up discarding anything but
the blobs at the same path, which may go too far.  Having name hash
value that are close by no longer has any meaning in such a system.

I hope you can find a solution that strikes a good balance at the
end of the series (I saw only the first step), but I suspect an easy
way to avoid the downsides you observed is to use both.  Compare
with a handful of blobs taken from nearby commits (the original
object order is roughly in traversal order, and you can take
advantage of that fact) from exactly the same path (using your
"uniform distribution") before comparing with the blobs with close
value (of the current function) like the current implementation
does, may go a long way.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/abd4999ba418667a1c8fc79871aacb171315ba1b.

[gitgitgadget[bot]]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 9/9/24 2:06 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> 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.
> > Yes, as I explained in the other message, this "collision" is an
> integral part of the design to allow us gather candidates together
> that may yield good deltas among them.  In addition, header files
> whose names end with ".h" tend to share a bit comment at the
> beginning of them in many projects, and the proximity (not
> "collision") of the hash value is used to make them delta candidates
> with each other.
> > I do agree that considering files at the same path from different
> (but close-by) revisions as the prime candidates is very important,
> but if you spread the "collissions" very thin by using "uniform
> distribution", I am afraid that you'd end up discarding anything but
> the blobs at the same path, which may go too far.  Having name hash
> value that are close by no longer has any meaning in such a system.

You are right that some "nearby" paths are lost in this change, and
this can be measured by trying to use this option in the pack-objects
process underneath a small 'git push'.

The thing that surprised me is just how effective this is for the
creation of large pack-files that include many versions of most
files. The cross-path deltas have less of an effect here, and the
benefits of avoiding name-hash collisions can be overwhelming in
many cases.

> I hope you can find a solution that strikes a good balance at the
> end of the series (I saw only the first step), but I suspect an easy
> way to avoid the downsides you observed is to use both.  Compare
> with a handful of blobs taken from nearby commits (the original
> object order is roughly in traversal order, and you can take
> advantage of that fact) from exactly the same path (using your
> "uniform distribution") before comparing with the blobs with close
> value (of the current function) like the current implementation
> does, may go a long way.

Funny you should say that, since the RFC I finally submitted [1]
actually does just that. The --path-walk option changes the object
walk to consider batches of objects based on their path, computes
deltas among that batch, and then does the normal name-hash pass
later. This seems to really strike the balance that you are
looking for and solves the issues where small pushes need to stay
small. It also fixes some problematic cases even when pushing a
single commit.

[1] https://lore.kernel.org/git/pull.1786.git.1725935335.gitgitgadget@gmail.com/

However, the --path-walk option requires significant implementation
of a "path walk API" and my RFC doesn't even do threading right.
The --path-walk version (probably) doesn't work with delta islands
or other features the same way as the drop-in change to the name-
hash heuristic can. For that reason, I think there is likely some
long-term value to the --full-name-hash option even though the
--path-walk option will be better in many cases.

Thanks,
-Stolee

[gitgitgadget[bot]]

On the Git mailing list, Taylor Blau wrote (reply to this):

On Mon, Sep 09, 2024 at 10:37:30PM -0400, Derrick Stolee wrote:
> > I do agree that considering files at the same path from different
> > (but close-by) revisions as the prime candidates is very important,
> > but if you spread the "collissions" very thin by using "uniform
> > distribution", I am afraid that you'd end up discarding anything but
> > the blobs at the same path, which may go too far.  Having name hash
> > value that are close by no longer has any meaning in such a system.
>
> You are right that some "nearby" paths are lost in this change, and
> this can be measured by trying to use this option in the pack-objects
> process underneath a small 'git push'.
>
> The thing that surprised me is just how effective this is for the
> creation of large pack-files that include many versions of most
> files. The cross-path deltas have less of an effect here, and the
> benefits of avoiding name-hash collisions can be overwhelming in
> many cases.

I think that Junio's suggestion is pretty interesting (though please
take my comments here with a grain of salt, since I haven't read the
other series yet, and am not sure how much of this is redundant).

Imagine computing both the full and existing name-hash values for each
blob/tree in the pack. Then objects would be sorted in the delta
selection window by similar full-name hash and similar regular name hash
values.

I'm not sure which value you'd actually record in the pack, though.
Ideally there is a hash function which captures some information about
the full path as well as the final path component, so we could use a
single value here, though I suspect the implementation would be more
complicated than what is presented here.

> > I hope you can find a solution that strikes a good balance at the
> > end of the series (I saw only the first step), but I suspect an easy
> > way to avoid the downsides you observed is to use both.  Compare
> > with a handful of blobs taken from nearby commits (the original
> > object order is roughly in traversal order, and you can take
> > advantage of that fact) from exactly the same path (using your
> > "uniform distribution") before comparing with the blobs with close
> > value (of the current function) like the current implementation
> > does, may go a long way.
>
> Funny you should say that, since the RFC I finally submitted [1]
> actually does just that. The --path-walk option changes the object
> walk to consider batches of objects based on their path, computes
> deltas among that batch, and then does the normal name-hash pass
> later. This seems to really strike the balance that you are
> looking for and solves the issues where small pushes need to stay
> small. It also fixes some problematic cases even when pushing a
> single commit.

Interesting.

> [1] https://lore.kernel.org/git/pull.1786.git.1725935335.gitgitgadget@gmail.com/

> However, the --path-walk option requires significant implementation
> of a "path walk API" and my RFC doesn't even do threading right.
> The --path-walk version (probably) doesn't work with delta islands
> or other features the same way as the drop-in change to the name-
> hash heuristic can. For that reason, I think there is likely some
> long-term value to the --full-name-hash option even though the
> --path-walk option will be better in many cases.

I suspect that this is going to be a significant sticking point. Not
supporting multi-threading is work-able for GitHub (since we set
pack.threads=1 today), but lacking support for delta-islands makes this
a non-starter to run at GitHub.

Do you imagine that the --path-walk option could be made to work with
delta islands? I'm not all that worried about who does that work, but
more interested at this point in whether or not it's even possible to
implement.

Thanks,
Taylor

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes:

> The thing that surprised me is just how effective this is for the
> creation of large pack-files that include many versions of most
> files. The cross-path deltas have less of an effect here, and the
> benefits of avoiding name-hash collisions can be overwhelming in
> many cases.

Yes, "make sure we notice a file F moving from directory A to B" is
inherently optimized for short span of history, i.e. a smallish push
rather than a whole history clone, where the definition of
"smallish" is that even if you create optimal delta chains, the
length of these delta chains will not exceed the "--depth" option.

If the history you are pushing modified A/F twice, renamed it to B/F
(with or without modification at the same time), then modified B/F
twice more, you'd want to pack the 5-commit segment and having to
artificially cut the delta chain that can contain all of these 5
blobs into two at the renaming commit is a huge loss.

Compared to that, a whole history clone is a very different story,
as we will have to chomp delta chains at some depth anyway.  Before
the rename, it is reasonable to assume that A/F have evolved
incrementally for number of revisions, and after that rename it is
expected B/F will evolve incrementally for number of revisions
before it gets renamed again.  It is just the matter of choosing
where in that long stretch of content evolution we would cut the
delta chain, and the commit that renamed the path may just be a
good, if not absolute optimal, point.

So I do agree that this is an important case to optimize for.  At
some point, even when taking only the blobs at the same path as
delta base candidates, your true best base may be outside of the
--window in the list of candidates (sorted by size in decreasing
order), but at that point you would be increasing window to find
better delta base, not to skip unrelated blobs that happened to have
thrown into the same hash bucket due to the design that optimizes
for different case, so we can say that it is worth spending the
extra cycle and memory, if you need a larger window to gain even
better packing result.

> Funny you should say that, since the RFC I finally submitted [1]
> actually does just that. The --path-walk option changes the object
> walk to consider batches of objects based on their path, computes
> deltas among that batch, and then does the normal name-hash pass
> later. This seems to really strike the balance that you are
> looking for and solves the issues where small pushes need to stay
> small. It also fixes some problematic cases even when pushing a
> single commit.

;-).

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> Derrick Stolee <stolee@gmail.com> writes:
>
>> The thing that surprised me is just how effective this is for the
>> creation of large pack-files that include many versions of most
>> files. The cross-path deltas have less of an effect here, and the
>> benefits of avoiding name-hash collisions can be overwhelming in
>> many cases.
>
> Yes, "make sure we notice a file F moving from directory A to B" is
> inherently optimized for short span of history, i.e. a smallish push
> rather than a whole history clone, where the definition of
> "smallish" is that even if you create optimal delta chains, the
> length of these delta chains will not exceed the "--depth" option.
>
> If the history you are pushing modified A/F twice, renamed it to B/F
> (with or without modification at the same time), then modified B/F
> twice more, you'd want to pack the 5-commit segment and having to
> artificially cut the delta chain that can contain all of these 5
> blobs into two at the renaming commit is a huge loss.

Which actually leads me to suspect that we probably do not even have
to expose the --full-name-hash option to the end users in "git repack".

If we are doing incremental that would fit within the depth setting,
it is likely that we would be better off without the full-name-hash
optimization, and if we are doing "repack -a" for the whole
repository, especially with "-f", it would make sense to do the
full-name-hash optimization.

If we can tell how large a chunk of history we are packing before we
actually start calling builtin/pack-objects.c:add_object_entry(), we
probably should be able to even select between with and without
full-name-hash automatically, but I do not think we know the object
count before we finish calling add_object_entry(), so unless we are
willing to compute and keep both while reading and pick between the
two after we finish reading the list of objects, or something, it
will require a major surgery to do so, I am afraid.

[gitgitgadget[bot]]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 9/10/24 10:56 AM, Taylor Blau wrote:
> On Mon, Sep 09, 2024 at 10:37:30PM -0400, Derrick Stolee wrote:
>>> I do agree that considering files at the same path from different
>>> (but close-by) revisions as the prime candidates is very important,
>>> but if you spread the "collissions" very thin by using "uniform
>>> distribution", I am afraid that you'd end up discarding anything but
>>> the blobs at the same path, which may go too far.  Having name hash
>>> value that are close by no longer has any meaning in such a system.
>>
>> You are right that some "nearby" paths are lost in this change, and
>> this can be measured by trying to use this option in the pack-objects
>> process underneath a small 'git push'.
>>
>> The thing that surprised me is just how effective this is for the
>> creation of large pack-files that include many versions of most
>> files. The cross-path deltas have less of an effect here, and the
>> benefits of avoiding name-hash collisions can be overwhelming in
>> many cases.
> > I think that Junio's suggestion is pretty interesting (though please
> take my comments here with a grain of salt, since I haven't read the
> other series yet, and am not sure how much of this is redundant).
> > Imagine computing both the full and existing name-hash values for each
> blob/tree in the pack. Then objects would be sorted in the delta
> selection window by similar full-name hash and similar regular name hash
> values.
> > I'm not sure which value you'd actually record in the pack, though.
> Ideally there is a hash function which captures some information about
> the full path as well as the final path component, so we could use a
> single value here, though I suspect the implementation would be more
> complicated than what is presented here.

Is the name hash stored in the pack itself? I know that it is stored
in the 'struct object_entry' data in the packing data. While we could
add another uint32_t into that struct to store both hash values, this
would increase the memory requirements of repacking by four bytes per
object. The struct seemed to be very clear about trying as hard as
possible to avoid doing that.

But maybe an alternative could be replacing that 32-bit number with
an index into an array of paths that have their hash values stored
there.

This would still involve two passes, but might still be possible. I'll
think on this.

>>> I hope you can find a solution that strikes a good balance at the
>>> end of the series (I saw only the first step), but I suspect an easy
>>> way to avoid the downsides you observed is to use both.  Compare
>>> with a handful of blobs taken from nearby commits (the original
>>> object order is roughly in traversal order, and you can take
>>> advantage of that fact) from exactly the same path (using your
>>> "uniform distribution") before comparing with the blobs with close
>>> value (of the current function) like the current implementation
>>> does, may go a long way.
>>
>> Funny you should say that, since the RFC I finally submitted [1]
>> actually does just that. The --path-walk option changes the object
>> walk to consider batches of objects based on their path, computes
>> deltas among that batch, and then does the normal name-hash pass
>> later. This seems to really strike the balance that you are
>> looking for and solves the issues where small pushes need to stay
>> small. It also fixes some problematic cases even when pushing a
>> single commit.
> > Interesting.
> >> [1] https://lore.kernel.org/git/pull.1786.git.1725935335.gitgitgadget@gmail.com/
> >> However, the --path-walk option requires significant implementation
>> of a "path walk API" and my RFC doesn't even do threading right.
>> The --path-walk version (probably) doesn't work with delta islands
>> or other features the same way as the drop-in change to the name-
>> hash heuristic can. For that reason, I think there is likely some
>> long-term value to the --full-name-hash option even though the
>> --path-walk option will be better in many cases.
> > I suspect that this is going to be a significant sticking point. Not
> supporting multi-threading is work-able for GitHub (since we set
> pack.threads=1 today), but lacking support for delta-islands makes this
> a non-starter to run at GitHub.
> > Do you imagine that the --path-walk option could be made to work with
> delta islands? I'm not all that worried about who does that work, but
> more interested at this point in whether or not it's even possible to
> implement.
This is part of the reason why I think the --full-name-hash option is
an interesting consideration. It doesn't have any obvious reason why
it couldn't work with features like delta islands, so it may provide
some quick wins in "large enough" repositories, or at least "large in
the right way".

I unfortunately don't know enough about how the delta islands feature
works to be confident in the possibility of integrating it with the
--path-walk option. At minimum, it would require two object walks:
the first would mark the objects and the second would do the delta
compression with those markings in mind.

But if there is a way to combine both approaches with a two-pass
delta compression technique, then this may be all avoided. I'll give
it a try.

Thanks,
-Stolee

[gitgitgadget[bot]]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 9/10/24 4:36 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> >> Derrick Stolee <stolee@gmail.com> writes:
>>
>>> The thing that surprised me is just how effective this is for the
>>> creation of large pack-files that include many versions of most
>>> files. The cross-path deltas have less of an effect here, and the
>>> benefits of avoiding name-hash collisions can be overwhelming in
>>> many cases.
>>
>> Yes, "make sure we notice a file F moving from directory A to B" is
>> inherently optimized for short span of history, i.e. a smallish push
>> rather than a whole history clone, where the definition of
>> "smallish" is that even if you create optimal delta chains, the
>> length of these delta chains will not exceed the "--depth" option.
>>
>> If the history you are pushing modified A/F twice, renamed it to B/F
>> (with or without modification at the same time), then modified B/F
>> twice more, you'd want to pack the 5-commit segment and having to
>> artificially cut the delta chain that can contain all of these 5
>> blobs into two at the renaming commit is a huge loss.
> > Which actually leads me to suspect that we probably do not even have
> to expose the --full-name-hash option to the end users in "git repack".
> > If we are doing incremental that would fit within the depth setting,
> it is likely that we would be better off without the full-name-hash
> optimization, and if we are doing "repack -a" for the whole
> repository, especially with "-f", it would make sense to do the
> full-name-hash optimization.

Depending on how much we learn from others testing the --full-name-hash
option, I could see the potential that -a could imply --full-name-hash.
I hesitate to introduce that in the first release with this option,
though.

> If we can tell how large a chunk of history we are packing before we
> actually start calling builtin/pack-objects.c:add_object_entry(), we
> probably should be able to even select between with and without
> full-name-hash automatically, but I do not think we know the object
> count before we finish calling add_object_entry(), so unless we are
> willing to compute and keep both while reading and pick between the
> two after we finish reading the list of objects, or something, it
> will require a major surgery to do so, I am afraid.

It's also possible that we could check the list of paths at HEAD to
see how many collisions the default name-hash gives. In cases like
the Git repository, there are very few collisions and thus we don't
need to use --full-name-hash. Restricting to just HEAD (or the
default ref) is not a complete analysis, but might be a good
heuristic.

Thanks,
-Stolee

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/0e183fd7d986884713f908b61263c9537de79d62.

[gitgitgadget[bot]]

There are issues in commit ab5a3e562ae3b944f43e658284e4736f283468df: pack-objects: use 64-bit name hash Lines in the body of the commit messages should be wrapped between 60 and 76 characters. Indented lines, and lines without whitespace, are exempt

[gitgitgadget[bot]]

On the Git mailing list, Jeff King wrote (reply to this):

On Tue, Sep 10, 2024 at 05:05:09PM -0400, Derrick Stolee wrote:

> > I'm not sure which value you'd actually record in the pack, though.
> > Ideally there is a hash function which captures some information about
> > the full path as well as the final path component, so we could use a
> > single value here, though I suspect the implementation would be more
> > complicated than what is presented here.
> 
> Is the name hash stored in the pack itself? I know that it is stored
> in the 'struct object_entry' data in the packing data. While we could
> add another uint32_t into that struct to store both hash values, this
> would increase the memory requirements of repacking by four bytes per
> object. The struct seemed to be very clear about trying as hard as
> possible to avoid doing that.

It's stored in the .bitmap files, since otherwise a pack-objects which
uses bitmaps to serve a fetch would have no clue of their path names.
See the "HASH_CACHE" bitmap extension.

You generally don't want to make deltas out of two entries in the bitmap
(they're already in the same pack, so we'd usually skip them), but you
do want to consider making on-the-fly deltas against other objects.

I guess we may also consider deltas between objects in two packs that
are both covered by the same midx bitmap.

> But maybe an alternative could be replacing that 32-bit number with
> an index into an array of paths that have their hash values stored
> there.

Yes, that would work, though how big is that path array going to be?
Uncompressed linux.git is probably 3-4MB, which actually doesn't sound
_too_ bad. You could obviously go a long way with prefix compression,
too.

But if I understand the proposal, it is just replacing one 32-bit hash
with another. You could just store that in the bitmap instead (or if the
direction is to use both, introduce a new extension to store both).
Obviously you'll get lousy results if the bitmap reader does not use the
same algorithm for its non-bitmap objects, but I don't think this is
something you'd be flipping back and forth on.

> This is part of the reason why I think the --full-name-hash option is
> an interesting consideration. It doesn't have any obvious reason why
> it couldn't work with features like delta islands, so it may provide
> some quick wins in "large enough" repositories, or at least "large in
> the right way".
> 
> I unfortunately don't know enough about how the delta islands feature
> works to be confident in the possibility of integrating it with the
> --path-walk option. At minimum, it would require two object walks:
> the first would mark the objects and the second would do the delta
> compression with those markings in mind.

The delta islands code already does its own tree walk to propagate the
bits down (it does rely on the base walk's show_commit() to propagate
through the commits).

Once each object has its island bitmaps, I think however you choose to
come up with delta candidates (whether the current type/size/namehash
sorted list, or some path walking), you should be able to use it. It's
fundamentally just answering the question of "am I allowed to delta
between these two objects".

Of course the devil may be in the details. ;)

-Peff

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/1205ee7a9e02d58f43d24eef9eec62edb9cf39c3.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/e8c1936343dfb939475b9aa6f3523ab3df69b31d.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/3c327e20e8f183b01fcc42c2f0a1b6c22f60fb4a.

[gitgitgadget[bot]]

There are issues in commit ab5a3e562ae3b944f43e658284e4736f283468df: pack-objects: use 64-bit name hash Lines in the body of the commit messages should be wrapped between 60 and 76 characters. Indented lines, and lines without whitespace, are exempt

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/9ec06b90be52303eb4487abdc7d9bbe1a9ce3a6a.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/736928298d7e7e7dece06bc8e1441cc84f57852d.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/0afba8c52af75d138ef95b6eb4f5bf0010c91766.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/93a20553b3b4a2596a210a675304762a0fc5a9ab.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/d3805abeb4c354a0fe25de13e65a3c48203ab8c6.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/6aca86351d2dfbedda00aee255e2d86cc30ce375.

[derrickstolee]

/submit

[gitgitgadget[bot]]

Submitted as pull.1785.v2.git.1726692381.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1785/derrickstolee/full-name-v2

To fetch this version to local tag pr-1785/derrickstolee/full-name-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1785/derrickstolee/full-name-v2

[gitgitgadget[bot]]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 9/18/24 4:46 PM, Derrick Stolee via GitGitGadget wrote:
...
> Other things that have happened include investigations into ways to adapt
> the full-name hash to improve upon the name-hash. I did some experimenting
> with increasing the size of 'struct object_entry' by using a 64-bit hash
> value (name-hash, then full-name-hash) for a single-pass compression or two
> 32-bit hash values for a two-pass compression process. I include my WIP
> branch at [3] to show what I tried, though the single-pass option did not
> present any improvements and the two-pass option seems to be broken to the
> point that the compression is substantially worse. (I'll try to elaborate on
> this in a reply to this cover letter.)
>
> [3] https://github.com/derrickstolee/git/compare/full-name...derrickstolee:git:full-name-wip

To break down what I attempted in [3], let me break down a few things.

First, I tried using a 64-bit hash value [1]. This used the standard name-hash
as the most-significant digits and the full-name-hash as the least-significant
digits. The goal here was to still have locality from the name-hash but get a
good partition based on full-name-hash within those collisions.

However, when sorting this way, the boundaries of the full-name-hash partitions
are ineffective at getting good delta bases because the largest object from one
full-name-hash set is next to the smallest object from the next full-name-hash
set. Even when a full-name-hash set has size one, it is sorted roughly randomly
among the other colliding path names instead of grouped nicely with objects of
a similar size. This makes the results nearly identical to the 32-bit
full-name-hash implementation.

[1] https://github.com/derrickstolee/git/commit/aaa6befa3016667ea5eb10fdd6aa2b7fcec3a52e

Second, I tried storing two 32-bit hashes and doing a two-pass delta search [2].
In theory, this should be very similar to the --path-walk feature from the RFC.
However, I failed to make it work. Something about this version of a two-pass
walk was hitting some strange behavior. For example, I had to put in this extra
condition [4] if a best delta base was not found, or else we could get a
segfault.

[2] https://github.com/derrickstolee/git/commit/bf71271040ab93a624a8cdf5bc8aaff68e9b1b17

[4] https://github.com/derrickstolee/git/commit/fedc4fc543e50563f4748a5ffc45b51b530023e0

In fact, the results were not just _bad_ but they were _significantly worse_.

It took me a long time to report these details because they just didn't make
sense and I couldn't figure out what was going wrong. I'd be very grateful to
anyone who could explore these WIP commits and point out what I'm doing wrong
so I can learn and maybe we can get a boost to the feature.

Even if we had strong data from these examples, I'm not sure that we'd want
to add four bytes per object to the packing data, especially in a way that
impacts users that aren't even using the new feature. We would want to
explore options that use some kind of hashtable to map objects to their
64-bit hash values, perhaps. It also affects the .bitmap file format, which
would need modification even for a new 32-bit hash function (though one of
the same size could be used by adding an extension saying "I'm using hash
function v2" and leave the rest of the structure the same).

I would also like to test the performance against the threaded version of the
--path-walk feature, which I recently got working in my prototype branch [5].

[5] https://github.com/derrickstolee/git/pull/28/commits/a9fc233390ae00e3d4b156be64d6b3974e30d8a1

Thanks,
-Stolee

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/3c35eab8526dfb859292873df5cbcbf7899afcee.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/b6310a6fc24db4f2539801630a684c55a1cde9bc.

[gitgitgadget[bot]]

This branch is now known as ds/pack-name-hash-tweak.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/549ac4dd150075ddb596e9286a42922fb5eef2a2.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch ds/pack-name-hash-tweak on the Git mailing list:

In a repository with too many (more than --window size) similarly
named files, "git repack" would not find good delta-base candidate
and worse, it may not use a blob from exactly the same path as a
good delta-base candidate.  Optionally replace the name hash so
that only blobs at the same path and nothing else are used as
delta-base candidate.

Will merge to 'next'?
source: <pull.1785.v2.git.1726692381.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/070061dc071f6cb96c468ea95b4a4fb37bd33017.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch ds/pack-name-hash-tweak on the Git mailing list:

In a repository with too many (more than --window size) similarly
named files, "git repack" would not find good delta-base candidate
and worse, it may not use a blob from exactly the same path as a
good delta-base candidate.  Optionally replace the name hash so
that only blobs at the same path and nothing else are used as
delta-base candidate.

On hold.
cf. <34346998-deac-4e1f-9d5f-218f664e9e08@gmail.com>
source: <pull.1785.v2.git.1726692381.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

On the Git mailing list, Patrick Steinhardt wrote (reply to this), regarding 7e47fc8cb53647ad92c86801204c3089a5dfe8e6 (outdated):

On Wed, Sep 18, 2024 at 08:46:21PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
> diff --git a/t/helper/test-name-hash.c b/t/helper/test-name-hash.c
> new file mode 100644
> index 00000000000..15fb8f853c1
> --- /dev/null
> +++ b/t/helper/test-name-hash.c
> @@ -0,0 +1,23 @@
> +/*
> + * 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);
> + }
> +
> + return 0;
> +}

This patch breaks t5310 with the leak sanitizer enabled due to the
leaking `struct strbuf line`. It needs the following diff on top:

diff --git a/t/helper/test-name-hash.c b/t/helper/test-name-hash.c
index 15fb8f853c..e4ecd159b7 100644
--- a/t/helper/test-name-hash.c
+++ b/t/helper/test-name-hash.c
@@ -19,5 +19,6 @@ int cmd__name_hash(int argc UNUSED, const char **argv UNUSED)
        printf("%10"PRIu32"\t%10"PRIu32"\t%s\n", name_hash, full_hash, line.buf);
    }

+   strbuf_release(&line);
    return 0;
 }

I also plan to eventually have a deeper look at your patch series, but
didn't yet find the time to do so until now :(

Patrick

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/7d15aba45ae40ac008fc59b9401d1e4dba6a418b.

[gitgitgadget[bot]]

On the Git mailing list, Derrick Stolee wrote (reply to this), regarding 7e47fc8cb53647ad92c86801204c3089a5dfe8e6 (outdated):

On 9/24/24 3:02 AM, Patrick Steinhardt wrote:

> This patch breaks t5310 with the leak sanitizer enabled due to the
> leaking `struct strbuf line`. It needs the following diff on top:
> > diff --git a/t/helper/test-name-hash.c b/t/helper/test-name-hash.c
> index 15fb8f853c..e4ecd159b7 100644
> --- a/t/helper/test-name-hash.c
> +++ b/t/helper/test-name-hash.c
> @@ -19,5 +19,6 @@ int cmd__name_hash(int argc UNUSED, const char **argv UNUSED)
>           printf("%10"PRIu32"\t%10"PRIu32"\t%s\n", name_hash, full_hash, line.buf);
>       }
>   > + strbuf_release(&line);
>       return 0;
>   }

Thanks! I'll make sure this gets in the next version.

> I also plan to eventually have a deeper look at your patch series, but
> didn't yet find the time to do so until now :(
Thanks for taking the time, when you have it.

-Stolee

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/a6af47d74cff6353365b4d2298512b44bfe19d83.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch ds/pack-name-hash-tweak on the Git mailing list:

In a repository with too many (more than --window size) similarly
named files, "git repack" would not find good delta-base candidate
and worse, it may not use a blob from exactly the same path as a
good delta-base candidate.  Optionally replace the name hash so
that only blobs at the same path and nothing else are used as
delta-base candidate.

On hold.
cf. <34346998-deac-4e1f-9d5f-218f664e9e08@gmail.com>
source: <pull.1785.v2.git.1726692381.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/bbedfd00a31911eda4254fc261f0c2fad28d8353.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch ds/pack-name-hash-tweak on the Git mailing list:

In a repository with too many (more than --window size) similarly
named files, "git repack" would not find good delta-base candidate
and worse, it may not use a blob from exactly the same path as a
good delta-base candidate.  Optionally replace the name hash so
that only blobs at the same path and nothing else are used as
delta-base candidate.

On hold.
cf. <34346998-deac-4e1f-9d5f-218f664e9e08@gmail.com>
source: <pull.1785.v2.git.1726692381.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/135921b729e161a259bd4fa2cfb7162980c3792c.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/9ebb56e75c18235d7923b5ff717bf552e1461cb8.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch ds/pack-name-hash-tweak on the Git mailing list:

In a repository with too many (more than --window size) similarly
named files, "git repack" would not find good delta-base candidate
and worse, it may not use a blob from exactly the same path as a
good delta-base candidate.  Optionally replace the name hash so
that only blobs at the same path and nothing else are used as
delta-base candidate.

On hold.
cf. <34346998-deac-4e1f-9d5f-218f664e9e08@gmail.com>
source: <pull.1785.v2.git.1726692381.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/1b03dde54a6517b5177d384029fa01d784e6ace3.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch ds/pack-name-hash-tweak on the Git mailing list:

In a repository with too many (more than --window size) similarly
named files, "git repack" would not find good delta-base candidate
and worse, it may not use a blob from exactly the same path as a
good delta-base candidate.  Optionally replace the name hash so
that only blobs at the same path and nothing else are used as
delta-base candidate.

On hold.
cf. <34346998-deac-4e1f-9d5f-218f664e9e08@gmail.com>
source: <pull.1785.v2.git.1726692381.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/bb33886a67fcaa7496ca886819359285d8e4afa9.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/637f115ac76578a449b60af44c1485992a26ab92.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch ds/pack-name-hash-tweak on the Git mailing list:

In a repository with too many (more than --window size) similarly
named files, "git repack" would not find good delta-base candidate
and worse, it may not use a blob from exactly the same path as a
good delta-base candidate.  Optionally replace the name hash so
that only blobs at the same path and nothing else are used as
delta-base candidate.

On hold.
cf. <34346998-deac-4e1f-9d5f-218f664e9e08@gmail.com>
source: <pull.1785.v2.git.1726692381.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/e4c3466b3c842ba1567296e871eb560f5a1f68e0.