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.

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

Improve performance - Filter list of all tags first by those parsable to a semantic version #1374

Merged
merged 3 commits into from
Mar 9, 2018

Conversation

jkingry
Copy link
Contributor

@jkingry jkingry commented Feb 16, 2018

  • reduces Getting version tags from branch time for our repository
    from a 100 seconds to 2.5 seconds

- reduces `Getting version tags from branch` time for our repository
  from a 100 seconds to 2.5 seconds
@jkingry
Copy link
Contributor Author

jkingry commented Feb 16, 2018

I didn't see any existing performance tests, I've attempted a few times to write one using System.Diagnostics.Stopwatch but haven't yet succeeded. Are you open to using something like NBench or similar benchmark framework?

@jkingry jkingry changed the title Filter list of all tags first by those parsable to a semantic version Improve performance - Filter list of all tags first by those parsable to a semantic version Feb 22, 2018
@asbjornu
Copy link
Member

asbjornu commented Mar 9, 2018

I haven't tried it, but I think BenchmarkDotNet looks good. I'd happily accept pull requests with a few benchmark tests in it, especially if it's hooked up with some online service that can report performance degradation as Github checks to pull requests.

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

Please attend to the review comments and rebase this PR onto the HEAD of master and it looks good to me.

})
.SelectMany(c => tags.Where(t => c.Sha == t.Target.Sha).SelectMany(t =>
var tags = new List<Tuple<Tag, SemanticVersion>>();
foreach(var t in this.Repository.Tags)
Copy link
Member

Choose a reason for hiding this comment

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

Can you please insert a space between foreach and (?

.Where(tag => !olderThan.HasValue || ((Commit) tag.PeeledTarget()).When() <= olderThan.Value)
.ToList();
var allTags = new List<Tuple<Tag, SemanticVersion>>();
foreach(var tag in context.Repository.Tags)
Copy link
Member

Choose a reason for hiding this comment

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

Can you please insert a space between foreach and (?

IncludeReachableFrom = branch.Tip
})
.SelectMany(c => tags.Where(t => c.Sha == t.Target.Sha).SelectMany(t =>
var tags = new List<Tuple<Tag, SemanticVersion>>();
Copy link
Member

Choose a reason for hiding this comment

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

Can you please extract this tag filter logic out to a descriptive method?

@asbjornu
Copy link
Member

asbjornu commented Mar 9, 2018

Seeing how this might fix #1361, I'll ignore what I've written in the review and just go ahead and merge. A followup PR that addresses the comments would be appreciated, though. Thanks for this!

@asbjornu asbjornu merged commit 31411e9 into GitTools:master Mar 9, 2018
@jkingry jkingry deleted the feature/tag-performance branch March 10, 2018 15:55
jkingry added a commit to jkingry/GitVersion that referenced this pull request Mar 12, 2018
jkingry added a commit to jkingry/GitVersion that referenced this pull request Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants