Skip to content

[Spec][Ngram] 3/N: Fix synchronization issues in Ngram.cpp#21186

Merged
hnyls2002 merged 15 commits intosgl-project:mainfrom
kpham-sgl:kp/race-condition-fix
Mar 23, 2026
Merged

[Spec][Ngram] 3/N: Fix synchronization issues in Ngram.cpp#21186
hnyls2002 merged 15 commits intosgl-project:mainfrom
kpham-sgl:kp/race-condition-fix

Conversation

@kpham-sgl
Copy link
Copy Markdown
Collaborator

Motivation

Part of Ngram refactoring series, following #21181

Modifications

  • Fix a synchronization race where the insert queue could be drained before the worker finished updating the trie, causing synchronize() to return too early.
  • Replace the previous busy-wait polling with a condition-variable-based wait driven by a pending-insert counter.
  • Simplify worker shutdown by removing the separate quit_flag_ and relying on queue closure to terminate the background thread cleanly.

Accuracy Tests

Benchmarking and Profiling

Checklist

Review Process

  1. Ping Merge Oncalls to start the PR flow. See the PR Merge Process.
  2. Get approvals from CODEOWNERS and other reviewers.
  3. Trigger CI tests with comments or contact authorized users to do so.
    • /tag-run-ci-label, /rerun-failed-ci, /tag-and-rerun-ci
  4. After green CI and required approvals, ask Merge Oncalls to merge.

kpham-sgl and others added 12 commits March 23, 2026 04:29
- Rename module/extension (ngram_corpus_cpp), sources, and TrieCache→Trie
- Update ngram_worker and registered tests

Made-with: Cursor
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
synchronize() previously only checked if the queue was empty, missing
the window where the worker has dequeued an item but hasn't finished
inserting it. Use a pending_count tracked from asyncInsert through
insert completion, and wait on a condition_variable instead of spinning.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@sglang-bot sglang-bot force-pushed the kp/race-condition-fix branch from c5d14d4 to 4928754 Compare March 23, 2026 06:43
@hnyls2002
Copy link
Copy Markdown
Collaborator

/rerun-ut test_ngram_corpus.py

@hnyls2002
Copy link
Copy Markdown
Collaborator

/rerun-ut test_ngram_speculative_decoding.py

@github-actions
Copy link
Copy Markdown
Contributor

✅ Triggered /rerun-ut on 1-gpu-5090 runner:

cd test/ && python3 registered/spec/utils/test_ngram_corpus.py

@github-actions
Copy link
Copy Markdown
Contributor

✅ Triggered /rerun-ut on 1-gpu-h100 runner:

cd test/ && python3 registered/spec/test_ngram_speculative_decoding.py

@github-actions
Copy link
Copy Markdown
Contributor

🔗 View workflow run

@github-actions
Copy link
Copy Markdown
Contributor

🔗 View workflow run

@hnyls2002
Copy link
Copy Markdown
Collaborator

Condition variable + pending_count_ is a cleaner sync than spinning on queue empty; nit: confirm insert_queue_.close() always unblocks dequeue after shutdown so the worker thread cannot sit forever with pending_count_ > 0.

Interesting, can you open a fix for that?

@hnyls2002 hnyls2002 merged commit 59cb9a9 into sgl-project:main Mar 23, 2026
32 of 49 checks passed
0-693 pushed a commit to 0-693/sglang that referenced this pull request Mar 25, 2026
…ject#21186)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation npu run-ci speculative-decoding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants