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

add RAILS_MIN_THREADS, RAILS_MAX_THREADS, set default worker, preload… #2143

Merged
merged 17 commits into from
Apr 21, 2020

Conversation

jalevin
Copy link
Contributor

@jalevin jalevin commented Mar 2, 2020

… if using workers

Description

closes #1949

Please describe your pull request. Thank you for contributing! You're the best.

Your checklist for this pull request

  • [ x] I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • [ x] I have added appropriate tests if this PR fixes a bug or adds a feature.
  • [ x] My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

History.md Outdated Show resolved Hide resolved
History.md Outdated Show resolved Hide resolved
lib/puma/configuration.rb Outdated Show resolved Hide resolved
lib/puma/configuration.rb Outdated Show resolved Hide resolved
@nateberkopec
Copy link
Member

My guess on test failures would be that they're related to the change of changing the default from single mode to cluster mode, which I'm not even sure is a good idea.

Copy link
Contributor Author

@jalevin jalevin left a comment

Choose a reason for hiding this comment

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

Is cluster mode enabled changing the number of workers from 0 to 1 ?

lib/puma/configuration.rb Outdated Show resolved Hide resolved
History.md Outdated Show resolved Hide resolved
@olleolleolle
Copy link
Contributor

I liked the PUMA_MIN_THREADS naming.

Would it be easier to create this PR if it was only about the ENV vars, and left out the other stuff? Could make this change smaller and easier to review.

PS: this is awesome, thanks for taking the time to work on it!

@nateberkopec
Copy link
Member

I'm fine with the size-factor of this PR. Need to think about the Single vs Cluster for MRI thing.

@nateberkopec nateberkopec added feature waiting-for-review Waiting on review from anyone labels Mar 2, 2020
@nateberkopec nateberkopec added this to the 5.0.0 milestone Mar 2, 2020
@dentarg
Copy link
Member

dentarg commented Mar 2, 2020

Regarding "changing the default from single mode to cluster mode", I assume it would be a change for Puma 5, but nonetheless I guess the before_fork stuff (e.g. you should do DB.disconnect) could be a surprise for people running the default and not knowing about cluster mode.

@nateberkopec
Copy link
Member

OK. Let's keep Puma in singlemode by default. No need to force cluster by changing workers to 1. With that plus the other changes outlined above, LGTM.

@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Mar 5, 2020
@jalevin
Copy link
Contributor Author

jalevin commented Mar 9, 2020

FYI: test/test_config.rg:43 is a flaky test. I need to research this more, but randomly config returns 2 instead of 0. I'm not sure if that is coming from a test helper or something

@jalevin
Copy link
Contributor Author

jalevin commented Mar 9, 2020

Builds failing with TimeoutEveryTestCase::TestTookTooLong: execution expired...

History.md Outdated Show resolved Hide resolved
lib/puma/configuration.rb Outdated Show resolved Hide resolved
@nateberkopec nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Apr 13, 2020
@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Apr 20, 2020
@jalevin
Copy link
Contributor Author

jalevin commented Apr 20, 2020

@nateberkopec @ukolovda It looks like truffleruby does not support forking. Does this mean that truffle also does not support workers?

@@ -109,7 +109,7 @@ def in_background(&blk)
end

def workers_supported?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method feels like it should be in the lib/puma/detect.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -109,7 +109,7 @@ def in_background(&blk)
end

def workers_supported?
return false if Puma.jruby? || Puma.windows?
return false if Puma.jruby? || Puma.windows? || Puma.truffle?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we check ::Process.respond_to?(:fork) as well?
Does that single check eliminate the need to check for jruby/windows/truffle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in my nitpick commit. Thanks for the work on this PR!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sweet!

@fabioperrella
Copy link

Hi folks! Is there a reason to set max_threads default to 5 in MRI?

I couldn't find the why in the discussions or related issues.

@nateberkopec
Copy link
Member

@fabioperrella long version: https://www.speedshop.co/2020/05/11/the-ruby-gvl-and-scaling.html

short version: high threadcounts increase contention for the GVL. too much GVL contention == increased latency.

@jsmpereira
Copy link

@nateberkopec Thanks for that resource :)

@fabioperrella I was going to mention, from https://github.com/puma/puma/blob/master/5.0-Upgrade.md#upgrade:

If you are running MRI, default thread count on Puma is now 5, not 16. This may change the amount of threads running in your threadpool. We believe 5 is a better default for most Ruby web applications on MRI. Higher settings increase latency by causing GVL contention.

@fabioperrella
Copy link

great, tks!!

dentarg added a commit to dentarg/puma that referenced this pull request Nov 7, 2020
The default changed in Puma 5 with puma#2143.
dentarg added a commit to dentarg/puma that referenced this pull request Nov 7, 2020
nateberkopec added a commit that referenced this pull request Nov 9, 2020
* Correct test name

* Update preload_app! comment in dsl.rb

The default changed in Puma 5 with #2143.

* Upgrade guide: make it more clear about preload and phased restart

* Clarify when preload_app! is on by default

* Remove trailing whitespace

* Upgrade guide: what setting WEB_CONCURRENCY does

This is what happened in #2143.

Co-authored-by: Nate Berkopec <nate.berkopec@gmail.com>
NARKOZ added a commit to NARKOZ/puma that referenced this pull request Dec 6, 2020
dentarg added a commit that referenced this pull request Mar 22, 2021
cjlarose pushed a commit that referenced this pull request Mar 22, 2021
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
@dentarg dentarg mentioned this pull request Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make "puma-heroku" the default config
7 participants