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.

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

chore: add --inspect-file-watcher for watcherService #118771

Closed
wants to merge 1 commit into from
Closed

chore: add --inspect-file-watcher for watcherService #118771

wants to merge 1 commit into from

Conversation

deepak1556
Copy link
Collaborator

Refs #118613

@deepak1556 deepak1556 requested a review from bpasero March 12, 2021 00:00
@deepak1556 deepak1556 self-assigned this Mar 12, 2021
@deepak1556 deepak1556 mentioned this pull request Mar 12, 2021
Closed
bpasero
bpasero requested changes Mar 12, 2021
View reviewed changes
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

If we change DiskFileSystemProvider to accept the native environment service you might also need to update our remote server where the provider is used.

Can I learn more about the rationale of this change? Why do we need to debug the watcher exactly?

src/vs/platform/files/node/diskFileSystemProvider.ts
@@ -48,6 +51,7 @@ export class DiskFileSystemProvider extends Disposable implements

constructor(
private readonly logService: ILogService,
private readonly envArgs?: NativeParsedArgs,
Copy link
Member

Choose a reason for hiding this comment

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

I would have rather expected to depend on INativeEnvironmentService here.

src/vs/platform/files/electron-browser/diskFileSystemProvider.ts Show resolved Hide resolved
src/vs/platform/files/node/diskFileSystemProvider.ts Show resolved Hide resolved
@deepak1556
Copy link
Collaborator Author

deepak1556 commented Mar 12, 2021 •
edited
Loading

Can I learn more about the rationale of this change? Why do we need to debug the watcher exactly?

When we get report of high cpu usage with file watcher, I don't have a way to get JS cpu profile to understand whats happening in the process, it would also help to identify duplicates of #3998

@bpasero
Copy link
Member

bpasero commented Mar 12, 2021 •
edited
Loading

@deepak1556 well at least for when chokidar is used (macOS, Linux, single folder only) the explanation is quite simple: chokidar does a full folder scan over all files and folders right on startup. The CPU should eventually settle though and not stay high. If it stays high, this would hint at slow polling being used which I think @aeschli is doing in remote, maybe he can clarify.

@deepak1556
Copy link
Collaborator Author

Thanks for the context!

For the cases with constant high cpu usage and is not remote scenario, what is usual flow of debugging ? I was mainly targeting this PR for those unknown scenarios.

@bpasero
Copy link
Member

bpasero commented Mar 12, 2021

@deepak1556 yeah for those cases we have no way of debugging, so we can use this change to create perf profiles I guess?

@deepak1556
Copy link
Collaborator Author

Yep thats correct

@deepak1556 deepak1556 closed this Mar 23, 2022
@deepak1556 deepak1556 deleted the robo/inspect_watcher branch March 23, 2022 15:54
@github-actions github-actions bot locked and limited conversation to collaborators May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bpasero bpasero bpasero requested changes

Assignees

@deepak1556 deepak1556

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@deepak1556 @bpasero