Make Active Record's query cache an LRU #48110
Conversation
|
Lovely! I would be happy to no longer have to cover this in my workshops and conf talks 😆 50 feels low... but I'm unsure how to calibrate for the right number here. IIRC there is a branch in the Batches code too that avoids QueryCache. If QueryCache is LRU, do we still want that branch in there? Maybe not? QueryCache is already quite a hot path and not as fast as I would like... do we have a benchmark for it? |
The
No, I haven't benchmarked yet. But yes, I should do that. But I don't see a lot of opportunities for performance improvement here. |
Do you mean cache lookup, or the full relation-through-cache-hit-to-result flow? I expect the latter to be pretty slow [compared to caching an already-loaded relation, say], because this is such a low level cache.
Looks like it was the array allocation per cache lookup. Avoiding it in the no-binds case is an improvement, though still doesn't seem ideal. I don't have any better suggestions, though, short of separately LRUing each layer of the existing nested-hash structure. It's a pretty trivial implementation, but still maybe worth pulling out an LRU class? I recall thinking that an LRU would be useful somewhere at some point in the past... but it might've been for this very thing 😅 |
That's what Aaron remembers. But that was a decade ago, small allocations like this aren't as costly.
The "no-binds" case is rather rare though, I'm not even sure if it's worth it.
I guess if we used a dedicated class it would be easy to keep track the global size, but then it becomes hard to figure out which is the oldest inserted values. Either way I'm not convinced that this two-level hash is actually faster, I'll benchmark. |
|
Ok, so here's a benchmark: # frozen_string_literal: true
begin
require "bundler/inline"
rescue LoadError => e
$stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
raise e
end
gemfile(true) do
source "https://rubygems.org"
case ENV["RAILS"]
when "local"
gem "activerecord", path: File.expand_path(".")
when "edge"
gem "activerecord", github: "rails/rails"
else
gem "activerecord", "~> 7.0.0"
end
gem "sqlite3"
gem "benchmark-ips"
end
require "active_record"
require "minitest/autorun"
require "logger"
# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(nil)
ActiveRecord::Base.logger.level = Logger::INFO
conn = ActiveRecord::Base.connection
conn.class.class_eval do
public(:cache_sql)
end
binds = [ActiveRecord::Relation::QueryAttribute.new("id", "10", ActiveRecord::Type::Integer.new)]
Benchmark.ips do |x|
x.report("hit") do
conn.cache_sql("SELECT 1", "SQL", binds) { 1 }
end
end
Benchmark.ips do |x|
x.report("miss") do
conn.cache_sql("SELECT 0", "SQL", binds) { 1 }
conn.cache_sql("SELECT 1", "SQL", binds) { 1 }
conn.cache_sql("SELECT 2", "SQL", binds) { 1 }
conn.cache_sql("SELECT 3", "SQL", binds) { 1 }
conn.cache_sql("SELECT 4", "SQL", binds) { 1 }
conn.cache_sql("SELECT 5", "SQL", binds) { 1 }
conn.cache_sql("SELECT 6", "SQL", binds) { 1 }
conn.cache_sql("SELECT 7", "SQL", binds) { 1 }
conn.cache_sql("SELECT 8", "SQL", binds) { 1 }
conn.cache_sql("SELECT 9", "SQL", binds) { 1 }
conn.clear_query_cache
end
end7.0: main: This branch: Observations:
I'll profile a bit to see if I can close the gap a bit. |
|
Ok, so unsurprisingly the vast majority of the time is spent hashing the binds: https://bugs.ruby-lang.org/issues/18897 may make this significantly faster. I'll with Ruby 3.3 |
|
It helps a bit, but it's not stellar: I can't really think of any way to improve the performance though. That said, the |
|
Also thinking |
casperisfine
force-pushed
the
query-cache-lru
branch
from
3ee713e
to
ebd6b79
Compare
casperisfine
force-pushed
the
query-cache-lru
branch
2 times, most recently
from
12a9aaa
to
ea939a8
Compare
|
Ok, it's now controlled via I think the last thing here is to decide on the best default, but I feel like it's always going to be a bit arbitrary. |
casperisfine
force-pushed
the
query-cache-lru
branch
from
ea939a8
to
28b4415
Compare
I don't know how prevalent this really is, but I heard several time about users having memory exhaustion issues caused by the query cache when dealing with long running jobs. Overall it seems sensible for this cache not to be entirely unbounded.
casperisfine
force-pushed
the
query-cache-lru
branch
from
28b4415
to
89a5d6a
Compare
So I raised the default to 100. It's still very much arbitrary, but the idea is mostly to avoid pathological cases (the classic long running job). If someone has actual data, or strong opinion, I'm totally open to change the default. But ultimately a query result can be a handful of bytes or hundreds of megabytes. So no default will ever prevent all problems. |
| @@ -1,3 +1,25 @@ | |||
| * Active Record query cache is now evicts least recently used entries | |||
|
|
|||
| By default it only keeps the `50` most recently used queries. | |||
There was a problem hiding this comment.
That was fixed in #48192
I don't know how prevalent this really is, but I heard several time about users having memory exhaustion issues caused by the query cache when dealing with long running jobs.
Overall it seems sensible for this cache not to be entirely unbounded.
Opening this for feedback.
TODO:
Ping @tenderlove in (the unlikely) case you'd remember the context on 9ce0211 as I'm essentially reverting it.
Public complaints: