Skip to content

Conversation

@berlin-with0ut-return
Copy link
Contributor

@berlin-with0ut-return berlin-with0ut-return commented Nov 13, 2025

I have addressed the comments mentioned in the old PR: https://github.com/microsoft/mu_plus/pull/775/files#diff-6a9f6ae56a29a5f8862f4f1ca7e054186247d11cabc77db1120f8a0928f25f94

This PR adds the benchmark efi shell app as well as basic infrastructure for building it.

The Makefile was largely generated by Copilot although i have tested and checked it for consistency.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a comprehensive benchmark tool for measuring UEFI service performance, including infrastructure for building and testing various boot and runtime services.

  • Implements benchmarking for 30 different UEFI services across controller, event, image, memory, misc, protocol, and TPL categories
  • Provides statistical analysis with rolling stats for performance metrics
  • Includes build infrastructure with cargo-make for EFI application compilation

Reviewed Changes

Copilot reviewed 17 out of 21 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
services_benchmark_test/src/lib.rs Main library with benchmark execution and markdown output formatting
services_benchmark_test/src/main.rs UEFI entry point that initializes and invokes benchmarks
services_benchmark_test/src/measure.rs Defines benchmark functions and iteration counts for 30 services
services_benchmark_test/src/error.rs Error handling types for benchmark failures
services_benchmark_test/src/bench/*.rs Individual benchmark implementations for each service category
Cargo.toml Workspace configuration with dependencies
rust-toolchain.toml Rust toolchain specification
Makefile.toml Build automation for EFI applications

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@makubacki
Copy link
Collaborator

@berlin-with0ut-return, thanks for adding the CI workflow and deny.toml files. I've seen a lot of updates, please notify my GitHub account when you're ready for PR reviews.

@berlin-with0ut-return berlin-with0ut-return marked this pull request as draft November 18, 2025 00:33
@berlin-with0ut-return berlin-with0ut-return marked this pull request as ready for review November 19, 2025 00:02
@berlin-with0ut-return
Copy link
Contributor Author

@makubacki this is now ready for review

@makubacki makubacki added the type:feature-request A new feature proposal label Nov 20, 2025
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment

Thanks for integrating Codecov - We've got you covered ☂️

Copy link

@Javagedes Javagedes left a comment

Choose a reason for hiding this comment

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

I have two overarching comments about the current state of the PR:

You are for-going the safe abstractions the BootServices trait was created for, by using all of it's the unchecked methods. This is creating unnecessary unsafe in your code that you have not documented. You need to switch to the safe versions where applicable (e.g. connect_controller is always unsafe so nothing you can do about that).

The second comment is that you need to handle the conditional compilation in a way other than "if we are not compiling for a UEFI target, then conditionally compile everything out". Eventually we will want tests for the bits of this code that is testable, and right now that is impossible.

@berlin-with0ut-return berlin-with0ut-return merged commit 672e86a into OpenDevicePartnership:main Dec 17, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:feature-request A new feature proposal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants