Problem/Motivation
Every GET /node/{node}/advanced-comment-threads/{field_name}/refresh does the following:
- loads every comment on the field;
- runs
access('view')andsubjectfield access on each one; - builds the whole payload;
- only then hashes it for the
ETag.
References: src/Controller/CommentFragmentController.php:249-293, src/CommentIndexRepository.php:156-248 and src/Element/CommentHistory.php:47-82.
A 304 saves bandwidth but no server work. Phase 0 (#3623896) made the cacheability merge linear and removed baseline/newCommentIds from the refresh payload, so anonymous 304s now happen. Measured on Drupal 11.4.6, PHP 8.4, MariaDB 11.4, with database cache bins and a warm entity cache:
- 1,000 comments: about 0.4 s → 0.3 s per poll.
- 10,000 comments: about 14 s → 3.8 s per poll, still about +340 MB peak memory, which is more than a common 256 MB
memory_limit.
An offline benchmark of a snapshot hit plus the per-user overlay, JSON encoding and hashing: about 1 ms at 1,000 comments and about 10 ms at 10,000.
Proposed resolution
Implement ADR 0009 phase 1 (docs/adr/0009-shared-refresh-snapshot-cache.md, "Decision" and "Plan"):
- Snapshot builder service. It produces a scalar-only snapshot per host, field, sort direction,
published_onlyand access variant:orderedIds,parentById,childCounts,versions, pending ids, created times, owners whoseuidis visible, and the host author's ids. It loads entities in chunks (about 200), resetsentity.memory_cachebetween chunks and keeps no entities. - Storage. A dedicated bin (for example
cache.act_thread) throughVariationCache. Do not use ChainedFast: every comment write would flush the APCu layer for the whole bin. The initial cacheability isuser.permissions. The final cacheability is what the access results bubbled, minus the snapshot's owncomment:Ntags, plusnode:N, the ACT settings config tag and one host tag,act_comments:{entity_type}:{entity_id}. - Invalidation. Invalidate the host tag from the existing ACT hooks in
src/Hook/CommentHooks.php:85-111(insert, update, delete). Invalidate both the old and the new host when an update moves a comment. The delete hook is required: core'sComment::postDelete()does not invalidate the host. - Staleness rules. Do not store when the final max-age is 0. Otherwise apply a safety TTL of about 15 minutes.
- Rebuilds. Rebuild under a lock with a short wait (1–2 s). After that, fall back to a last-good copy at most a few seconds old, or build without storing. Never hold PHP workers for a whole large rebuild.
- Kill switch.
advanced_comment_threads.settings:refresh_snapshot_cache, on by default, with a schema entry and a post update. - Refresh.
refresh()andCommentHistory::lazyBuilder()take snapshot plus overlay (own/right-aligned ids, viewer signature, visit token, acknowledge URL). TheETagstays a SHA-256 of the JSON actually sent (ADR 0004).
Remaining tasks
- Implement the service, bin, invalidation, lock/fallback and kill switch.
- Kernel tests:
- hit, miss and invalidation on insert, update, move and delete;
max-age: 0access results are not stored;- per-user variants only when an access result adds
user; - anonymous responses are byte-identical across visitors.
- Update the test access hooks in
tests/modules/advanced_comment_threads_formatter_testthat read state without a tag ormax-age: 0. Under Drupal's access contract they must declare one (see ADR 0009 "Effects"). - Re-run the 1k/10k measurements; record hit latency and miss memory.
- Update
docs/loading-strategies.md"Known limitations" (the first bullet, around line 658).
User interface changes
None.
API changes
- New service and cache bin, both
@internal. - New config key
refresh_snapshot_cache. - The refresh payload shape is unchanged.
Data model changes
New config key only.
Comments
Comment #2
freelockComment #4
freelockDone in Beta1.