-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
Don't clean references on every file save #187
Don't clean references on every file save #187
Conversation
lib/next_ls/db.ex
Outdated
{:message_queue_len, count} = Process.info(self(), :message_queue_len) | ||
NextLS.DB.Activity.update(s.activity, count) | ||
|
||
NextLS.Logger.warning(s.logger, "CLEANING REFERENCES FOR #{filename}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this only for the video, can be removed before merging.
One thing that I am unsure about with the so, it should still work even if you have this code # lib/foo.ex
defmodule Foo do
defmodule Bar do
def run() do
end
end
def run() do
end
end or # lib/foo.ex
defmodule Foo do
def run() do
end
end
defmodule Bar do
def run() do
end
end |
lib/next_ls/db.ex
Outdated
@spec insert_reference(pid(), String.t()) :: :ok | ||
def clean_references(server, filename), do: GenServer.cast(server, {:clean_references, filename}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch! Bad copy&paste 🤦♂️
def trace(:start, env) do | ||
Process.send( | ||
parent_pid(), | ||
{{:tracer, :start}, URI.parse(env.file).path}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm fairly certain this is just a file path already, so you don't have to parse it as a URI and then convert it back to a path again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I've just removed the unused URI.parse
call
@dvic Cristian beat you to it 🤣 |
My understanding after reading the documentation is that in those scenarios the On the other hand I have a question about this @mhanberg: should we clean up the |
I think the symbol purging is fine where it is, but I did put it where it is because I was un clear about how many times start is called. |
Thanks 🙇🏻 |
@mhanberg I've just checked an even when having two modules on the same file and having nested modules the start is only called once per file when any of its contents change. |
Excellent |
f15553d
to
e89821e
Compare
e89821e
to
7527bb9
Compare
This pull request stops cleaning file references on every file save and, instead, cleans them only when we compile the file.
On the one hand we stop cleaning the file references on the
TextDocumentDidSave
notification. On the other hand we now clean the references on the:start
event of the compiler tracer.As per the Elixir docs:
Since this is only executed when compiling a file or defining a module we know that it is safe to to clean references at this point, since new ones will be populated during the compilation that just started.
The following video shows an example. Observe how the terminal at the bottom of the screen says "CLEANING REFERENCES FOR $FILE" when I update the file contents but doesn't do anything when I save without changing the file contents.
Screen.Recording.2023-08-16.at.19.51.36.mov
Closes #154