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

[WIP] Interconnect: enable connecting stateful lambdas #63

Conversation

4 participants
@TheHugeManatee
Copy link
Contributor

commented May 10, 2019

Previously, it was only possible to connect non-capturing lambdas. This should allow all lambdas to be connected through use of std::function.

i.e. this was not possible to do before:

    std::string receivedMessage{ "none" };
    Connection connection2 = Interconnect::connect(postman, &Postman::newMessage, [&receivedMessage](int, const std::string&msg) { receivedMessage = msg; });

This might lead to a drop in performance because of the added indirection of std::function. If this is a concern, it would probably be possible to find a SFINAE way to keep the old direct cast variant where applicable, and use the new pattern, however this would mean more complicated code. Also, modern compilers/STL implementations of std::function might be smart enough to detect and optimize the state-less case anyways?

@TheHugeManatee

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@mosra not sure if I should request against master or next? Also, on MSVC I can't run the InterconnectTest project (complains about a missing DLL entry point). However, everything compiles which should be a good sign ;)

@williamjcm

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

not sure if I should request against master or next?

master.

@TheHugeManatee TheHugeManatee changed the base branch from next to master May 10, 2019

@mosra

This comment has been minimized.

Copy link
Owner

commented May 10, 2019

Hi, thanks a lot for your contribution!

First things first:

not sure if I should request against master or next?

Against master please, the next branch is just a temporary branch that I'm using to stabilize things before they get to master, so it's full of half-done commits, compilation failures, angry commit messages and such. Would it be possible for you to rebase your change on master and force-push the branch to remove the unrelated changes from next? Thanks!

On MSVC I can't run the InterconnectTest project (complains about a missing DLL entry point)

Yeah, you need to first install to some location which is on the PATH and then run the tests, that works.


Now, for the actual code -- sorry to say that, but I can't accept code that uses std::function. The <functional> header is extremely heavy, getting even heavier with each new C++ standard, and including it would basically revert all work I'm doing to reduce compile times. This is also one of the reasons why capturing lambdas weren't supported until now. Having the std::function overhead also for plain function pointer isn't something I would like, either. But don't worry -- there is a solution:

A counter-suggestion: there's MemberConnectionData<Args...> and FunctionConnectionData<Args...>, would it be possible for you to add a third CapturingLambdaConnectionData<F, Args...> (let's say), which would store the actual lambda F by-value (instead of getting it wrapped in a std::function) in case the lambda is not convertible to a function pointer? That would avoid the <functional> include and won't pessimize the plain-function-pointer case either.

Hopefully my suggestion is clear enough, unfortunately right now I don't have enough time to flesh out a concrete implementation.

@mosra mosra added this to TODO in Interconnect via automation May 10, 2019

@TheHugeManatee

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Hi, thanks for the feedback! Sure, I understand your concerns - std::function is neither optimal regarding regarding compile-time or runtime, this was just the fastest way I could see to get stateful lambdas working, so I wanted to get your feedback on it.

I understand your suggestion, it was what I was looking to do initially, but the std::function solution seemed a simpler first shot at the problem. I will look into getting it done using the CapturingLambdaConnectionData approach you suggest! Will update once I get around to give it another shot, probably later this week!

Interconnect: enable connecting stateful lambdas
Previously, it was only possible to connect non-capturing lambdas. This should allow all lambdas to be connected through use of std::function

@TheHugeManatee TheHugeManatee force-pushed the TheHugeManatee:interconnect-stateful-lambdas branch from bfa2699 to 32c2116 May 21, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 21, 2019

Codecov Report

Merging #63 into master will increase coverage by 2.38%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #63      +/-   ##
========================================
+ Coverage   97.61%   100%   +2.38%     
========================================
  Files          80      1      -79     
  Lines        5451    336    -5115     
========================================
- Hits         5321    336    -4985     
+ Misses        130      0     -130
Impacted Files Coverage Δ
src/Corrade/Utility/FormatStl.h
src/Corrade/Utility/Tweakable.h
src/Corrade/Utility/Endianness.h
src/Corrade/Utility/Unicode.cpp
src/Corrade/Containers/ArrayViewStl.h
src/Corrade/Utility/System.cpp
src/Corrade/Utility/String.h
src/Corrade/PluginManager/PluginMetadata.h
...rrade/TestSuite/Implementation/BenchmarkCounters.h
src/Corrade/Utility/ConfigurationGroup.h
... and 68 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19b95e6...b3a2ebd. Read the comment docs.

