Closed roblourens closed 5 years ago
I suggest we do the following:
Analyse how many extensions in the marketplace use workspaceContains glob patterns and take a look if anyone of them would be negatively impacted if we would include the search excludes in there.
@sandy081 is looking into some tooling for such use cases.
@sandy081 can vscode-tools help us analyze this yet?
It would really help if a workspaceContains search would skip files.exclude
. But in theory, extensions that activate on SCM files, particular node_modules being present, etc are valid use cases, so I'm curious whether extensions like that actually exist.
We also have https://github.com/Microsoft/vscode/issues/33528 to look at this for findFiles
@roblourens Yes. It supports querying on activationEvent by passing a regEx value. For example
activationEvents:`^workspaceContains.*\\*.csx\`
will give you extensions activating on workspace containing .csx
files.
Thanks! I only see one that depends on **/node_modules
. A few others look in node_modules, but with no glob patterns, so it can be done without a search. But I guess it should be consistent whether glob patterns are present or not. One is actually mine, which just activates when a ./node_modules folder is present at all. I don't see any looking at .git/
Is this considered for November 2017? This would be important for me.
I wonder if there is a general way to limit the amount of resources workspaceContains
consumes. While useful for medium sized workspaces, it currently is too expensive for large ones.
One option might be to stop the search after some time (e.g., 10s) and treat it as no match (to avoid grabbing more resources).
@chrmarti I like your suggestion.
I would also add, perhaps there is a depth setting we could use, i.e. search 4 folder levels in, and not deeper, etc.
There is a max depth parameter for Ripgrep. The downside of that would be that it would break scenarios with few files in a deep folder structure. In other cases it might not change much, because already 4 levels can be too many files.
Maybe using file.excludes
and search.exclude
for filtering and a 10 seconds limit would improve things by using the knowledge in the settings about what is of interest while also having a fallback if that is too slow.
We then need to document that workspaceContains
should not be relied on exclusively, which is fair considering how much resources it can take.
Yes this is exactly what I want. If you are using the C# extension, and for some reason we can't find a single csproj or .cs file in 10 seconds, then you'll probably open a csharp file at some point (activating the extension by onLanguage) or run a csharp command. If none of this, you probably won't benefit from the extension.
We could also check opened files against the workspaceContains patterns, I don't think we do that currently.
@chrmarti Maybe opposite of that, I mean let we have search.include
instead
@Thaina search.include
wouldn't have sensible defaults, so it would only be of help here if the user configured it which is asking too much from the user for fixing this issue. search.include
might make sense on its own, you could open a new issue to track this as a feature request.
If you can't limit the scope of the search cache or persist it to disk, please add a setting to disable the cache entirely. #34487 is not an “improvement” in a repo with half a million files after npm install
. Rather: to solve a problem I didn't have, it's burning my battery for five minutes while chewing 2GB RAM each time the language server misses a change and forces me to reload.
@alexandrudima we can't change the behavior of workspaceContains overnight because it would require extensions to have a fallback activationEvent. Could we track workspaceContains searches that are terminated early, then each time a file is opened, check it against those patterns? Does that make sense?
We could try to limit the search to files that are 1 or 2 levels deep. I intuitively think 2 levels deep should cover most workspaceContains
cases.
The minimal change here would be to add a timeout to have a hard limit on the resource consumption and a fallback to matching opened files against the pattern when the timeout hit to avoid regressions.
Limiting the depth of the search and using the file.excludes
and search.exclude
settings to prune the traversal are improvements on top.
So this is being tackled for the August release?
I wonder if any extensions use workspaceContains to look for a more specific pattern? foo/bar/**/*.file
. That would be defeated by a depth limit. I like the timeout instead of the depth limit because
I would hesitate to apply files.exclude
or search.exclude
because that could break extensions. We might want to give extensions more warning on a change like that.
I don't think applying the exclusions is essential. Having the fallback you suggest that matches opened files against the patterns that had their search terminated by the timeout seems more important (and could also be used to buffer other changes like the exclusions).
What I don't like a lot about the timing suggestion is ... the timing aspect. Imagine you are an end user and half the times you open a specific folder an extension works and half the times it doesn't... But I guess we can live with that for the sake of being fast...
I suggest we use the existing body of extensions to drive this. Thanks to @jrieken here are the extensions that use workspaceContains
:
I've taken the time to select only those that use a glob pattern so we can look over them together. I've also taken the time to group them in "simple file searches" and "problematic":
Problematic:
arcsine.travetto-test-plugin/package.json
47,6: "workspaceContains:**/node_modules/@travetto/test"
carlowenig.angular-explorer/package.json
20,10: "workspaceContains:**/.angular-cli.json",
21,10: "workspaceContains:**/node_modules/@angular/core/package.json"
Orta.vscode-react-native-storybooks/package.json
18,6: "workspaceContains:**/node_modules/@storybook/react-native/package.json"
I would like to suggest that if we touch the behaviour of workspaceContains
we do it in one go and not change it every few months. i.e. if we are tackling its performance and changing its semantics I think we should do it entirely in one milestone. I suggest we do both ignoring of exclude patterns and the time limit in one go. This should make it less likely that the time limit is hit.
The download counts for the problematic ones are 166 + 271 + 22K. Their activation events are:
arcsine.travetto-test-plugin/package.json
"activationEvents": [
"onLanguage:typescript",
"onCommand:extension.triggerDebug",
"onCommand:extension.triggerDebugKey",
"workspaceContains:**/node_modules/@travetto/test"
]
carlowenig.angular-explorer/package.json
"activationEvents": [
"onView:angularExplorer"
]
Orta.vscode-react-native-storybooks/package.json
"activationEvents": [
"onCommand:extension.showStorybookPreview",
"workspaceContains:**/node_modules/@storybook/react-native/package.json"
],
The first one looks OK, it will get activated quite quickly if you open a TS file. The second one has since changed the version of package.json
on master and gave up on workspaceContains
. I think the only problematic one is the third one. I suggest we quickly reach out to the third extension and let them know that they will be broken by our optimizations.
TL;DR
.git
, node_modules
, whatever is configured)Orta.vscode-react-native-storybooks
and let them know they will be impacted@roblourens @chrmarti Is that OK with you?
What about aborting the search as soon as all lookups are resolved?
What about aborting the search as soon as all lookups are resolved?
That is already the case today. As soon as a single result is found, the search is stopped.
Other extensions could be implicitly depending on files only found under an excluded path, and they will no longer be activated.
This one will break too:
PhilAlsford.git-duet-vscode/package.json
19,10: "workspaceContains:.git/*/**"
I wonder if we should skip files.exclude
but not search.exclude
? I think of the first as "these aren't really part of my project" and the second as "these are not source files that I'm constantly working in". Someone might have important project config files under search.exclude.
Activating when a certain node module is installed seems like a valid scenario. The two above would have to activate on * instead, or just onCommand.
Other extensions could be implicitly depending on files only found under an excluded path, and they will no longer be activated.
But that is the whole point of my previous comment. We searched through extensions that use workspaceContains
and looked at the ones using globs and picked up the ones that will probably be impacted. The data might not be 100% up-to-date (@jrieken's download was not 100% up-to-date), so please try to grep for workspaceContains
on your data and find more extensions that will be impacted.
I agree about PhilAlsford.git-duet-vscode
, we missed that, @jrieken's download was not super up-to-date, we should reach out to this author too. But the impact is small (83 installs).
Activating when a certain node module is installed seems like a valid scenario
This can be done via a file
probe, that goes on a different code path than disk search. i.e. workspaceContains:node_modules/myModule/package.json
. This file will be probed directly using fs
and not with DiskSearch
.
I don't know which one of the excludes to choose. I would say search.exclude
because that seems to be the configuration for searching and we are technically launching a search.
If nothing works, then yes, extensions should activate on *
. The whole point of activation events is that we save CPU time by not activating extensions eagerly, but if the workspaceContains
activation event ends up costing 1 minute of CPU, then we might as well have activated that extension on start-up, and it would have probably consumed 10-100ms of CPU by activating... So IMHO we need to be aggressive here and exclude as much as possible, and then institute a 5second limit, and if the time limit is hit, then we should just give up and activate the extension...
The default for search is to follow symlinks, that doesn't combine well with ignoring the exclude patterns when package managers like cnpm
extensively use symlinks to minimize disk usage. Ignoring symlinks just for workspaceContains
might help, but would also be confusing, ignoring symlinks in general might break too many users.
Should we consider deprecating glob patterns for workspaceContains
and just activate these extensions as if they activate on *
?
@chrmarti
Should we consider deprecating glob patterns for
workspaceContains
and just activate these extensions as if they activate on*
?
My opinion is to not give up, and continue to have workspaceContains
with glob patterns. But the search will be limited to whatever the search UI searches. If you can't find it in the UI, then an extension trying to activate will not find it. Extensions that are ok with this limitation (probably 95%) can continue to use workspaceContains
. Extensions which are not ok with this limitation can move to use *
.
Sounds good. We could in addition serialize the glob searches, currently all (on for each extension with workspaceContains
) are run in parallel.
We could in addition serialize the glob searches, currently all (on for each extension with
workspaceContains
) are run in parallel.
Should we make that into a separate issue. I am not quite sure how to tackle that. Today, the logic is the following:
workspaceContains
with at least a glob patternDiskSearch
query and ask it to stop at the first match with any of the glob patternsIf we were to launch a single DiskSearch
query for multiple extensions we would not know which of the extensions to activate.
Not sure how to combine the searches, my suggestion is to simply run one glob search after the other instead of all at the same time. That might take some pressure from already slow machines (we might need to lower parallelization in Ripgrep to truly achieve that, @roblourens?). With that we could start the timer once and when it finishes just activate all extensions that haven't had their search run yet.
I think that one rg process will use about all the CPU that it can, so I don't know if running just one at a time is that much better than running a few at a time.
With that we could start the timer once and when it finishes just activate all extensions that haven't had their search run yet.
This is possibly a less risky way to fall back than checking opened files, although you could end up activating a lot of extensions that you didn't mean to.
Telling rg to use fewer threads is also something we can do. I don't know how that fits into the extension api though.
Simple example of the above: https://github.com/Microsoft/vscode/pull/57047
Applying the exclude patterns is easier with https://github.com/Microsoft/vscode/issues/57046
I don't know whether we still want to do this for August this late? We could add some telemetry around how long these searches are taking and use that to help decide what the timeout value should actually be.
I think with #57046 we'll get that telemetry anyway
Just to add to the overall picture: @reggiew2, the reporter of #55649, saw the issue when opening the home folder and the reported hardware looks similar to mine, so I ran the following measurements with (the default) and without following symlinks on my home folder (hoping that would have a somewhat similar structure to the reported one):
time (rg --files --follow 2>/dev/null | wc -l)
14974573 files in 81.09 seconds
time (rg --files 2>/dev/null | wc -l)
1474396 files in 10.538 seconds
Not following symlinks ("search.followSymlinks": false
) might well help in this case.
I forgot: Ripgrep's default is to use .gitignore
files, so here are the same measurements with that turned off which is what workspaceContains
does:
time (rg --files --no-ignore --follow 2>/dev/null | wc -l)
17349358 files in 83.72 seconds
time (rg --files --no-ignore 2>/dev/null | wc -l)
2225053 files in 28.880 seconds
Yeah we can use the configured values of those settings to follow what someone said above, if normal search can find it, workspaceContains will find it.
I think we should put this in the first week of September so it has plenty of time in Insiders, unless anyone has any objection.
Fixed in https://github.com/Microsoft/vscode/pull/57047
Should use the user's excludes, and configured "use ignore files" and "follow symlinks" settings, and searches are limited to 5s, after which the extension is activated anyway.
The possible downside is that you activate extensions which actually shouldn't be activated. Hopefully this will be ok.
I've noticed extensions being activated from timeout when my macbook is doing a lot. If I set "window.restoreWindows": "all"
and reopen vscode with several windows, then sometimes one will have an extension activated from timeout.
I added telemetry so we can see how much this happens in the wild
Besides what we can see in telemetry, I want to get an idea of when this will happen. I published an extension that does nothing but show a notification when it's activated, and is activated on a workspaceContains pattern that doesn't exist. If anyone wants to help out, you can install this extension https://marketplace.visualstudio.com/items?itemName=roblourens.vscode-workspacecontains-canary and take note when you see the notification.
If you open a very large workspace, or many windows all at once, or something like that, then you will see the notification and that is expected and by design. But if you see it when it's not expected, then you can take note of what else your computer was doing at the time that might have slowed search down. Then let me know if you see it happening at random times, and if so, we may want to raise the limit by a couple seconds, or tweak it somehow.
For example, I was thinking that instead of measuring 5s in extensionHostMain where we look at the workspaceContains pattern, we might want to include a timeout as a search option and measure closer to where the search is actually performed. Not across the extension API boundary but if we measure at the point where a search provider is invoked, that will avoid a couple IPC hops that could be blocked when the computer is very busy.
From my understanding, this is how it's supposed to work:
"workspaceContains:**/node_modules/express/package.json"
**/node_modules
excluded in files.exclude
, the extension should not activate. However, in testing, this is not the case.node_modules
specified in .gitignore
and "search.useIgnoreFiles": true
, the extension should not activate. However, in testing, this is not the case.When I use Ripgrep, I get roughly 4 - 5 individual rg.exe
in Task Manager,each roughly taking 5 - 15% CPU (Making my usage go to a constant 77% and slowing my PC heavily) and between 5 - 30 MB RAM each
VSCode 1.29.0-exploration (observed this in insiders) Windows 10 1803 17134.345
Can you open a new issue with more details, because it's not clear what you mean and probably isn't relevant to this issue
https://github.com/Microsoft/vscode/issues/34487 made one improvement by batching workspaceContains searches.
Keeping this one open to consider excluding .git and node_modules in a smart way, which could give a huge improvement.
cc @jrieken @alexandrudima