Improve time complexity of IndexOf. #5800
Conversation
ghost
assigned 
Codecov Report
@@ Coverage Diff @@
## master #5800 +/- ##
==========================================
+ Coverage 36.94% 36.95% +0.01%
==========================================
Files 620 620
Lines 47416 47416
Branches 6323 6321 -2
==========================================
+ Hits 17518 17524 +6
+ Misses 29115 29111 -4
+ Partials 783 781 -2 |
GitUI/CommitInfo/CommitInfo.cs
Outdated
| @@ -78,7 +78,7 @@ public partial class CommitInfo : GitModuleControl | |||
| private List<string> _branches; | |||
| private string _branchInfo; | |||
| private string _gitDescribeInfo; | |||
| [CanBeNull] private IList<string> _sortedRefs; | |||
| [CanBeNull] private IDictionary<string, int> _sortedRefs; | |||
There was a problem hiding this comment.
As this is no longer technically "sorted" maybe _indexByRefName is a better name for the field.
| var length = headLength + s.Length; | ||
|
|
||
| for (var i = 0; i < _otherList.Count; i++) | ||
| if (s.StartsWith("remotes/")) |
There was a problem hiding this comment.
Can the user of this comparer persist the data in sanitised form so we don't have to allocate temporary strings here?
There was a problem hiding this comment.
I don't know. Could you try to convince me that it is worth to care about this? These are short-lived strings and AFAIK the GC deals pretty well with them. The gain in performance caused by avoiding string allocations is almost none if any at all (The old version of this method with string allocations seems to be faster - didn't do a benchmark, just compared freezes times when navigating through blame lines). The gain achieved by replacing List with Dictionary is significant, noticeable with a bare eye. There are many other places where optimization could be done and I would rather optimize them before doing so low level optimizations like avoiding allocations.
There was a problem hiding this comment.
There was a problem hiding this comment.
@jbialobr I don't doubt there's already a gain, the implementation looks great. I'm just wary of comparers that allocate. I don't even know how this comparer is used, but if it was impacting perf due to algorithmic complexity, then presumably there were quite a few calls. Right now it's allocating temp strings for each comparison. If the inputs were all canonical (ref/ form or whatever) then you could eliminate the allocations and the StartsWith call.
The complexity was O(n), which is not acceptable for a method that is used to compare elements in a sorting algorithm. The new time complexity is O(1).
jbialobr
force-pushed
the
jb/sortTime
branch
from
e935300
to
45bfffe
Compare
The complexity was O(n), which is not acceptable for a method
that is used to compare elements in a sorting algorithm.
The new time complexity is O(1).