Goto macro definition is too slow

[misaki214]

Describe the bug
After on demand indexing, goto macro definition is too slow.

To Reproduce

Expected behavior
Get a response in a second.

Actual behavior
Get a response more than 5s.

Context

• erlang_ls version (tag/sha):
• Editor used:
• LSP client used:

[robertoaloi]

Hi @misaki214 and thanks for the report!

These times are extremely high compared to our experience, so it would be good to understand what is going on in your case. A couple of things you could try:

1. Check if you have multiple instances of Erlang LS running (epmd -names). Multiple LS instances (or other programs) could clog your machine and result in performance degradation
2. Attach to the Erlang LS node and verify how much time is spent on the server side. You can run something like erl -sname debug -remsh ERLANG_LS_NODE_HERE and then use the following redbug expression to verify how much time is actually spent handling the request:

redbug:start("els_definition_provider:handle_request->return", [{print_msec, true}, {arity, true}, {print_return, false}, {msgs, 10000}, {time, 10000}]).

3. While attached to the node, you could run observer:start() and check message queues/memory/cpu/etc

Any specific reason why you suspect the on-demand indexing as the root cause? E.g. if you try a prior version and then switch back to a recent one, can you reproduce the performance issue consistently only with the new version?

[plux]

I can confirm the issue.
It seems like the function that recursively traverses includes is quite slow when the -define is located in a header file which is included from a lot of places.
I did this simple fix which skips files that aren't .hrl, which makes it significantly faster.
I'm not sure if we want to support the use case of including .erl files, in that case this solution isn't valid.
In that case I think we could speed it up if we do a a reference index lookup for the macro POI to find reference candidates, and after that check if the header file is reachable from candidate references.

diff --git a/apps/els_lsp/src/els_scope.erl b/apps/els_lsp/src/els_scope.erl
index 66c1ae3..4e4e625 100644
--- a/apps/els_lsp/src/els_scope.erl
+++ b/apps/els_lsp/src/els_scope.erl
@@ -65,12 +65,18 @@ find_includers_loop(Uri, Acc0) ->

 -spec find_includers(uri()) -> [uri()].
 find_includers(Uri) ->
-    IncludeId = els_utils:include_id(els_uri:path(Uri)),
-    IncludeLibId = els_utils:include_lib_id(els_uri:path(Uri)),
-    lists:usort(
-        find_includers(include, IncludeId) ++
-            find_includers(include_lib, IncludeLibId)
-    ).
+    Path = els_uri:path(Uri),
+    case filename:extension(Path) of
+        <<".hrl">> ->
+            IncludeId = els_utils:include_id(Path),
+            IncludeLibId = els_utils:include_lib_id(Path),
+            lists:usort(
+                find_includers(include, IncludeId) ++
+                    find_includers(include_lib, IncludeLibId)
+            );
+        _ ->
+            []
+    end.

[misaki214]

I'm not sure if we want to support the use case of including .erl files, in that case this solution isn't valid.

Hi @plux , I think we need support the use case of including .erl files.
Because Find All References and Rename Symbol is slow too.

[plux]

Yes, but is it really common that people -include .erl files?
I've never seen that.

[misaki214]

-include .erl files

It means -include("filename.erl"). ?
If so, we needn't support it.

[plux]

-include .erl files

It means -include("filename.erl"). ? If so, we needn't support it.

Yep that's what I meant.

I implemented my other proposal in this branch:
https://github.com/plux/erlang_ls/tree/improve-macro-refs-performance

Go ahead and give it a spin!