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

♻️ Use LSP based file watcher #1610

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

♻️ Use LSP based file watcher #1610

wants to merge 5 commits into from

Conversation

SPGoding
Copy link
Member

@SPGoding SPGoding commented Oct 5, 2024

Resolves #1414.

For testing purposes, the Output tab includes outputs prefixed with [FsWatcher] with the raw LSP changes, added files, changed files, and unlinked files, whenever file system changes are detected.

@SPGoding
Copy link
Member Author

SPGoding commented Oct 5, 2024

During testing I noticed there was a bug with VS Code's built-in file watcher:

  1. Create a file at a/b/c/file.txt
  2. Rename a to a2
  3. Rename c to c2

Expected Results:

Raw LSP changes should include

{
  changes: [
    {
      uri: 'file:///a2/b/c2',
      type: 1
    },
    {
      uri: 'file:///a2/b/c',
      type: 3
    }
  ]
}

Actual Results:

Raw LSP changes instead include

{
  changes: [
    {
      uri: 'file:///a/b/c2',
      type: 1
    },
    {
      uri: 'file:///a/b/c',
      type: 3
    }
  ]
}

Notice that folder a2's old name a is shown in the output. The new LspFsWatcher will be unable to send correct events as the path included in LSP raw changes does not actually exist.

@misode misode self-requested a review October 7, 2024 14:42
@misode
Copy link
Member

misode commented Oct 7, 2024

During testing I noticed there was a bug with VS Code's built-in file watcher: [...]

@SPGoding During testing I could not reproduce this issue. I correctly get the addition of 'file:///a2/b/c2', and removal of 'file:///a2/b/c'. Either this is OS-dependant or I am following the steps incorrectly.

Regardless of me being able to reproduce, is this a blocking issue meaning we can't go forward with this LSP based watcher approach? Or are there workarounds to resolve this?

Copy link
Member

@misode misode left a comment

Choose a reason for hiding this comment

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

In addition to solving the renaming bug on Windows, this also seems to greatly improve performance! While before it would take upwards of 60 seconds to load the Gamemode 4 repo (cached), it now only takes 16 seconds!

I experienced one issue when deleting a very large folder, with a OOM crash.

packages/core/src/common/externals/index.ts Outdated Show resolved Hide resolved
const watcherDisposable = await connection.client.register(
ls.DidChangeWatchedFilesNotification.type,
{
watchers: [{ globPattern: '**/*' }],
Copy link
Member

Choose a reason for hiding this comment

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

In #1607 Afro started working on being able to exclude certain patterns from the watcher. From testing, the watcher seems to be very performant so this may not be necessary, but I did see raw LSP changes in .git/FETCH_HEAD for example. Do these have potential to break anything? If we want to do any filtering, should we do it here or later in LspFsWatcher.onLspDidChangeWatchedFiles?

// Find all files under the deleted URI and send 'unlink' events for them as well.
for (const watchedUri of this.#watchedFiles) {
if (core.fileUtil.isSubUriOf(watchedUri, uri)) {
this.emit('unlink', watchedUri)
Copy link
Member

Choose a reason for hiding this comment

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

This seems like the biggest performance concern. I tried deleting a huge nested folder (2k+ files) in an already huge project (Gamemode 4, 45k+ files). Instead of sending a single Deleted change object, the vscode LSP watcher decided to send a changes list containing 2k+ elements. This doesn't happen when deleting normal sized folders. In the output log, I saw that it was logging a single unlink event roughly every 6 seconds, and it crashed with out of memory about a minute in.

Would it make sense to skip looping over all watched files to find nested files if the first condition (this.#watchedFiles.has(uri)) matches? If that's the case a file was deleted and no nested files should exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it make sense to skip looping over all watched files to find nested files if the first condition (this.#watchedFiles.has(uri)) matches?

That would be a nice change

I also think this could be potentially optimized further by using a tree structure to keep the #watchedFiles instead of using a flat string set, but that might involve some changes to Project as well.

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.

Cannot rename folder
2 participants