[Core][Multimodal] Skip redundant placeholder scan when token match succeeds - #52925
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment Once the PR is approved or has the If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. Agent GuidelinesIMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban. 🚀 |
|
@claude review |
DarkLight1337
added
the
verified
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review found no bugs. The design concern raised earlier in this thread (the early-return path bypassing processor-specific overrides of _apply_token_matches/_find_mm_placeholders) has been addressed in the latest commit: _apply_token_matches_with_placeholders is now an overridable instance method, and Gemma3/Gemma3n implement their own override that calls their custom _apply_token_matches and _find_mm_placeholders, so the fast path no longer silently skips their newline-merging logic. Because this touches core, performance-critical multimodal prompt-matching logic used by all models and changes the override contract subclasses rely on, a maintainer look at the final design is still worthwhile before merge.
Extended reasoning...
Overview
This PR optimizes _apply_prompt_updates in vllm/multimodal/processing/processor.py to skip the redundant _find_mm_placeholders rescan when all token matches succeed, by collecting placeholders directly during token matching via a new _apply_token_matches_with_placeholders helper. It also updates gemma3_mm.py and gemma3n_mm.py to override this new method (since they already override _apply_token_matches/_find_mm_placeholders for newline-token handling), and adds substantial new parametrized tests in tests/multimodal/test_processing.py.
Security risks
None identified. This is internal prompt/token processing logic with no user-facing input parsing changes, auth, or external data handling.
Level of scrutiny
This warrants above-average scrutiny: it changes a core code path (_apply_prompt_updates) exercised by every multimodal model on every request, and it changes the override contract for subclasses (previously _apply_token_matches/_find_mm_placeholders; now also _apply_token_matches_with_placeholders). A subtle mismatch here could silently produce wrong placeholder positions for some model, which is hard to catch without targeted testing.
Other factors
The PR went through several rounds of maintainer feedback (DarkLight1337) about exactly this override-bypass concern, and the current commit implements the maintainer's own suggested fix (explicit overridable method with Gemma3/Gemma3n implementations) rather than the previous ad hoc runtime-type-check workaround. Verified that Gemma3 and Gemma3n are the only two processors in the repo overriding _apply_token_matches/_find_mm_placeholders, and both now have a corresponding _apply_token_matches_with_placeholders override, so no other processor is silently bypassed. Test coverage was expanded with a new test_apply_token_matches_with_placeholders test plus reuse of existing tokenized cases; I could not execute the test suite in this environment (no venv installed) so I relied on static review of the logic and diff.
DarkLight1337
left a comment
DarkLight1337
left a comment
There was a problem hiding this comment.
LGTM, thanks for your patience!
|
/ci run |
github-actions
Bot
added
the
ready
|
✅ Triggered Buildkite CI #84828 for commit |
…ucceeds (vllm-project#52925) Co-authored-by: shenyiqin <shenyiqin1@huawei.com> Signed-off-by: Zhu, Zufang <zufang.zhu@intel.com>
…ucceeds (vllm-project#52925) Co-authored-by: shenyiqin <shenyiqin1@huawei.com>
2 participants
Purpose
CLOSE #52924
_apply_prompt_updatesfirst calls_apply_token_matches. If any token match fails, it falls back to_apply_text_matches. The current code always calls_find_mm_placeholdersafter matching, even when all token matches succeed. This call scans for placeholders again through the following path:_find_mm_placeholders→find_mm_placeholders→_iter_placeholders→prompt[start:end] == content_tokens.The table below shows the placeholder scan time at different image resolutions with 2,500 input text tokens:
_apply_prompt_updates_find_mm_placeholdersfind_mm_placeholders_iter_placeholdersEach function in the scan path takes almost as long as
_apply_prompt_updates. This shows that placeholder scanning takes most of the total time. Theslice comparerow measuresprompt[start_idx:end_idx_full] == content_tokens_full, which is the main bottleneck. As image resolution increases, this comparison grows from 2.934 ms to 71.419 ms and from 57% to 95% of the total_apply_prompt_updatestime.When all token matches succeed, token matching already knows each placeholder position. The code does not need to scan the prompt again. This PR collects placeholders during token matching and returns early when all matches succeed. It therefore skips
_find_mm_placeholders. If any token match fails, the code still uses the existing text fallback path.The following benchmarks compare
_apply_prompt_updatesbefore and after this change. The first benchmark fixes the image resolution and varies the input text length. The second fixes the input text length and varies the image resolution.As the input text grows, the speedup increases from 2.6× to 13.9× at 720p and from 2.9× to 50.9× at 3k. With 2,500 input text tokens, the optimized
_apply_prompt_updatestakes 0.601–1.712 ms across the tested image resolutions. This gives an 8.5×–44.0× speedup. Both benchmarks skip_find_mm_placeholderswhen all token matches succeed.Behavior
Token-matching path: the code returns early only when every token match succeeds across all modalities (
update_idx is not None). The unit tests show that the new path produces the samenew_token_idsas the old path for general token-matching cases._apply_token_matches_with_placeholderscollects placeholders directly and removes empty modality lists.Text fallback path: if any token match fails, the code still calls
_apply_text_matchesand_find_mm_placeholders. This PR does not change the fallback path.For typical
PromptReplacementupdates, direct collection and rescanning return the same placeholder positions. ForPromptInsertion, inserted tokens may match adjacent tokens in the original prompt. A rescan can then select an earlier range that includes an original token. Direct collection records the exact range inserted by the update. The issue below tracks this case.Related work and performance
Related issue
Related to #52924.
This issue describes how
find_mm_placeholderscan return the wrong start position when tokens inserted byPromptInsertionoverlap with adjacent tokens in the original prompt. This PR records the inserted token position directly. It does not treat an adjacent original token as part of the placeholder.Related PR
Merged PR #51774 also improves multimodal prompt updates, but it targets a different bottleneck. This PR builds placeholders directly when all token matches succeed and skips the redundant
_find_mm_placeholdersscan. The results below measure this PR on top of #51774.In the tables,
#51774is the baseline after that PR merged.#51774 + This PRadds this PR.The setup matches #51774. It uses 1,000,000 input tokens and 10,000 image placeholders. Each placeholder expands to 50 tokens, so the updated prompt contains 1,490,000 tokens. This PR reduces average latency by 45.0%.
_apply_prompt_updates_apply_prompt_updatesWe measured 64 requests with one 1728×3072 image and 2,500 input text tokens. Average latency drops from 3.432 ms to 1.409 ms. Maximum latency drops from 13.749 ms to 1.481 ms, an 89.2% reduction. The new maximum is only 5.1% above the average. The standard deviation drops from 2.587 ms to 0.024 ms, which shows more stable latency.
_apply_prompt_updates_apply_prompt_updatesTest Plan
This PR adds
test_apply_token_matches_with_placeholders. The test reuses the five tokenized cases fromtest_find_update_tokensand covers:PromptInsertionandPromptReplacement;new_token_idsagainst manually constructed expected output when token matching succeeds;PlaceholderFeaturesInfofields (modality,item_idx,start_idx, andtokens) against manually constructed expected output;We also keep the refactored
test_find_update_tokensto verify that existing token-update results do not change.We run
test_apply_matches_many_shared_targets_scales_linearlyto verify that the new helper still uses the linearly scaling prompt-update planner.pytest tests/multimodal/test_processing.py::test_apply_token_matches_with_placeholders \ tests/multimodal/test_processing.py::test_find_update_tokens \ tests/multimodal/test_processing.py::test_apply_matches_many_shared_targets_scales_linearly \ -qTest Result