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

Don't clean references on every file save #187

Merged

Conversation

crbelaus
Copy link
Contributor

@crbelaus crbelaus commented Aug 16, 2023

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:

:start - (since v1.11.0) invoked whenever the compiler starts to trace a new lexical context. A lexical context is started when compiling a new file or when defining a module within a function

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

{: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}")
Copy link
Contributor Author

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.

@mhanberg
Copy link
Collaborator

One thing that I am unsure about with the :start trace is whether it is called only for a file, or if it is called multiple times if you define submodules or multiple modules in the same file

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

Comment on lines 22 to 23
@spec insert_reference(pid(), String.t()) :: :ok
def clean_references(server, filename), do: GenServer.cast(server, {:clean_references, filename})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec is wrong

Copy link
Contributor Author

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},
Copy link
Collaborator

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

Copy link
Contributor Author

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

@mhanberg
Copy link
Collaborator

@dvic Cristian beat you to it 🤣

@dvic
Copy link
Contributor

dvic commented Aug 16, 2023

@dvic Cristian beat you to it 🤣

hah that's okay, did not start working on it

@crbelaus awesome job :)!

@crbelaus
Copy link
Contributor Author

One thing that I am unsure about with the :start trace is whether it is called only for a file, or if it is called multiple times if you define submodules or multiple modules in the same file

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

My understanding after reading the documentation is that in those scenarios the :start callback will be called only once, but I will verify it later today and report back.

On the other hand I have a question about this @mhanberg: should we clean up the symbols table as well as the references table?

@mhanberg
Copy link
Collaborator

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.

@mhanberg
Copy link
Collaborator

but I will verify it later today and report back.

Thanks 🙇🏻

@crbelaus
Copy link
Contributor Author

@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.

@mhanberg
Copy link
Collaborator

Excellent

lib/next_ls/db.ex Outdated Show resolved Hide resolved
@mhanberg mhanberg force-pushed the dont-clean-references-until-compile branch from f15553d to e89821e Compare August 17, 2023 15:00
@mhanberg mhanberg enabled auto-merge (squash) August 17, 2023 15:01
@mhanberg mhanberg force-pushed the dont-clean-references-until-compile branch from e89821e to 7527bb9 Compare August 17, 2023 15:04
@mhanberg mhanberg merged commit 481acc4 into elixir-tools:main Aug 17, 2023
3 checks passed
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.

References: determine best place to wipe out a file's references
3 participants