Skip to content

fix(try): defer top-level <esi:try> dispatch to drain_queue for parallel select()#55

Merged
vagetman merged 1 commit into
mainfrom
vadim/try-sync-fix
Jun 17, 2026
Merged

fix(try): defer top-level <esi:try> dispatch to drain_queue for parallel select()#55
vagetman merged 1 commit into
mainfrom
vadim/try-sync-fix

Conversation

@vagetman

Copy link
Copy Markdown
Collaborator

Summary

Processor::process_queue was popping each QueuedElement::Try at the head of the queue and calling process_try_block synchronously. process_try_block walks the attempt's includes through execute_isolated and then blocks on its own inner drain_queue, so every top-level <esi:try> ran to completion before the next sibling could start — turning the expected max(latency) across sibling tries into Σ(latency).

This change leaves Try elements on the queue (push_front + break, mirroring the existing PendingRequest include path) so the end-of-stream drain_queue can fan all attempts' includes into a single select() pool and await them in parallel — which is precisely what its TryBlockTracker / AttemptTracker / SlotEntry machinery was built for.

Impact

Real-world production trace: an aggregate template with 28 sequential <esi:try> blocks went from ~4s average latency down to ~600ms after the fix — close to the slowest single <esi:include> in the page, which is the expected lower bound for max(latency) parallel dispatch.

Likely origin

  • a13aef3 ("fix(try): includes inside attempt blocks now see correct variable state") added the inline process_try_block call and stripped select() out of drain_queue. In that world the inline handling was internally consistent.
  • 6747f57 ("refactor(try): parallel try-block dispatch with flat-buf slot tracking") restored a parallel select()-based drain_queue and added a QueuedElement::Try arm to its Step 1 walk — but did not remove the inline call upstream in process_queue. The parallel arm was therefore unreachable: every Try was consumed before it could be queued for drain_queue.

The fix is three lines removed and the rest replaced with a deferral that mirrors the PendingRequest include arm directly above it.

Trade-off

Static content emitted by the parser after a Try waits for end-of-input before drain_queue flushes it, instead of streaming out during parsing. Once drain_queue starts, content flushes incrementally in document order as each slot resolves, so the practical TTFB cost is bounded by how long after the first Try the input stream stays open — typically tens of milliseconds for a template body.

The previous inline path flushed that content earlier, but at the cost of Σ-latency dispatch across sibling tries, which dominates in real templates.

process_queue ran each top-level Try synchronously via process_try_block,
serializing sibling tries (Σ-latency instead of max). Push it back on
the queue and break, matching the PendingRequest path; drain_queue then
fans all attempts' includes through one select() pool.
@vagetman vagetman marked this pull request as ready for review June 16, 2026 19:49
@vagetman vagetman requested a review from kailan June 16, 2026 19:53

@kailan kailan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good and, based on my understanding, shouldn't have any adverse side effects.

@vagetman vagetman merged commit 3cb83ec into main Jun 17, 2026
3 checks passed
@vagetman vagetman deleted the vadim/try-sync-fix branch June 17, 2026 19:57
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.

2 participants