CPU can incur a slow path on non-contiguous magnitudes - #47351
Conversation
`stft[..., :-1]` produces a non-contiguous view, and squaring its magnitude keeps it non-contiguous. On some CPU backends (observed on a ROCm PyTorch CPU build) the subsequent `mel_filters.T @ magnitudes` matmul falls onto a pathological strided-GEMM path that runs ~8x slower (~43 ms vs ~3 ms for a 30 s audio window), dominating Whisper feature extraction. Forcing `magnitudes` contiguous costs ~0.18 ms but restores the fast matmul path, cutting per-request audio preprocessing substantially with no change in output. Signed-off-by: Varalakshmi Bayanagari <varalakshmi.bayanagari@amd.com>
vasqu
left a comment
There was a problem hiding this comment.
Happy to add the fix on 2 conditions
- Shorten the comment -->
cpu can incurr a slow path on non-contiguous magnitutes - Add a fast test that can show this or a benchmark with this PR so we can verify as well
Hi @vasqu, the posted benchmarks in the description are from before/after change. Kindly let me know if you need anymore information. |
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
|
@vbayanag I meant like a small benchmark script :D the numbers are impressive but imo for anyone stumbling on this, it's always nice to have the exact code that produced the numbers along with it |
Hi @vasqu, I added a test case in the relevant test location. Please check. Local testing shows 5x uplift. This number is different from what's in description, likely due to difference in the way metrics were extracted. The initial posted numbers were extracted from feature_extractor API call from the trace file of end-to-end inference testing of whisper model, whereas the uplift observed now is localised only to the matmul operation.
System details:
|
| self.assertTrue(pt_processed.input_features.dtype == torch.float32) | ||
|
|
||
| @require_torch | ||
| def test_torch_extract_fbank_features_contiguous_magnitudes(self): |
There was a problem hiding this comment.
Ok sorry for backtracking a bit but I'd rather have the benchmark within the PR description. A benchmark as test is probably brittle, wdyt?
My goal is just to have documentation that shows why it was done and for ppl to repro
There was a problem hiding this comment.
Hi @vasqu, I think that this small test case would be evidence of the change I am submitting. By having a test case that's part of the code, it stays persistent and can be removed upon future rebuttals.
If you still think benchmark is unnecessary, I can remove that part from the test case and leave the rest alone. I can post the benchmark script on description. Kindly let me know what you think
There was a problem hiding this comment.
I can post the benchmark script on description.
Yea I think this is the better option tbh. It doesn't really make much sense as standalone test. We should refer to the benchmark script in some shape or form, e.g. a PR reference (that has it in the description) or a gh gist.
There was a problem hiding this comment.
Done.
- I removed benchmark code from test case and added in the description.
- The test case now only verifies the validity of the matrix when changed to contiguous.
Removed median timing measurement for mel_filters matrix multiplication.
vasqu
left a comment
There was a problem hiding this comment.
Thanks, one small nit and can you run make style for CI?
| Correctness (and the non-contiguity of the raw view) is asserted so this | ||
| stays stable in CI; the per-call timings are printed for reference | ||
| (run with `-s`) but not asserted, since the size of the speedup is | ||
| backend-dependent. |
There was a problem hiding this comment.
Let's also add a reference to this PR in the description please
How to do that? |
|
You just have to run |
Signed-off-by: vara lakshmi bayanagari <varalakshmi.bayanagari@amd.com>
Done |
Signed-off-by: vara lakshmi bayanagari <varalakshmi.bayanagari@amd.com>
|
[For maintainers] Suggested jobs to run (before merge) run-slow: whisper |
CI recapDashboard: View test results in Grafana |
|
Merging it, thanks for your patience! |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
…7351) * Make Whisper mel magnitudes contiguous to avoid slow strided matmul `stft[..., :-1]` produces a non-contiguous view, and squaring its magnitude keeps it non-contiguous. On some CPU backends (observed on a ROCm PyTorch CPU build) the subsequent `mel_filters.T @ magnitudes` matmul falls onto a pathological strided-GEMM path that runs ~8x slower (~43 ms vs ~3 ms for a 30 s audio window), dominating Whisper feature extraction. Forcing `magnitudes` contiguous costs ~0.18 ms but restores the fast matmul path, cutting per-request audio preprocessing substantially with no change in output. Signed-off-by: Varalakshmi Bayanagari <varalakshmi.bayanagari@amd.com> * Added test case to assert and benchmark * enabled warmup in test case * Remove median timing function from tests Removed median timing measurement for mel_filters matrix multiplication. * Apply make style (ruff) Signed-off-by: vara lakshmi bayanagari <varalakshmi.bayanagari@amd.com> * Reference PR huggingface#47351 in test docstring; drop stale -s note Signed-off-by: vara lakshmi bayanagari <varalakshmi.bayanagari@amd.com> --------- Signed-off-by: Varalakshmi Bayanagari <varalakshmi.bayanagari@amd.com> Signed-off-by: vara lakshmi bayanagari <varalakshmi.bayanagari@amd.com>
…7351) * Make Whisper mel magnitudes contiguous to avoid slow strided matmul `stft[..., :-1]` produces a non-contiguous view, and squaring its magnitude keeps it non-contiguous. On some CPU backends (observed on a ROCm PyTorch CPU build) the subsequent `mel_filters.T @ magnitudes` matmul falls onto a pathological strided-GEMM path that runs ~8x slower (~43 ms vs ~3 ms for a 30 s audio window), dominating Whisper feature extraction. Forcing `magnitudes` contiguous costs ~0.18 ms but restores the fast matmul path, cutting per-request audio preprocessing substantially with no change in output. Signed-off-by: Varalakshmi Bayanagari <varalakshmi.bayanagari@amd.com> * Added test case to assert and benchmark * enabled warmup in test case * Remove median timing function from tests Removed median timing measurement for mel_filters matrix multiplication. * Apply make style (ruff) Signed-off-by: vara lakshmi bayanagari <varalakshmi.bayanagari@amd.com> * Reference PR huggingface#47351 in test docstring; drop stale -s note Signed-off-by: vara lakshmi bayanagari <varalakshmi.bayanagari@amd.com> --------- Signed-off-by: Varalakshmi Bayanagari <varalakshmi.bayanagari@amd.com> Signed-off-by: vara lakshmi bayanagari <varalakshmi.bayanagari@amd.com>
What does this PR do?
stft[..., :-1]produces a non-contiguous view. Asobserved on MI355X ROCm vLLM latest build image, the audio preprocessing functionmel_filters.T @ magnitudesfalls onto a strided-GEMM path that runs ~8x slower (~43 ms vs ~3 ms for a 30 s audio window), dominating Whisper feature extraction.Forcing
magnitudescontiguous costs ~0.18 ms but restores the fast matmul path, cutting per-request audio preprocessing substantially.Fixes # (issue)
Make the CPU op run on contiguous memory layout for ~8x speedup
Before/after (single 30 s window, ROCm vLLM image)
mel_filters.T @ magnitudes.contiguous()overheadBenchmark Code
Code Agent Policy
The Transformers repo is currently being overwhelmed by a large number of PRs and issue comments written by
code agents. These often are low-quality, or fix extremely minor issues that occur rarely or never in practice.
As a result, we're instituting a rule that first-time contributors should not use code agents to submit PRs or issues.
We'd also ask autonomous "OpenClaw"-like agents not to open any PRs or issues.
Issues/PRs from first-time contributors that violate this rule will probably just be closed without review, and we
might block you, especially if you open more than one or appear to be deliberately ignoring this. We especially do not
want new contributors to jump in on random issues to contribute an agent-written fix. This creates lots of noise
for reviewers and other users and will almost certainly get you blocked.
For more information, please read
CONTRIBUTING.md.Before submitting
Pull Request checks?
to it if that's the case.
Who can review?
@eustlb @ebezzam @vasqu