Sorry, something went wrong.
There was a problem hiding this comment.
This PR precomputes parameterized interface GUIDs to reduce expensive compile-time SHA-1 work in generated C++/WinRT projections and module builds.
Changes:
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file| cppwinrt/winmd_signature.h | Adds shared signature building, GUID extraction, and SHA-1 GUID computation helpers. |
| cppwinrt/helpers.h | Adds generic-instantiation discovery across interfaces, bases, and method signatures. |
| cppwinrt/code_writers.h | Emits guarded pinterface_guid<T> specializations for discovered instantiations. |
| cppwinrt/file_writers.h | Invokes generic specialization emission from namespace .0.h generation. |
| strings/base_identity.h | Adds a precomputed marker to the primary pinterface_guid template. |
| strings/base_reference_produce.h | Adds standard precomputed IReference / IReferenceArray GUID specializations. |
| natvis/type_resolver.cpp | Refactors natvis GUID generation to use shared WinMD signature helpers. |
| test/test/pinterface_guid_precomputed.cpp | Adds non-module coverage for precomputed GUID values and flags. |
| test/test/test.vcxproj | Includes the new non-module test source. |
| test/test_cpp20_module/pinterface_guid_precomputed.cpp | Adds module-boundary coverage for precomputed GUID values and flags. |
| test/test_cpp20_module/test_cpp20_module.vcxproj | Includes the new C++20 module test source. |
Sorry, something went wrong.
| // The pinterface signature format is: pinterface({open-type-guid};{arg-signature}) | ||
| // These signatures are part of the WinRT ABI specification and are immutable. | ||
| static void add_well_known_ireference_instantiations( | ||
| std::map<std::string, generic_inst_info>& instantiations) |
There was a problem hiding this comment.
Aren't all these in the winmds as interface attributes already? midlrt should be precomputing even the "no specified [uuid()]" interfaces?
Sorry, something went wrong.
There was a problem hiding this comment.
You're talking about the 2 guids under iref_guid and iref_arr_guid? Yes, I suppose they are. Since I was trying to unconditionally insert these instantiations even though they aren't mentioned, I guess I got a little greedy and hardcoded these interfaces so I wouldn't have to synthesize a GenericTypeInstSig and look up the various bits. But, yes, it's probably better to use the actual attributes from IReference in the winmd, and synthesize the GenericTypeInstSig so it shared the rest of the computation code.
But if you're talking about the pinterface guids for the various specializations, those aren't in the winmd. Yes, midlrt precomputes the guids for certain specializations and emits them in the header files, but that data does not go into the winmd.
Sorry, something went wrong.
There was a problem hiding this comment.
@jonwis The IReference logic is now "less special". I'm using the same GenericTypeIntSig path as the other specializations we discover in the winmds.
Sorry, something went wrong.
| } | ||
|
|
||
| // ---- SHA1 computation ---- | ||
| struct sha1_context |
There was a problem hiding this comment.
Is there a platform-standard crypto engine we should be using here instead of handrolling?
Sorry, something went wrong.
There was a problem hiding this comment.
I'll check it out. We're already handrolling in the constexpr compile-time code, so we've already broken the seal, but it's probably better to not have two separate implementations.
I know we have Windows.Security.Cryptography.Core.HashAlgorithmProvider. but I assume that won't work on non-Windows platforms, and I don't know if that API is available on all the Windows editions we need to run on. If this were app code, I'd be all over that, but I'm not too keen on converting stuff back and forth from a dynamically allocated IBuffer in this perf-sensitive code.
There is BCryptHashData, but has the same concerns around availability and cross-plat.
Outside of that, there aren't any truly standard implementations, but a fewseem to enjoy a degree of widespread use and seem to be somewhat trusted: OpenSSL, Crypto++, and Botan, with a heavy preference toward OpenSSL for the "official-ness". We aren't doing anything actually sensitive, just the SHA-1 hash (which will already raise some security warnings we'll need to deal with), but better safe than sorry if we're going to take on a third-party dependency.
Neither appear to have an official Nuget package (there are some that claim to have those packages, but with questionable provenance), so those are out. There are vcpkg bundles of each, though, so that could be an option, but cppwinrt has never had vcpkg support, so that's a whole other lift to add it to the build, CI, and official pipelines.
Sounds like something for "future us" to figure out...
Sorry, something went wrong.
There was a problem hiding this comment.
I think adding vcpkg hooks to the build is out of scope at the moment, but I want to take an IOU on that work, and use it to find an off-the-shelf SHA-1 implementation.
Sorry, something went wrong.
|
OMG, when I had the GitHub agent try to fix a build break, it completely nuked the PR title and description. Going to find the old one and change it back. |
Sorry, something went wrong.
Sorry, something went wrong.
C++/WinRT provides very powerful constexpr magic to allow users to calculate names and guids of parameterized types without requiring them to declare these instantiations in MIDL.
The catch is that the guid work is expensive because it involves, among other things, a SHA-1 hash computation. This happens at compile-time, so there is no runtime impact, but it does impact build times. The impact is particularly noticeable when many such pinterface specializations are needed, leading to build-time workarounds like https://github.com/microsoft/microsoft-ui-xaml/pull/209/changes
With this change, we resursively walk types in the namespace, looking for generic type instantiations in implmented/base interfaces, as well as method params and return types. We precalculate the guid of these generic types from inside cppwinrt.exe and emit them in the impl/.0.h namespace headers.
This can't possibly capture every possible generic instantiation, but at least we can streamline the types we find in the winmd. I also stamp out IReference pinterface specializations for the "standard" types.
For example, Windows.Foundation.Collections.IPropertySet will now trigger explicit specializations of winrt::impl::pinterface_guid for the following WFC types:
These type combinations may be encountered in multiple different namespaces, and unlike vanilla inline variables and functions, C++ compilers don't like it when you provide multiple explicit template specializations of the same type, even when those specializations are identical, so I'm guarding them in preprocessor guards, like so:
Benchmark results
I created a benchmark file that gets guid_of<T>() on 60 or so generics that get precomputed pinterface guids (IRerefence<T> of built-in types, aforementioned pinterfaces from IPropertyValue, etc), and built it 10 times with these changes, as well as with the current master baseline. I also timed how long cppwinrt.exe took, to measure the runtime impact of precalculating these guids.
I did some profiling, and somewhat surprisingly, the actual guid calculation is barely a blip in the increased cppwinrt.exe. The additional time is dominated by the additional metadata walk, specifically, getting signature blobs to obtain generic type instantiations, and params/returns for method signatures, because those tend to return a temporary std::vector of stuff instead of the lighter-weight table traversals. There's some opportunity to optimize the winmd reader here, along with making type resolution (aka find_required) faster than the current std::map approach.