Skip to content

feat: Cache parameters & return type during call_unsafe_wdf_function_binding macro expansion #295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Mar 20, 2025

Conversation

leon-xd
Copy link
Contributor

@leon-xd leon-xd commented Feb 14, 2025

Overview

Currently, each invocation of the call_unsafe_wdf_function_binding macro loads the entire types.rs file into memory, and parses it for relevant information. This is very inefficient because types.rs is generated to be over 200,000 lines long, and we are searching the entire file for just a few lines of information.

This PR instead introduces a caching mechanism that stores the relevant information from types.rs in storage, so that we don't have to re-parse the entire file every time we need to look up a type or return value. This is done in generate_derived_ast_fragments by generating a temporary scratch directory, dumping the relevant information into a file in that directory, and then loading that file into memory when we need to look up the parameters & return value. This generated file is only 32 KB, as opposed to types.rs, which is 5 MB.

Results

Using the crox tool we can evaluate the difference in performance of this change. The previous version of this implementation took around 3 seconds per invocation, as seen here:
image

After this change, the first invocation of this macro increases to a bit under 5 seconds:
image

But after each file is cached, each subsequent invocation of the macro takes around 3 milliseconds.
image

The time saved per build scales with the number of WDF function invocations, meaning this greatly improves the developer experience.

@wmmc88 wmmc88 changed the title feat: Caching parameters & return type during call_unsafe_wdf_function_binding macro expansion feat: Cache parameters & return type during call_unsafe_wdf_function_binding macro expansion Feb 15, 2025
@wmmc88 wmmc88 requested review from Copilot and wmmc88 February 15, 2025 02:07
Copy link
Contributor

@Copilot 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.

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

@NateD-MSFT
Copy link
Contributor

NateD-MSFT commented Feb 19, 2025

Low priority question: what does the benchmarking look like with

[profile.dev.build-override]
opt-level = 3

[profile.release.build-override]
opt-level = 3

set in cargo.toml to optimize macros used during build? This saved me significant time compiling a complex driver using lots of calls to call_unsafe_wdf_function_binding; I expect your change easily outperforms this, but I'm curious if enabling this in the cargo.toml still provides meaningful speedups with your change.

@wmmc88
Copy link
Collaborator

wmmc88 commented Feb 20, 2025

Low priority question: what does the benchmarking look like with

[profile.dev.build-override]
opt-level = 3

[profile.release.build-override]
opt-level = 3

set in cargo.toml to optimize macros used during build? This saved me significant time compiling a complex driver using lots of calls to call_unsafe_wdf_function_binding; I expect your change easily outperforms this, but I'm curious if enabling this in the cargo.toml still provides meaningful speedups with your change.

Oh that's interesting. I didn't know proc-macros always compile with no optimizations. If having opt-level = 3 does improve on top of what this pr does (I'm guessing if a driver uses one wdf call, it will use many... so call_unsafe_wdf_function_binding speedup should dominate over slowdown of the proc-macro's compilation), we should consider:

  1. Recommending everyone use this setting
  2. Adding it as part of our template for cargo-wdk

Something else of note from the cargo docs:

Note: When a dependency is both a normal dependency and a build dependency, Cargo will try to only build it once when --target is not specified. When using build-override, the dependency may need to be built twice, once as a normal dependency and once with the overridden build settings. This may increase initial build times.

Because of this, it may be worth using a more targeted approach:

[profile.dev.package.<PKG NAME>]
opt-level = 3

We can have the above snippet for all packages that:

  • are known to likely only be used in build scripts (ex. bindgen)
  • are known to take long time executing in our build in proc-macros or in build.rs (ex. bindgen, syn, ripgrep?, etc)

Copy link
Collaborator

@wmmc88 wmmc88 left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this and making this change! I've definitely been meaning to make this change for a long while now, but never had the time to :) Glad you could implement it and that its showing significant benefit

I requested various comments mostly on testing/structuring for testing, some small style things, and some potential perf bumps.

@leon-xd leon-xd requested review from wmmc88 and Copilot February 26, 2025 07:38
Copy link
Contributor

@Copilot 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.

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

crates/wdk-macros/tests/unit-tests-input/generated-types-robust.rs:13

  • [nitpick] There's an extra space before the period in the comment. Consider removing the space after TableIndex for consistency.
// Build script should ignore any entry that doesn't end in `TableIndex` .

wmmc88
wmmc88 previously approved these changes Feb 27, 2025
@gurry
Copy link
Contributor

gurry commented Feb 28, 2025

2. Adding it as part of our template for cargo-wdk

We can certainly add it to our templates. Just need to investigate and establish which build dependencies should receive this treatment.

FYI @krishnakumar4a4 and @svasista-ms. No action needed right now but something to consider for later.

@leon-xd leon-xd added this pull request to the merge queue Mar 20, 2025
Merged via the queue into microsoft:main with commit 93cf3c4 Mar 20, 2025
62 checks passed
@leon-xd leon-xd deleted the optimize-macro-expansion branch March 20, 2025 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants