Skip to content
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

Enable WINRT_LEAN_AND_MEAN #15215

Merged
merged 5 commits into from
Apr 24, 2023
Merged

Enable WINRT_LEAN_AND_MEAN #15215

merged 5 commits into from
Apr 24, 2023

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Apr 20, 2023

WINRT_LEAN_AND_MEAN removes a bunch of less often used parts of the C++/WinRT headers:

  • std::hash specializations for every object
  • operator <<(ostream) overloads for any IStringable
  • Interface producers for interfaces that are marked "exclusive"

There's only one place where we were using even one of these.

Enabling this saves us (optimistically) 30 seconds of build time on the CI agents and shrinks our largest PCH (TerminalApp, x64, Debug) by about 150MiB.

It's not huge, but it's not nothing.

@DHowett
Copy link
Member Author

DHowett commented Apr 20, 2023

Okay, there is at least one place where we are using std::hash on a WinRT type -- this is a big "no" with WINRT_LEAN_AND_MEAN

@zadjii-msft
Copy link
Member

Relevant from moons ago: #9621 (comment)

@DHowett
Copy link
Member Author

DHowett commented Apr 20, 2023

Dang, excellent memory!

It looks like cppwinrt implements std::hash<winrt::T> by, uh... getting the IUnknown pointer and calling hash<void*>. I wonder if that even helps anything for the XAML objects we are hashing.

@DHowett
Copy link
Member Author

DHowett commented Apr 20, 2023

It takes about 17 minutes to build without this change (based on #15206, recent comparable)

@DHowett
Copy link
Member Author

DHowett commented Apr 20, 2023

It might have saved us around 30 seconds per leg on agent build. It also makes the PCHs smaller as a treat.

TerminalAppLib PCH (debug x64) before: 1955 MB
TerminalAppLib PCH (debug x64) after: 1799 MB

Comment on lines 19 to 29
namespace std
{
template<>
struct hash<::winrt::Windows::UI::Xaml::DataTemplate> : public hash_winrt_object_as_pointer
{
};
template<>
struct hash<::winrt::Windows::UI::Xaml::Controls::Primitives::SelectorItem> : public hash_winrt_object_as_pointer
{
};
}
Copy link
Member

@lhecker lhecker Apr 21, 2023

Choose a reason for hiding this comment

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

You can also just pass hash_winrt_object_as_pointer directly as a trailing template parameter to the two hashmaps. That would seem cleaner to me. With using statements you can make the template names a bit shorter.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's fair. i was going for "maximal equivalence in minimal lines deleted". I'll probably move over to this

Comment on lines 12 to 16
size_t operator()(const ::winrt::Windows::Foundation::IUnknown& value) const noexcept
{
void* const abi_value = winrt::get_abi(value.try_as<::winrt::Windows::Foundation::IUnknown>());
return std::hash<void*>{}(abi_value);
}
Copy link
Member

Choose a reason for hiding this comment

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

The extra try_as seems redundant? It might make sense to do

size_t operator()(const auto& value) const noexcept
{
    return std::hash<void*>{}(winrt::get_abi(value));
}

or alternatively

size_t operator()(const auto& value) const noexcept
{
    return til::hash(winrt::get_abi(value));
}

Due to the novel design choices in std::unordered_map I suspect there's not a particularly big difference between the two though, so I don't think it matters what you pick.

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh. I copied this directly out of winrt/base.h.

@DHowett DHowett changed the title WIP Try building with WINRT_LEAN_AND_MEAN Enable WINRT_LEAN_AND_MEAN Apr 21, 2023
@DHowett DHowett marked this pull request as ready for review April 21, 2023 22:20
@zadjii-msft zadjii-msft enabled auto-merge (squash) April 24, 2023 21:42
@zadjii-msft zadjii-msft merged commit 2cfd73d into main Apr 24, 2023
@zadjii-msft zadjii-msft deleted the dev/duhowett/mean-and-lean branch April 24, 2023 22:14
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.

3 participants