Skip to content

perf: unify benchmark workloads - #1944

Open
Benoît Cortier (CBenoit) wants to merge 13 commits into
cbenoit-headless-corpus-replayfrom
cbenoit-unified-benchmark-entry-points
Open

perf: unify benchmark workloads#1944
Benoît Cortier (CBenoit) wants to merge 13 commits into
cbenoit-headless-corpus-replayfrom
cbenoit-unified-benchmark-entry-points

Conversation

@CBenoit

@CBenoit Benoît Cortier (CBenoit) commented Sep 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Consolidate graphics, bulk, server encoding, and qualified partial replay workloads under ironrdp-bench.

Keep each selected replay workload independently preflighted and identifiable, with strict framebuffer fingerprints outside focused Criterion timing and a strict single-execution standalone measurement.

Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com

@github-actions github-actions Bot added risk/low Self-contained change with no cross-crate behavioral effect scope/core Touches the core architectural tier scope/tooling Build, CI, release, or developer tooling size/XXL Size: 1300 or more counted lines or 50 or more files labels Sep 10, 2026
github-actions[bot]
github-actions Bot reviewed Sep 10, 2026
View reviewed changes

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR folds the standalone benches crate into ironrdp-bench, fixes a real rfx_enc stride bug, uses representative nonzero fixtures with black_boxing and output checks, and adds manifest-qualified partial-replay workloads with strict preflight verification and a single-execution standalone measurement. Independent review found no correctness, safety, protocol, or API defects; behavior changes are intentional and well validated. Published findings are low-severity tooling cleanups: an unused bytesize dependency, a speculative public prepare_from_cache wrapper, a tautological usage test, manifest-validation logic duplicated from xtask, and a hardcoded workload list that can silently diverge from manifest performance entries. The derivable-criterion-string candidate is rejected because the manifest-declared exact Criterion identity is an intentional, README-documented validation feature, and the candidate itself concedes the tradeoff.

Comment thread crates/ironrdp-bench/Cargo.toml Outdated
Comment thread crates/ironrdp-bench/src/replay.rs Outdated
Comment thread crates/ironrdp-bench/src/bin/capture_replay_bench.rs Outdated
Comment thread crates/ironrdp-bench/src/replay.rs
Comment thread crates/ironrdp-bench/src/replay.rs
@github-actions github-actions Bot added the ai-reviewed/1 One automated review completed label Sep 10, 2026
@github-actions github-actions Bot added risk/medium Behavioral change that does not substantially alter a core public API and removed risk/low Self-contained change with no cross-crate behavioral effect labels Sep 10, 2026
@CBenoit
Benoît Cortier (CBenoit) added this pull request to stack #1955 September 11, 2026 01:24
Copilot AI reviewed Sep 11, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Criterion repeats expensive preflight work, and Tokio features are implicitly supplied by transitive dependencies.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Centralizes shared benchmark workloads in ironrdp-bench, adds qualified capture-replay measurements, and strengthens codec and bulk benchmarks.

Changes:

  • Adds strict Criterion and standalone capture-replay workloads.
  • Moves perfenc into ironrdp-bench.
  • Improves graphics and bulk benchmark fixtures and validation.
File summaries
File Description
xtask/src/main.rs Dispatches capture-replay benchmarks.
xtask/src/cli.rs Adds the capture-replay command.
xtask/src/bench.rs Parses and validates performance workloads.
xtask/README.md Documents benchmark workflows.
crates/ironrdp-server/src/encoder/rfx.rs Exposes encoded output sizes.
crates/ironrdp-bulk/benches/bulk_compression.rs Separates cold, history, and passthrough cases.
crates/ironrdp-bench/src/replay.rs Implements qualified replay preparation and validation.
crates/ironrdp-bench/src/lib.rs Exports shared workloads.
crates/ironrdp-bench/src/bin/perfenc.rs Adds the relocated server-encoding benchmark.
crates/ironrdp-bench/src/bin/capture_replay_bench.rs Adds standalone strict replay measurement.
crates/ironrdp-bench/corpus.toml Qualifies two performance captures.
crates/ironrdp-bench/Cargo.toml Registers dependencies and benchmark targets.
crates/ironrdp-bench/benches/capture_replay.rs Adds Criterion replay benchmarks.
crates/ironrdp-bench/benches/bench.rs Improves graphics fixtures and output checks.
Cargo.toml Removes the legacy benchmark workspace member.
Cargo.lock Updates benchmark package dependencies.
benches/src/perfenc.rs Removes the old benchmark binary.
benches/Cargo.toml Removes the legacy package manifest.
ARCHITECTURE.md Documents the consolidated benchmark package.
AGENTS.md Updates benchmark placement guidance.
Review details
  • Files reviewed: 19/20 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread crates/ironrdp-bench/Cargo.toml Outdated
