otp icon indicating copy to clipboard operation
otp copied to clipboard

Refactor code server to use a single queue

Open josevalim opened this issue 1 year ago • 10 comments

Prior to this patch, the code server had two internal queues, one to track module loading and another to track on_load callbacks. This pull requests refactors the code to have a single queue, in order to fix bugs and improve maintainability.

Closes #7466. Closes #8510.

josevalim avatar Aug 25 '24 00:08 josevalim

CT Test Results

    2 files     70 suites   1h 5m 20s :stopwatch: 1 551 tests 1 309 :white_check_mark: 242 :zzz: 0 :x: 1 767 runs  1 493 :white_check_mark: 274 :zzz: 0 :x:

Results for commit 72f2c90f.

:recycle: This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

github-actions[bot] avatar Aug 25 '24 00:08 github-actions[bot]

Ideally this would be merged on maint because it fixes a regression on OTP 27 but it is a conceptually large change. My suggestion is to merge it to master and, once 27.1 is out, backport it to maint, which should give us time to battle test it for 27.2.

josevalim avatar Aug 25 '24 00:08 josevalim

Thanks for your contribution. I plan to look at this after ICFP (in 10 days) approx

kikofernandez avatar Aug 29 '24 07:08 kikofernandez

Please don't merge this yet. I have tried this branch in a project and, while I didn't notice any bugs, compilation times for Elixir (which loads several modules) was affected negatively.

josevalim avatar Sep 09 '24 14:09 josevalim

False alarm, I had mixed my Erlang versions. Overall, I didn't measure any performance degradation in my usual code loading benchmarks. :)

josevalim avatar Sep 09 '24 14:09 josevalim

Looking into it this week

kikofernandez avatar Sep 25 '24 13:09 kikofernandez

This was on hold due to other investigations regarding current issues in code_server. As soon as I am given green light, I review this PR

kikofernandez avatar Oct 14 '24 10:10 kikofernandez

Glad to hear so! 🎉 Code loading is a state machine and the previous version was encoding the states via anonymous functions and in two separate queues, which made it very hard to follow. :)

josevalim avatar Oct 18 '24 09:10 josevalim

Completely agree, it was pretty tricky to follow the logic. This was a great improvement.

Since the next OTP release 27.2 is around beginning of December, I am wondering if we can place this already on maint, instead of master. That gives us close to two months of testing

kikofernandez avatar Oct 18 '24 09:10 kikofernandez

As long as we are comfortable with potentially doing a 27.2.1 with this commit reverted, it sounds good to me. :D

josevalim avatar Oct 18 '24 09:10 josevalim

Update This will be merged in maint after I add the following types

on_load = #{} :: #{module() => {on_load_file(), client_pid(), on_load_pid()}},
loading = #{} :: #{module() => [{loading_action(), client_pid()}]}}).

kikofernandez avatar Oct 24 '24 16:10 kikofernandez

Please do some squashing of the commits before merging this. At least squash the second commit into the first, or squash all commits into one.

bjorng avatar Oct 25 '24 06:10 bjorng

@kikofernandez let me know if you want me to do the squashing. :)

josevalim avatar Oct 25 '24 06:10 josevalim

sure, go ahead! I will merge if our internal builds look good after lunch

kikofernandez avatar Oct 25 '24 06:10 kikofernandez