Is update_worker_count a good name? Could it be confused with worker processes?

How would one use this? Could you show an example dummy app using this?

As @dentarg mentioned.

Maybe change update_worker_count to update_thread_pool_min_max? I've never liked ThreadPool related methods using the term worker.

Re an example, it's not clear how to use this with several workers running (just a quick look)...

Thanks for your replies, @dentarg and @MSP-Greg! And apologies for a long delay. I have updated the method name to update_thread_pool_min_max. I am open to suggestions for a better name if you have any.

With regard to how we use this method, we set up a timer that periodically calculates the optimal worker count based on the metrics we collect with the gvl_timing gem, and then we call update_thread_pool_min_max to adjust the thread pool size. The code snippet below shows how we do this:

class OurAwesomeGem
  ...

  def start_timer
    @timer ||= Thread.new do
      loop do
        avg_duration, avg_io, avg_gvl = average_metrics_from_gvl_timing

        max = calculate_optimal_worker_count(avg_duration, avg_io, avg_gvl)

        # This is a `Puma::Server` object. `min` could always be 1, so this is setting `max` only.
        @server.update_thread_pool_min_max(max: max)

        sleep interval
      end
    end
  end
  
  ...
end

In order to grab the Puma::Server object, we use a Puma plugin that hooks into the Puma lifecycle events. The code
for the plugin is as follows:

# our_awesome_gem/lib/puma/plugin/our_awesome_gem.rb so this could be loaded with
# `--plugin our_awesome_gem`:
Puma::Plugin.create do
  def start(launcher)
    launcher.config.configure do |user_dsl, *|
      if launcher.config.options[:workers] > 0
        # Cluster mode:
        user_dsl.on_worker_boot do
          OurAwesomeGem.start_timer
        end

        user_dsl.on_thread_start do
          OurAwesomeGem.puma_server ||= Puma::Server.current
        end
      else
        # Single mode:
        user_dsl.on_booted do
          OurAwesomeGem.puma_server = Puma::Server.current
          OurAwesomeGem.start_timer
        end
      end
    end
  end
end

The second commit in this PR 1a3a682 has been added. Without this the Puma::Server.current would return nil in the on_thread_start hook when running in cluster mode. Similarly, the same call in the on_booted hook would return nil in single mode without 1a3a682.

Another thing I noticed was that It seems like Puma's lifecycle events and hooks had been added as people needed them, and there seems to be room for improvement in the API design. I wonder if we could think about how to make these events and hooks more predictable, regardless of cluster/single mode? Issues like sidekiq/sidekiq#6661 have already been reported, so it would be great if I could get some feedback on this.

@yuki24

Not sure if there was a reason for not doing so, if not, please rebase? I can have a look tomorrow. We may need an 'integration' test for this, especially if a clustered test is done...

@yuki24

Good day. By using the following, each request is sent, the response is generated, and the 'client' reads it. Then the next request is sent. So, it's not filling up the server with a 'backlog' of requests.

10.times { send_http_read_response(GET_11) }

The following does create a 'backlog' and the test passes. It creates the requests with a small delay in the app.

  def test_update_thread_pool_min_max
    @app = ->(env) do
      sleep 0.001
      [200, {}, [env['rack.url_scheme']]]
    end

    server_run(min_threads: 1, max_threads: 1, auto_trim_time: 1)

    assert_equal 1, @server.stats[:running]

    @server.update_thread_pool_min_max(min: 3, max: 3)

    # By making multiple requests, we can ensure that the pool is filled up to the max.
    req_ary = send_http_array 20
    resp_ary = req_ary.map { |req| req.read_response }

    assert_equal 3, @pool.max
    assert_equal 3, @pool.min
    assert_equal 3, @server.max_threads
    assert_equal 3, @server.min_threads
    assert_equal 3, @pool&.spawned
  end

EDIT:

I should probably have more coffee, but if one increases the ThreadPool#min, I'm not sure if it will affect a server that's 'idle'.

@yuki24 and @nateberkopec

I pushed the test fix, CI is passing. Question, and this pertains to the PR, and maybe Puma:

What happens if a min value is set higher than the max value? A warning would certainly be expected, the min value could be ignored, or the max could be set to the new min value. Not.sure.

EDIT: also, what happens if max is set lower than min? Again, not sure if this is covered

@MSP-Greg Thank you for reviewing! Really appreciated the test fix ❤️

What happens if a min value is set higher than the max value? A warning would certainly be expected, the min value could be ignored, or the max could be set to the new min value. Not.sure.

EDIT: also, what happens if max is set lower than min? Again, not sure if this is covered

I just merged your suggestion. I also think we may want to raise an exception in both the cases. At the same time, I don't think I fully understand the risk of raising an exception from within a web server. It's probably safer to just warn than an exception. What do you think?

For posterity: codex 5.2 xhigh found those two issues, I watched it verify them manually, and I attest they are correct.

Addressed the comments and rebased against main and the builds are all ✅ Let me know if there is anything else I can do here.