Rebase: improve perfs when displaying list of commits #11197

pmiossec · 2023-09-07T14:00:51Z

by retrieving all the commits data with only one git log call instead of one by commit.
It is possible as when rebasing, all the commits belong to the same branch.

Kept a fall back to previous way to retrieve commit data for 1 commit if commit is not in the one loaded.

Results to show the rebase popup with 7448 commits rebased: 2s instead of 4m50 previously

Fixes #10698 and fixes #11196

Test methodology

Manual

Test environment(s)

Git Extensions 33.33.33
Build acee834 (Dirty)
Git 2.42.0.windows.2
Microsoft Windows NT 10.0.22621.0
.NET 6.0.21
DPI 96dpi (no scaling)
Portable: False
Microsoft.WindowsDesktop.App Versions

    Microsoft.WindowsDesktop.App 6.0.21 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
    Microsoft.WindowsDesktop.App 7.0.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

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

pmiossec · 2023-09-07T15:04:09Z

@dosercz You could try the portable version here: https://ci.appveyor.com/project/gitextensions/gitextensions/build/artifacts

gerhardol

+1
Has not run

GitCommands/RevisionReader.cs

GitCommands/Git/GitModule.cs

gerhardol

Approving, assuming the question is fixed

gerhardol · 2023-09-08T21:12:35Z

GitCommands/Git/GitModule.cs

