FormBrowse init optimizations #9729

gerhardol · 2021-11-14T00:22:08Z

Proposed changes

@RussKie has refactored FormBrowse init and filter handling recently.
After this handling some logic like InternalInitialize() was running several times.
This included a few Git commands too, delaying the startup slightly.
There are some test instabilities for FormBrowse, this is an attempt to at least improve investigation.

I initially got som stability issues with this change when testing in isolation but this works better when restructuring commits.
'Loading Revisions' didn't finish in 100 iterations can still occur

Test methodology

There are tests for this already

Merge strategy

Rebase merge

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

gerhardol

Some explanations to changes

gerhardol · 2021-11-14T10:52:32Z

GitUI/CommandsDialogs/FormBrowse.cs

@@ -447,11 +448,9 @@ protected override void OnLoad(EventArgs e)
        {
            _formBrowseMenus.CreateToolbarsMenus(ToolStripMain, ToolStripFilters, ToolStripScripts);

-            HideVariableMainMenuItems();


Run from SetGitModule(), always run before opening module or dashboard so no explicit call needed.

gerhardol · 2021-11-14T10:56:40Z

GitUI/CommandsDialogs/FormBrowse.cs

            RefreshSplitViewLayout();
            LayoutRevisionInfo();
            SetSplitterPositions();
-            InternalInitialize(false);


Method has repo specific setup and runs from RefreshRevisions()
Previously needed here as it must run after RevisionGrid.ForceRefreshRevisions()

gerhardol · 2021-11-14T10:59:40Z

GitUI/CommandsDialogs/FormBrowse.cs


            // We're launching the main form, and whilst the module has been set for us we still need to formally switch to it
            SetGitModule(this, new GitModuleEventArgs(new GitModule(Module.WorkingDir)));

            RevisionGrid.ResumeRefreshRevisions();
+            RefreshRevisions();


Previously, RevisionGrid.ResumeRefreshRevisions() would run RevisionGrid.ForceRefreshRevisions() only, so some other methods in RefreshRevisions() had to run in OnLoad()

gerhardol · 2021-11-14T11:53:33Z

GitUI/CommandsDialogs/FormBrowse.cs

@@ -572,22 +570,33 @@ private void RefreshRevisions()
                return;
            }

-            _gitStatusMonitor.InvalidateGitWorkingDirectoryStatus();


Run from SetGitModule()

GitUI/CommandsDialogs/FormBrowse.cs

gerhardol · 2021-11-14T11:58:17Z

GitUI/CommandsDialogs/FormBrowse.cs

            {
-                revisionDiff.RefreshArtificial();
-                RevisionGrid.ForceRefreshRevisions();
-                RevisionGrid.IndexWatcher.Reset();


Runs in RevisionGrid.ForceRefreshRevisions()

gerhardol · 2021-11-14T12:00:36Z

GitUI/CommandsDialogs/FormBrowse.cs

+
+            InternalInitialize();
+
+            RefreshGitStatusMonitor();


Let the separate git-status calls run after the UI is refreshed

gerhardol · 2021-11-14T12:13:27Z

GitUI/CommandsDialogs/FormBrowse.cs

@@ -1800,7 +1810,6 @@ private void SetGitModule(object sender, GitModuleEventArgs e)

                    // If we're applying custom branch or revision filters - reset them
                    RevisionGrid.DisableFilters();
-                    SetPathFilter(pathFilter: null);


This triggers a Grid refresh, so this must not occur (see also comments for the commit).

Maybe the CanRefresh check in
Maybe add RevisionGrid.SuspendRefreshRevisions()/RevisionGrid.ResumeRefreshRevisions() in GitSetModule to permanently avoid this as the refresh is done in UICommands_PostRepositoryChanged()

Edit: Changed this to use RevisionGrid.SuspendRefreshRevisions()/RevisionGrid.ResumeRefreshRevisions() in GitSetModule and Debug.Assert in RequestRefresh.

This is likely because of the changes to ResumeRefreshRevisions

gerhardol · 2021-11-14T12:15:40Z

GitUI/CommandsDialogs/FormBrowse.cs

@@ -572,22 +570,33 @@ private void RefreshRevisions()
                return;
            }

-            _gitStatusMonitor.InvalidateGitWorkingDirectoryStatus();
-            RequestRefresh();


Renamed to RefreshGitStatusMonitor

GitUI/CommandsDialogs/FormBrowse.cs

RussKie · 2021-11-17T11:00:11Z

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

-            if (_updatingFilters == 0)
-            {
-                ForceRefreshRevisions();
-            }


