[Perf] Use the non-draining NPUStream getter in the kernel launch paths - #280
Conversation
`c10_npu::getCurrentNPUStream().stream()` calls `repo->MakeSureQueueEmpty()`, blocking the caller until the task queue has drained. The `stream(false)` overload returns the same `aclrtStream` without it. At every site changed here the stream is only captured and then consumed inside the lambda given to `OpCommand::SetCustomHandler()`, which the task queue executes in FIFO order, so ordering against previously enqueued work is guaranteed by the queue and the drain is redundant. `csrc/pac_kernels.cpp` already relies on this. `csrc/framework_hal.cpp` is left alone: it calls `device_index()`, which never drains. Measured on 910B2, CANN 8.5.1, torch_npu 2.9.0, the host-side cost of the `multi_layer_kv_transfer` call drops from 127.8 to 18.0 us for scatter and 88.6 to 20.4 us for gather. Every op whose stream acquisition changed keeps its exact checksum against a build of this tree without the patch. Fixes LMCache#219 Signed-off-by: zihanlin-ai <linzihan.huawei@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Also ran the repository's own kernel suites on 910B2 / CANN 8.5.1 / torch_npu 2.9.0.post1: |
|
Thanks for the contribution, I was wondering whether you can add a simple integration test with gsm8k or the likes ? Wdyt ? @chloroethylene |
|
Could you please provide some information on the testing results regarding accuracy, such as on MMLU or GSM8K? |
|
Validated |
|
GSM8K (250, Qwen3-8B/A3): base/head 85.6%; all outputs identical; wall +0.02%. No regression. |
|
@matthewygf @chloroethylene FYI—MMLU/GSM8K results above. |
|
lgtm! thanks for your contributions! |
Fixes #219
What changed
Replace all six remaining
c10_npu::getCurrentNPUStream().stream()calls in kernel-launch paths withstream(false).stream()drains the torch_npu task queue before returning. These streams areonly consumed by
OpCommandcustom handlers enqueued on the current stream, soqueue and device-stream ordering already preserve dependencies.
stream(false)returns the same
aclrtStreamwithout the redundant drain.csrc/pac_kernels.cppalready uses this form.csrc/framework_hal.cppisunchanged because
device_index()does not drain the queue.Validation
Environment: 910B2, CANN 8.5.1, torch_npu 2.9.0.post1.
Host-side
multi_layer_kv_transferlatency over 200 iterations:round trips were bit-exact.
TASK_QUEUE_ENABLE=0, both builds measured 40–44 us.csrc/mem_kernels.cppandcsrc/utils.cppalso overlap with #246; I can rebaseif needed.