Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Active Record: remove the query cache #300

Merged
merged 1 commit into from
May 10, 2024

Conversation

casperisfine
Copy link
Contributor

@casperisfine casperisfine commented May 9, 2024

Followup: #297

The idea was to totally skip the native code that can't be optimized much, but I suppose benchmarking the binding code kinda make sense anyway.

@maximecb @eregon

------------  -----------  ----------  ---------  ----------  ------------  -----------
bench         interp (ms)  stddev (%)  yjit (ms)  stddev (%)  yjit 1st itr  interp/yjit
activerecord  185.3        3.5         85.5       4.8         1.95          2.17       
------------  -----------  ----------  ---------  ----------  ------------  -----------

Followup: #297

The idea was to totally skip the native code that can't be optimized
much, but I suppose benchmarking the binding code kinda make sense
anyway.
@k0kubun
Copy link
Member

k0kubun commented May 9, 2024

Don't we use this feature in production? If we do, we might want to make the cache hit ratio closer to an actual production environment, e.g. 1% cache miss and 99% cache hit. If the code to handle the cached case is used for 99% of the time, it's what matters the most.

@eregon
Copy link
Contributor

eregon commented May 9, 2024

If we do, we might want to make the cache hit ratio closer to an actual production environment, e.g. 1% cache miss and 99% cache hit.

The cache hit rate is likely to be very small for real apps, because the cache hit is only when doing exactly the same query twice (or more) in the same request, which I would think is rather rare. The cache does not work across requests: https://guides.rubyonrails.org/caching_with_rails.html#sql-caching

@k0kubun
Copy link
Member

k0kubun commented May 9, 2024

👍 The change seems fair in that case.

@casperisfine
Copy link
Contributor Author

The cache hit rate is likely to be very small for real apps,

That's what I thought a while ago, so I instrumented it in production hoping to make a cache to remove it, but the hit rate was surprisingly high (something like 10%, been a while).

Turns out, when you decouple code it's not uncommon for two pieces of code to need to fetch the same data, so you end up duplicating queries.

However I do agree that query cache hits should ideally not happen at all.

@casperisfine casperisfine merged commit 22e2b2f into main May 10, 2024
4 checks passed
@casperisfine casperisfine deleted the activerecord-remove-query-cache branch May 10, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants