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

jalevin · 2020-03-02T01:14:11Z

… 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.

… if using workers

History.md

lib/puma/configuration.rb

nateberkopec · 2020-03-02T02:42:24Z

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.

jalevin

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

lib/puma/configuration.rb

History.md

olleolleolle · 2020-03-02T05:50:47Z

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 · 2020-03-02T13:31:00Z

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

dentarg · 2020-03-02T16:44:09Z

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 · 2020-03-05T20:52:53Z

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.

… if using workers

jalevin · 2020-03-09T08:52:03Z

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 · 2020-03-09T09:00:33Z

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

History.md

lib/puma/configuration.rb

… fix_up_config_defaults

jalevin · 2020-04-20T07:04:11Z

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

jalevin · 2020-04-20T07:07:04Z

lib/puma/plugin.rb

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

    def workers_supported?


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

@nateberkopec

jalevin · 2020-04-20T07:13:46Z

lib/puma/plugin.rb

@@ -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?


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

@nateberkopec

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

fabioperrella · 2020-09-24T15:52:53Z

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 · 2020-09-24T17:06:54Z

@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 · 2020-09-24T17:22:24Z

@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 · 2020-09-24T19:34:44Z

great, tks!!

The default changed in Puma 5 with puma#2143.

This is what happened in puma#2143.

* 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>

The default value was changed in puma#2143

See https://github.com/puma/puma/blob/v5.2.2/lib/puma/dsl.rb#L615-L623 The default changed in Puma 5 with #2143. Docs later updated in #2481.

The default value was changed in puma#2143

See https://github.com/puma/puma/blob/v5.2.2/lib/puma/dsl.rb#L615-L623 The default changed in Puma 5 with puma#2143. Docs later updated in puma#2481.

add RAILS_MIN_THREADS, RAILS_MAX_THREADS, set default worker, preload…

b606a33

… if using workers

jalevin mentioned this pull request Mar 2, 2020

Make "puma-heroku" the default config #1949

Closed

jalevin added 2 commits March 1, 2020 16:25

fix formatting. update history

d37ba21

fix formatting

70e2ae9

nateberkopec requested changes Mar 2, 2020

View reviewed changes

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

jalevin commented Mar 2, 2020

View reviewed changes

lib/puma/configuration.rb Outdated Show resolved Hide resolved

History.md Outdated Show resolved Hide resolved

nateberkopec added feature waiting-for-review Waiting on review from anyone labels Mar 2, 2020

nateberkopec added this to the 5.0.0 milestone Mar 2, 2020

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 added 4 commits March 9, 2020 00:29

add RAILS_MIN_THREADS, RAILS_MAX_THREADS, set default worker, preload…

0593668

… if using workers

fix conflict

2ba0d6e

fix typo

5a95dae

stupid whitespace

ebc1422

nateberkopec requested changes Mar 9, 2020

View reviewed changes

History.md Outdated Show resolved Hide resolved

lib/puma/configuration.rb Outdated Show resolved Hide resolved

jalevin and others added 4 commits April 12, 2020 22:37

Look to use if user is pruning bundler. update cli tests.

d8b72e8

Merge branch 'master' into fix_up_config_defaults

e1a1750

fix formatting

d24455c

Merge branch 'fix_up_config_defaults' of github.com:jalevin/puma into…

b257dfe

… fix_up_config_defaults

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

remove comments, should be ready.. lets see if tests still fail

b2dad7b

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 added 4 commits April 19, 2020 17:48

ITS THE FINAL COUNTDOWWNNNNN.....(yeah, Ill rebase)

e552e6c

fix typo in test

3645f75

tweak for truffleruby

6200e52

add methods to detect truffle. exclude truffle from allowing workers

c12340b

jalevin commented Apr 20, 2020

View reviewed changes

fix test for truffle ruby

338099f

nateberkopec merged commit dc3b9b8 into puma:master Apr 21, 2020

murny mentioned this pull request May 25, 2020

[Security] Bump puma from 4.3.3 to 4.3.5 ualbertalib/jupiter#1655

Merged

dentarg added a commit to dentarg/puma that referenced this pull request Nov 7, 2020

Update preload_app! comment in dsl.rb

494645e

The default changed in Puma 5 with puma#2143.

dentarg mentioned this pull request Nov 7, 2020

No longer do phased-restart on systemd but works on run locally #2476

Closed

dentarg added a commit to dentarg/puma that referenced this pull request Nov 7, 2020

Upgrade guide: what setting WEB_CONCURRENCY does

5477d54

This is what happened in puma#2143.

dentarg mentioned this pull request Nov 7, 2020

Do not enable preloading by default #2483

Closed

NARKOZ added a commit to NARKOZ/puma that referenced this pull request Dec 6, 2020

Update default number of threads for MRI in docs

06d67ec

The default value was changed in puma#2143

NARKOZ mentioned this pull request Dec 6, 2020

Update default number of threads for MRI in docs [ci skip] #2505

Merged

8 tasks

dentarg mentioned this pull request Jan 27, 2021

Document workers and threads environment variables [changelog skip] #2536

Merged

dentarg added a commit that referenced this pull request Mar 22, 2021

preload_app! is not always disabled by default

5e0c1c0

See https://github.com/puma/puma/blob/v5.2.2/lib/puma/dsl.rb#L615-L623 The default changed in Puma 5 with #2143. Docs later updated in #2481.

dentarg mentioned this pull request Mar 22, 2021

preload_app! is not always disabled by default [ci skip] #2581

Merged

7 tasks

cjlarose pushed a commit that referenced this pull request Mar 22, 2021

preload_app! is not always disabled by default (#2581)

1acb30d

See https://github.com/puma/puma/blob/v5.2.2/lib/puma/dsl.rb#L615-L623 The default changed in Puma 5 with #2143. Docs later updated in #2481.

JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022

Update default number of threads for MRI in docs

3ddbe0f

The default value was changed in puma#2143

dentarg mentioned this pull request Sep 20, 2022

6.0.0 #2918

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jalevin commented Mar 2, 2020 •

edited

nateberkopec commented Mar 2, 2020

jalevin left a comment

olleolleolle commented Mar 2, 2020

nateberkopec commented Mar 2, 2020

dentarg commented Mar 2, 2020

nateberkopec commented Mar 5, 2020

jalevin commented Mar 9, 2020 •

edited

jalevin commented Mar 9, 2020

jalevin commented Apr 20, 2020 •

edited

jalevin Apr 20, 2020

jalevin Apr 22, 2020

jalevin Apr 20, 2020

jalevin Apr 22, 2020

nateberkopec Apr 22, 2020

jalevin Apr 22, 2020

fabioperrella commented Sep 24, 2020

nateberkopec commented Sep 24, 2020

jsmpereira commented Sep 24, 2020

fabioperrella commented Sep 24, 2020

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

		def workers_supported?

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

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

Conversation

jalevin commented Mar 2, 2020 • edited

Description

Your checklist for this pull request

nateberkopec commented Mar 2, 2020

jalevin left a comment

Choose a reason for hiding this comment

olleolleolle commented Mar 2, 2020

nateberkopec commented Mar 2, 2020

dentarg commented Mar 2, 2020

nateberkopec commented Mar 5, 2020

jalevin commented Mar 9, 2020 • edited

jalevin commented Mar 9, 2020

jalevin commented Apr 20, 2020 • edited

jalevin Apr 20, 2020

Choose a reason for hiding this comment

jalevin Apr 22, 2020

Choose a reason for hiding this comment

jalevin Apr 20, 2020

Choose a reason for hiding this comment

jalevin Apr 22, 2020

Choose a reason for hiding this comment

nateberkopec Apr 22, 2020

Choose a reason for hiding this comment

jalevin Apr 22, 2020

Choose a reason for hiding this comment

fabioperrella commented Sep 24, 2020

nateberkopec commented Sep 24, 2020

jsmpereira commented Sep 24, 2020

fabioperrella commented Sep 24, 2020

jalevin commented Mar 2, 2020 •

edited

jalevin commented Mar 9, 2020 •

edited

jalevin commented Apr 20, 2020 •

edited