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

evaluate generic server-side snippets in resolve stage #1857

Merged
merged 1 commit into from
Aug 31, 2021

Conversation

Eskibear
Copy link
Contributor

See #1838 , perf issue about evaluating generic snippets.

There are two groups of Snippets:

  • Generic snippets (e.g. foreach, fori, ifelse, etc.)
  • Type definition snippets (e.g. class, interface, etc.)

Currently at completion stage, we evaluate template patterns in given context to provide insertText and documentation. At resolve stage we do nothing.

Evaluation is relatively expensive. This PR does below things:

  • For generic snippets, delay resolving evaluation, providing corresponding TextEdit and documentation at resolve stage. In insertText, placeholders are directly from template patterns.
  • For type definition snippets, remove unused data field as they are not processed in at resolve stage.

@Eskibear
Copy link
Contributor Author

/**
* An edit which is applied to a document when selecting this completion.
* When an edit is provided the value of insertText is ignored.
.....
textEdit?: TextEdit | InsertReplaceEdit;

It's implemented based on LSP specs above. After "completion", insertText is effective to make sure the snippet structure is correct. After being resolved, textEdit becomes effective, and all the placeholders are filled by corresponding variable names.

Below is a sample response for foreach snippet:

public class App
{
    public static void main( String[] args )
    {
        foreach^
    }
}
Trace - 上午12:38:58] Received response 'textDocument/completion - (80)' in 17ms.
Result: {
    "isIncomplete": false,
    "items": [
        {
            "label": "foreach",
            "kind": 15,
            "detail": "iterate over an array or Iterable",
            "insertText": "for (${1:iterable_type} ${2:iterable_element} : ${3:iterable}) {\n\t$TM_SELECTED_TEXT${0}\n}",
            "insertTextFormat": 2,
            "data": {
                "rid": "5",
                "uri": "file:///C:/Users/admin/Desktop/demo/src/main/java/com/example/App.java",
                "pid": "0"
            }
        }
    ]
}

[Trace - 上午12:38:58] Received response 'completionItem/resolve - (82)' in 6ms.
Result: {
    "label": "foreach",
    "kind": 15,
    "detail": "iterate over an array or Iterable",
    "documentation": {
        "kind": "markdown",
        "value": "```java\nfor (String string : args) {\n\t\n}\n```"
    },
    "insertText": "for (${1:iterable_type} ${2:iterable_element} : ${3:iterable}) {\n\t$TM_SELECTED_TEXT${0}\n}",
    "insertTextFormat": 2,
    "textEdit": {
        "range": {
            "start": {
                "line": 10,
                "character": 15
            },
            "end": {
                "line": 10,
                "character": 15
            }
        },
        "newText": "for (${1:String} ${2:string} : ${3:args}) {\n\t$TM_SELECTED_TEXT${0}\n}"
    }
}

@Eskibear
Copy link
Contributor Author

About the perf boost, I simply tested spring-petclinic project on a Macbook Pro(2016), 8G-RAM, 2.9GHz Dual-Core i5.
Left side with light theme is v0.81.0, right side with dark theme the LS has included this PR.

By typing an f (with several snippet candidates like fori, foreach), triggering completion for 3 times, the avg time cost is reduced from ~190ms to ~80ms.
Screen Shot 2021-08-26 at 1 43 33 PM

For a single completion of foreach snippet, now most time cost is moved to resolve stage.
Screen Shot 2021-08-26 at 1 44 47 PM

Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Works for me. I can confirm the resolution is now done in resolveCompletionItem. Just some minor changes to make.

Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Change looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants