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

ui: Compile boost::signals2 only once #13634

Merged
merged 1 commit into from Aug 13, 2018

Conversation

Projects
None yet
6 participants
@MarcoFalke
Copy link
Member

commented Jul 11, 2018

ui is one of the modules that poison other modules with boost/signals2 headers. This moves the include to the cpp file and uses a forward declaration in the header.

Locally this speeds up the incremental build (building everything that uses the ui module) with gcc by ~5% for me. Gcc uses ~5% less memory.

Would be nice if someone could verify the numbers roughly.

I presume the improvements will be more pronounced if the other models would stop exposing the boost header as well.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

Concept ACK.

Though, I wonder if we do this, it would make sense to move this away from boost::signals completely for the UI interface (registering interface objects as delegates instead) instead of working to hide boost::signals. But no strong opinion on that.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

Concept ACK

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

This seemed like a straightforward refactor with a clear win. Not sure if the "registering interface objects as delegates instead" is as simple as that.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #10973 (Refactor: separate wallet from node by ryanofsky)
  • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@kallewoof
Copy link
Member

left a comment

utACK fa15f97

@@ -72,43 +75,69 @@ class CClientUIInterface
MSG_ERROR = (ICON_ERROR | BTN_OK | MODAL)
};

#define ADD_SIGNALS_DECL_WRAPPER(signal_name) \
boost::signals2::connection signal_name##_connect(std::function<signal_name##Sig> fn); \
void signal_name##_disconnect(std::function<signal_name##Sig> fn);

This comment has been minimized.

Copy link
@kallewoof

kallewoof Aug 7, 2018

Member

OCD-nit: remove ending ; to avoid ;; when used

boost::signals2::signal<bool (const std::string& message, const std::string& caption, unsigned int style), boost::signals2::last_value<bool> > ThreadSafeMessageBox;
bool ThreadSafeMessageBox (const std::string& message, const std::string& caption, unsigned int style);
using ThreadSafeMessageBoxSig = bool(const std::string& message, const std::string& caption, unsigned int style);
ADD_SIGNALS_DECL_WRAPPER(ThreadSafeMessageBox);

This comment has been minimized.

Copy link
@kallewoof

kallewoof Aug 7, 2018

Member

Maybe overkill but

#define ADD_SIGNALS_DECL_WRAPPER(signal_name, rtype, args...) \
    rtype signal_name(args);\
    using signal_name##Sig = rtype(args);\
    [...]

then e.g.

    ADD_SIGNALS_DECL_WRAPPER(ThreadSafeMessageBox, bool, const std::string& message, const std::string& caption, unsigned int style);

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 7, 2018

Author Member

Oh thanks! That makes the diff in the header file look a lot cleaner.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1807-uiSignals2Wrap branch from fa15f97 to fa5ce27 Aug 7, 2018

@kallewoof
Copy link
Member

left a comment

re-utACK fa5ce27

@ken2812221

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

utACK fa5ce27

@MarcoFalke MarcoFalke merged commit fa5ce27 into bitcoin:master Aug 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

ken2812221 pushed a commit to ken2812221/bitcoin that referenced this pull request Aug 13, 2018

Merge bitcoin#13634: ui: Compile boost::signals2 only once
fa5ce27 ui: Compile boost:signals2 only once (MarcoFalke)

Pull request description:

  ui is one of the modules that poison other modules with `boost/signals2` headers. This moves the include to the cpp file and uses a forward declaration in the header.

  Locally this speeds up the incremental build (building everything that uses the ui module) with gcc by ~5% for me. Gcc uses ~5% less memory.

  Would be nice if someone could verify the numbers roughly.

  I presume the improvements will be more pronounced if the other models would stop exposing the boost header as well.

Tree-SHA512: 078360eba330ddbca4268bd8552927eae242a239e18dfded25ec20be72650a68cd83af7ac160690249b943d33ae35d15df1313f1f60a0c28b9526853aa7d1e40

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1807-uiSignals2Wrap branch Aug 13, 2018

laanwj added a commit that referenced this pull request Aug 25, 2018

Merge #13961: util: Replace boost::signals2 with std::function
ddddce0 util: Replace boost::signals2 with std::function (MarcoFalke)

Pull request description:

  This removes the `#include <boost/signals2/signal.hpp>` from `util.h` (hopefully speeding up the build time and reducing the memory usage further after  #13634)

  The whole translation interface is replaced by a function `G_TRANSLATION_FUN` that is set to nullptr in units that don't need translation. (Thus only set in the gui)

Tree-SHA512: 087c717358bbed8bdb409463e225239d667f1ced381abb10e7cd31a41dcdd2cebe20b43c2ee86f0f8e55d53301f75e963f07421a99a7ff4c0cad2c6a375c5ab1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.