Performance Improvement #28

Merged
merged 2 commits into from May 14, 2012

Conversation

Projects
None yet
2 participants
@nufyoot
Contributor

nufyoot commented May 11, 2012

Hey Phil Haack! I love this project and have found it to be a great tool for visualizing our local Git repos. I noticed that strarting up the application would take a while, especially if there were a few merged branches. I added this section of code and compared the results of our local repos along with your SeeGit repo and the visual graph matches. For your SeeGit repo, AddCommitsToGraph was cut down from 94,199 calls to 108.

Let me know what you think!

If an edge already exists, don't run through the commit graph again. …
…This should speed up the loading of the graph.
@Haacked

View changes

SeeGitApp/Models/RepositoryGraphBuilder.cs
@@ -93,6 +93,10 @@ private void AddCommitsToGraph(Commit commit, CommitVertex childVertex)
_graph.AddEdge(edge);
_edges.Add(edge.Id, edge);
}
+ else
+ {

This comment has been minimized.

@Haacked

Haacked May 14, 2012

Owner

If we add an else case here, we should invert the if statement. I find:

if (!something)
{
}
else
{
}

Harder to read and grok than:

if (something)
{}
else
{}

Also, at first glance, I'm not sure how this works properly. Is it because if the edge exists, we must have already added this commit and its parents?

@Haacked

Haacked May 14, 2012

Owner

If we add an else case here, we should invert the if statement. I find:

if (!something)
{
}
else
{
}

Harder to read and grok than:

if (something)
{}
else
{}

Also, at first glance, I'm not sure how this works properly. Is it because if the edge exists, we must have already added this commit and its parents?

@nufyoot

This comment has been minimized.

Show comment
Hide comment
@nufyoot

nufyoot May 14, 2012

Contributor

That's a good point. In that case, would you keep the else since the if would just return? Example:

if (_edges.ContainsKey(edge.Id))
{
    return;
}
_graph.AddEdge(edge);
_edges.Add(edge.Id, edge);

As for the reason. For each branch in the graph it will ask to add an edge from commit A to commit B and from commit A to commit C (given that A was a merge of B and C). When the ancestors of B and C come back to D (given that D is a common ancestor of B and C) then both B and C have started a graph walk and will walk the graph twice. The larger the repository, the worse this would become.

So, using A, B, C and D from above, and introducing E (where E is an ancestor of D) then this code states "If you've already added an edge from D to E (perhaps A -> B -> D -> E) then we don't need to walk the graph past E (perhaps A -> C -> D -> E)."

I hope I have this correct. Also, I would love to contribute more to this project if it's alright with you!

Contributor

nufyoot commented May 14, 2012

That's a good point. In that case, would you keep the else since the if would just return? Example:

if (_edges.ContainsKey(edge.Id))
{
    return;
}
_graph.AddEdge(edge);
_edges.Add(edge.Id, edge);

As for the reason. For each branch in the graph it will ask to add an edge from commit A to commit B and from commit A to commit C (given that A was a merge of B and C). When the ancestors of B and C come back to D (given that D is a common ancestor of B and C) then both B and C have started a graph walk and will walk the graph twice. The larger the repository, the worse this would become.

So, using A, B, C and D from above, and introducing E (where E is an ancestor of D) then this code states "If you've already added an edge from D to E (perhaps A -> B -> D -> E) then we don't need to walk the graph past E (perhaps A -> C -> D -> E)."

I hope I have this correct. Also, I would love to contribute more to this project if it's alright with you!

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked May 14, 2012

Owner

It is definitely cool with me if you want to contribute more. :) I'm going to be spending more time on this soon and would love the help.

Owner

Haacked commented May 14, 2012

It is definitely cool with me if you want to contribute more. :) I'm going to be spending more time on this soon and would love the help.

Haacked added a commit that referenced this pull request May 14, 2012

Merge pull request #28 from nufyoot/master
Performance Improvement

@Haacked Haacked merged commit f921d40 into Haacked:master May 14, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment