-
Notifications
You must be signed in to change notification settings - Fork 652
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
attempt to fix performance issues with a cache #4208
Conversation
@david-driscoll can you provide the logs before and after the change, with the timestamps? You can remove the sensitive information. Now if the fix works as you say, maybe it makes sense to use same caching approach in the other |
So logging just halts, while it's calculating source. I found 2 gaps where it's sitting around, so it's concieveable that it's happening else where, just not as prevelent. When I was debugging I did have 74 source branches, which isn't a lot of numbers, but when you're iterating at least 2 times, things add up. I will do a quick test, cause I still have a 10 second wait or so while it's working.
|
I gave it a shot, and went from 14s this morning to 12s. Honestly that's within margin of error, so I can't say it really did much. If you want I'll be glad to push the code, it doesn't break anything that I'm aware of (unit tests are running now), however I don't have any evidence it will have any real effect. |
I have discovered similar behavior with this repo as well: https://github.com/RocketSurgeonsGuild/Nuke which is public, so clone away! As long as you're on a branch other than |
@arturcic anything else you need from me? |
@david-driscoll first of all can you first squash the commits you currently have, the result will have only the change that we need. Second, can you also use the Lazy collections for |
aa596c9
to
68fd907
Compare
68fd907
to
825f62c
Compare
@arturcic I've added in the other collection changes. And looks like you squashed, so I did the same. |
@david-driscoll thank you, I will have a look tomorrow |
Will this make it into a (preview) release any time soon? |
@christophwille it will be included in the next release, whenever this PR gets merged |
825f62c
to
ebf9170
Compare
ebf9170
to
ec25813
Compare
Thank you @david-driscoll for your contribution! |
Description
This is an attempt to fix some of the performance problems (see #4159). As best I can tell each time that the commits are requested from a given
IBranch
the git library appears to enumerate again and again.Related Issue
#4159
Motivation and Context
1.5 minutes to compute a version is a lot! 😄
How Has This Been Tested?
Best way I was able to test, was use the current version (6.0.2) and then compare to the results after caching. The difference in performance was 1 and a half minutes on my machine to 10 seconds.
Screenshots (if appropriate):
Checklist: