Reduce calls to pool_configs

[eileencodes]

Updated commit message:

This is a partial fix for #45906 to allocate less objects when iterating
through pool configs. My original attempt to fix this wasn't feasible
due to how the query cache works. The idea to use a dedicated each_*
came from jonathanhefner and I incorporated it here.

Using the following script:

Benchmark.memory do |x|
  x.report("all") do
    ActiveRecord::Base.connection_handler.all_connection_pools.each { }
  end

  x.report("each") do
    ActiveRecord::Base.connection_handler.each_connection_pool { }
  end

  x.compare!
end

We can see that less objects are allocated with the each method. This is
even more important now that we are calling all_connection_pools in
more placed since #45924 and #45961.

Calculating -------------------------------------
                 all   408.000  memsize (     0.000  retained)
                         7.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
                each   328.000  memsize (     0.000  retained)
                         3.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
                each:        328 allocated
                 all:        408 allocated - 1.24x more

Note that if this is run again on this branch all_connection_pools
will have even more allocations due to the deprecation warning that was
introduced. However removing that shows similar numbers to the benchmark
here.

Co-authored-by: Jonathan Hefner jonathan@hefner.pro

We already have the pools that had the query cache enabled so while
looping through the pools to disable the query cache we can also release
the connection at the same time.

This should reduce calls to pool_configs from complete. There's no
performance difference in the benchmark-ips, but I can see that we no
longer hit pool_configs from this path.

This is related to #45906. We can't memoize the pool_configs
object easily (at least a ton of tests fail), but this will at least
reduce allocations. I'll continue investingating improvements.

[eileencodes]

Actually I'm not sure we can do this 😕 I realize now when the query cache is enabled we only collect the pools where it wasn't already enabled. So then in complete, the reason we were double looping is to ensure that all the pools get released, not just the ones that recently had the query cache turned on.

I think we'd need to bring back #41046 which was reverted due to causing slower response times in the GH API. @jhawthorn and @matthewd can you weigh in here?

[jonathanhefner]

Would dedicated each_* methods be appropriate here? Something like:

Diff

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_handler.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_handler.rb
index 6a3fd5bd55..58d65cbd02 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/connection_handler.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_handler.rb
@@ -96,6 +96,16 @@ def all_connection_pools
         connection_name_to_pool_manager.values.flat_map { |m| m.pool_configs.map(&:pool) }
       end
 
+      def each_connection_pool # :nodoc:
+        return enum_for(__method__) unless block_given?
+
+        connection_name_to_pool_manager.each_value do |manager|
+          manager.each_pool_config do |pool_config|
+            yield pool_config.pool
+          end
+        end
+      end
+
       def connection_pool_list(role = ActiveRecord::Base.current_role)
         connection_name_to_pool_manager.values.flat_map { |m| m.pool_configs(role).map(&:pool) }
       end
diff --git a/activerecord/lib/active_record/connection_adapters/pool_manager.rb b/activerecord/lib/active_record/connection_adapters/pool_manager.rb
index a5e450cfd1..9f2b695c49 100644
--- a/activerecord/lib/active_record/connection_adapters/pool_manager.rb
+++ b/activerecord/lib/active_record/connection_adapters/pool_manager.rb
@@ -23,6 +23,14 @@ def pool_configs(role = nil)
         end
       end
 
+      def each_pool_config(&block)
+        return enum_for(__method__) unless block
+
+        @role_to_shard_mapping.each_value do |shard_map|
+          shard_map.each_value(&block)
+        end
+      end
+
       def remove_role(role)
         @role_to_shard_mapping.delete(role)
       end
diff --git a/activerecord/lib/active_record/query_cache.rb b/activerecord/lib/active_record/query_cache.rb
index b825c202c3..e7d5e34232 100644
--- a/activerecord/lib/active_record/query_cache.rb
+++ b/activerecord/lib/active_record/query_cache.rb
@@ -27,14 +27,14 @@ def uncached(&block)
 
     def self.run
       pools = []
-      pools.concat(ActiveRecord::Base.connection_handler.all_connection_pools.reject { |p| p.query_cache_enabled }.each { |p| p.enable_query_cache! })
+      pools.concat(ActiveRecord::Base.connection_handler.each_connection_pool.reject { |p| p.query_cache_enabled }.each { |p| p.enable_query_cache! })
       pools
     end
 
     def self.complete(pools)
       pools.each { |pool| pool.disable_query_cache! }
 
-      ActiveRecord::Base.connection_handler.all_connection_pools.each do |pool|
+      ActiveRecord::Base.connection_handler.each_connection_pool do |pool|
         pool.release_connection if pool.active_connection? && !pool.connection.transaction_open?
       end
     end

That saves a portion of the allocations:

Benchmark.memory do |x|
  x.report("all") do
    ActiveRecord::Base.connection_handler.all_connection_pools.each { }
  end

  x.report("each") do
    ActiveRecord::Base.connection_handler.each_connection_pool { }
  end

  x.compare!
end

Calculating -------------------------------------
                 all   408.000  memsize (     0.000  retained)
                         7.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
                each   328.000  memsize (     0.000  retained)
                         3.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
                each:        328 allocated
                 all:        408 allocated - 1.24x more

[jhawthorn]

I think we'd need to bring back #41046 which was reverted due to causing slower response times in the GH API. @jhawthorn and @matthewd can you weigh in here?

Yeah, my recollection is that that was our issue. Because this returned all the pools, not just the ones we'd just enabled, it would end up disabling the query cache for some other pools instead of restoring pools to their original state.

[eileencodes]

I took @jonathanhefner's suggestion (and gave credit in the commit) to reduce allocations here. We aren't able to do the original change I tried because we want to release all connections, not only ones that had the query cache turned on. We really do need to get a test in for that at some point...although it may be kind of difficult to write.

[jhawthorn]

(non-blocking) we could probably reduce one more allocation, by directly setting pools = ActiveRecord::Base.connection_handler.each_connection_pool.reject .... I don't think we need concat, since reject will always return a new array and this is the only thing we're concatenating into it.