Fix CUDA decode_jpeg writing into memory the caller's stream is still using - #9606
Fix CUDA decode_jpeg writing into memory the caller's stream is still using#9606karkuspeter wants to merge 1 commit into
Conversation
decode_images() allocates its output tensors while the caller's stream is current, then submits the nvJPEG work on the decoder's private pool stream. The caching allocator recycles a block as soon as it is freed on the stream that owns it, since all later use is supposed to be ordered on that same stream. The decoder is therefore handed memory that kernels queued on the caller's stream have not finished using, and nvJPEG overwrites it early, silently corrupting unrelated tensors in the calling thread. Record an event on the caller's stream once the buffers are allocated and have the decoder's stream wait on it. This is the missing counterpart to the syncStreams() that already makes the caller wait for the decoder afterwards.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9606
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Hi @karkuspeter! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks for the PR @karkuspeter , I'm not going to land it because I'm going to deprecate the torchvision decoders in the next version (~next month). The new jpeg cuda decoder is in TorchCodec now, which I just released in TorchCodec 0.16 https://meta-pytorch.org/torchcodec/stable/generated/torchcodec.decoders.decode_jpeg.html#torchcodec.decoders.decode_jpeg
Let me know if that one still has the same issue - happy to consider a fix in TorchCodec instead.
|
Thanks — I tested TorchCodec 0.16. It still corrupts, but not because of the wrapper: that design is One harness for both, on an H100 (driver 575.57.08, torch 2.13.0+cu129, released wheels). Each
Draining the caller's stream before the decode makes every row 0/200. The progressive rows are the discriminator: that JPEG takes nvJPEG's software path, which orders What remains on baseline JPEGs is nvJPEG itself. In a standalone CUDA program — no framework, one One correction to this PR. It is verified (199/200 corrupted before, 0/200 after, plus the regression For the TorchCodec side I've opened meta-pytorch/torchcodec#1634, which host-synchronizes the caller's This PR is your call, and either way is fine by me. 0.28 silently corrupts unrelated GPU memory today |
|
thanks, I'll close this and follow-up on meta-pytorch/torchcodec#1634 |
The bug
CUDAJpegDecoder::decode_images()gets its output tensors fromprepare_buffers(), which callstorch::stable::empty()while the caller's stream is current. It then submits the nvJPEG work onthe decoder's own stream, taken from torch's stream pool in the constructor. Nothing makes that
private stream wait on the caller's stream first.
That breaks the caching allocator's contract. The allocator hands a freed block straight back out for
reuse on the stream it was freed on, because every access to a block allocated on a stream is expected
to be ordered on that stream. So when the caller frees a tensor while kernels reading it are still
queued — an ordinary temporary going out of scope — the allocator can hand that same block to
prepare_buffers()on the nextdecode_jpeg()call, and nvJPEG writes the decoded image into it froman unordered stream, possibly long before those queued kernels run.
The damage does not show up in the decoded image. It shows up in whatever unrelated tensor was
overwritten: silently wrong values, and NaNs once the clobbered bytes are read as floats. No threads
and no explicit streams are needed in user code, only a caller with GPU work in flight, which is the
normal case when decoding is interleaved with compute.
decode_jpegs_cuda()already closes the other half of this loop: afterdecode_images()returns, itmakes the caller's current stream wait on the decoder's stream, so the caller cannot read the image
before it has been written. The missing half is the decoder waiting for the caller before it writes.
This predates the stable-ABI port (#9533). v0.28.0 and earlier allocate the outputs with
torch::empty()and decode onat::cuda::getStreamFromPool()in exactly the same way, so it is not aregression, and released versions are affected.
The fix
Record an event on the caller's current stream once the buffers exist, and have the decoder's stream
wait on it, through the
syncStreams()helper already in this file. It is the mirror image of the syncthat already runs after the decode.
Reproduction
Single-threaded, no explicit streams, public APIs only. Each iteration fills a few tensors with known
values and synchronizes, queues a long-running kernel followed by one reduction per tensor, frees the
tensors while those reductions are still queued, calls
decode_jpeg, then synchronizes and checks thereductions against their exact expected value. Nothing in that sequence is racy: a wrong sum can only
mean the decoder wrote into memory whose readers had not run yet.
repro.pyOn
main:The corrupted sums are arbitrary: NaN,
5.3e+44, small integers, whatever the decoded pixels happento be when read as floats.
Verification
Built from source on an H100 80GB, torch
2.12.0.dev20260408+cu128,mainat fc872e4, nativeallocator. Each leg below was run on a build of this exact tree, and on a build with only the C++
change reverted.
The reproducer and its controls, 200 iterations each:
mainrepro.pyrepro.py --decode nonerepro.py --decode cpurepro.py --stall 0repro.py --caller-stream sideThe controls matter as much as the failure: with no decode, with a CPU decode, or with nothing in
flight on the caller's stream, the harness is clean, so the corruption is specifically the CUDA decode
writing over the caller's queued reads.
--caller-stream sidecallsdecode_jpegwith a differentstream current from the one the freed tensors belonged to, so the outputs come from that stream's pool,
nothing aliases and nothing breaks — which is what makes the allocation-time stream the culprit.
The script's
--decode mimicleg replaces the decode with the same allocate-here/write-there patternwritten by hand in Python — allocate while the caller's stream is current, write from another stream,
host-sync that stream, no torchvision involved. It corrupts at the same rate on either build (19/20 in
a 20-iteration run), which is what pins the mechanism to the stream pattern rather than to nvJPEG.
The regression test added here, five consecutive runs each way, no flakiness in either direction:
No regressions in the existing tests.
pytest test/test_image.py -k "jpeg or cuda"selects 93 testsincluding the new one:
No measurable cost. Decode throughput for a 1920x1080 JPEG, A/B/A over three builds, 5x100 decodes
each:
blackandclang-formatare clean on the changed files;flake8reports only two pre-existing E231selsewhere in
test/test_image.py.Notes
record_stream(caller_stream)on them before returning, so the calling thread would never wait forits own queued work.
record_streamis not exposed through the stable ABI this file now uses(
torch/csrc/stable/c/shim.hoffers get/set current stream, stream-from-pool and streamsynchronize), so the event wait is the smaller change. Happy to go the other way if you prefer.
decode_images()already ends withcudaStreamSynchronize(stream), so the decode is host-synchronous either way. The patch only addsthe missing ordering edge.
torch.cuda._sleepto keep the caller's stream busy, and skips if the allocatorhappened not to hand the decoder one of the freed blocks, so it cannot fail spuriously. Happy to
replace the private helper with something else.
corrupted output from
encode_jpegon CUDA, and while Encode jpeg cuda sync #8929 added a sync there, the encoder capturescurrent_streamin its constructor, so a later call can end up synchronizing against a stream thatis no longer the caller's.