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

Avoid expensive check with --files #603

Merged
merged 1 commit into from
Sep 18, 2017

Conversation

chrmarti
Copy link
Contributor

@chrmarti chrmarti commented Sep 8, 2017

Fixes #600

Stacktraces show that this check is the culprit. Listing all files in my test folder with 235k files on Windows now takes about 4.2 seconds. It didn't finish before I canceled before that when MsMpEng.exe kicked in.

The stacktrace:
image

/cc @BurntSushi @roblourens

@roblourens
Copy link
Contributor

Excellent detective work @chrmarti! A little background @BurntSushi, with this change we will be able to use ripgrep to power quick open in VS Code as well.

@chrmarti
Copy link
Contributor Author

The Mac CI build failed with an empty log.

@BurntSushi
Copy link
Owner

@chrmarti Nice detective work! I think my key issue here is that this only fixes the problem when using the --files flag, but based on your observations, it sounds like searching the files will result in a tremendous bottleneck.

So I'm not quite sure what to do. It seems like we should try to fix the root issue, but maybe it would be a good idea to just take this in for now?

@roblourens
Copy link
Contributor

I think Defender is doing the "right thing" here, and in text search, by scanning files when they are opened. It does cache results when possible, so this won't happen every time, but it will happen often. That was basically the result of our discussion with the Defender team. So I think the best thing we can do for now is to remove an unnecessary file open when using --files.

@BurntSushi
Copy link
Owner

@roblourens Hmm all right, that's good enough for me!

@BurntSushi BurntSushi merged commit 1136f8a into BurntSushi:master Sep 18, 2017
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.

3 participants