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

"Add all imports" is inefficient when resolving static imports #3383

Closed
Brahvim opened this issue Nov 9, 2023 · 4 comments · Fixed by eclipse-jdtls/eclipse.jdt.ls#2994
Closed

Comments

@Brahvim
Copy link

Brahvim commented Nov 9, 2023

The "Add all imports" context menu option is not functioning in most cases. Sometimes, it just takes very long to become active.

Issues such as this and #3186 are the reason why I need to keep rolling the extension's version back to v1.18.0.

Environment
  • Operating System: KDE Neon
  • JDK version: jdk-17.0.7+7
  • Visual Studio Code version: 1.84.1
  • Java extension version: v1.24.0
@rgrunber
Copy link
Member

#3186 should be fixed in 1.24.0.

I tried calling the "Add all missing imports" option in a project with ~50 imports and the response is almost instant. However, I then tried on a file containing ~100 imports and there was a clear reduction in performance :

[Trace - 14:21:42] Sending request 'codeAction/resolve - (82)'.
Params: {
    "title": "Add all missing imports",
    "data": {
        "pid": "1",
        "rid": "11"
    },
    "kind": "source",
    "diagnostics": []
}


[Trace - 14:21:42] Received notification 'window/logMessage'.
Params: {
    "type": 3,
    "message": "Nov. 13, 2023, 2:21:42 p.m. >> codeAction/resolve"
}


[Info  - 14:21:42] Nov. 13, 2023, 2:21:42 p.m. >> codeAction/resolve
[Trace - 14:21:49] Received request 'workspace/executeClientCommand - (65)'.
Params: {
    "command": "java.action.organizeImports.chooseImports",
    "arguments": [
        "file:/home/rgrunber/git/lemminx/org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/XMLAssert.java",
        [
            {
                "candidates": [
                    {
                        "fullyQualifiedName": "javax.sql.rowset.Predicate",
                        "id": "javax.sql.rowset.Predicate@-611838662"
                    },
                    {
                        "fullyQualifiedName": "java.util.function.Predicate",
                        "id": "java.util.function.Predicate@-1665014902"
                    },
                    {
                        "fullyQualifiedName": "com.google.common.base.Predicate",
                        "id": "com.google.common.base.Predicate@-1013014867"
                    }
                ],
                "range": {
                    "start": {
                        "line": 59,
                        "character": 40
                    },
                    "end": {
                        "line": 59,
                        "character": 49
                    }
                }
            },
            {
                "candidates": [
                    {
                        "fullyQualifiedName": "java.util.List",
                        "id": "java.util.List@-117617312"
                    },
                    {
                        "fullyQualifiedName": "com.overzealous.remark.convert.List",
                        "id": "com.overzealous.remark.convert.List@-389112771"
                    }
                ],
                "range": {
                    "start": {
                        "line": 168,
                        "character": 2
                    },
                    "end": {
                        "line": 168,
                        "character": 6
                    }
                }
            },
            ...
            ...
            ...
        ],
        true
    ]
}

So it went from "instant" to ~7s . That's definitely odd. If I had to guess I'd say it might be that certain kind of imports add significantly more time to the action but I'd have to investigate.

@rgrunber
Copy link
Member

image

Looks like the problem is the static import favourite resolution. If I disable the favourite static imports, then regular performance returns. Unforunately, it's difficult to do on the client-side because even when the list is made empty, we provide some values from the server-side.

This portion of code is what is taking nearly all the time : https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/34f6473dbf659c47714111393ea2b085902af54f/org.eclipse.jdt.core.manipulation/core%20extension/org/eclipse/jdt/internal/corext/util/JavaModelUtil.java#L1327-L1369 .

The code is creating a new compilation unit of the form :

package [packagename]
  public class [Name] {
  static {
    [identifier]|
  }
}

, triggering auto-completion to the right of [identifer], and collecting all method/field imports that match. [identifier] represents an unresolved static reference. There were ~300 static import references in the file. Some performance improvements that come to mind :

  • cache any discovered imports. I noticed out of ~300, ~50 were just for assertEquals and each one was recalculated from scratch 😐
  • Re-use the compilation unit working copy and just set its contents instead of creating a new one every time

These will need to be done upstream in JDT though

@rgrunber rgrunber changed the title "Add all imports" is broken "Add all imports" is inefficient when resolving static imports Nov 14, 2023
@rgrunber rgrunber self-assigned this Nov 14, 2023
@rgrunber
Copy link
Member

rgrunber commented Nov 21, 2023

The paste event handling also performs "organize imports" : https://github.com/eclipse-jdtls/eclipse.jdt.ls/blob/45aeb8db2655847ee4efad7f97de7e7926c3007b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/PasteEventHandler.java#L324 .

So basically, the behaviour in this issue may also extend to problems of the form "pasting text is slow", when paste support is enabled.

@rgrunber
Copy link
Member

This was fixed upstream. As soon as JDT-LS updates to say the 4.31 I-builds (eg. I20231129-1800 ), we should get the performance improvement in the vscode-java pre-releases.

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

Successfully merging a pull request may close this issue.

2 participants