perf(graphics): optimize RemoteFX decoding - #1793
Conversation
ironrdp-bench measures the encoder, but RDP clients spend their time in the decoder. Add native benchmarks and a WASM harness so that we can test WASM performance.
Benchmarks show anywhere from ~10% to 45% improvement.
bitvec::BitSlice incurs a bounds-check on each bit read. Elimnate the bounds-check by buffering 64 bits in a register so that each read becomes a shift and a mask. This results in a 3-3.5x speedup for rlgr::decode. Keep the original decoder so that we can use it as an oracle to test whether the new one produces identical results. I'll come back with a follow up change to apply this improvement to the ZGFX decoder and the SRL decoder.
| let n_index = compute_n_index(code_remainder); | ||
|
|
||
| let val1 = load_be_u32(try_split_bits!(bits, n_index)); | ||
| let val2 = code_remainder - val1; |
There was a problem hiding this comment.
This looked like a possible underflow.
github-actions
Bot
added
maintainer-required
|
This pull request is Please split it into focused pull requests that can each be reviewed on their own. When the parts build on each other, stacked pull requests let you open each one on top of the last without waiting for the one below to merge. Stacks require every branch to live in this repository, so from a fork, please open separate pull requests instead. Automated review resumes once the change is below the |
There was a problem hiding this comment.
Pull request overview
Adds native and WASM RemoteFX benchmarks and optimizes RLGR decoding with a buffered bit reader.
Changes:
- Adds deterministic frame, tile, and pipeline-stage benchmarks.
- Replaces bitvec-based RLGR reads with a buffered reader and regression tests.
- Updates
yuvto 0.8.17.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
AGENTS.md |
Lists the new benchmark crate. |
Cargo.lock |
Locks the new crate and yuv update. |
crates/ironrdp-graphics/Cargo.toml |
Updates yuv. |
crates/ironrdp-graphics/src/rlgr.rs |
Optimizes RLGR decoding and adds tests. |
crates/ironrdp-rfxbench/.gitignore |
Excludes generated WASM artifacts. |
crates/ironrdp-rfxbench/Cargo.toml |
Defines the benchmark crate. |
crates/ironrdp-rfxbench/README.md |
Documents benchmark usage. |
crates/ironrdp-rfxbench/benches/rfx_decode.rs |
Adds native Criterion benchmarks. |
crates/ironrdp-rfxbench/src/lib.rs |
Implements fixtures and decode drivers. |
crates/ironrdp-rfxbench/src/wasm.rs |
Exposes the WASM benchmark ABI. |
crates/ironrdp-rfxbench/tests/roundtrip.rs |
Validates generated fixtures. |
crates/ironrdp-rfxbench/wasm/countsimd.mjs |
Counts WASM SIMD instructions. |
crates/ironrdp-rfxbench/wasm/package.json |
Adds the WASM harness dependency. |
crates/ironrdp-rfxbench/wasm/run.mjs |
Runs and reports WASM benchmarks. |
| let (Ok(width), Ok(height)) = (u16::try_from(width), u16::try_from(height)) else { | ||
| return 0; | ||
| }; |
| // Capture each stage's input once, then restore it per iteration. The | ||
| // restore is a 8 KiB memcpy and is subtracted by measuring it too | ||
| // (`stage/restore`), which is far cheaper than the stages themselves. |
| #[cfg(target_arch = "wasm32")] | ||
| mod wasm; |
| ironrdp-pdu = { path = "../ironrdp-pdu", features = ["std"] } | ||
| ironrdp-session.path = "../ironrdp-session" |
| # Built wasm modules and the npm install for the harness. | ||
| wasm/build/ | ||
| wasm/node_modules/ | ||
| wasm/package-lock.json |
| - `ironrdp-rdpdr-native` — native RDPDR backend | ||
| - `ironrdp-rdpsnd-native` — native RDPSND backend | ||
| - `ironrdp-bench` — benchmarking harness | ||
| - `ironrdp-rfxbench` — RemoteFX decode benchmarks and fixtures (native + WASM) |
| num-derive.workspace = true # TODO: remove | ||
| num-traits.workspace = true # TODO: remove | ||
| yuv = { version = "0.8", features = ["rdp"] } | ||
| yuv = { version = "0.8.17", features = ["rdp"] } |
There was a problem hiding this comment.
issue: Avoid specifying minor versions.
suggestion: Bump in Cargo.lock only instead.
rationale: I assume the performance is impacted by this specific patch, but I don’t think it’s critical enough for us to lock in a specific version in the Cargo.toml manifest.
Benoît Cortier (CBenoit)
changed the title
Benoît Cortier (CBenoit)
added
the
ai-review/allow-oversized
GitHub Actions
Inactive
There was a problem hiding this comment.
The way testing is done in this PR is going against the general practices of IronRDP, as it’s typical with LLM-generated code in general, even though I’ve created skills and it should be mentioned in the AGENTS.md file, but I will say: it’s okay for now. I see a lot of similar code was merged recently, so I would rather do an intentional clean up later. I enabled the automatic reviewing for oversized PRs, that being said I think this can be split into smaller PRs: the benchmarks in one, and the actual optimizations in another. Ideally, I would like to keep some form of documentation of this effort, maybe a .md file in the docs/ folder? Open to ideas here. Also, I think we already have a benchmarking crate, so I wonder if it’s really necessary to create a new one again. I assume it’s because you need a WASM harness? We may need to think about how we organize benchmarks across the workspace in general.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Adds a benchmark suite for the various stages of the RemoteFX decode pipeline, then implements a couple changes that result in noticeable improvements to the metrics.
The optimized bit reader can likely be reused in a few places in the EGFX decode pipeline, but I'll save that for later to keep the change focused on RemoteFX.
This is my first foray into Rust benchmarking, and I also wanted a way to run the benchmarks in WASM since the IronRDP client I work with most runs in the browser. Let me know if I've done something non-standard or if you have any suggestions for improvements.