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

Refactor code server to use a single queue #8744

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

josevalim
Copy link
Contributor

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.

Copy link
Contributor

github-actions bot commented Aug 25, 2024

CT Test Results

    2 files     70 suites   1h 5m 20s ⏱️
1 551 tests 1 309 ✅ 242 💤 0 ❌
1 767 runs  1 493 ✅ 274 💤 0 ❌

Results for commit 72f2c90.

♻️ 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

@josevalim
Copy link
Contributor Author

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 josevalim force-pushed the jv-single-queue branch 2 times, most recently from 83ceaf7 to 6baba91 Compare August 25, 2024 08:23
@kikofernandez
Copy link
Contributor

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

@josevalim
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@kikofernandez
Copy link
Contributor

Looking into it this week

@kikofernandez
Copy link
Contributor

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 kikofernandez added stalled waiting for input by the Erlang/OTP team testing currently being tested, tag is used by OTP internal CI and removed stalled waiting for input by the Erlang/OTP team labels Oct 16, 2024
@lucioleKi lucioleKi removed the testing currently being tested, tag is used by OTP internal CI label Oct 17, 2024
@kikofernandez kikofernandez added the testing currently being tested, tag is used by OTP internal CI label Oct 18, 2024
Copy link
Contributor

@kikofernandez kikofernandez 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
These changes make the code_server easier to follow.
We are running some internal tests and will merge next week if all is green

@josevalim
Copy link
Contributor Author

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. :)

@kikofernandez
Copy link
Contributor

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

@josevalim
Copy link
Contributor Author

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

@kikofernandez kikofernandez changed the base branch from master to maint October 24, 2024 16:38
@kikofernandez kikofernandez changed the base branch from maint to master October 24, 2024 16:38
@kikofernandez
Copy link
Contributor

kikofernandez commented Oct 24, 2024

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 kikofernandez changed the base branch from master to maint October 24, 2024 19:23
@kikofernandez kikofernandez added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Oct 24, 2024
@bjorng
Copy link
Contributor

bjorng commented Oct 25, 2024

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.

@josevalim
Copy link
Contributor Author

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

@kikofernandez
Copy link
Contributor

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

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 erlang#7466.
Closes erlang#8510.
@kikofernandez kikofernandez added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Oct 25, 2024
@kikofernandez kikofernandez merged commit 75d1c4e into erlang:maint Oct 25, 2024
20 checks passed
garazdawi added a commit to garazdawi/otp that referenced this pull request Nov 20, 2024
The PR erlang#8744 instroduced a call to read_file_info that needs to
be allowed for the testcase to pass.
garazdawi added a commit to garazdawi/otp that referenced this pull request Nov 25, 2024
The PR erlang#8744 instroduced a call to read_file_info that needs to
be allowed for the testcase to pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in code_server.erl (badmatch: error) Loading a module that contains on_load may hang
6 participants