perf: document zero-initialized scratch buffers in Rust fallback functions#1474
Open
Nicolas0315 wants to merge 1 commit intomemorysafety:mainfrom
Open
Conversation
…tions In the Rust fallback functions (those ending in _rust), scratch buffers like `let mut mid = [[0i16; MID_STRIDE]; 135]` are zero-initialized but don't need to be — the C version (dav1d's mc_tmpl.c, looprestoration_tmpl.c, itx_tmpl.c) declares these as uninitialized stack variables. The MaybeUninit pattern was already applied successfully in cdef.rs (PR memorysafety#1397) where buffers are passed to ASM calls. However, in mc.rs, looprestoration.rs, and itx.rs, the Rust fallback buffers are used with safe Rust indexing, making MaybeUninit more invasive for limited benefit since these functions are only used when ASM is unavailable. No buffers were found inside loops that could be hoisted out. This commit adds TODO(perf) comments documenting each buffer that matches an uninitialized C counterpart, following the pattern established in PR memorysafety#1397 and memorysafety#1399. Files annotated: - src/mc.rs: 10 buffers across put_8tap, prep_8tap, put_bilin, prep_bilin (regular and scaled variants), and warp_affine functions - src/looprestoration.rs: 5 function-level annotations covering wiener_rust, selfguided_filter, sgr_5x5_rust, sgr_3x3_rust, sgr_mix_rust - src/itx.rs: 2 buffers in inv_txfm_add and inv_txfm_add_wht_wht_4x4
kkysen
requested changes
Mar 2, 2026
Collaborator
kkysen
left a comment
There was a problem hiding this comment.
I'm not sure how important fixing these are considering they're in the Rust fallback functions, where safety is probably more important than performance. But leaving comments is good. Just had a couple tweaks for them.
Comment on lines
+99
to
+101
| // before read, matching the uninitialized C counterpart in dav1d's itx_tmpl.c. | ||
| // Could use MaybeUninit to avoid zeroing, but this is a Rust fallback function | ||
| // (only used when ASM is unavailable). See PR #1397 and #1399. |
Collaborator
There was a problem hiding this comment.
Suggested change
| // before read, matching the uninitialized C counterpart in dav1d's itx_tmpl.c. | |
| // Could use MaybeUninit to avoid zeroing, but this is a Rust fallback function | |
| // (only used when ASM is unavailable). See PR #1397 and #1399. | |
| // before read, matching the uninitialized C counterpart in dav1d's `itx_tmpl.c`. | |
| // Could use `MaybeUninit` to avoid zeroing, but this is a Rust fallback function | |
| // (only used when asm is unavailable). See PR #1397 and #1399. |
Could you fix these for the others, too?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In the Rust fallback functions (those ending in
_rust), scratch buffers likelet mut mid = [[0i16; MID_STRIDE]; 135]are zero-initialized but don't need to be — the C version (dav1d'smc_tmpl.c) declares these as uninitialized stack variables.The
MaybeUninitpattern was already applied successfully incdef.rs(PR #1397) where buffers are passed to ASM calls. However, inmc.rs,looprestoration.rs, anditx.rs, the Rust fallback buffers are used with safe Rust indexing, makingMaybeUninitmore invasive for limited benefit since these functions are only used when ASM is unavailable.What this PR does
TODO(perf)comments documenting each buffer that matches an uninitialized C counterpartFiles annotated
src/mc.rs: 10 buffers acrossput_8tap_rust,prep_8tap_rust,put_8tap_scaled_rust,prep_8tap_scaled_rust,put_bilin_rust,prep_bilin_rust,put_bilin_scaled_rust,prep_bilin_scaled_rust,warp_affine_8x8_rust,warp_affine_8x8t_rustsrc/looprestoration.rs: 5 function-level annotations coveringwiener_rust,selfguided_filter,sgr_5x5_rust,sgr_3x3_rust,sgr_mix_rustsrc/itx.rs: 2 buffers ininv_txfm_addandinv_txfm_add_wht_wht_4x4_rustWhy not apply MaybeUninit here?
Unlike
cdef.rswhere the buffers are passed directly to ASM calls (alreadyunsafe), these buffers are used with safe Rust indexing. UsingMaybeUninitwould require either:unsafeblocks withassume_initcallsBoth approaches add complexity for functions that are only exercised when platform-specific ASM implementations are unavailable. The comments serve as documentation for future optimization if profiling shows these Rust fallbacks are performance-critical.
Related PRs
tmp_bufbefore calling padding and filter funcs #1397: AppliedMaybeUninitpattern incdef.rsfor ASM-passed bufferslr_bakonce (hoisted from the loop) infn rav1d_cdef_brow#1399: Related scratch buffer optimization