Improve blame performance #10840

pmiossec · 2023-03-27T22:42:20Z

Proposed changes

Some small improvements on blame data generation:
* cache author lines instead of building it every times
(because it happens often that multiple lines belong to same commit)
* Use TryGetValue to get the commit and avatar image
* better heuristic for initial line capacity

Test methodology

Manual

Test environment(s)

Git Extensions 33.33.33
Build 25c1314 (Dirty)
Git 2.38.1.windows.1
Microsoft Windows NT 10.0.19045.0
.NET 6.0.14
DPI 96dpi (no scaling)

Merge strategy

Merge commit

✒️ I contribute this code under The Developer Certificate of Origin.

pmiossec · 2023-03-27T22:51:41Z

Future work: During my investigation, I saw that
https://github.com/gitextensions/gitextensions/blob/master/GitUI/Avatars/AvatarDownloader.cs#L74 is really slow.

For example, like I said in PR description when no avatar is in cache, blame on GitModule.cs takes:

6m40s when downloading avatars
6s when generating the initials!!!

But a quick test replacing the use of WebClient by httpclient (⚠️ without proxy configuration!) reduce the time to around 50s (instead of more than 6 min). I 🤞 that it's not the proxy configuration that introduce the perf cost but there is some investigation to do...

I have not replaced the code because I don't know how to reproduce the same proxy configuration than with using WebClient.
If someone knows how to do it....

gerhardol · 2023-03-28T07:25:48Z

I have been using this commit for some months and did some measurements with the change.
This improves the load time for a bigger module like GitModule.cs with a few ms, so not big compared to the Git command that requires seconds. Still a relevant change.

mstv

+2 LGTM

I have been using this commit for some months

Me, too.

GitCommands/Git/GitModule.cs

GitUI/UserControls/BlameControl.cs

GitCommands/Settings/AppSettings.cs

gerhardol

(approved but agree with mstv, should be changed)

GitCommands/Settings/AppSettings.cs

pmiossec · 2023-03-28T22:48:51Z

Done.

RussKie · 2023-03-29T07:47:27Z

Disable fetching avatar by default (because it really too slow: 6m40s with avatars / 6s without)

Can you please point me to where we're doing this?

But a quick test replacing the use of WebClient by httpclient (⚠️ without proxy configuration!) reduce the time to around 50s (instead of more than 6 min). I 🤞 that it's not the proxy configuration that introduce the perf cost but there is some investigation to do...

I have not replaced the code because I don't know how to reproduce the same proxy configuration than with using WebClient. If someone knows how to do it....

I think as a first step we can provide an implementation without a proxy, and then bolt on a proxy support. Unless, of course, you're behind proxy and need it :)
I don't remember when was the last time I was behind a proxy, so I won't be much help here.

A quick search for how to configure proxy with HttpClient yields these: https://stackoverflow.com/a/58560252/2338036 and https://stackoverflow.com/a/55242207/2338036.

RussKie

👍

RussKie · 2023-03-29T07:34:33Z

GitCommands/Git/GitModule.cs

@@ -3378,7 +3378,7 @@ internal GitBlame ParseGitBlame(string output, Encoding encoding)
            // is a blank line, and third is an introductory paragraph about the project.

            Dictionary<ObjectId, GitBlameCommit> commitByObjectId = new();
-            List<GitBlameLine> lines = new(capacity: 256);
+            List<GitBlameLine> lines = new(capacity: Math.Min(Math.Max(256, output.Length / 120), 1000));


What are these magic numbers?

It's a heuristic to size the list (to prevent multiple reallocation) depending on the size of the file content to blame with some limits to not allocate a too small list (256 as before) or too big (1000) one.
To 'estimate' the number of lines, I took a number bigger than what nearly all projects will have as mean source code length to guarantee that we will not over-allocate (because we blame nearly 90% of the time source code, right!?!) which is a length of 120 characters...

It would be good to add some hints about this.
I remember looking at this line some minutes when I tried this commit out to understand it.
(Not affecting approval.)

I have added a comment...

pmiossec · 2023-03-30T16:46:37Z

Disable fetching avatar by default (because it really too slow: 6m40s with avatars / 6s without)

Can you please point me to where we're doing this?

I have remove the code for another PR as @gerhardol suggestion and updated the description just a few minutes before your comment...

But a quick test replacing the use of WebClient by httpclient (⚠️ without proxy configuration!) reduce the time to around 50s (instead of more than 6 min). I 🤞 that it's not the proxy configuration that introduce the perf cost but there is some investigation to do...
I have not replaced the code because I don't know how to reproduce the same proxy configuration than with using WebClient. If someone knows how to do it....

I think as a first step we can provide an implementation without a proxy, and then bolt on a proxy support. Unless, of course, you're behind proxy and need it :) I don't remember when was the last time I was behind a proxy, so I won't be much help here.

A quick search for how to configure proxy with HttpClient yields these: https://stackoverflow.com/a/58560252/2338036 and https://stackoverflow.com/a/55242207/2338036.

I will have a look but it will be part of the other PR on avatar handling where I already fix or improve some other things...

* cache author lines instead of building it every times (because it happens often that multiple lines belong to same commit) * Use TryGetValue() to get the commit and avatar image * better heuristic for initial line capacity based on evaluation of multiple repositories on how many characters are needed on average in a git blame to "describe" a line of a text file (surprisingly consistent around 120 characters!)

ghost assigned pmiossec Mar 27, 2023

pmiossec requested a review from gerhardol March 27, 2023 22:42

mstv reviewed Mar 28, 2023

View reviewed changes

GitCommands/Git/GitModule.cs Outdated Show resolved Hide resolved

GitUI/UserControls/BlameControl.cs Outdated Show resolved Hide resolved

GitUI/UserControls/BlameControl.cs Outdated Show resolved Hide resolved

mstv reviewed Mar 28, 2023

View reviewed changes

GitCommands/Settings/AppSettings.cs Outdated Show resolved Hide resolved

gerhardol approved these changes Mar 28, 2023

View reviewed changes

GitCommands/Settings/AppSettings.cs Outdated Show resolved Hide resolved

pmiossec force-pushed the improve_blame_performance branch from 25c1314 to 35d337e Compare March 28, 2023 22:47

mstv approved these changes Mar 28, 2023

View reviewed changes

RussKie reviewed Mar 29, 2023

View reviewed changes

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 30, 2023

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 30, 2023

pmiossec force-pushed the improve_blame_performance branch from 35d337e to 4f25331 Compare March 31, 2023 17:07

gerhardol approved these changes Mar 31, 2023

View reviewed changes

RussKie approved these changes Mar 31, 2023

View reviewed changes

RussKie merged commit 9647b99 into gitextensions:master Mar 31, 2023
2 checks passed

ghost added this to the vNext milestone Mar 31, 2023

pmiossec deleted the improve_blame_performance branch April 1, 2023 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve blame performance #10840

Improve blame performance #10840

pmiossec commented Mar 27, 2023 •

edited

pmiossec commented Mar 27, 2023

gerhardol commented Mar 28, 2023

mstv left a comment

gerhardol left a comment

pmiossec commented Mar 28, 2023

RussKie commented Mar 29, 2023

RussKie left a comment

RussKie Mar 29, 2023

pmiossec Mar 30, 2023

gerhardol Mar 30, 2023

pmiossec Mar 31, 2023

pmiossec commented Mar 30, 2023

Improve blame performance #10840

Improve blame performance #10840

Conversation

pmiossec commented Mar 27, 2023 • edited

Proposed changes

Test methodology

Test environment(s)

Merge strategy

pmiossec commented Mar 27, 2023

gerhardol commented Mar 28, 2023

mstv left a comment

Choose a reason for hiding this comment

gerhardol left a comment

Choose a reason for hiding this comment

pmiossec commented Mar 28, 2023

RussKie commented Mar 29, 2023

RussKie left a comment

Choose a reason for hiding this comment

RussKie Mar 29, 2023

Choose a reason for hiding this comment

pmiossec Mar 30, 2023

Choose a reason for hiding this comment

gerhardol Mar 30, 2023

Choose a reason for hiding this comment

pmiossec Mar 31, 2023

Choose a reason for hiding this comment

pmiossec commented Mar 30, 2023

pmiossec commented Mar 27, 2023 •

edited