Improve performance under batch operations

This commit seeks to improve the performance under batch operations by
holding off compilations until after a period of quiescence. Prior, we
were building every 100 or so milliseconds, but this would cause builds
to happen during batch operations.

Instead, this PR dramatically simplifies the build process to utilize
timeouts to detect a quiet period after which builds can commence.

I also found a missed case while transforming diagnostics into elixir
that caused crashes that took down the project node. I was unaware that
the uri of a diagnostic can be nil (how?!?).

Prior to this PR, mass renames would never finish in emacs, and now they
can.

apps/remote_control/lib/lexical/remote_control/build.ex

@@ -9,7 +9,7 @@ defmodule Lexical.RemoteControl.Build do
   require Logger
   use GenServer
 
-  @tick_interval_millis 50
+  @timeout_interval_millis 50
 
   # Public interface
 
@@ -67,32 +67,30 @@ defmodule Lexical.RemoteControl.Build do
   @impl GenServer
   def handle_continue(:ensure_build_directory, %State{} = state) do
     State.ensure_build_directory(state)
-    schedule_tick()
     {:noreply, state}
   end
 
   @impl GenServer
   def handle_call({:force_compile_file, %Document{} = document}, _from, %State{} = state) do
     State.compile_file(state, document)
-    {:reply, :ok, state}
+    {:reply, :ok, state, @timeout_interval_millis}
   end
 
   @impl GenServer
   def handle_cast({:compile, force?}, %State{} = state) do
-    State.compile_project(state, force?)
-    {:noreply, state}
+    new_state = State.on_project_compile(state, force?)
+    {:noreply, new_state, @timeout_interval_millis}
   end
 
   @impl GenServer
   def handle_cast({:compile_file, %Document{} = document}, %State{} = state) do
     new_state = State.on_file_compile(state, document)
-    {:noreply, new_state}
+    {:noreply, new_state, @timeout_interval_millis}
   end
 
   @impl GenServer
-  def handle_info(:tick, %State{} = state) do
-    new_state = State.on_tick(state)
-    schedule_tick()
+  def handle_info(:timeout, %State{} = state) do
+    new_state = State.on_timeout(state)
     {:noreply, new_state}
   end
 
@@ -101,8 +99,4 @@ defmodule Lexical.RemoteControl.Build do
     Logger.warning("Undefined message: #{inspect(msg)}")
     {:noreply, project}
   end
-
-  defp schedule_tick do
-    Process.send_after(self(), :tick, @tick_interval_millis)
-  end
 end

apps/remote_control/lib/lexical/remote_control/build/state.ex

@@ -14,33 +14,54 @@ defmodule Lexical.RemoteControl.Build.State do
 
   use RemoteControl.Progress
 
-  defstruct project: nil, build_number: 0, uri_to_source_and_edit_time: %{}
+  defstruct project: nil,
+            build_number: 0,
+            uri_to_document: %{},
+            project_compile: :none
 
   def new(%Project{} = project) do
     %__MODULE__{project: project}
   end
 
-  def on_tick(%__MODULE__{} = state) do
-    {new_state, compiled_uris} =
-      Enum.reduce(state.uri_to_source_and_edit_time, {state, []}, fn
-        {uri, {document, edit_time}}, {state, compiled_uris} ->
-          if should_compile?(edit_time) do
-            new_state = increment_build_number(state)
-            compile_file(new_state, document)
-            {new_state, [uri | compiled_uris]}
-          else
-            {state, compiled_uris}
-          end
-      end)
+  def on_timeout(%__MODULE__{} = state) do
+    case state.project_compile do
+      :none -> :ok
+      :force -> compile_project(state, true)
+      :normal -> compile_project(state, false)
+    end
+
+    new_state =
+      state.uri_to_document
+      |> Map.values()
+      |> Enum.reduce(
+        state,
+        fn document, state ->
+          new_state = increment_build_number(state)
+          compile_file(new_state, document)
+          new_state
+        end
+      )
 
+    %__MODULE__{new_state | uri_to_document: %{}, project_compile: :none}
+  end
+
+  def on_file_compile(%__MODULE__{} = state, %Document{} = document) do
     %__MODULE__{
-      new_state
-      | uri_to_source_and_edit_time: Map.drop(state.uri_to_source_and_edit_time, compiled_uris)
+      state
+      | uri_to_document: Map.put(state.uri_to_document, document.uri, document)
     }
   end
 
+  def on_project_compile(%__MODULE__{} = state, force?) do
+    if force? do
+      %__MODULE__{state | project_compile: :force}
+    else
+      %__MODULE__{state | project_compile: :normal}
+    end
+  end
+
   def compile_scheduled?(%__MODULE__{} = state, uri) do
-    Map.has_key?(state.uri_to_source_and_edit_time, uri)
+    Map.has_key?(state.uri_to_document, uri)
   end
 
   def ensure_build_directory(%__MODULE__{} = state) do
@@ -65,7 +86,7 @@ defmodule Lexical.RemoteControl.Build.State do
     end
   end
 
-  def compile_project(%__MODULE__{} = state, initial?) do
+  defp compile_project(%__MODULE__{} = state, initial?) do
     state = increment_build_number(state)
     project = state.project
 
@@ -108,14 +129,6 @@ defmodule Lexical.RemoteControl.Build.State do
     end)
   end
 
-  def on_file_compile(%__MODULE__{} = state, %Document{} = document) do
-    %__MODULE__{
-      state
-      | uri_to_source_and_edit_time:
-          Map.put(state.uri_to_source_and_edit_time, document.uri, {document, now()})
-    }
-  end
-
   def compile_file(%__MODULE__{} = state, %Document{} = document) do
     project = state.project
 
@@ -201,15 +214,6 @@ defmodule Lexical.RemoteControl.Build.State do
     "Building #{Project.display_name(project)}"
   end
 
-  defp now do
-    System.system_time(:millisecond)
-  end
-
-  defp should_compile?(last_edit_time) do
-    millis_since_last_edit = now() - last_edit_time
-    millis_since_last_edit >= edit_window_millis()
-  end
-
   defp to_ms(microseconds) do
     microseconds / 1000
   end
@@ -218,10 +222,6 @@ defmodule Lexical.RemoteControl.Build.State do
     [columns: true, token_metadata: true]
   end
 
-  defp edit_window_millis do
-    Application.get_env(:remote_control, :edit_window_millis, 250)
-  end
-
   defp increment_build_number(%__MODULE__{} = state) do
     %__MODULE__{state | build_number: state.build_number + 1}
   end

apps/remote_control/test/lexical/remote_control/build/state_test.exs

@@ -6,7 +6,6 @@ defmodule Lexical.RemoteControl.Build.StateTest do
   alias Lexical.RemoteControl.Build.State
   alias Lexical.RemoteControl.Plugin
 
-  import Lexical.Test.EventualAssertions
   import Lexical.Test.Fixtures
 
   use ExUnit.Case, async: false
@@ -67,22 +66,89 @@ defmodule Lexical.RemoteControl.Build.StateTest do
     {:ok, document: document}
   end
 
-  describe "throttled compilation" do
-    setup [:with_metadata_project, :with_a_valid_document]
+  def with_patched_compilation(_) do
+    patch(Build.Document, :compile, :ok)
+    patch(Build.Project, :compile, :ok)
+    :ok
+  end
+
+  describe "throttled document compilation" do
+    setup [:with_metadata_project, :with_a_valid_document, :with_patched_compilation]
 
     test "it doesn't compile immediately", %{state: state, document: document} do
-      new_state =
-        state
-        |> State.on_file_compile(document)
-        |> State.on_tick()
+      new_state = State.on_file_compile(state, document)
 
       assert State.compile_scheduled?(new_state, document.uri)
