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

Switching to HashSet for the list of files to be prefetched to improv… #827

Merged
merged 2 commits into from Feb 23, 2019

Conversation

turbonaitis
Copy link
Contributor

@turbonaitis turbonaitis commented Feb 22, 2019

…e performance, when the list is long.

Addreses #826

Before this change, if we were prefetching a list of ~4k files, the traversal of that list in ShouldIncludeResult method took ~70ms. After the change, the lookup in the HashSet is almost instant (testing showed it to take 0ms)

Copy link
Member

@kewillford kewillford left a comment

Choose a reason for hiding this comment

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

Approved with one suggestion.

GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs Outdated Show resolved Hide resolved
Copy link
Member

@nickgra nickgra left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me too.
Your GitHub issue calls out some bad performance numbers that led you to make this change, can we update the PR description/commit with some before/after on how big of an improvement this change is?

@jrbriggs
Copy link
Member

Thanks for the contribution!

@jrbriggs jrbriggs merged commit e6fcae7 into microsoft:master Feb 23, 2019
@jrbriggs jrbriggs added this to the M149 milestone Feb 23, 2019
@turbonaitis turbonaitis deleted the fetchPerformance branch April 17, 2019 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants