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

Heavy refactor of namespacing #266

Merged
merged 7 commits into from
Jul 19, 2023
Merged

Heavy refactor of namespacing #266

merged 7 commits into from
Jul 19, 2023

Conversation

scohen
Copy link
Collaborator

@scohen scohen commented Jul 13, 2023

I did not like how namespacing turned out: Two separate mix tasks, one of which had a lot of code that didn't have defined responsibilities. it was hard to understand, and left the _build directory in a bad state.

After looking at some of the files in a release, it occurred to me that we had all we needed was to perform namepacing on those files rather than messing up the _build directory with namespaced beams. This path lead to here, where we rewrite the artifacts in the release directory, and leave our current project alone.

The effect of this is that you can make namespacing happen much more quickly, without the clean / build / namespace / clean process that we had before.

Oh yeah, I parallelized writing the beams. It's super fast now.

This approach has the following advantages:

  • Dependencies added to remote_control will be auto-namespaced
  • There is one place where dependencies are managed, in namespace.ex
  • All logic for finding and applying namespacing to different file types, lives in separate modules
  • The _build directory is still usable after you run mix release
  • There is only one task
  • You don't need to clean and recompile dependencies before the task runs
  • It's super, super fast. Build times went down from from 1 minute to 8 seconds
  • this also fixes the problem with Jason's encoder

@scohen scohen force-pushed the rework-namespacing branch 4 times, most recently from a2cfe4d to 1fa3e66 Compare July 13, 2023 20:41
I did not like how namespacing turned out, two tasks, one with a lot
of code that didn't have defined responsibilities, it was hard to
understand, and left the code in a bad state.

After looking at some of the files in a release, it occurred to me
that we had all we needed was to perform namepacing on those files
rather than messing up the _build directory with namespaced
beams. This path lead to here, where we rewrite things in the
release's directory and its artifacts.

The effect of this is that you can make namespacing happen much more
quickly, without the clean / build / namespace / clean process that we
had before.

Oh yeah, I parallelized writing the beams. It's super fast now.
@scottming
Copy link
Collaborator

scottming commented Jul 14, 2023

There's a huge increase in speed.👍

one issues:

if NAMESPACE=1 mix release lexical --overwrite, then do mix release lexical --overwrite, we will encounter:

023-07-14T16:12:14.771990+08:00 error: ** Task 'Elixir.Lexical.Plugin.Runner.Coordinator' terminating, ** Started from <0.6132.0>, ** When function  == fun 'Elixir.LexicalCredo':diagnose/1, **      arguments == [#{'__struct__' => 'Elixir.Lexical.Project',env_variables => #{},mix_env => nil,mix_exs_uri => <<"file:///Users/scottming/Code/lexical/mix.exs">>,'mix_project?' => true,mix_target => nil,project_module => 'Elixir.Lexical.LanguageServer.MixProject',root_uri => <<"file:///Users/scottming/Code/lexical">>}],
 ** Reason for termination == , 
** 
  {function_clause,[{'Elixir.LexicalCredo',diagnose,[#{'__struct__' => 'Elixir.Lexical.Project',env_variables => #{},mix_env => nil,mix_exs_uri => <<"file:///Users/scottming/Code/lexical/mix.exs">>,'mix_project?' => true,mix_target => nil,project_module => 'Elixir.Lexical.LanguageServer.MixProject',root_uri => <<"file:///Users/scottming/Code/lexical">>}],[{file,"lib/lexical_credo.ex"},{line,24}]},
  {'Elixir.Task.Supervised',invoke_mfa,2,[{file,"lib/task/supervised.ex"},{line,89}]},
  {'Elixir.Task.Supervised',reply,4,[{file,"lib/task/supervised.ex"},{line,34}]},
  {proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,240}]}]}

2023-07-14T16:12:14.792863+08:00 error: crasher: initial call: 'Elixir.LexicalCredo':diagnose/1, pid: <0.6132.0>, registered_name: [], error: {function_clause,[{'Elixir.LexicalCredo',diagnose,[#{'__struct__' => 'Elixir.Lexical.Project',env_variables => #{},mix_env => nil,mix_exs_uri => <<"file:///Users/scottming/Code/lexical/mix.exs">>,'mix_project?' => true,mix_target => nil,project_module => 'Elixir.Lexical.LanguageServer.MixProject',root_uri => <<"file:///Users/scottming/Code/lexical">>}],[{file,"lib/lexical_credo.ex"},{line,24}]},{'Elixir.Task.Supervised',invoke_mfa,2,[{file,"lib/task/supervised.ex"},{line,89}]},{'Elixir.Task.Supervised',reply,4,[{file,"lib/task/supervised.ex"},{line,34}]},{proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,240}]}]}, ancestors: ['Elixir.Lexical.Plugin.Runner.Supervisor.TaskSupervisor','Elixir.Lexical.Plugin.Runner.Application.Supervisor',<0.163.0>], message_queue_len: 0, messages: [], links: [<0.165.0>], dictionary: [{'$callers',[<0.166.0>]}], trap_exit: false, status: running, heap_size: 17731, stack_size: 28, reductions: 16520; neighbours:

no plugin diagnostics

then do a NAMESPACE=1 mix release lexical --overwrite again, everything works again.

@scohen
Copy link
Collaborator Author

scohen commented Jul 14, 2023

@scottming thats due to the protocols, rebuild deps and things should continue working

@scohen
Copy link
Collaborator Author

scohen commented Jul 14, 2023

actually, @scottming, that seems like a problem with namespacing being applied to the plugin and reloading the module.
Restarting the server should take care of it.

@scottming
Copy link
Collaborator

actually, @scottming, that seems like a problem with namespacing being applied to the plugin and reloading the module.
Restarting the server should take care of it.

I've already reopened the editor. this problem still exists.

@scottming
Copy link
Collaborator

scottming commented Jul 15, 2023

And I found another bug:

if you rm -rf _build .lexical, then do NAMESPACE=1 mix release lexical --overwrite, it will encounter:

2023-07-15T09:05:43.471323+08:00 error: ** Task 'Elixir.LXRelease.Plugin.Runner.Coordinator' terminating, ** Started from <0.9330.0>, ** When function  == fun 'Elixir.LexicalCredo':diagnose/1, **      arguments == [#{'__struct__' => 'Elixir.LXRelease.Project',env_variables => #{},mix_env => nil,mix_exs_uri => <<"file:///Users/scottming/Code/lexical/mix.exs">>,'mix_project?' => true,mix_target => nil,project_module => 'Elixir.Lexical.LanguageServer.MixProject',root_uri => <<"file:///Users/scottming/Code/lexical">>}], ** Reason for termination == , ** {function_clause,[{'Elixir.LexicalCredo',diagnose,[#{'__struct__' => 'Elixir.LXRelease.Project',env_variables => #{},mix_env => nil,mix_exs_uri => <<"file:///Users/scottming/Code/lexical/mix.exs">>,'mix_project?' => true,mix_target => nil,project_module => 'Elixir.Lexical.LanguageServer.MixProject',root_uri => <<"file:///Users/scottming/Code/lexical">>}],[{file,"lib/lexical_credo.ex"},{line,24}]},{'Elixir.Task.Supervised',invoke_mfa,2,[{file,"lib/task/supervised.ex"},{line,89}]},{'Elixir.Task.Supervised',reply,4,[{file,"lib/task/supervised.ex"},{line,34}]},{proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,240}]}]}
2023-07-15T09:05:43.489678+08:00 error: crasher: initial call: 'Elixir.LexicalCredo':diagnose/1, pid: <0.9330.0>, registered_name: [], error: {function_clause,[{'Elixir.LexicalCredo',diagnose,[#{'__struct__' => 'Elixir.LXRelease.Project',env_variables => #{},mix_env => nil,mix_exs_uri => <<"file:///Users/scottming/Code/lexical/mix.exs">>,'mix_project?' => true,mix_target => nil,project_module => 'Elixir.Lexical.LanguageServer.MixProject',root_uri => <<"file:///Users/scottming/Code/lexical">>}],[{file,"lib/lexical_credo.ex"},{line,24}]},{'Elixir.Task.Supervised',invoke_mfa,2,[{file,"lib/task/supervised.ex"},{line,89}]},{'Elixir.Task.Supervised',reply,4,[{file,"lib/task/supervised.ex"},{line,34}]},{proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,240}]}]}, ancestors: ['Elixir.LXRelease.Plugin.Runner.Supervisor.TaskSupervisor','Elixir.LXRelease.Plugin.Runner.Application.Supervisor',<0.164.0>], message_queue_len: 0, messages: [], links: [<0.166.0>], dictionary: [{'$callers',[<0.167.0>]}], trap_exit: false, status: running, heap_size: 28690, stack_size: 28, reductions: 17271; neighbours:
2023-07-15T09:05:43.490007+08:00 error: supervisor: {local,'Elixir.LXRelease.Plugin.Runner.Supervisor.TaskSupervisor'}, errorContext: child_terminated, reason: {function_clause,[{'Elixir.LexicalCredo',diagnose,[#{'__struct__' => 'Elixir.LXRelease.Project',env_variables => #{},mix_env => nil,mix_exs_uri => <<"file:///Users/scottming/Code/lexical/mix.exs">>,'mix_project?' => true,mix_target => nil,project_module => 'Elixir.Lexical.LanguageServer.MixProject',root_uri => <<"file:///Users/scottming/Code/lexical">>}],[{file,"lib/lexical_credo.ex"},{line,24}]},{'Elixir.Task.Supervised',invoke_mfa,2,[{file,"lib/task/supervised.ex"},{line,89}]},{'Elixir.Task.Supervised',reply,4,[{file,"lib/task/supervised.ex"},{line,34}]},{proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,240}]}]}, offender: [{pid,<0.9330.0>},{id,undefined},{mfargs,{'Elixir.Task.Supervised',start_link,undefined}},{restart_type,temporary},{shutdown,5000},{child_type,worker}]
2023-07-15T09:05:43.494741+08:00 error: Task Elixir.LexicalCredo failed because {:function_clause, [{LexicalCredo, :diagnose, [%LXRelease.Project{root_uri: "file:///Users/scottming/Code/lexical", mix_exs_uri: "file:///Users/scottming/Code/lexical/mix.exs", mix_project?: true, mix_env: nil, mix_target: nil, env_variables: %{}, project_module: Lexical.LanguageServer.MixProject}], [file: 'lib/lexical_credo.ex', line: 24]}, {Task.Supervised, :invoke_mfa, 2, [file: 'lib/task/supervised.ex', line: 89]}, {Task.Supervised, :reply, 4, [file: 'lib/task/supervised.ex', line: 34]}, {:proc_lib, :init_p_do_apply, 3, [file: 'proc_lib.erl', line: 240]}]}

and just like you said, restarting the LSP will fix this. but I haven't tested in another project yest, just lexical itself.

@scohen
Copy link
Collaborator Author

scohen commented Jul 17, 2023

@scottming I think that second one is due to protocols being missing, can you check?

Improvement:
 Rather than call Namespace.init(), moved to computing values on load.

Bugfix:
 If namespacing was applied to the mix tasks, the module attributes
 would get rewriteten, which meant the root modules were incorrect for
 several apps. Having them as strings "hides" them from the
 namespacing code, and things work on first load again.
@scohen
Copy link
Collaborator Author

scohen commented Jul 17, 2023

@scottming please take another look, I made a change that addresses both issues.

Namespaced completions are noise, we don't ever want them to show up.
@scottming
Copy link
Collaborator

This problem is fixed: #266 (comment)

And this problem still exists: #266 (comment), you can reproduced by: NAMESPACE=1 mix release lexical && mix release lexical --overwrite, then check the project.log, but interesting thing is it can be fixed by remove the .lexical.

@scohen
Copy link
Collaborator Author

scohen commented Jul 18, 2023

@scottming I'm not seeing that problem, can you give me detailed reproduction steps?

Like:
What's the initial state of the build directory? Has it been cleaned? Has it been compiled?
Is the .lexical directory present?

In other words, remove the build and deps directories and give me instructions to reproduce the error.

NAMESPACE=1 mix release lexical && mix release lexical --overwrite

This is not a valid command, it namespaces the first time and doesn't namespace the second time, and that definitely causes problems.

@scottming
Copy link
Collaborator

This is not a valid command, it namespaces the first time and doesn't namespace the second time, and that definitely causes problems.

Okay, that's what I did. And it looks like we're now namespacing fast enough that we don't need to worry about this problem.

Copy link
Collaborator

@scottming scottming left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just some minor comments.

@scohen scohen merged commit a56fbac into main Jul 19, 2023
4 checks passed
@scohen scohen deleted the rework-namespacing branch July 19, 2023 14:12
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.

2 participants