Support discarding the CST to preserve memory #1733

msujew · 2024-10-29T16:09:14Z

This (surprisingly small) change enables us to discard the CST on closed documents (in LSP use cases) to reduce our memory footprint. I've seen several crashes of Langium based language servers on large (> 100mb) workspaces, and this change should help alleviate those issues.

This change includes:

A new Environment service that identifies whether we're currently running as a language server. Running in a CLI environment usually requires full access to the CST.
A new CstParserMode that allows to discard parsed CSTs. The default is Retain, but Discard will be used when we encounter a closed document while running as a language server. This also goes in tandem with the new discardCst function that removes CSTs from ASTs that were parsed in Retain mode. Useful for documents that have been closed.
A new $segments property has been added for caching of CST data. In particular, it contains the full range of any AST node and the ranges for all properties. Ranges for keywords are not stored (yet).
This new property is used extensively in the LSP related services to find the correct positions of properties within closed files. Therefore, most services that attempt to find some specific position/range now use the $segments property.

Specific tests for the discard behavior will follow soon. This PR just shows the feasibility and backwards compatibility of the changes at hand so far.

danieldietrich

Hi Mark,
I just did a quick 'scan' of the code and asked questions.
When I read "cache" my reflex is "and what about invalidation?", i.e. how do you ensure that the closed file contents don't deviate from the cached regions?
Daily reminder (and note to myself): It would be easier to have code formatting in a separate commit ;)
Thanks
Daniel

packages/langium/src/workspace/ast-descriptions.ts

packages/langium/src/lsp/call-hierarchy-provider.ts

packages/langium/src/references/name-provider.ts

msujew · 2024-10-30T10:48:37Z

When I read "cache" my reflex is "and what about invalidation?", i.e. how do you ensure that the closed file contents don't deviate from the cached regions?

As soon as the document is changed (either on disk, or via the textDocument/open notification), the language server will be notified by the language client and reparses the document. Reparsing the document automatically invalidates the cache, so we don't need to worry about desync in that regard.

danieldietrich · 2024-10-30T12:24:43Z

When I read "cache" my reflex is "and what about invalidation?", i.e. how do you ensure that the closed file contents don't deviate from the cached regions?

As soon as the document is changed (either on disk, or via the textDocument/open notification), the language server will be notified by the language client and reparses the document. Reparsing the document automatically invalidates the cache, so we don't need to worry about desync in that regard.

Sounds all good! Thx

danieldietrich

(see my last review)

spoenemann · 2024-11-06T14:10:58Z

packages/langium/src/lsp/definition-provider.ts

-            )];
+        if (goToLink && goToLink.target.$segments) {
+            const name = this.nameProvider.getNameProperty(goToLink.target);
+            if (name) {


Shall we use the whole composite node as target in case a name is not available?

spoenemann · 2024-11-06T14:23:07Z

packages/langium/src/lsp/document-update-handler.ts

+            // Whenever the user reopens the document, the CST will be rebuilt
+            discardCst(document.parseResult.value);
+        }
+        // Discard the diagnostics for the closed document


Shall we make this configurable (without method overriding)? It could be simply a public property on the service class to turn this behavior on or off?

I imagine there are users/language implementors who would like to keep all problem markers even for closed documents.

packages/langium/src/parser/async-parser.ts

packages/langium/src/syntax-tree.ts

spoenemann · 2024-11-07T11:26:23Z

packages/langium/src/validation/document-validator.ts

@@ -61,6 +61,9 @@ export class DefaultDocumentValidator implements DocumentValidator {

    async validateDocument(document: LangiumDocument, options: ValidationOptions = {}, cancelToken = CancellationToken.None): Promise<Diagnostic[]> {
        const parseResult = document.parseResult;
+        if (!parseResult.value.$cstNode) {


Why the early-exit here?

Because the validator requires the CST to identify where to place the validation markers. If I want to place a diagnostic on a keyword, the validator doesn't know where to place it, because all information about that keyword was lost when we discarded the CST.

But the lexer and parser errors don't need the CST, so we should at least report those.

spoenemann · 2024-11-07T11:33:09Z

packages/langium/src/workspace/environment.ts

+    readonly locale: string;
+}
+
+export interface Environment extends EnvironmentInfo {


Do we really need this as a separate service? AFAICT the information is only used by the WorkspaceManager.

I feel like this could benefit other services as well and is well suited as a stand-alone service.

msujew · 2024-11-07T11:44:54Z

@spoenemann I agree, this is something for a v4.0 of Langium. This PR isn't finished anyway - the formatter needs a refactoring as the new way of doing comments in the CST broke the formatter and I still haven't gotten to writing any tests for this.

msujew · 2025-02-13T21:56:04Z

I fixed a few issues with this PR and everything should now work as expected. Ready for review :)

spoenemann

This change improves the memory usage for large workspaces. But it actually worsens the situation when you open a large file because the parser attaches both $cstNode and $segments to the AST.

Could we change that so the parser attaches either $cstNode or $segments, but not both? Then the discardCst function can take care of gathering the necessary information from the CST to create $segments. Of course that's more effort then, but I think it's acceptable given that it's done only when a document is closed by the user, so it can happen in the background without the user noticing it.

The LSP services would have to support both ways to obtain range information, but that might be extracted into utility functions to keep it simple.

spoenemann · 2025-03-06T11:15:18Z

packages/langium/src/parser/langium-parser.ts

@@ -198,6 +204,7 @@ export class LangiumParser extends AbstractLangiumParser {
    private lexerResult?: LexerResult;
    private stack: any[] = [];
    private assignmentMap = new Map<AbstractElement, AssignmentElement | undefined>();
+    private currentMode: CstParserMode = CstParserMode.Retain;


Minor: name this currentCstMode to be more specific

spoenemann · 2025-03-06T11:19:42Z

packages/langium/src/parser/langium-parser.ts

-        return obj;
+        obj.$cstNode = cstNode;
+        delete obj.$segments.comment;
+        obj.$segments.comment = this.commentProvider.getComment(obj);


Why delete if it's immediately assigned?

spoenemann · 2025-03-06T11:31:54Z

packages/langium/src/references/name-provider.ts

@@ -39,7 +46,11 @@ export class DefaultNameProvider implements NameProvider {
        return undefined;


Should this return node[this.getNameProperty(node)]?

spoenemann · 2025-03-06T12:18:31Z

packages/langium/src/workspace/ast-descriptions.ts

+        const nameProperty = this.nameProvider.getNameProperty(node);
+        const nameSegment: DocumentSegment | undefined = nameProperty
+            ? node.$segments?.properties.get(nameProperty)[0]
+            : undefined;


The description providers are rather central services. I suggest to keep the CST-based segment resolution as fallback.

msujew added ast AST structure related issue performance Issues related to the runtime performance of Langium labels Oct 29, 2024

danieldietrich reviewed Oct 29, 2024

View reviewed changes

packages/langium/src/workspace/ast-descriptions.ts Outdated Show resolved Hide resolved

packages/langium/src/lsp/call-hierarchy-provider.ts Show resolved Hide resolved

packages/langium/src/references/name-provider.ts Show resolved Hide resolved

danieldietrich approved these changes Oct 30, 2024

View reviewed changes

spoenemann reviewed Nov 6, 2024

View reviewed changes

spoenemann reviewed Nov 7, 2024

View reviewed changes

msujew marked this pull request as draft November 7, 2024 11:45

msujew mentioned this pull request Jan 14, 2025

Improve performance of hidden node parsing #1790

Merged

msujew force-pushed the msujew/discard-cst branch from 5104b23 to 60b5a57 Compare February 13, 2025 21:55

msujew marked this pull request as ready for review February 13, 2025 21:55

msujew added this to the v4.0.0 milestone Feb 19, 2025

msujew force-pushed the msujew/discard-cst branch from 60b5a57 to dbb2956 Compare February 19, 2025 16:06

Discard CST to preserve memory

6d91794

msujew force-pushed the msujew/discard-cst branch from dbb2956 to 6d91794 Compare February 26, 2025 13:07

spoenemann reviewed Mar 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support discarding the CST to preserve memory #1733

Support discarding the CST to preserve memory #1733

msujew commented Oct 29, 2024

danieldietrich left a comment

msujew commented Oct 30, 2024

danieldietrich commented Oct 30, 2024

danieldietrich left a comment

spoenemann Nov 6, 2024

spoenemann Nov 6, 2024

spoenemann Nov 7, 2024

msujew Nov 7, 2024

spoenemann Mar 6, 2025 •

edited

Loading

spoenemann Nov 7, 2024

msujew Nov 7, 2024

msujew commented Nov 7, 2024

msujew commented Feb 13, 2025

spoenemann left a comment

spoenemann Mar 6, 2025

spoenemann Mar 6, 2025

spoenemann Mar 6, 2025

spoenemann Mar 6, 2025

		@@ -39,7 +46,11 @@ export class DefaultNameProvider implements NameProvider {
		return undefined;

Support discarding the CST to preserve memory #1733

Are you sure you want to change the base?

Support discarding the CST to preserve memory #1733

Conversation

msujew commented Oct 29, 2024

danieldietrich left a comment

Choose a reason for hiding this comment

msujew commented Oct 30, 2024

danieldietrich commented Oct 30, 2024

danieldietrich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spoenemann Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msujew commented Nov 7, 2024

msujew commented Feb 13, 2025

spoenemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spoenemann Mar 6, 2025 •

edited

Loading