Comment thread crates/ironrdp-bench/benches/capture_replay.rs
Comment thread xtask/README.md Outdated
@github-actions github-actions Bot added risk/low Self-contained change with no cross-crate behavioral effect and removed risk/medium Behavioral change that does not substantially alter a core public API labels Sep 11, 2026
@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-unified-benchmark-entry-points branch from 219b3f3 to deee19a Compare September 11, 2026 02:36
Copilot AI reviewed Sep 11, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The benchmark migration, validation contracts, tests, and documentation are internally consistent with no blocking issues identified.

Review details
  • Files reviewed: 19/20 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@github-actions github-actions Bot removed the ai-reviewed/1 One automated review completed label Sep 11, 2026
@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-unified-benchmark-entry-points branch from 45517f8 to aa7b2bf Compare September 11, 2026 08:33
@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-unified-benchmark-entry-points branch from aa7b2bf to 73a0c7b Compare September 11, 2026 08:36
@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-unified-benchmark-entry-points branch from 73a0c7b to 514ed04 Compare September 12, 2026 23:32
@CBenoit
perf: consolidate benchmark entry points
61f52e9 
Move the server encoding binary into the benchmark package and make\nits whole-process workload validate encoded output.\n\nDistinguish supported bulk compression streams from intentional\n16 KiB passthrough behavior, and use representative graphics fixtures.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
test: verify benchmark work
a363017 
Reject skipped compression and empty encoder output while running\nfocused benchmark workloads.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
perf: add qualified replay workloads
5b0257e 
Reuse prepared replay state for verified partial-capture processing\nbenchmarks and whole-process measurements.\n\nKeep codec setup outside focused timing and report history throughput\nfor every processed packet.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
perf: isolate replay benchmark measurements
798ad6b 
Preflight only the selected capture and hash its output outside\nfocused processing timing.\n\nKeep the standalone selector strict in one replay execution.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
perf(xtask): validate capture replay selector
bf47cfa 
Use manifest-declared eligibility and Criterion identities so invalid capture selectors fail before launching a no-op benchmark.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
docs(agents): document benchmark consolidation
9de12ec 
Record the consolidated benchmark package and remove obsolete root benchmark references.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
chore: simplify benchmark tooling
827e79c 
Remove unused benchmark code and verify that Criterion workloads match manifest-qualified captures.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
test(bench): isolate perfenc frame fixtures
3fdec2a 
Use unique temporary files for concurrent EOF and truncation tests.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
test(bench): use core atomic import
b814376 
Keep perfenc fixture-isolation tests compliant with workspace lints.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
fix(bench): declare Tokio runtime features
86b9474 
Document perfenc aggregate output validation accurately.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
fix: relocate benchmark corpus cache
3dcfd7b 
Load verified replay captures from the dedicated benchmark data root.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
refactor: simplify benchmark workloads
b469c74 
Reuse verified preflight packets and the shared replay-summary contract.

Validate encoder dimensions once at startup and reject unconsumed CLI input.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-unified-benchmark-entry-points branch from 514ed04 to b469c74 Compare September 12, 2026 23:34
@github-actions

github-actions Bot commented Sep 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This pull request may overlap with #1943.

Both add headless capture replay of qualified RDP captures, including a CLI to run a single capture and preflight verification, building on the same corpus/manifest approach. #1943's "headless corpus replay" with metadata-only gap fingerprints and manifest verification overlaps with this PR's PartialReplayWorkload (prepare/verify/replay), capture-replay-bench CLI, and xtask bench capture-replay command over corpus.toml captures no-nla-accepted and no-nla-smartcard.

This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide.

Note

LLM-assisted content (no human feedback).

@CBenoit
fix(xtask): keep benchmark metadata private
88087bc 
Match the private corpus-manifest boundary inherited from replay qualification changes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot mentioned this pull request Sep 12, 2026
Open
@github-actions github-actions Bot added the kind/protocol Affects RDP or related protocol behavior label Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments
Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@mamoreau-devolutions Marc-André Moreau (mamoreau-devolutions) mamoreau-devolutions approved these changes

Assignees

No one assigned

Labels

ai-reviewed/2 Final automated review completed kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/low Self-contained change with no cross-crate behavioral effect scope/core Touches the core architectural tier scope/tooling Build, CI, release, or developer tooling size/XXL Size: 1300 or more counted lines or 50 or more files triage/overlap This issue or pull request already exists or overlaps

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@CBenoit @mamoreau-devolutions