Skip to content

Exclude **/node_modules/** from .vscode/launch.json searches #1234

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

Merged
testforstephen merged 2 commits into from
Oct 19, 2022
+3 −1
Merged

Exclude **/node_modules/** from .vscode/launch.json searches #1234

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
432b3c4
Exclude **/node_modules/** from .vscode/launch.json searches
gluxon Oct 10, 2022
a54192d
Merge branch 'main' into ignore-node-modules-launch-search
testforstephen Oct 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/debugCodeLensProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,9 @@ async function launchJsonExists(workspace?: vscode.Uri): Promise<boolean> {
}

const workspaceFolder = vscode.workspace.getWorkspaceFolder(workspace);
const results: vscode.Uri[] = await vscode.workspace.findFiles(".vscode/launch.json");
// Excluding "**/node_modules/**" as a common cause of excessive CPU usage.
// https://github.com/microsoft/vscode/issues/75314#issuecomment-503195666
const results: vscode.Uri[] = await vscode.workspace.findFiles(".vscode/launch.json", "**/node_modules/**");

@testforstephen testforstephen Oct 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm OK with excluding both node_modules and .git.

@gluxon gluxon Oct 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! So to be clear, updating this argument **/node_modules/** to {**/node_modules/**,.git/**} would be ideal?

I'll test that out to make sure I have the right glob syntax.

@testforstephen testforstephen Oct 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be interesting if there is some performance comparison data to share.

@gluxon gluxon Oct 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks like the 2nd argument appends rather than replaces. The existing command in this PR:

await vscode.workspace.findFiles(".vscode/launch.json", "**/node_modules/**");

runs the following process:

'/Applications/Visual Studio Code.app/Contents/Resources/app/node_modules.asar.unpacked/@vscode/ripgrep/bin/rg' \
    --files \
    --hidden \
    --case-sensitive \
    -g '/.vscode/launch.json' \
    -g '!**/node_modules/**' \
    -g '!**/.git' \
    -g '!**/.svn' \
    -g '!**/.hg' \
    -g '!**/CVS' \
    -g '!**/.DS_Store' \
    -g '!**/Thumbs.db' \
    --no-ignore \
    --follow \
    --no-config \

The -g '!**/.git' exclusion comes from VS Code's default settings.

{
  "files.exclude": {
    "**/.git": true,
    "**/.svn": true,
    "**/.hg": true,
    "**/CVS": true,
    "**/.DS_Store": true,
    "**/Thumbs.db": true
  }
}

https://github.com/microsoft/vscode/blob/2798da9d47387c0fb6ab4ebbead95efb5a85c250/src/vs/workbench/contrib/files/browser/files.contribution.ts#L143

It would be interesting if there is some performance comparison data to share.

Definitely. I was curious too, but hadn't had time until now to run some tests.

Without the **/node_modules/** exclusion, the rg process runs for over an hour at 1100%+ CPU in the repo my team works on. I can run the process overnight to see exactly how long it takes if that's something we're curious about.

Here's some data for comparing whether .git is excluded. For my specific machine and repo:

node_modules Excluded .git Excluded Trial 1 Trial 2 Average
❌ ✅ 1 hour+ - 1 hour+
✅ ❌ 536ms 493ms 514.5ms
✅ ✅ 788ms 477ms 632.5ms

It looks like excluding .git is fairly negligible in terms of timing. Either way, I don't think we have to do something explicit here since .git should be excluded unless users explicitly set files.excludes["**/.git"] to false?

@testforstephen testforstephen Oct 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great to see the perf data, thanks for sharing that. Looks like it's enough to exclude "node_modules" only.

@jdneo jdneo Nov 1, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like this utility can get another improvement if we set the workspaceFolder as the base part of the relative pattern

https://github.com/microsoft/vscode/blob/4b759b0492d1df969e8595e3db0f66f6058c3643/src/vscode-dts/vscode.d.ts#L2116

And then limit the search maxResults to 1

https://github.com/microsoft/vscode/blob/4b759b0492d1df969e8595e3db0f66f6058c3643/src/vscode-dts/vscode.d.ts#L12171

@testforstephen testforstephen Nov 1, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I created an issue to track the further optimization. #1252

@gluxon gluxon Nov 6, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jdneo Those both look like great suggestions. Thanks!

return !!results.find((launchJson) => vscode.workspace.getWorkspaceFolder(launchJson) === workspaceFolder);
}

Expand Down