From 95a03e39e571b5c92672433e5ac7d83a9412d81d Mon Sep 17 00:00:00 2001 From: Steve Cohen Date: Thu, 5 Sep 2024 10:16:53 -0700 Subject: [PATCH] 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. --- .../lib/lexical/remote_control/build.ex | 20 ++--- .../lib/lexical/remote_control/build/state.ex | 76 ++++++++-------- .../remote_control/build/state_test.exs | 86 ++++++++++++++++--- .../lexical.plugin.diagnostic.result.ex | 8 +- .../lib/lexical/document/store.ex | 2 +- 5 files changed, 128 insertions(+), 64 deletions(-) diff --git a/apps/remote_control/lib/lexical/remote_control/build.ex b/apps/remote_control/lib/lexical/remote_control/build.ex index 3f80812c6..0137a3527 100644 --- a/apps/remote_control/lib/lexical/remote_control/build.ex +++ b/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 diff --git a/apps/remote_control/lib/lexical/remote_control/build/state.ex b/apps/remote_control/lib/lexical/remote_control/build/state.ex index 44883068b..2f1587e17 100644 --- a/apps/remote_control/lib/lexical/remote_control/build/state.ex +++ b/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 diff --git a/apps/remote_control/test/lexical/remote_control/build/state_test.exs b/apps/remote_control/test/lexical/remote_control/build/state_test.exs index f838ec52e..6860f800c 100644 --- a/apps/remote_control/test/lexical/remote_control/build/state_test.exs +++ b/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 diff --git a/apps/server/lib/lexical/convertibles/lexical.plugin.diagnostic.result.ex b/apps/server/lib/lexical/convertibles/lexical.plugin.diagnostic.result.ex index 71e18923b..e6251213e 100644 --- a/apps/server/lib/lexical/convertibles/lexical.plugin.diagnostic.result.ex +++ b/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)) diff --git a/projects/lexical_shared/lib/lexical/document/store.ex b/projects/lexical_shared/lib/lexical/document/store.ex index f438859e9..d305ba35d 100644 --- a/projects/lexical_shared/lib/lexical/document/store.ex +++ b/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)