puma icon indicating copy to clipboard operation
puma copied to clipboard

Set the worker process count automatically with WEB_CONCURRENCY=auto

Open codergeek121 opened this issue 1 year ago • 8 comments

Description

Implements the WEB_CONCURRENCY=auto feature described in #3437.

This implementation does not require concurrent-ruby at all (in puma).

An alternative could be to require "concurrent/utility/processor_counter" inline. I'd be happy to adapt 👍

Closes #3437.

Your checklist for this pull request

  • [x] I have reviewed the guidelines for contributing to this repository.
  • [x] I have added (or updated) 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.
  • [x] If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • [x] If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • [x] I have updated the documentation accordingly.
  • [ ] All new and existing tests passed, including Rubocop.

codergeek121 avatar Aug 09 '24 21:08 codergeek121

Would concurrent-ruby be required at that point? The rails application would only require it when the config/applicaton.rb file is required.

I think inlining the implementation could be a better idea since requiring outside of the bundler environment could also cause other problems.

rafaelfranca avatar Aug 09 '24 21:08 rafaelfranca

You're right, it wouldn't be required in most setups. I adapted the implementation to require processor_counter inline.

I also added concurrent-ruby as a runtime_dependency with the same version constraints as Rails' ActiveSupport.

codergeek121 avatar Aug 10 '24 08:08 codergeek121

I think I managed to write an integration test without requiring concurrent-ruby in the main process. Also got most Github Actions to pass, the failures seem unrelated/flaky.

codergeek121 avatar Aug 11 '24 20:08 codergeek121

@codergeek121

JFYI, The JRuby tests are failing due to expired certs. The Ubuntu-20.04 Ruby 3.1 job is probably an intermittent issue. I'll rerun it...

EDIT: Ubuntu-20.04 Ruby 3.1 job passed when run a 2nd time.

MSP-Greg avatar Aug 12 '24 00:08 MSP-Greg

Great, thank you for double checking :)

codergeek121 avatar Aug 12 '24 16:08 codergeek121

Are you able to rebase?

dentarg avatar Sep 21 '24 22:09 dentarg

Yes, I've been able to rebase 👍

codergeek121 avatar Sep 23 '24 12:09 codergeek121

This a general comment, and I haven't had time to check on it.

As an example, Etc.nprocessors reports 20 on my desktop, which is due to the Intel 'Hyper-threading'. I actually only have 10 cores. So, I'm not sure what the best/optimum setting would be, 10, 20, or somewhere in between?

I think concurrent/utility/processor_counter has methods for both (physical cores and 'Hyper-threading' cores).

MSP-Greg avatar Sep 23 '24 14:09 MSP-Greg

Hmm, Ruby 2.4 in macOS failed https://github.com/puma/puma/actions/runs/11664819104/job/32476175139?pr=3439

Seems related

  test/runner --verbose
  shell: /bin/bash -e {0}
  env:
    CI: true
    PUMA_TEST_DEBUG: true
    TESTOPTS: -v
    PUMA_NO_RUBOCOP: true
    TERM: BugMinitest
    PUMA_CI_RACK: 
    TMPDIR: /Users/runner/work/_temp
    PATH: /usr/local/opt/pipx_bin:/Users/runner/.cargo/bin:/usr/local/opt/curl/bin:/usr/local/bin:/usr/local/sbin:/Users/runner/bin:/Users/runner/.yarn/bin:/Users/runner/Library/Android/sdk/tools:/Users/runner/Library/Android/sdk/platform-tools:/Library/Frameworks/Python.framework/Versions/Current/bin:/Library/Frameworks/Mono.framework/Versions/Current/Commands:/usr/bin:/bin:/usr/sbin:/sbin:/Users/runner/.dotnet/tools
    PUMA_MAKE_WARNINGS_INTO_ERRORS: true

ruby 2.4.10p364 (2020-03-31 revision 67879) [x86_64-darwin19]
RUBYOPT: -r/Users/runner/hostedtoolcache/Ruby/2.4.10/x64/lib/ruby/site_ruby/2.4.0/bundler/setup
                         Puma::MiniSSL                   OpenSSL
OPENSSL_LIBRARY_VERSION: OpenSSL 1.1.1h  22 Sep 2020     OpenSSL 1.1.1h  22 Sep 2020
        OPENSSL_VERSION: OpenSSL 1.1.1h  22 Sep 2020     OpenSSL 1.1.1h  22 Sep 2020

Run options: --verbose --seed 51014

# Running:

TestWebConcurrencyAuto#test_web_concurrency_with_concurrent_ruby_available = 
Error: Process completed with exit code 1.

dentarg avatar Nov 04 '24 16:11 dentarg

Re-running it

dentarg avatar Nov 04 '24 16:11 dentarg

Nice! Thank you for finishing this up and the reviews :)

codergeek121 avatar Nov 04 '24 21:11 codergeek121

Something funky with the test... now we had this fluke in the macos-13 ruby 2.5 job https://github.com/puma/puma/actions/runs/11672946770/job/32502922503?pr=3544#step:10:929

  1) Failure:
TestWebConcurrencyAuto#test_web_concurrency_with_concurrent_ruby_available [/Users/runner/work/puma/puma/test/test_web_concurrency_auto.rb:27]:
--- expected
+++ actual
@@ -1,3 +1 @@
-# encoding: US-ASCII
-#    valid: true
-"0"
+"4"

dentarg avatar Nov 04 '24 23:11 dentarg

@dentarg

I also had test problems locally.

Wondering whether we should add concurrent-ruby to the main Gemfile, since

  1. It's cached by setup-ruby
  2. It's loaded on demand only if env['WEB_CONCURRENCY'] == 'auto'
  3. No need for a bundle install command for one test
  4. Using cli_server means the test won't load concurrent-ruby in the main test process

MSP-Greg avatar Nov 05 '24 01:11 MSP-Greg

That all sounds good to me 👍🏻

dentarg avatar Nov 05 '24 06:11 dentarg

See #3545

MSP-Greg avatar Nov 05 '24 17:11 MSP-Greg