Skip to content

Perf/default shared prefix catalog capacity - #274

Open
giveen wants to merge 2 commits into
Neroued:masterfrom
giveen:perf/default-shared-prefix-catalog-capacity
Open

giveen wants to merge 2 commits into
Neroued:masterfrom
giveen:perf/default-shared-prefix-catalog-capacity

Conversation

@giveen

@giveen giveen commented Sep 17, 2026 •

Copy link
Copy Markdown

Problem and scope

normalize_engine_options() defaults the Engine-wide shared-prefix catalog
(max_shared_prefixes) to max(max_concurrency, 4), but a single request can legitimately
produce up to 7 distinct shared-prefix candidates (4 explicit markers + 3 engine-automatic:
tool boundary, leading-instruction boundary, full prompt — src/models/qwen3_5/frontend/frontend.cpp:495,
out.opportunities.reserve(7U)). At low concurrency, the catalog fills from one or two requests'
own candidates. Ordinary client traffic (no explicit cache hints) gets DefaultAutomatic evidence,
which by design can only fill vacant catalog slots and never evict to make room
(docs/maintainer/resource-scheduling-and-context-cache.md, §7.2 evidence table). Once the
undersized catalog fills, affected requests permanently stop getting shared-prefix cache hits and
silently re-prefill their full prompt on every subsequent call — with no error, log warning, or
client-visible signal.

The maintainer's own scheduling doc already stated the correct 7-candidate ceiling two sentences
before citing the wrong constant as the default:
https://github.com/giveen/ninfer/blob/5b4303c0ea0e8ab2be3efa54a677829f3edab6e5/docs/maintainer/resource-scheduling-and-context-cache.md#L508-L514

Scope: this PR changes only the default value of max_shared_prefixes and its documentation. It
does not change the candidate-generation logic, the evidence/pressure model, or any explicitly
configured deployment (--max-shared-prefixes N continues to be honored verbatim).

Implementation

Where the code and docs said 4 (pre-fix, commit 5b4303c):

  1. The default computation itself:
    https://github.com/giveen/ninfer/blob/5b4303c0ea0e8ab2be3efa54a677829f3edab6e5/src/runtime/engine/model_instance.cpp#L115-L116
  2. The constant it borrowed (meant for explicit-marker count, not total per-request candidates):
    https://github.com/giveen/ninfer/blob/5b4303c0ea0e8ab2be3efa54a677829f3edab6e5/include/ninfer/types.h#L22
  3. Public EngineOptions header comment documenting the default as contract:
    https://github.com/giveen/ninfer/blob/5b4303c0ea0e8ab2be3efa54a677829f3edab6e5/include/ninfer/types.h#L130
  4. docs/serving.md CLI reference table:
    https://github.com/giveen/ninfer/blob/5b4303c0ea0e8ab2be3efa54a677829f3edab6e5/docs/serving.md#L788
  5. The maintainer doc's own default-sizing rationale (self-contradicting the line cited above):
    https://github.com/giveen/ninfer/blob/5b4303c0ea0e8ab2be3efa54a677829f3edab6e5/docs/maintainer/resource-scheduling-and-context-cache.md#L512-L514

Fix, commit 0f5f96f:

Tradeoffs considered: catalog slot count and actual Device/Host KV residency are independent
capacity axes in this design (docs/maintainer/resource-scheduling-and-context-cache.md, §3.1) —
raising the default from 4 to 7 slots adds bookkeeping/descriptor capacity, not additional KV
memory reservation by itself. I have not directly measured the marginal resident-memory or
workspace cost of the larger catalog; flagging this as unverified rather than asserting it is
negligible.

Verification

