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

Bypass file_server for some file existence checks #390

Merged
merged 1 commit into from
Oct 19, 2020

Conversation

mattbaker
Copy link
Contributor

@mattbaker mattbaker commented Oct 19, 2020

This addresses what I believe is the root cause of #381.

During indexing the WorkspaceSymbols genserver works its way through the result of :code.all_loaded/0, calling File.exists? in the course of its work for each entry in the list of loaded modules. This occurs concurrently across n processes where n is the scheduler count. On my current project of decent size :code.all_loaded/0 returns 550 entries.

The File.exists? checks are filling the message queue of Erlang's file_server process, and I think this is causing other file operations to block until it can work through the queue. If you launch the Observer you can see this happen on the process tab, and it would explain why seemingly independent parts of the Elixir LS application are blocking each other.

I think the regression itself would have been introduced in #110 with the introduction of workspace symbols support. I'm unclear on why it got worse for people in the latest Elixir LS release, but as I mentioned in #381 I wonder if the Elixir and OTP upgrade is a piece of the puzzle? Maybe someone has a better idea.

This PR passes the :raw option to File.exists?, which causes the file existence check to bypass the Erlang file server, which I believe is safe under these circumstances.

This seemed to resolve the specific issue I outlined in #381. On my machine it also reduced the cumulative workspace symbol indexing time by 50% for the initial indexing phase and 70% for subsequent updates. I suspect this is because the individual tasks spawned by WorkspaceSymbols aren't blocking each other's File.exists? checks.

As an aside on the subject of performance — after this change I tried Task.async_stream as well as the custom process_chunked family of functions and found no difference between them. It may be worth revisiting if we can simply use Task.async_stream since it will be familiar to more people. It is, of course, also possible I measured incorrectly!

Perhaps @benwilson512 or @BlueHotDog could give this change a try to see if it helps?

@lukaszsamson
Copy link
Collaborator

Nice find @mattbaker. The same fix is also needed in elixir_sense.
Regarding the 0.6 release, I don't think that OTP/Elixir version is the culprit here. We are not using 1.11/23 to compile releases - in 0.6 we updated the build from 1.7/20 to 1.8/21. There were also reports of speed regression on 1.9/22. My guess is that #350 made this locking issue more prominent.

As an aside on the subject of performance — after this change I tried Task.async_stream as well as the custom process_chunked family of functions and found no difference between them. It may be worth revisiting if we can simply use Task.async_stream since it will be familiar to more people. It is, of course, also possible I measured incorrectly!

I explained my original reasoning behind a custom process_chunked in #110 (comment). With this fix applied it may no longer be valid though.

@mattbaker
Copy link
Contributor Author

Ah, that's interesting @lukaszsamson, thanks!

And thanks for the information on process_chunked, I'd missed that comment. Since they seem to be behaving equivalently it doesn't seem worth changing, but it's always good to have options :)

@axelson
Copy link
Member

axelson commented Oct 19, 2020

Thanks for this @mattbaker ❤️! This looks like it should help with the speed significantly.

@axelson axelson merged commit eceed51 into elixir-lsp:master Oct 19, 2020
@benwilson512
Copy link

Hey folks, I'm checking this out locally to test. Two questions I'm hoping I can get answer to: Does format on save wait for all compilation to complete before formatting? Does it reformat all files in the code base or just the one that was changed?

@mattbaker
Copy link
Contributor Author

@benwilson512 - this is my understanding:

  • It only formats the file in question.
  • Formatting does not wait on compilation, but it probably should in umbrella apps. I think that's the origin of Error - Cannot format file from current directory (Currently in deps/telemetry/deps/mimerl)") #252.
    It looks like the code that pulls .formatter.exs options in Elixir makes use of File.cwd/0, and the current working directory (apparently) changes during the compilation of umbrella apps. I poked around the language server and Elixir and came up empty handed with regards to a clean fix.

@mattbaker
Copy link
Contributor Author

Played around with #391 as a solution to the second bullet, but I'm not sure how I feel about it.

lukaszsamson added a commit to elixir-lsp/elixir_sense that referenced this pull request Oct 25, 2020
@mattbaker mattbaker deleted the file-exists-raw branch November 16, 2020 19:29
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.

4 participants