TheHugeManatee added some commits May 21, 2019

Interconnect: connecting stateful lambdas without <functional>
Seems to work but currently induces overhead with non-capturing lambdas
Interconnect: fixed overload for non-capturing lambda
however, there is still an open problem of unresolved linkage..
@TheHugeManatee

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Ok, so I gave it another try but it seems like it's not as simple as I thought.
I introduced a new AbstractLambdaConnectionData as a base class for type erasure, and a derived template class with the lambda type as its template type.
The code compiles, but ultimately fails linking with the following:

6>c:\src\corrade\src\corrade\interconnect\emitter.h(292): warning C5046: 'Corrade::Interconnect::connect': Symbol involving type with internal linkage not defined
6>LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:REF' specification
6>Test.obj : error LNK2019: unresolved external symbol "class Corrade::Interconnect::Connection __cdecl Corrade::Interconnect::connect<struct `public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterMultipleInheritanceVirtual(void)'::`2'::Diamond,struct `public: void __cdecl Corrade::Interconnect::Test::A0x10c491b6::Test::emitterMultipleInheritanceVirtual(void)'::`2'::Diamond,class <lambda_524b08efc336622e3a605b8f87004497>,int,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &>(struct `public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterMultipleInheritanceVirtual(void)'::`2'::Diamond &,class Corrade::Interconnect::Emitter::Signal (__cdecl `public: void __cdecl Corrade::Interconnect::Test::A0x10c491b6::Test::emitterMultipleInheritanceVirtual(void)'::`2'::Diamond::*)(int,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &),class <lambda_524b08efc336622e3a605b8f87004497>)" (??$connect@UDiamond@?1??emitterMultipleInheritanceVirtual@Test@?A0x10c491b6@3Interconnect@Corrade@@QEAAXXZ@U1?1??234356@QEAAXXZ@V<lambda_524b08efc336622e3a605b8f87004497>@@HAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Interconnect@Corrade@@YA?AVConnection@01@AEAUDiamond@?1??emitterMultipleInheritanceVirtual@Test@?A0x10c491b6@501@QEAAXXZ@P83?1??456501@QEAAXXZ@EAA?AVSignal@Emitter@01@HAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@ZV<lambda_524b08efc336622e3a605b8f87004497>@@@Z) referenced in function "public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterMultipleInheritanceVirtual(void)" (?emitterMultipleInheritanceVirtual@Test@?A0x10c491b6@1Interconnect@Corrade@@QEAAXXZ)
6>Test.obj : error LNK2019: unresolved external symbol "class Corrade::Interconnect::Connection __cdecl Corrade::Interconnect::connect<struct `public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterIdenticalSignals(void)'::`2'::Widget,struct `public: void __cdecl Corrade::Interconnect::Test::A0x10c491b6::Test::emitterIdenticalSignals(void)'::`2'::Widget,class <lambda_51ff6dd60c2adc2fb9ec9684372dfeba> >(struct `public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterIdenticalSignals(void)'::`2'::Widget &,class Corrade::Interconnect::Emitter::Signal (__cdecl `public: void __cdecl Corrade::Interconnect::Test::A0x10c491b6::Test::emitterIdenticalSignals(void)'::`2'::Widget::*)(void),class <lambda_51ff6dd60c2adc2fb9ec9684372dfeba>)" (??$connect@UWidget@?1??emitterIdenticalSignals@Test@?A0x10c491b6@3Interconnect@Corrade@@QEAAXXZ@U1?1??234356@QEAAXXZ@V<lambda_51ff6dd60c2adc2fb9ec9684372dfeba>@@$$V@Interconnect@Corrade@@YA?AVConnection@01@AEAUWidget@?1??emitterIdenticalSignals@Test@?A0x10c491b6@501@QEAAXXZ@P83?1??456501@QEAAXXZ@EAA?AVSignal@Emitter@01@XZV<lambda_51ff6dd60c2adc2fb9ec9684372dfeba>@@@Z) referenced in function "public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterIdenticalSignals(void)" (?emitterIdenticalSignals@Test@?A0x10c491b6@1Interconnect@Corrade@@QEAAXXZ)
6>Test.obj : error LNK2019: unresolved external symbol "class Corrade::Interconnect::Connection __cdecl Corrade::Interconnect::connect<struct `public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterIdenticalSignals(void)'::`2'::Widget,struct `public: void __cdecl Corrade::Interconnect::Test::A0x10c491b6::Test::emitterIdenticalSignals(void)'::`2'::Widget,class <lambda_8ea730efdcc83b08a1278d9c86828d85> >(struct `public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterIdenticalSignals(void)'::`2'::Widget &,class Corrade::Interconnect::Emitter::Signal (__cdecl `public: void __cdecl Corrade::Interconnect::Test::A0x10c491b6::Test::emitterIdenticalSignals(void)'::`2'::Widget::*)(void),class <lambda_8ea730efdcc83b08a1278d9c86828d85>)" (??$connect@UWidget@?1??emitterIdenticalSignals@Test@?A0x10c491b6@3Interconnect@Corrade@@QEAAXXZ@U1?1??234356@QEAAXXZ@V<lambda_8ea730efdcc83b08a1278d9c86828d85>@@$$V@Interconnect@Corrade@@YA?AVConnection@01@AEAUWidget@?1??emitterIdenticalSignals@Test@?A0x10c491b6@501@QEAAXXZ@P83?1??456501@QEAAXXZ@EAA?AVSignal@Emitter@01@XZV<lambda_8ea730efdcc83b08a1278d9c86828d85>@@@Z) referenced in function "public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterIdenticalSignals(void)" (?emitterIdenticalSignals@Test@?A0x10c491b6@1Interconnect@Corrade@@QEAAXXZ)
6>Test.obj : error LNK2019: unresolved external symbol "class Corrade::Interconnect::Connection __cdecl Corrade::Interconnect::connect<struct `public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterIdenticalSignals(void)'::`2'::Widget,struct `public: void __cdecl Corrade::Interconnect::Test::A0x10c491b6::Test::emitterIdenticalSignals(void)'::`2'::Widget,class <lambda_e1b66ccf5b9ddfb9f41f05110f8b6513> >(struct `public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterIdenticalSignals(void)'::`2'::Widget &,class Corrade::Interconnect::Emitter::Signal (__cdecl `public: void __cdecl Corrade::Interconnect::Test::A0x10c491b6::Test::emitterIdenticalSignals(void)'::`2'::Widget::*)(void),class <lambda_e1b66ccf5b9ddfb9f41f05110f8b6513>)" (??$connect@UWidget@?1??emitterIdenticalSignals@Test@?A0x10c491b6@3Interconnect@Corrade@@QEAAXXZ@U1?1??234356@QEAAXXZ@V<lambda_e1b66ccf5b9ddfb9f41f05110f8b6513>@@$$V@Interconnect@Corrade@@YA?AVConnection@01@AEAUWidget@?1??emitterIdenticalSignals@Test@?A0x10c491b6@501@QEAAXXZ@P83?1??456501@QEAAXXZ@EAA?AVSignal@Emitter@01@XZV<lambda_e1b66ccf5b9ddfb9f41f05110f8b6513>@@@Z) referenced in function "public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::emitterIdenticalSignals(void)" (?emitterIdenticalSignals@Test@?A0x10c491b6@1Interconnect@Corrade@@QEAAXXZ)
6>Test.obj : error LNK2019: unresolved external symbol "class Corrade::Interconnect::Connection __cdecl Corrade::Interconnect::connect<class Corrade::Interconnect::Test::`anonymous namespace'::Postman,class Corrade::Interconnect::Test::A0x10c491b6::Postman,class <lambda_75f05457d65063a918fe638095b12b1f>,int,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &>(class Corrade::Interconnect::Test::`anonymous namespace'::Postman &,class Corrade::Interconnect::Emitter::Signal (__cdecl Corrade::Interconnect::Test::A0x10c491b6::Postman::*)(int,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &),class <lambda_75f05457d65063a918fe638095b12b1f>)" (??$connect@VPostman@?A0x10c491b6@Test@Interconnect@Corrade@@V12345@V<lambda_75f05457d65063a918fe638095b12b1f>@@HAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Interconnect@Corrade@@YA?AVConnection@01@AEAVPostman@?A0x10c491b6@Test@01@P834501@EAA?AVSignal@Emitter@01@HAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@ZV<lambda_75f05457d65063a918fe638095b12b1f>@@@Z) referenced in function "public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::function(void)" (?function@Test@?A0x10c491b6@1Interconnect@Corrade@@QEAAXXZ)
6>Test.obj : error LNK2019: unresolved external symbol "class Corrade::Interconnect::Connection __cdecl Corrade::Interconnect::connect<class Corrade::Interconnect::Test::`anonymous namespace'::Postman,class Corrade::Interconnect::Test::A0x10c491b6::Postman,class <lambda_2612a01e5a73b2f536a0d81afdb1ad20>,int,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &>(class Corrade::Interconnect::Test::`anonymous namespace'::Postman &,class Corrade::Interconnect::Emitter::Signal (__cdecl Corrade::Interconnect::Test::A0x10c491b6::Postman::*)(int,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &),class <lambda_2612a01e5a73b2f536a0d81afdb1ad20>)" (??$connect@VPostman@?A0x10c491b6@Test@Interconnect@Corrade@@V12345@V<lambda_2612a01e5a73b2f536a0d81afdb1ad20>@@HAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Interconnect@Corrade@@YA?AVConnection@01@AEAVPostman@?A0x10c491b6@Test@01@P834501@EAA?AVSignal@Emitter@01@HAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@ZV<lambda_2612a01e5a73b2f536a0d81afdb1ad20>@@@Z) referenced in function "public: void __cdecl Corrade::Interconnect::Test::`anonymous namespace'::Test::function(void)" (?function@Test@?A0x10c491b6@1Interconnect@Corrade@@QEAAXXZ)
6>C:\src\corrade\build\src\Corrade\Interconnect\Test\Debug\InterconnectTest.exe : fatal error LNK1120: 6 unresolved externals