Coupled with above comment I'm finding this change the most puzzling. This was meant to guard against nested unbalanced suspensions (i.e., without a resume call), and execute a refresh only on the outer scope.
Now it feels like we may be returning into a realm where we may be calling multiple refreshes.

See above, the refresh is always called explicitly after Suspend/Resume.

RussKie · 2021-11-18T06:51:30Z

GitUI/CommandsDialogs/FormBrowse.cs

            }

-            toolStripButtonPush.DisplayAheadBehindInformation(Module.GetSelectedBranch());
+            Debug.Assert(RevisionGrid.CanRefresh, "Already loading revisions when running RefreshRevisions(). This could cause the commits in the grid to be loaded several times.");
+            RevisionGrid.ForceRefreshRevisions();


💭 Generally speaking we should strive to avoid calling this API.

RussKie

In general the changes look sensible and positive, however I am not quite content with the changes to ResumeRefreshRevisions - I spent a considerable time checking various code paths (primarily in the context of filters) to reduce redundant refreshes.

gerhardol · 2021-11-19T22:14:06Z

This PR is required to avoid that the left panel is init twice in #9734, and for #9735 that can save seconds opening a repo.
(After #9735 the tests seem to be more stable too.)

In general the changes look sensible and positive, however I am not quite content with the changes to ResumeRefreshRevisions - I spent a considerable time checking various code paths (primarily in the context of filters) to reduce redundant refreshes.

I also have spent considerable time debugging and trying to understand what the intention was. The handling was not easy before, but the changed handling does not go all the way. I tried to explain the changes in my review, another try here:
The major change is to handle startup in the same way as a repo change, remove the special handling.

The handling after GE has started once is not changed much, just some adjustments.

SetGitModule() -> PostRepositoryChanged() -> RefreshRevisions()
This will then make all repo related changes including RevisionGrid.EnableRefreshRevisions().

The startup had another path:

FormBrowse() DisableGuard; init all (including some repo related like init left panel as in #9734), SetGitModule() etc except RevisionGrid.EnableRefreshRevisions(), EnableGuard that triggers RevisionGrid.EnableRefreshRevisions().
A few repo related then runs (again) in OnLoad() to get the init correctly. Especially InternalInitialize() does repo related setup.

This PR changes the start:

FormBrowse() DisableGuard; init avoiding repo related, EnableGuard then explicit SetGitModule() that will run repo related similar to at a repo switch, including RevisionGrid.EnableRefreshRevisions().
SetGitModule() has the guard internally too. Before #9733 RevisionGrid.EnableRefreshRevisions() was actually run before PostRepositoryChanged() (so in a tiny repo the revisions could have been refreshed twice).
OnLoad() has no repo related.

This change was probably not enough, SetGitModule() should be called from OnLoad() instead.

RevisionGrid.ResumeRefreshRevisions(); should maybe be renamed to RevisionGrid.EnableRefreshRevisions();

The naming was inspired by SuspendLayout and ResumeLayout. There's also a PerformLayout and I've been thinking maybe renaming ForceRefreshRevisions into PerformRefreshRevisions... 🤔

These are now pure guards, the refresh is always done in post repo change.

All refresh is done from SetGitModule() -> PostRepoChanged

Not quite. There are quite a few calls to ForceRefreshRevisions throughout, e.g. applying various filters, and this mechanism guarded against chained refreshes.

All revision reading related to repo changes is now in PostRepositoryChanged().
There are of course refresh in other situations too. If there happen when updating the repo in SetGitModule(), they are now ignored as there will be an explicit call.

RussKie · 2021-11-21T07:12:24Z

I ran it locally with few different filters, and I couldn't observer any reentrancy I was worried about. So 👍 from me.
Proposed few API tweaks to compliment the change.

gerhardol · 2021-11-21T11:24:21Z

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

+        public bool CanRefresh => !_isRefreshingRevisions && _updatingFilters == 0;
+
+        /// <summary>
+        ///  Queries git for the new set of revisions, and refreshed the grid.


Rename is of course fine, will just change this wording.

Suggested change

/// Queries git for the new set of revisions, and refreshed the grid.

/// Queries git for the new set of revisions and refreshes the grid.

OK to squash and merge?
I would like to get #9735 ready to merge, that is both improving startup a lot and seem to improve test stability.

gerhardol · 2021-11-21T12:46:05Z

GitUI/CommandsDialogs/FormBrowse.cs

@@ -582,7 +582,7 @@ private void RefreshRevisions()
            }

            Debug.Assert(RevisionGrid.CanRefresh, "Already loading revisions when running RefreshRevisions(). This could cause the commits in the grid to be loaded several times.");
-            RevisionGrid.ForceRefreshRevisions();
+            RevisionGrid.RefreshRevisions();


This must be PerformRefreshRevisions() (that must be public again.

Reorganize the startup so also first startup runs all repo related operations in PostRepositoryChanged. Before some init operations had to run several times at the startup.

ghost assigned gerhardol Nov 14, 2021

gerhardol force-pushed the feature/i9553-browse-init branch from 48c425d to d644fbe Compare November 14, 2021 10:42

gerhardol mentioned this pull request Nov 14, 2021

RevisionFilter: Indicate filters with button state and tooltip #9728

Merged

gerhardol force-pushed the feature/i9553-browse-init branch from d644fbe to 26f9b6e Compare November 14, 2021 11:50

gerhardol commented Nov 14, 2021

View reviewed changes

gerhardol marked this pull request as ready for review November 14, 2021 12:25

gerhardol requested a review from RussKie November 14, 2021 12:25

gerhardol force-pushed the feature/i9553-browse-init branch from 26f9b6e to fbf148f Compare November 14, 2021 17:45

gerhardol mentioned this pull request Nov 14, 2021

PathFilter: Set FallbackFollowedFile at every filter update #9733

Merged

gerhardol force-pushed the feature/i9553-browse-init branch from fbf148f to dad22be Compare November 14, 2021 23:51

gerhardol mentioned this pull request Nov 16, 2021

LeftPanel was initiated twice at startup #9734

Merged

gerhardol force-pushed the feature/i9553-browse-init branch from 3901b2b to dad22be Compare November 16, 2021 23:05

gerhardol mentioned this pull request Nov 16, 2021

Share GitRef() at startup #9735

Merged

RussKie reviewed Nov 17, 2021

View reviewed changes

RussKie reviewed Nov 18, 2021

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 Nov 18, 2021

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

gerhardol force-pushed the feature/i9553-browse-init branch from dad22be to cee96f0 Compare November 19, 2021 22:33

RussKie force-pushed the master branch from 1dc221f to f915d70 Compare November 20, 2021 06:59

gerhardol force-pushed the feature/i9553-browse-init branch from cee96f0 to cc53627 Compare November 20, 2021 12:24

RussKie force-pushed the feature/i9553-browse-init branch from cc53627 to 9e74d91 Compare November 21, 2021 05:02

gerhardol commented Nov 21, 2021

View reviewed changes

gerhardol force-pushed the feature/i9553-browse-init branch from 9e74d91 to 29a10f5 Compare November 21, 2021 16:24

gerhardol mentioned this pull request Nov 21, 2021

Flaky FormBrowse tests #9750

Open

RussKie approved these changes Nov 22, 2021

View reviewed changes

gerhardol force-pushed the feature/i9553-browse-init branch from 29a10f5 to 2d8d21b Compare November 22, 2021 19:35

FormBrowse init optimizations

2b1d42b

Reorganize the startup so also first startup runs all repo related operations in PostRepositoryChanged. Before some init operations had to run several times at the startup.

gerhardol force-pushed the feature/i9553-browse-init branch from 2d8d21b to 2b1d42b Compare November 22, 2021 20:02

gerhardol merged commit 46a0256 into gitextensions:master Nov 22, 2021

ghost added this to the vNext milestone Nov 22, 2021

gerhardol deleted the feature/i9553-browse-init branch November 22, 2021 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FormBrowse init optimizations #9729

FormBrowse init optimizations #9729

gerhardol commented Nov 14, 2021 •

edited

gerhardol left a comment

gerhardol Nov 14, 2021

gerhardol Nov 14, 2021

gerhardol Nov 14, 2021

gerhardol Nov 14, 2021

gerhardol Nov 14, 2021

gerhardol Nov 14, 2021

gerhardol Nov 14, 2021 •

edited

RussKie Nov 18, 2021

gerhardol Nov 14, 2021

RussKie Nov 17, 2021

gerhardol Nov 17, 2021

RussKie Nov 18, 2021

RussKie left a comment

gerhardol commented Nov 19, 2021

RussKie commented Nov 21, 2021

gerhardol Nov 21, 2021

gerhardol Nov 21, 2021

	/// Queries git for the new set of revisions, and refreshed the grid.
	/// Queries git for the new set of revisions and refreshes the grid.

FormBrowse init optimizations #9729

FormBrowse init optimizations #9729

Conversation

gerhardol commented Nov 14, 2021 • edited

Proposed changes

Test methodology

Merge strategy

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol Nov 14, 2021 • edited

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

RussKie left a comment

Choose a reason for hiding this comment

gerhardol commented Nov 19, 2021

RussKie commented Nov 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol commented Nov 14, 2021 •

edited

gerhardol Nov 14, 2021 •

edited