Skip to content

Sped up the Normalization by removing an unnecessary O(n^2) loop #2990

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

Merged
arturcic merged 1 commit into from
Feb 14, 2022
Merged

Sped up the Normalization by removing an unnecessary O(n^2) loop #2990

Show file tree
Hide file tree
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/GitVersion.Core/Core/GitPreparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,15 +314,14 @@ private void CreateOrUpdateLocalBranchesFromRemoteTrackingOnes(string remoteName
{
var remoteTrackingReferenceName = remoteTrackingReference.Name.Canonical;
var branchName = remoteTrackingReferenceName.Substring(prefix.Length);
var localCanonicalName = "refs/heads/" + branchName;
var localReferenceName = ReferenceName.FromBranchName(branchName);

var referenceName = ReferenceName.Parse(localCanonicalName);
Copy link
Member

@arturcic arturcic Feb 8, 2022

Choose a reason for hiding this comment

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

I think this line is needed - we need to parse the localCanonicalName

Copy link
Contributor Author

@ni-fgenois ni-fgenois Feb 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Are you sure? Is the ReferenceName.Parse doing any validation? Because we are not using referenceName anywhere

Copy link
Member

@arturcic arturcic Feb 9, 2022

Choose a reason for hiding this comment

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

Yes it does

GitVersion/src/GitVersion.Core/Git/ReferenceName.cs

Line 29 in 3626ce5

public static ReferenceName Parse(string canonicalName)

Copy link
Contributor Author

@ni-fgenois ni-fgenois Feb 9, 2022

Choose a reason for hiding this comment

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

The following validation snippet from ReferenceName.Parse : IsPrefixedBy(canonicalName, LocalBranchPrefix)
will match the constructed string "refs/heads/" + branchName;

What do you think about changing the construction of the string to ReferenceName.LocalBranchPrefix + branchName; instead?

Copy link
Contributor Author

@ni-fgenois ni-fgenois Feb 9, 2022

Choose a reason for hiding this comment

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

Or adding a factory method ReferenceName.LocalBranch(branchName)?

Copy link
Member

@asbjornu asbjornu Feb 10, 2022

Choose a reason for hiding this comment

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

The point is to get an instance of the class ReferenceName, which implements IComparabale and thus takes care of all reference comparison logic, such as disregarding refs/heads. Why do we only want to compare against only the local canonical name here?

Copy link
Contributor Author

@ni-fgenois ni-fgenois Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@asbjornu
Well, the problem here is actually using the IComparable interface for searching a matching IReference. Doing such a linear lookup leads to O(n) algorithmic complexity, and as we are already within a loop, this leads to a total O(n^2) algorithm complexity.

We should instead use an data structure with logarithmic lookups directly in order to take this first algorithmic complexity down to a O(1), and the whole tjomg to an O(n) algorithmic complexity.

Lucky for us, the IReferenceCollection class does provide such an access (IReference? this[string name] { get; }), but the key of that dictionnary seems to be a CanonicalName.

Copy link
Contributor Author

@ni-fgenois ni-fgenois Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@arturcic I've modified my code so we use a ReferenceName to query the IReferenceCollection structure. And I've added a factory method to ReferenceName

// We do not want to touch our current branch
if (this.repository.Head.Name.EquivalentTo(branchName)) continue;

if (this.repository.Refs.Any(x => x.Name.Equals(referenceName)))
if (this.repository.Refs[localReferenceName] is not null)
{
var localRef = this.repository.Refs[localCanonicalName]!;
var localRef = this.repository.Refs[localReferenceName]!;
if (localRef.TargetIdentifier == remoteTrackingReference.TargetIdentifier)
{
this.log.Info($"Skipping update of '{remoteTrackingReference.Name.Canonical}' as it already matches the remote ref.");
Expand All @@ -335,7 +334,7 @@ private void CreateOrUpdateLocalBranchesFromRemoteTrackingOnes(string remoteName
}

this.log.Info($"Creating local branch from remote tracking '{remoteTrackingReference.Name.Canonical}'.");
this.repository.Refs.Add(localCanonicalName, remoteTrackingReference.TargetIdentifier, true);
this.repository.Refs.Add(localReferenceName.Canonical, remoteTrackingReference.TargetIdentifier, true);

var branch = this.repository.Branches[branchName]!;
this.repository.Branches.UpdateTrackedBranch(branch, remoteTrackingReferenceName);
Expand Down
1 change: 1 addition & 0 deletions src/GitVersion.Core/Git/IReferenceCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ public interface IReferenceCollection : IEnumerable<IReference>
{
IReference? Head { get; }
IReference? this[string name] { get; }
IReference? this[ReferenceName referenceName] { get; }
void Add(string name, string canonicalRefNameOrObjectish, bool allowOverwrite = false);
void UpdateTarget(IReference directRef, IObjectId targetId);
IEnumerable<IReference> FromGlob(string prefix);
Expand Down
3 changes: 3 additions & 0 deletions src/GitVersion.Core/Git/ReferenceName.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ public static ReferenceName Parse(string canonicalName)
}
throw new ArgumentException($"The {nameof(canonicalName)} is not a Canonical name");
}

public static ReferenceName FromBranchName(string branchName) => Parse(LocalBranchPrefix + branchName);

public string Canonical { get; }
public string Friendly { get; }
public string WithoutRemote { get; }
Expand Down
2 changes: 2 additions & 0 deletions src/GitVersion.LibGit2Sharp/Git/ReferenceCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ public IReference? this[string name]
}
}

public IReference? this[ReferenceName referenceName] => this[referenceName.Canonical];

public IReference? Head => this["HEAD"];

public IEnumerable<IReference> FromGlob(string prefix) => this.innerCollection.FromGlob(prefix).Select(reference => new Reference(reference));
Expand Down