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

Tune delays in user_sup #4895

Closed
wants to merge 2 commits into from
Closed

Tune delays in user_sup #4895

wants to merge 2 commits into from

Conversation

richcarl
Copy link
Contributor

Discovered while investigating startup and shutdown times:

  • The shell is delayed 100 ms because of the sleep time in the loop that checks if the user driver is started. By checking every 1 ms instead, this is more or less eliminated. Also, waiting a full 10 seconds until giving up seems way too much, so I lowered it to 5.
  • Waiting a whole second before killing the user driver seems excessive, since this holds up the whole shutdown sequence. I'm not sure what's a good number these days or if there are better solutions, but lowering it to 10 ms makes it less of a bother.
  • It seems brutal to terminate the system with halt() if running as slave node when the master user driver cannot be found. Changed to returning error just like in the normal case.

These patches are mainly meant as suggestions.

@@ -108,7 +111,8 @@ wait_for_user_p(N) ->
link(Pid),
{ok, Pid};
_ ->
receive after 100 -> ok end,
%% minimal sleep so the shell is not delayed more than needed
receive after 1 -> ok end,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there is a way to make 'user' send a reply back instead of busy-wait loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I don't like the "wait for name to become registered" method. Adding proper synchronization would be more elegant, but obviously a bigger change, and I wasn't sure if there was a reason somewhere for not relying on the user process to send an ack.
I'm not particularly worried about the busy-waiting though. A millisecond is still a fairly long time in this context.

Copy link
Contributor

Choose a reason for hiding this comment

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

When user is called (i.e. when a non-interactive shell is started) the name should always be registered when the apply returns as far as I can tell. However, for interactive shells user_drv is called which registers the user name asynchronously which is why the spinning is needed. At least that is how I read the code.

@richcarl Did you find the delay also in non-interactive shells?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there is no delay when using -noshell. This is still worth doing something about, because you can feel those extra 100 ms between typing 'erl' and getting the interactive shell prompt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garazdawi And yes, the user_drv process is spawned off and does its own registration, whereas the user.erl start code spawns the process and does the registration before returning. If there's no particular reason for letting these behave as they like, it would be straighforward to make both do a synchronized start.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's no particular reason for letting these behave as they like, it would be straighforward to make both do a synchronized start.

No reason that I can think of. We already synchronize today by spinning on the name, so might as well make the sync more explicit by making user_drv send an ack when it knows that user exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to try to implement ack for user_drv, or should we go with your current solution at the moment?

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label May 31, 2021
@garazdawi garazdawi self-assigned this May 31, 2021
@garazdawi garazdawi added the waiting waiting for changes/input from author label Jun 8, 2021
@garazdawi garazdawi added the bug Issue is reported as a bug label Apr 6, 2022
@garazdawi garazdawi changed the title Tune delays Tune delays in user_sup Apr 6, 2022
@garazdawi garazdawi added this to the OTP-26.0 milestone Apr 7, 2022
garazdawi added a commit to garazdawi/otp that referenced this pull request May 11, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request May 15, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request May 18, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request Jun 4, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request Jun 21, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request Jul 1, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request Jul 4, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request Jul 7, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request Jul 8, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request Aug 10, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request Aug 18, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request Aug 22, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
@garazdawi
Copy link
Contributor

Fixed in #6144

@garazdawi garazdawi closed this Aug 28, 2022
garazdawi added a commit to garazdawi/otp that referenced this pull request Aug 28, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request Aug 29, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
garazdawi added a commit to garazdawi/otp that referenced this pull request Aug 29, 2022
In order to avoid spinning in `user_sup` we start user
during the synchronous phase of `user_drv` startup.

See erlang#4895 for more details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM waiting waiting for changes/input from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants