User data provider should not iterate over raw changes

[bpasero]

Looking at perf traces from #124723 I noticed how quite a bit of time can be spend here:

vscode/src/vs/workbench/services/userData/common/fileUserDataProvider.ts

Line 123 in 363534d

private handleFileChanges(changes: readonly IFileChange[]): void {

And I was wondering how it is possible that you can access the raw file changes until I noticed how you actually listen to the raw events here:

vscode/src/vs/workbench/services/userData/common/fileUserDataProvider.ts

Line 42 in 363534d

this._register(this.fileSystemProvider.onDidChangeFile(e => this.handleFileChanges(e)));

But doing so bypasses all our logic in the file service, e.g. to produce a ternary search tree for paths from the FileChangesEvent. I am also considering to add some debouncing logic for the processing of these raw events in case thousands of events are fired in short period of time. I cannot do this on the level of the provider though because that is API.

Can we please revisit that and see if the code would benefit from operating directly on FileChangesEvent? It provides powerful methods like contains and affects that can be used to lookup with a resource.

[sandy081]

@bpasero I can definitely debounce the listener. But I am not sure if I can avoid iterating because, currently I am filtering for user data resources and map them to user data scheme. I can skip the filter and just do mapping.

[bpasero]

@sandy081 remind me again why the FileUserDataProvider needs to depend on the local fileSystemProvider? Can you not use IFileService.onDidChangeFile simply to listen to changes?

[sandy081]

I use vscode-userdata scheme for user data resources (settings, keybindings...) which needs to have a FSP registered for this scheme. FileUserDataProvider is just a wrapper on top of local fileSystemProvider that is just forwarding calls and events by changing the scheme.

[bpasero]

Could you listen to the changes via IFileService method and look at the scheme being file to do the same thing instead of listening to the events of the provider? You can then emit the events from your provider after having listened. Would just have to make sure that you are not ending up in an endless loop (i.e. when you emit events, ignore file changes from the file service).

[sandy081]

I think I can actually remove this FileUserDataProvider because it is used only in Desktop and I can use local file scheme and local filesystemprovider.

[bpasero]

👏

[sandy081]

I am using userdata scheme in Web and using the same in Desktop is providing good abstraction in the code. We check at various places in code for this scheme and handle things differently just like file scheme, for eg undo. If you think this change is worth that it improves perf efficiently, I would remove it. Otherwise I would focus on following improvements:

• Debouncing the event listening

Could you listen to the changes via IFileService method and look at the scheme being file to do the same thing instead of listening to the events of the provider?

Not sure how this is different from what I am doing now ?

[bpasero]

Not sure how this is different from what I am doing now ?

The file service has logic to debounce when spam occurs, so we do not fall into the trap of reacting to thousands of events:

vscode/src/vs/platform/files/common/fileService.ts

Lines 977 to 982 in 3252199

	private static readonly FILE_EVENTS_THROTTLING = {
	maxChangesChunkSize: 500 as const, // number of changes we process per interval
	maxChangesBufferSize: 30000 as const, // total number of changes we are willing to buffer in memory
	coolDownDelay: 200 as const, // rest for 100ms before processing next events
	warningscounter: 0 // keep track how many warnings we showed to reduce log spam
	};

[sandy081]

Could you listen to the changes via IFileService method and look at the scheme being file to do the same thing instead of listening to the events of the provider? You can then emit the events from your provider after having listened.

It seems FileChangesEvent does not have access to changes for me to iterate and trigger change event on the resources I care.

[bpasero]

There is:

vscode/src/vs/platform/files/common/files.ts

Lines 772 to 786 in 33c149f

	/**
	* @deprecated use the `contains` or `affects` method to efficiently find
	* out if the event relates to a given resource. these methods ensure:
	* - that there is no expensive lookup needed (by using a `TernarySearchTree`)
	* - correctly handles `FileChangeType.DELETED` events
	*/
	get rawAdded(): TernarySearchTree<URI, IFileChange> | undefined { return this.added; }
	/**
	* @deprecated use the `contains` or `affects` method to efficiently find
	* out if the event relates to a given resource. these methods ensure:
	* - that there is no expensive lookup needed (by using a `TernarySearchTree`)
	* - correctly handles `FileChangeType.DELETED` events
	*/
	get rawDeleted(): TernarySearchTree<URI, IFileChange> | undefined { return this.deleted; }

We can add rawUpdated back.

[bpasero]

See #139793