Memoize Site.instance to fix N+1-style DB round-trips - #687
Closed
maxkadel wants to merge 2 commits into
Closed
Conversation
Site.instance (upstream Hyku) uses first_or_create with no memoization, so every call is a fresh DB round-trip. Profiling a single /catalog page load found it called ~650-680 times via the `delegate :account, ..., to: :instance` line alone (see pals-stress-tests repo for the full profiling writeup). This test asserts the intended fix's behavior -- currently fails against unmemoized Site.instance. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Site.instance (upstream Hyku) uses first_or_create with no memoization -- every call is a fresh DB round-trip. Profiling found a single /catalog page load calls it ~650-680 times, almost entirely through the `delegate :account, ..., to: :instance` line (the ubiquitous Site.account accessor), not through one obvious N+1 loop. Memoizes via RequestStore (already a transitive dependency), matching how current_account is already memoized in ApplicationController and HykuHelper. RequestStore clears automatically at each request boundary, lining up with when Apartment re-resolves the tenant -- no risk of a stale Site leaking across tenants on a reused Puma thread. Verified locally (docker compose): confirmed genuine red (2 of 4 examples fail) with this file removed, and green with it restored. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
maxkadel
force-pushed
the
site-instance-memoization
branch
from
July 29, 2026 13:38
78572dc to
3d57ab1
Compare
maxkadel
changed the base branch from
pals-stress-test-staging-nginx-repro
to
main
July 29, 2026 13:39
maxkadel
marked this pull request as draft
July 29, 2026 14:32
maxkadel
marked this pull request as ready for review
July 29, 2026 14:54
Member
Author
|
I'm not sure why it's not showing up on the main PR, but CI is running here - https://github.com/notch8/palni_palci_knapsack/actions/runs/30469565277 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Site.instance(upstream Hyku,hyrax-webapp/app/models/site.rb) usesfirst_or_create, which is never memoized — every call is a fresh database round-trip./catalogpage load (see pals-stress-tests for the full writeup) foundSite.instancecalled ~650-680 times per page render, almost entirely through thedelegate :account, ..., to: :instanceline — i.e. the ubiquitousSite.accountconvenience accessor, not one obvious N+1 loop.app/models/site_decorator.rbmemoizing it per-request viaRequestStore(already a transitive dependency, no new gem needed) — the same pattern already used forcurrent_accountinApplicationController/HykuHelper.RequestStoreclears automatically at each request boundary, lining up with when Apartment re-resolves the tenant, so there's no risk of a staleSiteleaking across tenants on a reused Puma thread.hyrax-webappsubmodule directly, so this can be cleanly contributed back upstream to Hyku later.Test plan
spec/models/site_decorator_spec.rbfirst — confirmed it fails against unmemoizedSite.instance(first_or_createcalled 2x instead of 1x, different objects returned across calls) before adding the decorator.site_decorator.rb, including that the existing global-tenant (NilSite) branch and post-RequestStore.clear!behavior are both unaffected.docker compose(removed/restored the decorator file to directly confirm the red→green transition, not just trust the diff)./catalogrender dropped from ~1510 to 249 (~84%), the two dominant query patterns from the profiling are completely gone.🤖 Generated with Claude Code