Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP Optimize create copy of GitModule() #6373

Closed

Conversation

gerhardol
Copy link
Member

@gerhardol gerhardol commented Mar 17, 2019

Part of #6357

Separating the second part of #6359 to not mixup discussions
https://github.com/gitextensions/gitextensions/pull/6359/files/07b797b3460c0ceef870f73cab169e9a7cafacdf#diff-a77f67d7f76305a8eeaae638ef09120a

Proposed changes

Make a copy of gitModule instead of creating a new GitModule.
This is the pattern used in similar situations, for instance GitStatusMonitor.

This avoids a call to git rev-parse --common-dir when querying submodules structure/status.
This is not critical in itself, but it is a distraction for #6359.

Note: A previous version (in #6357) used an optimized constructor to create a new instance. This is maybe the pattern that should be used in all similar situations.

Test methodology

  • Open refresh a repo with changes in several submodule. For example GE repo with extra files in the submodules
  • Verify that there is not a separate call to git-rev-parse for each of the modified repos

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

@ghost ghost assigned gerhardol Mar 17, 2019
@ghost ghost added the status: ready label Mar 17, 2019
@codecov
Copy link

codecov bot commented Mar 17, 2019

Codecov Report

Merging #6373 into master will decrease coverage by <.01%.
The diff coverage is 51.36%.

@@            Coverage Diff             @@
##           master    #6373      +/-   ##
==========================================
- Coverage   46.84%   46.83%   -0.01%     
==========================================
  Files         688      688              
  Lines       51952    51971      +19     
  Branches     6836     6839       +3     
==========================================
+ Hits        24335    24339       +4     
- Misses      26293    26297       +4     
- Partials     1324     1335      +11
Flag Coverage Δ
#production 36.32% <49.28%> (+0.03%) ⬆️
#tests 97.49% <100%> (-0.05%) ⬇️

@RussKie
Copy link
Member

RussKie commented Mar 19, 2019

Make a copy of gitModule instead of creating a new GitModule.
This is the pattern used in similar situations, for instance GitStatusMonitor.

That's actually the only instance of such use I could find through the codebase.
We always construct a new GitModule instance.

@gerhardol
Copy link
Member Author

That's actually the only instance of such use I could find through the codebase.

I found this too in a quick search:

F:\dev\gc\gitextensions_4\GitUI\UserControls\RevisionGrid\RevisionGridControl.cs(749): GitModule capturedModule = Module;

There are at least 20 occurrences in constructors, that is about the same though.

I find about 20 occurrences of new GitModule(path), none to create a thread local module.

For the three (at least) occurrences copying the variable as well as the possibly other occurrences, the creation could be optimized (similar to what I used in the original version):

        public GitModule(GitModule module) : this(module.WorkingDir)
        {
            _gitCommonDirectory = module._gitCommonDirectory;
        }

However, when I test again, I do not see the problem. Will modify the solution some.

@gerhardol gerhardol force-pushed the feature/i6357-gitmodule-copy branch from 0b3a337 to 769c95a Compare March 21, 2019 22:42
@gerhardol gerhardol changed the title Optimize create copy of GitModule() WIP Optimize create copy of GitModule() Mar 21, 2019
@gerhardol
Copy link
Member Author

Pushed an update of the Copy API, as well as avoid creating GitModules for all changed submodules.
The majority of the improvements is from #6389, this primarily removes a distraction.
The second part could be separated (and the API improved).
Set to WIP for now, leave it open for the discussions for #6372.
If not done earlier, this is relevant for remaining parts of #5769

Submodule structure is separated from detailed status
Previously, the structure was always recalculated when status was updated
Also, the status is only calculated for the submodule trees where
git-status has detected changes.

This has simplified the code as well decreased the time before the
detailed status is displayed. The status was also flashing for several
seconds while updating (occurred regularly if any submodule had a change).
@gerhardol gerhardol force-pushed the feature/i6357-gitmodule-copy branch from 769c95a to c213a44 Compare April 7, 2019 08:36
@gerhardol
Copy link
Member Author

Rebased on #6445 (in WIP)
Just two changes remains.
This is not making much difference though (but should maybe be applied in other situations).

@gerhardol
Copy link
Member Author

Will not push this change and likely not other potential changes
(plan to close #5769 without this)

@gerhardol gerhardol closed this Apr 15, 2019
@ghost ghost removed the status: ready label Apr 15, 2019
@gerhardol gerhardol deleted the feature/i6357-gitmodule-copy branch November 14, 2021 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants