-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
x/tools/gopls: code action to organize imports blocking #41969
Comments
/cc @heschik |
Please include a log. |
@heschik I haven't managed to pin down the circumstances that lead to the worst case performance, but this is a run after a machine boot: Notice the call took ~17s. My recollection/expectation (based on previous discussions about this) is that this call should never be blocking on a refresh of the goimports cache. Rather it should use the current state of the cache, and that cache should be refreshed periodically. |
Sorry, I don't understand. There are no completion requests in that log; what took 17s? |
We're not talking about completion here, rather a code action call:
|
Oh, right, sorry. Churning through email too quickly. What you're saying about not blocking is only for autocomplete. It doesn't apply to imports on save, which is essentially a call to the goimports command line. Unlike autocomplete, imports on save is not a best effort call; it takes as long as it takes to get an answer. If the cache is cold, because you haven't done any autocompletes yet or whatever, then it may indeed be quite slow. 17 seconds seems unreasonable but 3 is in line with what we see from Macs. I suspect you will see the exact same behavior with the goimports command line after a reboot. You can pass -v and show a log if you like, but most likely this is just going to boil down to Macs being bad at filesystems as usual. |
Interesting. This must be a combination of my misremembering and an unfortunate (pathological) refactor which triggered this wide search.
This is on Linux FWIW. Thanks for the response. I'll close this as working as intended, unless you think there's mileage in making imports on save best efforts? |
I think that's explicitly not what users want; if you hit save you expect your imports to show up, not for it to silently give up after a second or two. Obviously faster would be better. If it's Linux on an SSD this is somewhat more surprising. I would be curious to see a goimports -v run, but it's unlikely there's going to be much to do. A single-character "import" is a pathological worst case for goimports since it has to fully parse every package with that letter in the name looking for exports. |
Anecdotally, before gopls I don't remember having slow goimports in my project. With gopls it often takes seconds when I save. (Note that my save hook does imports and formatting, and I'm not 100% sure it is imports). There are other vectors of slowness such as gopls computing other code actions beyond just imports, or perhaps gopls deciding it needs to run a "go list" or what not before it gets into the imports code. |
I agree that there are many possible causes but in this case Paul has privately shared a goimports run that is exactly as slow as gopls, so here it's just the nonspecific search over a large module cache. I don't think there's anything in particular to do, unfortunately. Perhaps refuse to search for single-letter imports, but I'm sure that would annoy someone. |
@heschik: Is this OK to close? |
Unless we're prepared to compromise on something, I don't have any ideas to improve the situation. So yeah. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
I don't have a specific repro per se, just noting per a conversation on
#gopls-dev
an apparent change in behaviour.The crux of the issue is that a code action call to organize imports takes "a long time" where before it did not. In
govim
we trigger this as part of a blocking pre-save routine, hence a change in this behaviour is easily noticeable."Long time" might be anything up to 20 seconds, but more commonly is around 3 seconds. Whilst not long in the context of the history of the universe, in the context of a blocking UI call it feels like an age 😄
This appears to be most easily triggered by refactoring code in, say, a test file, where a reference
t
to a*testing.T
is moved out of scope of the declaration oft
, thereby triggering the organize imports code to try and resolve a package for the qualified identifiert
.Notes on what I've observed so far:
govim
) and re-opening causes a longer delay (3 secs) compared to re-saving post an initial save (200ms) despite the fact I am using a remote LSP setup where the remote instance is not destroyed after the last client closesWhat did you expect to see?
(Based on my understanding of how things should work following the last major "refactor")
No increased execution time of the organize imports code action call. It should return an answer based on the currently available cached package data.
Periodically, the goimports cache should be refreshed async from any LSP client calls.
What did you see instead?
Per above
cc @muirdm based on the report from Slack where you also saw this behaviour.
cc @stamblerre
FYI @leitzler
The text was updated successfully, but these errors were encountered: