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

Common: Avoid std::function overhead in ScopeGuard #8630

Merged
merged 1 commit into from Feb 17, 2020

Conversation

leoetlino
Copy link
Member

So far in all our uses of ScopeGuard, the type of the callable is
usually just a lambda or a function pointer, so there is no need
to rely on std::function's type erasure.

While the cost of using std::function is probably negligible, it still
causes some unnecessary overhead that can be avoided by making
ScopeGuard a templated class. Thanks to class template argument
deduction in C++17 most existing usages do not even need to be changed.

See https://godbolt.org/z/KcoPni for a comparison between
a ScopeGuard that uses std::function and one that doesn't

So far in all our uses of ScopeGuard, the type of the callable is
usually just a lambda or a function pointer, so there is no need
to rely on std::function's type erasure.

While the cost of using std::function is probably negligible, it still
causes some unnecessary overhead that can be avoided by making
ScopeGuard a templated class. Thanks to class template argument
deduction in C++17 most existing usages do not even need to be changed.

See https://godbolt.org/z/KcoPni for a comparison between
a ScopeGuard that uses std::function and one that doesn't
@CookiePLMonster
Copy link
Contributor

Very good change, I had it in mind more than once in the past so glad to see it done. However, I am pretty sure m_finalizer doesn't need to be a std::optional and you can "null" it by assigning {} to it.

@leoetlino
Copy link
Member Author

Unfortunately that doesn't work for lambdas.

../Source/Core/Common/ScopeGuard.h:24:9: error: could not convert ‘((Common::ScopeGuard<Core::EmuThread(std::unique_ptr<BootParameters>, WindowSystemInfo)::<lambda()> >*)this)->Common::ScopeGuard<Core::EmuThread(std::unique_ptr<BootParameters>, WindowSystemInfo)::<lambda()> >::m_finalizer’ from ‘Core::EmuThread(std::unique_ptr<BootParameters>, WindowSystemInfo)::<lambda()>’ to ‘bool’
   24 |     if (m_finalizer)
      |         ^~~~~~~~~~~
      |         |
      |         Core::EmuThread(std::unique_ptr<BootParameters>, WindowSystemInfo)::<lambda()>

...

../Source/Core/Common/ScopeGuard.h:27:19: error: use of deleted function ‘Core::EmuThread(std::unique_ptr<BootParameters>, WindowSystemInfo)::<lambda()>& Core::EmuThread(std::unique_ptr<BootParameters>, WindowSystemInfo)::<lambda()>::operator=(const Core::EmuThread(std::unique_ptr<BootParameters>, WindowSystemInfo)::<lambda()>&)’
   27 |       m_finalizer = {};
      |       ~~~~~~~~~~~~^~~~
../Source/Core/Core/Core.cpp:524:71: note: a lambda closure type has a deleted copy assignment operator
  524 |   Common::ScopeGuard controller_guard{[init_controllers, init_wiimotes] {

Copy link
Contributor

@CookiePLMonster CookiePLMonster left a comment

Choose a reason for hiding this comment

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

Ah, that's fine then. Your godbolt example also shows that optimizers seem pretty good at realizing std::optional can be skipped, and it provides some much needed clarity here.

@JosJuice JosJuice merged commit 9cfe7f4 into dolphin-emu:master Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants