Serialize submodules info loading #6372

jbialobr · 2019-03-16T14:27:18Z

Fixes #6357

Proposed changes

Load submodules info with async api. It was not enough, the lag was smaller but still significant.
Load submodules info sequentially - this helped.

Screenshots

Before

After

Test methodology

Manual tests

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

codecov · 2019-03-16T14:37:17Z

Codecov Report

Merging #6372 into master will increase coverage by 0.17%.
The diff coverage is 58.78%.

@@            Coverage Diff            @@
##           master   #6372      +/-   ##
=========================================
+ Coverage   46.63%   46.8%   +0.17%     
=========================================
  Files         687     688       +1     
  Lines       51666   51927     +261     
  Branches     6807    6826      +19     
=========================================
+ Hits        24096   24306     +210     
- Misses      26262   26280      +18     
- Partials     1308    1341      +33

Flag	Coverage Δ
#production	`36.24% <48.3%> (+0.15%)`	⬆️
#tests	`97.52% <100%> (-0.03%)`	⬇️

jbialobr · 2019-03-16T14:32:45Z

GitCommands/Git/ExecutableExtensions.cs

+            Encoding outputEncoding = null,
+            bool stripAnsiEscapeCodes = true)
+        {
+            var result = await executable.ExecuteAsync(arguments, writeInput, outputEncoding, stripAnsiEscapeCodes);


It is not an async stream, but at least is non blocking.

What kind of stream is it? What would it take to make it properly asynchronous?

I believe it is https://docs.microsoft.com/pl-pl/dotnet/api/system.collections.generic.iasyncenumerable-1?view=netcore-3.0

GitCommands/Submodules/SubmoduleStatusProvider.cs

gerhardol · 2019-03-16T17:18:06Z

Thanks!
Works OK for me. There is still a little lag selecting commits while loading, but no "hanging" seen. 2.51 is still snappier loading the data, but 2.51 starts show the revision graph later, so there are other cons.
Would like to see this with #6359 that fixed my real life problem (but not the extreme setup)

gerhardol

For c62d4f9, I prefer #6359 instead
There are quite some changes otherwise, mostly seem fine. I would like to see the outcome of the discussion in #6357 (comment) before merging.

jbialobr · 2019-03-17T13:43:41Z

For c62d4f9, I prefer #6359 instead

👍

There are quite some changes otherwise, mostly seem fine. I would like to see the outcome of the discussion in #6357 (comment) before merging.

Even if we go for a dedicated task scheduler for background tasks, running tens of background tasks simultaneously will degrade the overall system performance.

RussKie

👍

RussKie · 2019-03-19T13:13:04Z

GitCommands/Git/GitModule.cs

        {
            var args = new GitArgumentBuilder("show") { $"HEAD:{path.ToPosixPath().Quote()}" };
-            var result = _gitExecutable.Execute(args);
+            var result = await _gitExecutable.ExecuteAsync(args).ConfigureAwaitRunInline();


Could you please explain how you chose when to use ConfigureAwaitRunInline() and when ConfigureAwait(false) calls?

I've found microsoft/vs-threading#292 (via dotnet/project-system#3662), however it is not clear to me why we aren't using one or the other.

I use ConfigureAwait(false) when I don't want the continuation to be executed on the main thread.
I use ConfigureAwaitRunInline when I know it is safe to continue on the thread that completed the awaited task (regarding both: accessing variables and continuation's duration time).

GitCommands/Submodules/SubmoduleStatusProvider.cs

RussKie · 2019-03-20T11:43:59Z

Thank you

…

On Wed, 20 Mar 2019 at 01:08, Janusz Białobrzewski ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In GitCommands/Git/GitModule.cs <#6372 (comment)> : > { var args = new GitArgumentBuilder("show") { $"HEAD:{path.ToPosixPath().Quote()}" }; - var result = _gitExecutable.Execute(args); + var result = await _gitExecutable.ExecuteAsync(args).ConfigureAwaitRunInline(); I use ConfigureAwait(false) when I don't want the continuation to be executed on the main thread. I use ConfigureAwaitRunInline when I know it is safe to continue on the thread that completed the awaited task (regarding both: accessing variables and continuation's duration time). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6372 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEMyXkeGrpF4k78nkqtZHOIoVR_ZndNtks5vYO9OgaJpZM4b31Pk> .

RussKie

LGTM, but I don't work with any repos with submodules (except the GE), so have no real opinion.

If there is too many submodules, then too many tasks are spawned causing delays in getting async output.

then refresh all items at once with a single switch to the main thread

gerhardol · 2019-03-30T13:04:05Z

How to continue this?
In everyday use #6359 is solving most issues for me. However, with just a few more submodules than what I use (or something else changed), the problem will occur.

This PR goes after the core problem even if there may be some issues left. From what I can tell, the changes are improvements anyway. Should this be merged anyway?
(At least first commit seem necessary.)

@jbialobr @RussKie
How to proceed?

I rebased and removed one change in c62d4f9 (rest of the commit is probably OK):
https://github.com/gerhardol/gitextensions/tree/jbialobr/_jb/fix%236357
Builds here:
https://ci.appveyor.com/project/gerhardol/gitextensions/builds/23473927/artifacts

There are other ways to limit job (#6373 to minor extent, some suggestions in #5369) too, but that will not handle situations with many submodule changes.

RussKie · 2019-03-31T03:03:02Z

I don't currently work with any repos that have submodules (other than GE), so I have no skin in the game.
The changes look sensible to me, hence my approval.

If you're happy with the changes, let's merge them.

gerhardol · 2019-03-31T13:18:09Z

If you're happy with the changes, let's merge them.

There are roughly three parts to this PR (partly from the PR description):

Load submodules info with async api. It was not enough, the lag was smaller but still significant.
First half, except 2nd commit

Good from what I know about async handling, but I am not the expert here, the existing GE code is my reference. If I were the maintainer, I would have squash merged these commits.

Pre-load module encoding.
2nd commit

Not needed after #6359. At least one line should be removed, maybe the other are an improvement.

Load submodules info sequentially
Second half of the commits

Probably OK change, no need to make massive starts of background requests.
However, the change should not be needed according to the discussions in #6357, but if it is an improvement, it should be merged.

@jbialobr @sharwell @drewnoakes @spdr870 @RussKie
How to proceed?

jbialobr · 2019-04-01T06:37:26Z

I would have squash merged these commits.

I see no value in squashing commits. It was already discussed.

Pre-load module encoding.
2nd commit

It is no longer a part of this PR.

However, the change should not be needed according to the discussions in #6357, but if it is an improvement, it should be merged.

In my opinion this change is not needed only if we implement refreshing those submodules which actually have changed. Otherwise querying for all the submodules at once puts an unnecessary pressure on the system resources.

GitUI/BranchTreePanel/RepoObjectsTree.Nodes.Submodules.cs

sharwell · 2019-04-01T11:00:06Z

UnitTests/GitCommandsTests/GitModuleTest.cs

-                (char code, ObjectId commitId) = moduleSub.GetSuperprojectCurrentCheckout();
-                Assert.AreEqual(32, code);
-                Assert.AreEqual(commitRef, commitId.ToString());
+                ThreadHelper.JoinableTaskFactory.Run(async () =>


💡 Can we just make the containing test method asynchronous?

I don't know. Is doing so compatible with JTF?

… we have to recheck if cancellation was requested. Co-Authored-By: jbialobr <jbialobr@o2.pl>

gerhardol

Thanks. Lets merge this!

I am updating to only request submodule status for trees with changes which also will serialize requests, but this is not in conflict with this PR

gerhardol · 2019-04-06T23:58:46Z

GitUI/CommandsDialogs/BrowseDialog/GitStatusMonitor.cs

                            var cmd = GitCommandHelpers.GetAllChangedFilesCmd(true, UntrackedFilesMode.Default,
                                noLocks: true);
-                            var output = module.GitExecutable.GetOutput(cmd);
+                            var output = await module.GitExecutable.GetOutputAsync(cmd).ConfigureAwait(false);


This change causes git-status to run on the UI thread. Is this really necessary?

Do you observe any issues related to this? This is an async call and does not block the UI.

No problem seem, just confused by it.

ghost assigned jbialobr Mar 16, 2019

ghost added the status: ready label Mar 16, 2019

jbialobr commented Mar 16, 2019

View reviewed changes

jbialobr mentioned this pull request Mar 16, 2019

UI freezes when updating submodule status #6357

Closed

gerhardol requested changes Mar 17, 2019

View reviewed changes

RussKie reviewed Mar 19, 2019

View reviewed changes

RussKie approved these changes Mar 20, 2019

View reviewed changes

gerhardol mentioned this pull request Mar 21, 2019

WIP Optimize create copy of GitModule() #6373

Closed

jbialobr added 10 commits March 22, 2019 08:43

Make GetCurrentSubmoduleChanges all-the-way-down-async

c82f101

Serialize submodules loading

6873dcf

Use async call

266c0a4

GetRemotesAsync

a2a3f28

GetConflictsAsync, GetSuperprojectCheckoutAsync

1f793e5

Missing FileAndForget

5f3be56

Update all details at once.

df49e15

Serialize loading details

9ec598d

If there is too many submodules, then too many tasks are spawned causing delays in getting async output.

Load details sequentially to not spawn too many background threads

d32e7d8

then refresh all items at once with a single switch to the main thread

Check for a cancellation request before each awaiting.

1ed5afc

jbialobr force-pushed the jb/fix#6357 branch from 57f082b to 1ed5afc Compare March 22, 2019 07:53

sharwell reviewed Apr 1, 2019

View reviewed changes

GitUI/BranchTreePanel/RepoObjectsTree.Nodes.Submodules.cs Outdated Show resolved Hide resolved

sharwell reviewed Apr 1, 2019

View reviewed changes

Pass the token to emphasize that it is not enough to pass it here and…

b2fdd78

… we have to recheck if cancellation was requested. Co-Authored-By: jbialobr <jbialobr@o2.pl>

gerhardol approved these changes Apr 1, 2019

View reviewed changes

jbialobr merged commit 703f607 into gitextensions:master Apr 2, 2019

ghost removed the status: ready label Apr 2, 2019

jbialobr deleted the jb/fix#6357 branch April 2, 2019 17:43

This was referenced Apr 6, 2019

Detailed submodule status in dropdown menu was not updated #6444

Merged

Separate submodule structure and status #6445

Merged

gerhardol reviewed Apr 6, 2019

View reviewed changes

gerhardol mentioned this pull request Apr 7, 2019

Improve Submodule Status performance #5769

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize submodules info loading #6372

Serialize submodules info loading #6372

jbialobr commented Mar 16, 2019 •

edited

codecov bot commented Mar 16, 2019 •

edited

jbialobr Mar 16, 2019

sharwell Apr 1, 2019

jbialobr Apr 1, 2019

gerhardol commented Mar 16, 2019

gerhardol left a comment

jbialobr commented Mar 17, 2019 •

edited

RussKie left a comment

RussKie Mar 19, 2019

jbialobr Mar 19, 2019

RussKie commented Mar 20, 2019 via email

RussKie left a comment

gerhardol commented Mar 30, 2019

RussKie commented Mar 31, 2019

gerhardol commented Mar 31, 2019

jbialobr commented Apr 1, 2019

sharwell Apr 1, 2019 •

edited

jbialobr Apr 1, 2019

gerhardol left a comment

gerhardol Apr 6, 2019

jbialobr Apr 7, 2019

gerhardol Apr 7, 2019

Serialize submodules info loading #6372

Serialize submodules info loading #6372

Conversation

jbialobr commented Mar 16, 2019 • edited

Proposed changes

Screenshots

Before

After

Test methodology

codecov bot commented Mar 16, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol commented Mar 16, 2019

gerhardol left a comment

Choose a reason for hiding this comment

jbialobr commented Mar 17, 2019 • edited

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie commented Mar 20, 2019 via email

RussKie left a comment

Choose a reason for hiding this comment

gerhardol commented Mar 30, 2019

RussKie commented Mar 31, 2019

gerhardol commented Mar 31, 2019

jbialobr commented Apr 1, 2019

sharwell Apr 1, 2019 • edited

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

Choose a reason for hiding this comment

jbialobr commented Mar 16, 2019 •

edited

codecov bot commented Mar 16, 2019 •

edited

jbialobr commented Mar 17, 2019 •

edited

sharwell Apr 1, 2019 •

edited