Skip to content

feature: make number of ripgrep threads configurable #213511

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 69 commits into from
Jun 6, 2024
Merged

feature: make number of ripgrep threads configurable #213511

merged 69 commits into from
Jun 6, 2024

Conversation

SimonSiefke
Copy link
Contributor

fixes #206030.

Testing

For testing, I used the process explorer to see the ripgrep args being used, e.g. if the --threads argument is passed correctly to ripgrep.

With many threads

In settings.json, search.threads is commented out. By default, multiple threads are used. The ripgrep cli args don't include the --threads option:

with-many-threads

With one thread

In settings.json, search.threads is set to 1. The ripgrep cli args include --threads 1:

with-one-thread

gjsjohnmurray
gjsjohnmurray approved these changes May 26, 2024
View reviewed changes
Copy link
Contributor

@gjsjohnmurray gjsjohnmurray left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM but unit test failures need fixing.

andreamah
andreamah suggested changes May 30, 2024
View reviewed changes
Copy link
Contributor

@andreamah andreamah left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Just some comments.

SimonSiefke and others added 16 commits May 30, 2024 21:48
@SimonSiefke
Merge branch 'main' into feature/ripgrep-threads
1f9028d
@SimonSiefke
update description
5a3d152
@SimonSiefke
remove unused code
aa93896
@SimonSiefke
remove unused code
118a9a6
@SimonSiefke @andreamah
Update src/vs/workbench/services/search/node/ripgrepFileSearch.ts
7977989 
Co-authored-by: Andrea Mah <31675041+andreamah@users.noreply.github.com>
@SimonSiefke @andreamah
Update src/vs/workbench/services/search/node/ripgrepTextSearchEngine.ts
e10e9ae 
Co-authored-by: Andrea Mah <31675041+andreamah@users.noreply.github.com>
@SimonSiefke
use ICommonQueryBuilderOptions
dbf93b1
@SimonSiefke
set threads property from caller
a8df3e8
@SimonSiefke
simplify code
782aaba
@SimonSiefke
Discard changes to src/vs/workbench/contrib/search/browser/anythingQu…
e35ce48 
…ickAccess.ts
@SimonSiefke
Discard changes to src/vs/workbench/contrib/searchEditor/browser/sear…
d1b74c8 
…chEditor.ts
@SimonSiefke
remove unused code
c1f070b
@SimonSiefke
Discard changes to src/vs/workbench/services/search/common/textSearch…
ddc3155 
…Manager.ts
@SimonSiefke
add back code
a3d2c37
@SimonSiefke
fix types
953862f
@SimonSiefke
update formatting
02f2a65
andreamah
andreamah reviewed Jun 4, 2024
View reviewed changes
andreamah
andreamah suggested changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@andreamah andreamah left a comment

Choose a reason for hiding this comment

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

Should be the last round of comments, thanks so much for making changes so quickly!

@andreamah
Copy link
Contributor

Okay, I'm testing it now- I realize that it says Num Threads here, which isn't the best
image

Could you change the outward-facing setting to maxThreads? Max Threads might look better.

andreamah
andreamah reviewed Jun 4, 2024
View reviewed changes
src/vs/workbench/api/node/extHostSearch.ts Outdated
@ILogService _logService: ILogService,
) {
super(extHostRpc, _uriTransformer, _logService);

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'));
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I changed it to read the configuration value when the search starts so that always the latest configuration value is used.

@andreamah
Copy link
Contributor

Should

vscode/src/vs/workbench/services/search/node/rawSearchService.ts

Line 125 in cbffb5d

const engine = new EngineClass(config);
also pass through numThreads?

@andreamah
Copy link
Contributor

I noticed that the string of added args ends up being
--threads '3'
Should the number end up being a string?

@andreamah
Copy link
Contributor

I noticed that the string of added args ends up being --threads '3' Should the number end up being a string?

I see both work on rg, so nvm on that.

@SimonSiefke
Copy link
Contributor Author

Should vscode/src/vs/workbench/services/search/node/rawSearchService.ts also pass through numThreads?

Yes, great find! I added the property there also!

andreamah
andreamah approved these changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@andreamah andreamah left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for sticking with me on all my requested changes 🚀

@vscodenpa vscodenpa added this to the June 2024 milestone Jun 6, 2024
@andreamah andreamah merged commit 0036990 into microsoft:main Jun 6, 2024
6 checks passed
@andreamah andreamah mentioned this pull request Jun 24, 2024
Closed
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Jul 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lramos15 lramos15 lramos15 approved these changes

@gjsjohnmurray gjsjohnmurray gjsjohnmurray approved these changes

@andreamah andreamah andreamah approved these changes

Assignees

@andreamah andreamah

Labels
None yet
Projects
None yet
Milestone
June 2024
Development

Successfully merging this pull request may close these issues.

ripgrep file search uses too many threads
5 participants
@SimonSiefke @andreamah @lramos15 @gjsjohnmurray @vscodenpa