+                {
+                    Author = data?.Author,
+                    ObjectId = data?.ObjectId,
+                    Subject = data?.Body,


Is body or Subject needed?
RevisionReader will not save Body (just subject) if the commit is older than 6 months.
(rename the _sixMonths variable to _oldestBody?)

public RevisionReader(GitModule module, bool hasReflogSelector, bool allBodies = false) : this(module, hasReflogSelector, module.LogOutputEncoding, allBodies ? 0 : new DateTimeOffset(DateTime.Now.ToUniversalTime() - TimeSpan.FromDays(30 * 6)).ToUnixTimeSeconds()) { }

Suggested change

Subject = data?.Body,

Subject = error ?? data?.Subject,

If the Body is not needed, then those changes in RevisionReader is not required, just enough to clarify with this line.
(I do not mind keep them, should be used later).

RussKie

Few nits and suggestions

GitCommands/Git/GitModule.cs

GitCommands/RevisionReader.cs

dosercz · 2023-09-12T09:26:11Z

@pmiossec I have tested portable version builded for this PR and it's fine, good job. It'll safe a lot of my time!

pmiossec · 2023-09-12T11:48:38Z

@gerhardol Could you give your point of view at least on the 2nd commit that get the bodies for older commits as I did some refactoring/renaming?

RussKie · 2023-09-12T13:56:56Z

GitCommands/Git/GitModule.cs

-                    var isApplying = currentCommitShortHash is not null && commitHash.StartsWith(currentCommitShortHash);
-                    isCurrentFound |= isApplying;
+            RevisionReader reader = new(this, hasReflogSelector: false, allBodies: true);
+            CancellationTokenSource cts = new(2 * 60_000);


A comment here would be great as well. This is two minutes? Why so long?

Suggested change

CancellationTokenSource cts = new(2 * 60_000);

CancellationTokenSource cts = new(TimeSpan.FromMinutes(2));

or whatever value how long we would block the UI if there are extremely many commits.

Hopefully two minutes will no longer be needed, but if there is a scenario where the new handling is not effective, keep it.

RussKie · 2023-09-12T13:58:13Z

👍

mstv

just tuning

mstv · 2023-09-12T19:40:21Z

GitCommands/Git/GitModule.cs


-                    string[] parts = todoCommit.Split(Delimiters.Space);
+            // Filter comment lines and keep only lines containing at least 3 columns
+            // (action, commit hash and commit subject -- that could contains space and be cut in more --)


nit:

Suggested change

// (action, commit hash and commit subject -- that could contains space and be cut in more --)

// (action, commit hash and commit subject -- that could contain spaces and be cut in more --)

mstv · 2023-09-12T19:57:41Z

GitCommands/Git/GitModule.cs

-                    var isApplying = currentCommitShortHash is not null && commitHash.StartsWith(currentCommitShortHash);
-                    isCurrentFound |= isApplying;
+            RevisionReader reader = new(this, hasReflogSelector: false, allBodies: true);
+            CancellationTokenSource cts = new(2 * 60_000);


Suggested change

CancellationTokenSource cts = new(2 * 60_000);

CancellationTokenSource cts = new(TimeSpan.FromMinutes(2));

or whatever value how long we would block the UI if there are extremely many commits.

GitCommands/Git/GitModule.cs

GitCommands/RevisionReader.cs

gerhardol

agree with most other comments, assume they will be resolved

GitCommands/RevisionReader.cs

gerhardol · 2023-09-12T21:33:56Z

GitCommands/Git/GitModule.cs

+                {
+                    Author = data?.Author,
+                    ObjectId = data?.ObjectId,
+                    Subject = data?.Body,


If the Body is not needed, then those changes in RevisionReader is not required, just enough to clarify with this line.
(I do not mind keep them, should be used later).

gerhardol · 2023-09-12T21:36:27Z

GitCommands/Git/GitModule.cs

-                    var isApplying = currentCommitShortHash is not null && commitHash.StartsWith(currentCommitShortHash);
-                    isCurrentFound |= isApplying;
+            RevisionReader reader = new(this, hasReflogSelector: false, allBodies: true);
+            CancellationTokenSource cts = new(2 * 60_000);


Hopefully two minutes will no longer be needed, but if there is a scenario where the new handling is not effective, keep it.

GitCommands/Git/GitModule.cs

mstv

👍

mstv · 2023-09-13T20:50:08Z

GitCommands/Git/GitModule.cs

@@ -2151,7 +2151,7 @@ public IReadOnlyList<PatchFile> GetInteractiveRebasePatchFiles()
            string commentChar = EffectiveConfigFile.GetString("core.commentChar", "#");

            // Filter comment lines and keep only lines containing at least 3 columns
-            // (action, commit hash and commit subject -- that could contains space and be cut in more --)
+            // (action, commit hash and commit subject -- that could contains spaces and be cut in more --)


Suggested change

// (action, commit hash and commit subject -- that could contains spaces and be cut in more --)

// (action, commit hash and commit subject -- that could contain spaces and be cut in more --)

mstv · 2023-09-13T21:00:00Z

GitCommands/Git/GitModule.cs

-                    isCurrentFound |= isApplying;
+            // retrieve commits bodies to display them in the grid or as tooltips
+            RevisionReader reader = new(this, hasReflogSelector: false, allBodies: true);
+            CancellationTokenSource cts = new(TimeSpan.FromSeconds(15));


This might be too short. It can take a few seconds for a few commits on a workstation.

Is it something you saw?
Because, in GE repo, I made a rebase of 12k commits and it took only 250ms that's why I concluded that in this case 15s was enough. But I have no problem to increase it. 30s?

git-log+parsing requires about 2 s for 100K commits so rebasing all commits in the linux repo would require 20s or so on my machine. 30s is safe

The git-rebase operation can require some time though

The cancelation token is only for the 'git log & parsing' so I'm happy you confirm that 15s is enough (to rebase 700k commits!) 🤣
But secured 30s is OK for me as we are talking about a worst case scenario that will nearly never happen...

Is it something you saw?

Yes, perhaps caused by hyperactive enterprise AV and / or Windows Update.

(Also git itself became quirky later yesterday. It reproducibly returned exit code 128 without error output when resetting single files using git checkout - vanished after reboot.)

GitCommands/Git/GitModule.cs

mstv · 2023-09-14T10:38:17Z

GitCommands/Git/GitModule.cs

-                    isCurrentFound |= isApplying;
+            // retrieve commits bodies to display them in the grid or as tooltips
+            RevisionReader reader = new(this, hasReflogSelector: false, allBodies: true);
+            CancellationTokenSource cts = new(TimeSpan.FromSeconds(15));


Is it something you saw?

Yes, perhaps caused by hyperactive enterprise AV and / or Windows Update.

(Also git itself became quirky later yesterday. It reproducibly returned exit code 128 without error output when resetting single files using git checkout - vanished after reboot.)

pmiossec · 2023-09-14T11:49:34Z

Do you still want to review something or I could squash and rebase everything?

mstv

👍

by retrieving all the commits data with only one `git log` call instead of one by commit. It is possible as when rebasing, all the commits belong to the same branch. Kept a fall back to previous way to retrieve commit data for 1 commit if commit is not in the one loaded. and read bodies of all rebased commits (even the ones older than 6 monthes like it was the case before. RevisionReader had to be adapted to do that.) Results with 7448 commits rebased: 2s instead of 4m50 previously Fixes gitextensions#10698 and gitextensions#11196

pmiossec · 2023-09-14T21:49:50Z

Squashed and rebased. Ready to be merged...

gerhardol · 2023-09-17T21:37:58Z

GitCommands/RevisionReader.cs

+            GitArgumentBuilder arguments = new("log")
+                {
+                    "-z",
+                    $"--pretty=format:\"{FullFormat}\"",


This should have been LogFormat, will change at another update

ghost assigned pmiossec Sep 7, 2023

gerhardol reviewed Sep 7, 2023

View reviewed changes

GitCommands/RevisionReader.cs Outdated Show resolved Hide resolved

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

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

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

pmiossec force-pushed the improve_perf_rebase branch from acee834 to 0c40e6d Compare September 7, 2023 22:24

gerhardol approved these changes Sep 8, 2023

View reviewed changes

RussKie reviewed Sep 10, 2023

View reviewed changes

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

GitCommands/Git/GitModule.cs Show resolved Hide resolved

GitCommands/RevisionReader.cs Outdated Show resolved Hide resolved

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

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

pmiossec mentioned this pull request Sep 12, 2023

Rebase: fix accuracy of data in list of commits displayed #11206

Merged

RussKie reviewed Sep 12, 2023

View reviewed changes

mstv reviewed Sep 12, 2023

View reviewed changes

gerhardol approved these changes Sep 12, 2023

View reviewed changes

gerhardol reviewed Sep 13, 2023

View reviewed changes

GitCommands/Git/GitModule.cs Show resolved Hide resolved

mstv reviewed Sep 13, 2023

View reviewed changes

mstv reviewed Sep 14, 2023

View reviewed changes

pmiossec force-pushed the improve_perf_rebase branch from 9983162 to edf2cc2 Compare September 14, 2023 11:54

mstv approved these changes Sep 14, 2023

View reviewed changes

pmiossec force-pushed the improve_perf_rebase branch from edf2cc2 to eb9df4b Compare September 14, 2023 21:49

pmiossec merged commit e9d2885 into gitextensions:master Sep 15, 2023
4 checks passed

ghost added this to the vNext milestone Sep 15, 2023

pmiossec deleted the improve_perf_rebase branch September 15, 2023 11:02

gerhardol reviewed Sep 17, 2023

View reviewed changes

RussKie added the release-notes-worthy label Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebase: improve perfs when displaying list of commits #11197

Rebase: improve perfs when displaying list of commits #11197

pmiossec commented Sep 7, 2023 •

edited

pmiossec commented Sep 7, 2023

gerhardol left a comment

gerhardol left a comment

gerhardol Sep 8, 2023

mstv Sep 12, 2023

gerhardol Sep 12, 2023

RussKie left a comment

dosercz commented Sep 12, 2023

pmiossec commented Sep 12, 2023 •

edited

RussKie Sep 12, 2023

mstv Sep 12, 2023

gerhardol Sep 12, 2023

RussKie commented Sep 12, 2023

mstv left a comment

mstv Sep 12, 2023

mstv Sep 12, 2023

gerhardol left a comment

gerhardol Sep 12, 2023

gerhardol Sep 12, 2023

mstv left a comment

mstv Sep 13, 2023

mstv Sep 13, 2023

pmiossec Sep 13, 2023

gerhardol Sep 13, 2023

pmiossec Sep 14, 2023

mstv Sep 14, 2023

mstv Sep 14, 2023

pmiossec commented Sep 14, 2023

mstv left a comment

pmiossec commented Sep 14, 2023 •

edited

gerhardol Sep 17, 2023

	CancellationTokenSource cts = new(2 * 60_000);
	CancellationTokenSource cts = new(TimeSpan.FromMinutes(2));

	// (action, commit hash and commit subject -- that could contains space and be cut in more --)
	// (action, commit hash and commit subject -- that could contain spaces and be cut in more --)

Rebase: improve perfs when displaying list of commits #11197

Rebase: improve perfs when displaying list of commits #11197

Conversation

pmiossec commented Sep 7, 2023 • edited

Test methodology

Test environment(s)

Merge strategy

pmiossec commented Sep 7, 2023

gerhardol left a comment

Choose a reason for hiding this comment

gerhardol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie left a comment

Choose a reason for hiding this comment

dosercz commented Sep 12, 2023

pmiossec commented Sep 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie commented Sep 12, 2023

mstv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmiossec commented Sep 14, 2023

mstv left a comment

Choose a reason for hiding this comment

pmiossec commented Sep 14, 2023 • edited

Choose a reason for hiding this comment

pmiossec commented Sep 7, 2023 •

edited

pmiossec commented Sep 12, 2023 •

edited

pmiossec commented Sep 14, 2023 •

edited