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.

Sign up for GitHub

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

Jump to bottom

Improve time complexity of IndexOf. #5800

Merged
merged 1 commit into from
Nov 23, 2018
Merged

Improve time complexity of IndexOf. #5800

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 31 additions & 23 deletions GitUI/CommitInfo/CommitInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> _refsOrderDict;
private int _revisionInfoHeight;
private int _commitMessageHeight;

Expand Down Expand Up @@ -227,7 +227,7 @@ void StartAsyncDataLoad()

ThreadHelper.JoinableTaskFactory.RunAsync(() => LoadLinksForRevisionAsync(_revision)).FileAndForget();

if (_sortedRefs == null)
if (_refsOrderDict == null)
{
ThreadHelper.JoinableTaskFactory.RunAsync(() => LoadSortedRefsAsync()).FileAndForget();
}
Expand Down Expand Up @@ -295,10 +295,21 @@ string GetLinksForRevision()
async Task LoadSortedRefsAsync()
{
await TaskScheduler.Default.SwitchTo(alwaysYield: true);
_sortedRefs = Module.GetSortedRefs().ToList();
_refsOrderDict = ToDictionary(Module.GetSortedRefs());

await this.SwitchToMainThreadAsync(cancellationToken);
UpdateRevisionInfo();

IDictionary<string, int> ToDictionary(IReadOnlyList<string> list)
{
var dict = new Dictionary<string, int>();
for (int i = 0; i < list.Count; i++)
{
dict.Add(list[i], i);
}

return dict;
}
}

async Task LoadAnnotatedTagInfoAsync(IReadOnlyList<IGitRef> refs)
Expand Down Expand Up @@ -429,7 +440,7 @@ string GetDescribeInfoForRevision()

private void UpdateRevisionInfo()
{
if (_sortedRefs != null)
if (_refsOrderDict != null)
{
if (_annotatedTagsMessages != null &&
_annotatedTagsMessages.Count > 0 &&
Expand All @@ -445,13 +456,13 @@ private void UpdateRevisionInfo()
.Select(r => r.LocalName)
.ToList();

thisRevisionTagNames.Sort(new ItemTpComparer(_sortedRefs, "refs/tags/"));
thisRevisionTagNames.Sort(new ItemTpComparer(_refsOrderDict, "refs/tags/"));
_annotatedTagsInfo = GetAnnotatedTagsInfo(thisRevisionTagNames, _annotatedTagsMessages);
}

if (_tags != null && string.IsNullOrEmpty(_tagInfo))
{
_tags.Sort(new ItemTpComparer(_sortedRefs, "refs/tags/"));
_tags.Sort(new ItemTpComparer(_refsOrderDict, "refs/tags/"));
if (_tags.Count > MaximumDisplayedRefs)
{
_tags[MaximumDisplayedRefs - 2] = "…";
Expand All @@ -464,7 +475,7 @@ private void UpdateRevisionInfo()

if (_branches != null && string.IsNullOrEmpty(_branchInfo))
{
_branches.Sort(new ItemTpComparer(_sortedRefs, "refs/heads/"));
_branches.Sort(new ItemTpComparer(_refsOrderDict, "refs/heads/"));
if (_branches.Count > MaximumDisplayedRefs)
{
_branches[MaximumDisplayedRefs - 2] = "…";
Expand Down Expand Up @@ -711,12 +722,12 @@ protected override void DisposeCustomResources()

private sealed class ItemTpComparer : IComparer<string>
drewnoakes marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly IList<string> _otherList;
private readonly IDictionary<string, int> _orderDict;
drewnoakes marked this conversation as resolved.
Show resolved Hide resolved
private readonly string _prefix;

public ItemTpComparer(IList<string> otherList, string prefix)
public ItemTpComparer(IDictionary<string, int> orderDict, string prefix)
{
_otherList = otherList;
_orderDict = orderDict;
_prefix = prefix;
}

Expand All @@ -726,21 +737,18 @@ public int Compare(string a, string b)

int IndexOf(string s)
{
var head = s.StartsWith("remotes/") ? "refs/" : _prefix;
var tail = s;
var headLength = head.Length;
var length = headLength + s.Length;

for (var i = 0; i < _otherList.Count; i++)
if (s.StartsWith("remotes/"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the user of this comparer persist the data in sanitised form so we don't have to allocate temporary strings here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.microsoft.com/en-us/dotnet/api/system.string.intern

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

{
var other = _otherList[i];
s = "refs/" + s;
}
else
{
s = _prefix + s;
}

if (other.Length == length &&
other.StartsWith(head) &&
other.IndexOf(tail, headLength) == headLength)
{
return i;
}
if (_orderDict.TryGetValue(s, out var index))
{
return index;
}

return -1;
Expand Down