Re: [PATCH 2/2] diff: teach diff to read gitattribute diff-algorithm

[John Cai <johncai86@gmail.com>]

Hi Elijah,

On 9 Feb 2023, at 3:44, Elijah Newren wrote:

> Hi John,
>
> On Mon, Feb 6, 2023 at 12:47 PM John Cai <johncai86@gmail.com> wrote:
>>
> [...]
>> That being said, here's a separate issue. I benchmarked the usage of
>> .gitattributes as introduced in this patch series, and indeed it does look like
>> there is additional latency:
>>
>> $ echo "* diff-algorithm=patience >> .gitattributes
>> $ hyperfine -r 5 'git-bin-wrapper diff --diff-algorithm=patience v2.0.0 v2.28.0'                      ✭
>> Benchmark 1: git-bin-wrapper diff --diff-algorithm=patience v2.0.0 v2.28.0
>>   Time (mean ± σ):     889.4 ms ± 113.8 ms    [User: 715.7 ms, System: 65.3 ms]
>>   Range (min … max):   764.1 ms … 1029.3 ms    5 runs
>>
>> $ hyperfine -r 5 'git-bin-wrapper diff v2.0.0 v2.28.0'                                                ✭
>> Benchmark 1: git-bin-wrapper diff v2.0.0 v2.28.0
>>   Time (mean ± σ):      2.146 s ±  0.368 s    [User: 0.827 s, System: 0.243 s]
>>   Range (min … max):    1.883 s …  2.795 s    5 runs
>>
>> and I imagine the latency scales with the size of .gitattributes. Although I'm
>> not familiar with other parts of the codebase and how it deals with the latency
>> introduced by reading attributes files.
>
> Yeah, that seems like a large relative performance penalty.  I had the
> feeling that histogram wasn't made the default over myers mostly due
> to inertia and due to a potential 2% loss in performance (since
> potentially corrected by Phillip's 663c5ad035 ("diff histogram: intern
> strings", 2021-11-17)).  If we had changed the default diff algorithm
> to histogram, I suspect folks wouldn't have been asking for per-file
> knobs to use a better diff algorithm.  And the performance penalty for
> this alternative is clearly much larger than 2%, which makes me think
> we might want to just revisit the default instead of allowing per-file
> tweaks.

It seems like the performance penalty was because I was adding calls to parse
attribute files. Piggy backing off of the attribute parsing in userdiff.h will
allow us to not incur this performance penalty:

$ hyperfine -r 5 -L a bin-wrappers/git,git '{a} diff v2.0.0 v2.28.0'
Benchmark 1: git-bin-wrapper diff v2.0.0 v2.28.0
  Time (mean ± σ):      1.072 s ±  0.289 s    [User: 0.626 s, System: 0.081 s]
  Range (min … max):    0.772 s …  1.537 s    5 runs

Benchmark 2: git diff v2.0.0 v2.28.0
  Time (mean ± σ):      1.003 s ±  0.065 s    [User: 0.684 s, System: 0.067 s]
  Range (min … max):    0.914 s …  1.091 s    5 runs

Summary
  'git diff v2.0.0 v2.28.0' ran
    1.07 ± 0.30 times faster than 'git-bin-wrapper diff v2.0.0 v2.28.0'

>
> And on a separate note...
>
> There's another set of considerations we might need to include here as
> well that I haven't seen anyone else in this thread talk about:

These are some great questions. I'll do my best to answer them.
>
> * When trying to diff files, do we read the .gitattributes file from
> the current checkout to determine the diff algorithm(s)?  Or the
> index?  Or the commit we are diffing against?
> * If we use the current checkout or index, what about bare clones or
> diffing between two different commits?
> * If diffing between two different commits, and the .gitattributes has
> changed between those commits, which .gitattributes file wins?
> * If diffing between two different commits, and the .gitattributes has
> NOT changed, BUT a file has been renamed and the old and new names
> have different rules, which rule wins?

In the next version I plan on using Peff's suggestion of utilizing the existing
diff driver scheme [1]. I believe these four questions are addressed if we use
the existing userdiff.h API, which in turn calls the attr.h API. We check the
worktree, then fallback to the index.

By using the userdiff.h API, the behavior will match what users already expect
when they for instance set an external driver.

1. https://lore.kernel.org/git/Y+KQtqNPews3vBS8@coredump.intra.peff.net/

>
> * If per-file diff algorithms are adopted widely enough, will we be
> forced to change the merge algorithm to also pay attention to them?
> If it does, more complicated rename cases occur and we need rules for
> how to handle those.
> * If the merge algorithm has to pay attention to .gitattributes for
> this too, we'll have even more corner cases around what happens if
> there are merge conflicts in .gitattributes itself (which is already
> kind of ugly and kludged)

I see this feature as a user-experience type convenience feature, so I don't
believe there's need for the merge machinery to also pay attention to the diff
algorithm set through gitattrbutes. We can clarify this in the documentation.

>
>
> Anyway, I know I'm a bit animated and biased in this area, and I
> apologize if I'm a bit too much so.  Even if I am, hopefully my
> comments at least provide some useful context.

No problem! thanks for raising these issues.

thanks
John