feature: make number of ripgrep threads configurable#213511
feature: make number of ripgrep threads configurable#213511andreamah merged 69 commits intomicrosoft:mainfrom
Conversation
andreamah
left a comment
andreamah
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Just some comments.
src/vs/workbench/services/search/node/ripgrepTextSearchEngine.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrea Mah <31675041+andreamah@users.noreply.github.com>
Co-authored-by: Andrea Mah <31675041+andreamah@users.noreply.github.com>
andreamah
left a comment
andreamah
left a comment
There was a problem hiding this comment.
Should be the last round of comments, thanks so much for making changes so quickly!
src/vs/workbench/services/search/common/searchExtTypesInternal.ts
Outdated
Show resolved
Hide resolved
|
Okay, I'm testing it now- I realize that it says Could you change the outward-facing setting to |
|
|
||
| const outputChannel = new OutputChannel('RipgrepSearchUD', this._logService); | ||
| this._disposables.add(this.registerTextSearchProvider(Schemas.vscodeUserData, new RipgrepSearchProvider(outputChannel))); | ||
| this._numThreadsPromise = _configurationService.getConfigProvider().then(provider => provider.getConfiguration('search').get<number>('ripgrep.numThreads')); |
There was a problem hiding this comment.
I just tested this out, and it seems that it won't update correctly if I change the setting. This is probably because the configuration is initialized once here and isn't updated when changed. I think you should be able to listen to onDidChangeConfiguration on ExtHostConfigProvider to listen for this.
There was a problem hiding this comment.
Good point! I changed it to read the configuration value when the search starts so that always the latest configuration value is used.
|
Should also pass throughnumThreads?
|
|
I noticed that the string of added args ends up being |
I see both work on |
Yes, great find! I added the property there also! |
andreamah
left a comment
andreamah
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks for sticking with me on all my requested changes 🚀
| ) { | ||
| super(extHostRpc, _uriTransformer, _logService); | ||
|
|
||
| this.getNumThreads = this.getNumThreads.bind(this); |
There was a problem hiding this comment.
@SimonSiefke @lramos15 what is the idea behind this code?
6 participants
fixes #206030.
Testing
For testing, I used the process explorer to see the ripgrep args being used, e.g. if the
--threadsargument is passed correctly toripgrep.With many threads
In
settings.json,search.threadsis commented out. By default, multiple threads are used. The ripgrep cli args don't include the--threadsoption:With one thread
In
settings.json,search.threadsis set to 1. The ripgrep cli args include--threads 1: