Skip to content

MarkerService improvement: Origins#295122

Open
n-gist wants to merge 3 commits intomicrosoft:mainfrom
n-gist:markerService-origins
Open

MarkerService improvement: Origins#295122
n-gist wants to merge 3 commits intomicrosoft:mainfrom
n-gist:markerService-origins

Conversation

@n-gist
Copy link
Copy Markdown
Contributor

@n-gist n-gist commented Feb 13, 2026

This PR aims to improve markerService internal management by adding a third dimension, origin, to the existing 'resource' and owner dimensions

issue, source of intention #291739 (comment)

Currently, some marker providers and consumers have to use complex logic for updating/reading markers
For example, problem matchers update markers based on textModel open/close state for the given URI: AbstractProblemCollector.shouldApplyMatch(). When textModel is closed, problemCollectors resend markers, assuming that markerServer storage for the given text model may have been under the control of another marker provider
Another example is how MainThreadDiagnostics has to read markers and push them back filtered on MainThreadDiagnostics.dispose(), actually manipulating markers that do not belong to it

A specific case is when a document is opened by EH: window.openTextDocument API and closed - textModel not disposed immediately, resulting in not actual diagnostics information in markerService and Problems View

origin goals

origin dimension in markerService storage intends to achieve
1. Allow marker providers to push their updates without worrying about other providers activity
2. As a result, always up-to-date markers storage
3. Ability to change the currently active 'origin' without delays or performance impact
4. Give language extensions an option to switch from onDidOpen/CloseTextDocument events that are tied with textDocument instantiation/disposal, to tabGroups events, to make diagnostic information responsive based on the file editing state in UI
5. Makes it possible to resolve potential collisions when more than one extension declares itself a provider of similar markers

provisions of changes of MarkerService

origin dimension is added to MarkerService._data, which is renamed to _backyard and is still main storage of markers
_facade storage is added for maintaining an active slice of _backyard, a cache for MarkerService.read performance, and represents a visible from the outside collection of markers
'origin' property is removed from marker objects and added as argument to markerService methods
origin is string | undefined, representing two priority levels, where undefined is the lowest, and any strings are treated equally prioritized over undefined. At the moment, string is used only by MainThreadDiagnostics.extHostId. This does not resolve 'extension vs extension' yet but is sufficient for 'extension vs task problem-matcher' collision
_facade origins switching is automatic and based on origins of incoming markers. Main mechanics are concentrated in new _updateOriginForOwnerResource() and _removeOriginForOwnerResource() methods, which are good review starting points
_onMarkerChanged is fired for facade changes only
problemCollectors and mainThreadDiagnostics updates simplified
'applyTo' property of problem-matchers removed since the only reason for its existence seemed to be to help problem matchers avoid collisions with extensions

unclear points

After these changes, marker updates from extensions land in two ExtHostDiagnostics._collections, the one that belongs to the extension, and the second is _mirrorCollection. This results in double sets of markers given away by getDiagnostics(). In theory, this means that the language extension collection should not be included in ExtHostDiagnostics._collections, so that it doesn't participate in getDiagnostics() output. However, there is currently no option to filter spawned collections by this criterion. Therefore, the opposite is done — only _mirrorCollection is included in the output, which could be correct since, after all changes, _mirrorCollection became an always up-to-date reflection of MarkerService._facade. So this requires someone with a more knowledgeable perspective. This is either the incorrect solution, since other collections are also excluded from the getDiagnostics() output. Or, if the contents of these excluded collections are reflected in markerService, this location can be simplified even further. This would be logical, since otherwise getDiagnostics() output is richer than markerService content

statements

When implementing automatic switching of the active 'origin', it is necessary to ensure that marker providers are aware of the use of MarkerService.changeOne(..., markerData: IMarkerData[] | null). Passing null removes origin and its markers, and this may result in another origin being placed at the facade. For example, when there is a task problem matcher is running, and a file is opened for editing, the extension origin becomes active. If the extension sends null when all problems are fixed, origin is removed, and markerService will place problem-matcher diagnostics to the facade. Marker providers such as language extensions should send [] for the case of absent problems to retain origin activity and send null or call MarkerService.removeOriginForOwner on dispose. However, such cases should be immediately visible

test kit

In already normal working scenarios there should be no noticeable changes except the removed delay between disappearing diagnostics on textModel removal and replacement by problem matcher diagnostics

show
Check videos for the difference demonstration. Pay attention to the 'unused' problem in Problems View. When it is Error, origin is problem-matcher. When it is a Warning, origin is the extension

Before the changes, replacement is delayed on file close:
bChanges
After the changes, replacement is instant:
aChanges

Looking further to leverage the results of markerService improvements, here is a branch for testing the switching of the typescript-language-features extension from using workspace.onDidOpen/CloseTextDocument events, which results in an unsynced diagnostics state in case of closing files opened by window.openTextDocument() (resolution of #291739), to window.tabGroups events https://github.com/n-gist/vscode/tree/markerService-origins-test-typescript-extension-switchToTabGroupsEvents
Applying it over changes makes the behavior of opening .ts files from Explorer tab and by workspace.openTextDocument identical. Though it is not complete and sometimes updates markers after the file is closed for some reason by DiagnosticsManager.scheduleDiagnosticsUpdate(), but it is a subject of a different PR, just mentioning it as noticed

My test project, containing .vsix extensions to test workspace.openTextDocument late disposal issue and getDiagnostics() outputs

git clone https://github.com/n-gist/getDiagnostics-test.git

…ltering based on file open/close state. Remove applyTo property from tasks/problem-matchers
…ror collection data, as it is now in sync with markerService facade
@vs-code-engineering
Copy link
Copy Markdown
Contributor

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

@rzhao271

Matched files:

  • src/vs/workbench/contrib/preferences/browser/preferencesRenderers.ts

Comment on lines +161 to +162
private readonly _facade = new DoubleResourceMap<IOriginMarkers>();
private readonly _backyard = new DoubleResourceMap<Map<IMarkerOrigin, IOriginMarkers>>();
Copy link
Copy Markdown
Contributor Author

@n-gist n-gist Feb 13, 2026

Choose a reason for hiding this comment

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

Now I see no reason to have these separated, this may be combined to

private readonly _data = new DoubleResourceMap<{
    facade: IOriginMarkers;
    readonly all: Map<IMarkerOrigin, IOriginMarkers>
}>();

This should simplify markers updating a bit, and just adds a property access during read

Keeping it as is for now to not mess initial reviews

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.

3 participants