-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add API for listening to concurrent mix compilations #13896
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
4dbd8b0
Add API for listening to concurrent mix compilations
jonatanklosko 437e53e
Remove --no-path-loading
jonatanklosko f984062
Merge branch 'main' into jk-compile-notify
jonatanklosko 098f19e
self -> os_pid
jonatanklosko 974ac81
Improve iex started check
jonatanklosko 3d83683
Use safe file removal
jonatanklosko 420c12f
Don't call supervisor if child is already started
jonatanklosko edb1822
Fix list ordering race condition in tests
jonatanklosko d6cb6dd
Optimise diff
jonatanklosko 08cb166
Always lock compile.all
jonatanklosko 225bfd2
Configure via def project
jonatanklosko ec942f8
Update docs
jonatanklosko ac9899e
Up
jonatanklosko fec677f
Fix windows tests
jonatanklosko b824a5d
Support lazy value in pubsub broadcast
jonatanklosko File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
defmodule IEx.MixListener do | ||
@moduledoc false | ||
|
||
use GenServer | ||
|
||
@name __MODULE__ | ||
|
||
@spec start_link(keyword) :: GenServer.on_start() | ||
def start_link(_opts) do | ||
GenServer.start_link(__MODULE__, {}, name: @name) | ||
end | ||
|
||
@doc """ | ||
Unloads all modules invalidated by external compilations. | ||
""" | ||
@spec purge :: :ok | :noop | ||
def purge do | ||
GenServer.call(@name, :purge, :infinity) | ||
end | ||
|
||
@impl true | ||
def init({}) do | ||
{:ok, %{to_purge: MapSet.new()}} | ||
end | ||
|
||
@impl true | ||
def handle_call(:purge, _from, state) do | ||
for module <- state.to_purge do | ||
:code.purge(module) | ||
:code.delete(module) | ||
end | ||
|
||
status = if Enum.empty?(state.to_purge), do: :noop, else: :ok | ||
|
||
{:reply, status, %{state | to_purge: MapSet.new()}} | ||
end | ||
|
||
@impl true | ||
def handle_info({:modules_compiled, info}, state) do | ||
%{changed: changed, removed: removed} = info.modules_diff | ||
state = update_in(state.to_purge, &Enum.into(changed, &1)) | ||
state = update_in(state.to_purge, &Enum.into(removed, &1)) | ||
{:noreply, state} | ||
end | ||
|
||
def handle_info(_message, state) do | ||
{:noreply, state} | ||
end | ||
end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
defmodule Mix.PubSub do | ||
@moduledoc false | ||
|
||
# The Mix pub/sub is responsible for notifying other OS processes | ||
# about relevant concurrent events. | ||
# | ||
# The pub/sub consists of a local subscriber process that receives | ||
# events from other OS processes, and a listener supervisor, which | ||
# holds all listener processes that have been configured. Whenever | ||
# the subscriber receives an event, it sends a message to each of | ||
# the listeners. | ||
# | ||
# Inherently, a compilation may be required before the listener | ||
# modules are available, so we start the local subscriber process | ||
# separately with `start/0`, and then start the listeners later | ||
# with `start_listeners/0`. The subscriber is going to accumulate | ||
# events and reply them once the listenres are started. | ||
|
||
@spec start :: :ok | ||
def start do | ||
case Supervisor.start_child(Mix.Supervisor, Mix.PubSub) do | ||
jonatanklosko marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
{:ok, _pid} -> | ||
:ok | ||
|
||
{:error, {:already_started, _pid}} -> | ||
:ok | ||
|
||
{:error, reason} -> | ||
raise RuntimeError, "failed to start Mix.PubSub, reason: #{inspect(reason)}" | ||
end | ||
end | ||
|
||
@spec child_spec(term) :: Supervisor.child_spec() | ||
def child_spec(_opts) do | ||
children = [ | ||
Mix.PubSub.Subscriber | ||
] | ||
|
||
opts = [strategy: :one_for_one, name: Mix.PubSub] | ||
|
||
%{ | ||
id: Mix.PubSub, | ||
start: {Supervisor, :start_link, [children, opts]}, | ||
type: :supervisor | ||
} | ||
end | ||
|
||
@spec start_listeners :: :ok | ||
def start_listeners do | ||
case Supervisor.start_child(Mix.PubSub, listener_supervisor()) do | ||
{:ok, _pid} -> | ||
Mix.PubSub.Subscriber.flush() | ||
:ok | ||
|
||
{:error, {:already_started, _pid}} -> | ||
:ok | ||
|
||
{:error, reason} -> | ||
raise RuntimeError, | ||
"failed to start Mix.PubSub.ListenerSupervisor, reason: #{inspect(reason)}" | ||
end | ||
end | ||
|
||
defp listener_supervisor do | ||
children = Application.get_env(:mix, :listeners, []) ++ built_in_listeners() | ||
|
||
children = Enum.map(children, &Supervisor.child_spec(&1, [])) | ||
|
||
opts = [strategy: :one_for_one, name: Mix.PubSub.ListenerSupervisor] | ||
|
||
%{ | ||
id: Mix.PubSub.ListenerSupervisor, | ||
start: {Supervisor, :start_link, [children, opts]}, | ||
type: :supervisor | ||
} | ||
end | ||
|
||
defp built_in_listeners do | ||
iex_started? = List.keyfind(Application.started_applications(), :iex, 0) != nil | ||
jonatanklosko marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
if iex_started? do | ||
[IEx.MixListener] | ||
else | ||
[] | ||
end | ||
end | ||
end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
defmodule Mix.PubSub.Subscriber do | ||
@moduledoc false | ||
|
||
use GenServer | ||
|
||
@name __MODULE__ | ||
|
||
@spec start_link(keyword) :: GenServer.on_start() | ||
def start_link(_opts) do | ||
GenServer.start_link(__MODULE__, {}, name: @name) | ||
end | ||
|
||
@spec flush :: :ok | ||
def flush do | ||
GenServer.cast(@name, :flush) | ||
end | ||
|
||
@impl true | ||
def init({}) do | ||
build_path = Mix.Project.build_path() | ||
Mix.Sync.PubSub.subscribe(build_path) | ||
{:ok, %{acc: []}} | ||
end | ||
|
||
@impl true | ||
def handle_info(message, %{acc: nil} = state) do | ||
notify_listeners([message]) | ||
{:noreply, state} | ||
end | ||
|
||
def handle_info(message, state) do | ||
# Accumulate messages until the flush | ||
{:noreply, update_in(state.acc, &[message | &1])} | ||
end | ||
|
||
@impl true | ||
def handle_cast(:flush, state) do | ||
notify_listeners(Enum.reverse(state.acc)) | ||
{:noreply, %{state | acc: nil}} | ||
end | ||
|
||
defp notify_listeners(messages) do | ||
children = Supervisor.which_children(Mix.PubSub.ListenerSupervisor) | ||
|
||
for message <- messages do | ||
event = message_to_event(message) | ||
|
||
for {_, pid, _, _} <- children, is_pid(pid) do | ||
send(pid, event) | ||
end | ||
end | ||
end | ||
|
||
defp message_to_event({:modules_compiled, app, build_scm, modules_diff, os_pid}) do | ||
info = %{ | ||
app: app, | ||
build_scm: build_scm, | ||
modules_diff: modules_diff, | ||
self: os_pid == System.pid() | ||
jonatanklosko marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
} | ||
|
||
{:modules_compiled, info} | ||
end | ||
end |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 am worried about this code being expensive for really large applications (20k modules). In such cases, we are changing 1-100 files, but we now need to traverse 20k to compute these statistics.
We could simplify the stats: instead of sending added/changed/removed, we just send "changed", noting that it implies added/removed/changed, and the subscriber can then lookup in disk the actual status. For the language server case, this means we may look up some modules that have been removed, but that's fine. They are the minority in most cases. For IEx/Phoenix, they don't care, they will just purge all.
If we don't want a single field, we can also do with
changed/removed
. But I think a single field would totally work on this case. WDYT?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've just pushed an update to never traverse all the modules. If you still prefer to have a single field, let me know :)
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.
This looks fantastic, I believe we can ship it. Something we can also try to do is to pass a function to PubSub. If someone is listening, we compute the event and send it. If nobody is listening, we don't. WDYT?
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.
If we do that, the function would close over
all_modules
and we would end up copying possibly huge amounts of data when passing it to the process, which may be worse than before, no?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.
We should not be copying them at all? Couldn't we get open socket instances in the GenServer client and write to them directly?
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.
Oh I confused for the subscriber side. For publishing we only list the directory, so yeah we can make the message lazy. It will be a part of
Mix.Task.Compiler.notify_modules_compiled
API, but that should be fine. See b824a5d.