to be honest, I am unsure what to make of that, any suggestions?

@mosra

This comment has been minimized.

Copy link
Owner

commented May 25, 2019

I think I know where the problem is, the function pointer / lambda overload of connect() needs to be done a bit differently. I have some ideas and will look into this tomorrow -- ping me again in case I forget ;)

@mosra mosra added this to the 2019.0b milestone May 25, 2019

@mosra mosra self-assigned this May 25, 2019

@TheHugeManatee

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@mosra any updates? I am happy to give it another try, but this seems like less related to the code and more to linker/dll export, which might take too much time for me to dive into..

@mosra

This comment has been minimized.

Copy link
Owner

commented Jun 26, 2019

So! This made my hair grey 😆

To begin with, the internals of the Interconnect library were quite old and shitty (lots of heap allocations, virtual calls and other indirections) and quite inflexible to extend any further so the first thing I wanted to do was rework those to make them easier to add new features to. That's now done (took me almost a week, argh) and it opens the door to new possibilities such as connecting signals to signals, passing the emitter object as a parameter to the slot, combined member / lambda slots and so on. These will be added later if I get the time, for now they live in the fractal of TODO lists.

To ensure I'm actually improving the performance, I added a benchmark for the original code only to discover that std::unordered_map is so bad that calling erase() on a map with 10k items easily made the benchmark run minutes. In the end I had to dial down the benchmark to test only on 1k signal connections to be acceptably fast, but even that takes longer than a benchmark copying several 100 MB files. This made me realize that I desperately need to use some non-crappy hashmap implementation instead -- later. In the benchmarks, ~90% of the time was spent in std::unordered_map overhead, I managed to strip away 30% of that but still.

Besides that, the rework made me realize that virtual calls are not so slow after all, so while the reworked version is ~30% faster in shooting signals overall, most of the gains are due to everything else removed, moving away from virtual calls actually made the actual call slightly slower in debug (I have a bunch of ideas how this could get improved, but later.)

Finally, while doing final testing I realized signals across shared object boundaries got broken by the recent introduction of -fvisibility-inlines-hidden, so I had to investigate and fix that too (took me the whole today). That also finally helped me uncover and reproduce a long-standing bug under MinGW (see #72), but I don't know how to fix that one yet.

If you are interested, original benchmark is in 0b185b3 and the reworked version (supporting stateful lambdas, general function objects as well as std::function) is in d1cfa67. I added you as a co-author. Cheers!

@mosra mosra closed this Jun 26, 2019

Interconnect automation moved this from TODO to Done Jun 26, 2019

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.