Skip to content

bugfix: synchronize MTP compute stream after draft/validate forward.#1838

Open
yingxudeng wants to merge 1 commit into
xLLM-AI:mainfrom
yingxudeng:bugfix/mtp-compute-stream-sync
Open

bugfix: synchronize MTP compute stream after draft/validate forward.#1838
yingxudeng wants to merge 1 commit into
xLLM-AI:mainfrom
yingxudeng:bugfix/mtp-compute-stream-sync

Conversation

@yingxudeng

Copy link
Copy Markdown
Collaborator

Description

Related Issues

Change Type

  • Bug fix
  • New feature
  • Performance improvement
  • Refactor
  • Documentation
  • Test
  • Build or CI

Pull Request Checklist

Thank you for contributing to xLLM. Before requesting review, please make sure the following items are complete.

PR Title and Commit Messages

  • The PR title and each commit message follow the xLLM commit format: <type>: <subject>.

Allowed types: feat, bugfix, docs, test, refactor, chore, style, revert, perf, model, build, release.
The subject should use clear English, start with a verb, include at least 4 words, and end with ..

Pre-commit Checks

  • I have installed pre-commit by running pip install pre-commit or an equivalent command.
  • I have installed the hooks with pre-commit install.
  • I have run pre-commit run --all-files and fixed any reported issues.

If you are unsure how to set up pre-commit, see the pre-commit documentation.

Self Review

  • I have self-reviewed the code according to .agents/skills/code-review/references/custom-code-style.md, especially code written or assisted by AI.
  • I have rebased this PR onto the latest main branch.

Build and Test Coverage

  • Tests have been added or updated as needed.
  • CUDA: python setup.py build test has passed on a CUDA machine.
  • NPU: python setup.py build test has passed on an NPU machine.
  • MLU: python setup.py build test has passed on an MLU machine.

Reviewer Notes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request modifies run_llm_no_sync_impl in mtp_worker_impl.cpp to synchronize the compute stream and assert on its return value. The review feedback points out that performing synchronization inside a function explicitly designed for asynchronous execution (no_sync) violates its contract and can hinder performance pipelining. Additionally, the return value of synchronize() should be declared as a plain int rather than int32_t to match the API signature, as per the repository style guide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread xllm/core/runtime/mtp_worker_impl.cpp
Comment thread xllm/core/runtime/mtp_worker_impl.cpp
@yingxudeng

Copy link
Copy Markdown
Collaborator Author

bugfix:
F20260627 17:21:22.615046 3253981 mtp_worker_impl.cpp:318] Check failed: state.token_id == token_id (539 vs. 19) MTP decode target state token mismatch, request_id=chatcmpl-2551906557222686169-xaLgCSacqP7RJtXV2rzbQv
*** Check failure stack trace: ***
@ 0x6f6310 google::LogMessage::SendToLog()
@ 0x6f2e0c google::LogMessage::Flush()
@ 0x6f68e0 google::LogMessageFatal::~LogMessageFatal()
@ 0xc770f0 xllm::MTPWorkerImpl::step_decode()
@ 0xcbd35c xllm::SpeculativeWorkerImpl::step()
@ 0xc02388 _ZZN4xllm10WorkerImpl10step_asyncERKNS_12ForwardInputEENUlvE_clEv
@ 0x31e3720 xllm::ThreadPool::internal_loop()
@ 0x31e4410 _ZNSt6thread11_State_implINS_8_InvokerISt5tupleIJZN4xllm10ThreadPoolC4EmN5folly8FunctionIFvvEEEbRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEUlvE_EEEEE6_M_runEv
@ 0xffff878c9da4 (unknown)
@ 0xffff7825fbb4 (unknown)
@ 0xffff782c7f4c (unknown)
@ (nil) (unknown)

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