+      refute_called(Build.Document.compile(document))
+      refute_called(Build.Project.compile(_, _))
+    end
+
+    test "it compiles files when on_timeout is called", %{state: state, document: document} do
+      state
+      |> State.on_file_compile(document)
+      |> State.on_timeout()
+
+      assert_called(Build.Document.compile(document))
+      refute_called(Build.Project.compile(_, _))
+    end
+  end
+
+  describe "throttled project compilation" do
+    setup [:with_metadata_project, :with_a_valid_document, :with_patched_compilation]
+
+    test "doesn't compile immediately if forced", %{state: state} do
+      State.on_project_compile(state, true)
+      refute_called(Build.Project.compile(_, _))
+    end
+
+    test "doesn't compile immediately", %{state: state} do
+      State.on_project_compile(state, false)
+      refute_called(Build.Project.compile(_, _))
+    end
+
+    test "compiles if force is true after on_timeout is called", %{state: state} do
+      state
+      |> State.on_project_compile(true)
+      |> State.on_timeout()
+
+      assert_called(Build.Project.compile(_, true))
     end
 
-    test "it compiles after a timeout", %{state: state, document: document} do
-      state = State.on_file_compile(state, document)
+    test "compiles after on_timeout is called", %{state: state} do
+      state
+      |> State.on_project_compile(false)
+      |> State.on_timeout()
+
+      assert_called(Build.Project.compile(_, false))
+    end
+  end
+
+  describe "mixed compilation" do
+    setup [:with_metadata_project, :with_a_valid_document, :with_patched_compilation]
+
+    test "doesn't compile if both documents and projects are added", %{
+      state: state,
+      document: document
+    } do
+      state
+      |> State.on_project_compile(false)
+      |> State.on_file_compile(document)
+
+      refute_called(Build.Document.compile(_))
+      refute_called(Build.Project.compile(_, _))
+    end
 
-      refute_eventually(State.compile_scheduled?(State.on_tick(state), document.uri), 500)
+    test "compiles when on_timeout is called if both documents and projects are added", %{
+      state: state,
+      document: document
+    } do
+      state
+      |> State.on_project_compile(false)
+      |> State.on_file_compile(document)
+      |> State.on_timeout()
+
+      assert_called(Build.Document.compile(_))
+      assert_called(Build.Project.compile(_, _))
     end
   end
 end

apps/server/lib/lexical/convertibles/lexical.plugin.diagnostic.result.ex

@@ -41,12 +41,16 @@ defimpl Lexical.Convertible, for: Lexical.Plugin.V1.Diagnostic.Result do
     Conversions.to_lsp(range)
   end
 
-  defp lsp_range(%Diagnostic.Result{} = diagnostic) do
-    with {:ok, document} <- Document.Store.open_temporary(diagnostic.uri) do
+  defp lsp_range(%Diagnostic.Result{uri: uri} = diagnostic) when is_binary(uri) do
+    with {:ok, document} <- Document.Store.open_temporary(uri) do
       position_to_range(document, diagnostic.position)
     end
   end
 
+  defp lsp_range(%Diagnostic.Result{}) do
+    {:error, :no_uri}
+  end
+
   defp position_to_range(%Document{} = document, {start_line, start_column, end_line, end_column}) do
     start_pos = Position.new(document, start_line, max(start_column, 1))
     end_pos = Position.new(document, end_line, max(end_column, 1))

projects/lexical_shared/lib/lexical/document/store.ex

@@ -268,7 +268,7 @@ defmodule Lexical.Document.Store do
 
   @spec open_temporary(Lexical.uri() | Path.t(), timeout()) ::
           {:ok, Document.t()} | {:error, term()}
-  def open_temporary(uri, timeout \\ 5000) do
+  def open_temporary(uri, timeout \\ 5000) when is_binary(uri) do
     ProcessCache.trans(uri, 50, fn ->
       GenServer.call(name(), {:open_temporarily, uri, timeout})
     end)