Unit-level (ninfer_context_cache_defaults_test, new in this PR): fails against the pre-fix
commit at max_concurrency 1 and 2 (default shared-prefix catalog capacity is smaller than one request's own candidate ceiling); passes post-fix at 1/2/8. Full cmake --build build -j
(478 targets: every product binary, every Op-level GPU test) is clean on both commits. Neighboring
suites unaffected: ninfer_serve_options_test, ninfer_resource_manager_test,
ninfer_kv_capacity_test, ninfer_admission_policy_test, ninfer_context_cost_test,
ninfer_request_log_test all pass unchanged.

End-to-end, real model, workload/hardware/toolchain as described in the linked Issue's
Environment section (Qwen3.8-27B NVFP4, RTX 5090, CUDA 13.3, ninfer-serve --max-concurrency 2 --spec mtp --kv-dtype k8v4 ..., identical flags in both configurations — only
--max-shared-prefixes differed). Methodology: one warmup request discarded per trial, then 5
distinct ~450-token prompts sent via plain POST /v1/chat/completions (no cache hints — ordinary
client traffic), then the same 5 prompts resent and timings.cache_n /
usage.prompt_tokens_details.cached_tokens read from each response. 3 trials per configuration,
fresh server restart before every trial to eliminate cross-trial catalog contamination.

--max-shared-prefixes 4 default (this PR, = 7)
Round-2 hits, all 3 trials 3/5, 3/5, 3/5 5/5, 5/5, 5/5
Round-2 token-level hit rate 1335/2250 (59.3%), identical all 3 trials 2221/2250 (98.7%), identical all 3 trials
Requests permanently re-prefilling from scratch 2/5 0/5

Zero variance across all 6 trials — the admission decision is deterministic for a fixed catalog
state and prompt sequence, so repetition here confirms reproducibility rather than characterizing
noise.

This is not a universal "N% more cache hits" claim — it is evidence the ceiling is real and the fix
removes it for the specific workload shape that exercises it (several genuinely distinct
shared-prefix candidates at low max_concurrency). Deployments at max_concurrency ≥ 7, or
traffic that never produces more than 4 distinct shared prefixes, were never affected by this bug.

Not verified / limitations:

  • Marginal resident-memory or descriptor-bookkeeping cost of a 7-slot vs. 4-slot catalog was not
    directly measured (see Implementation section).
  • Did not check for the same undersizing pattern at other call sites of
    kMaximumExplicitPromptCacheMarkers; only the one fixed here was confirmed.
  • Did not test at max_concurrency values other than 2 against the real model (unit test covers
    1/2/8 at the config-normalization level only, not end-to-end).

giveen and others added 2 commits September 16, 2026 17:51
… candidate set

normalize_engine_options() defaulted max_shared_prefixes to
max(concurrency, kMaximumExplicitPromptCacheMarkers) i.e. max(C,4), but a
single request can produce up to kMaximumPreparedPromptCacheCandidatesPerRequest
(7) shared-prefix candidates: four explicit markers plus the engine's
automatic tool/leading-instruction/full-prompt candidates
(frontend.cpp opportunities.reserve(7U)). At low concurrency the catalog
filled from one request's own candidates, and DefaultAutomatic-evidence
traffic (ordinary clients) can only fill slack, never evict, so once full,
those requests permanently stopped getting shared-prefix cache hits.

Verified end-to-end against the real Qwen3.8-27B model: 5 distinct prompts
sent twice each. Pre-fix (catalog=4): round-2 hits 3/5, two requests fully
re-prefilled. Post-fix (catalog=7): 5/5 hits.

Updates the two other places that documented the old max(C,4) default
(the public EngineOptions header comment and docs/serving.md) and the
maintainer scheduling doc's default-sizing rationale, which already stated
the 7-candidate ceiling two sentences earlier. Adds a regression test for
the default computation itself.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wallawalla47 pushed a commit to Wallawalla47/ninfer-custom that referenced this pull request Sep 17, 2026
… a request's full candidate set

# Conflicts:
#	docs/serving.md
#	include/ninfer/types.h
#	src/runtime/engine/model_instance.cpp
Wallawalla47 pushed a commit to Wallawalla47/ninfer-custom that referenced this pull request Sep 24, 2026
…d stop leases at their reach

- A turn that finishes after a later-submitted turn of its session already
  holds the session binding now ranks just below that binding. Publishing
  last made the stale branch look most recently used, so pressure evicted the
  session's current continuation first (prefix scenario `concurrent`).
- Device KV lease growth stops at the request's reach (its ceiling plus the
  drafts a round may verify past it). The cushion kept asking for pages beyond
  the ceiling, so a full pool near the end of an answer was taken for a space
  shortfall and cut the answer short (scenario `rewrite-checkpoint-shared`).
- The anthropic prefix scenario expects the default shared catalog sized for a
  request's full candidate set (merged upstream PR Neroued#274) rather than four.
- The prompt-token goldens are kept per model generation: the Qwen3.8 template
  adds a reasoning-effort instruction to every thinking prompt.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant