search

NoAvailableAlias / nano-signal-slot

Pure C++17 Signals and Slots
MIT License
407 stars 60 forks source link

Call function's in order of connection #5

Closed rvt closed 9 years ago

rvt commented 9 years ago

Hello,

Is it possible to call each (member) function in order of which they are connected? Currently I noticed that once in a blue moon the call order get's swapped and some drawing functions are called in the incorrect order.

NoAvailableAlias commented 9 years ago

Hi, currently slot emission based on connection order is not maintained because the underlying data structure is a tree structure. For now I will add this caveat to the readme and look into possible solutions. I'm not sure if this behavior will be added as this was not originally part of the Nano-signal-slot design goals.

rvt commented 9 years ago

Hello,

I have made a small change to your library that changes std::map for tracked_connections to std::list. This should keep the order of emissions the same as the connections.

This seems to have a small additional benefit that the performance is slightly better.

Library construct destruct connect emission combined
std::list
Nano-signal-slot 21370 2122 3575 23318 1197
Nano-signal-slot 21307 2179 3622 23501 1227
Nano-signal-slot 21254 2127 3554 23621 1209
std::vector
Nano-signal-slot 25234 2321 1958 23754 948
Nano-signal-slot 25432 2332 1959 24066 955
std::map
Nano-signal-slot 17405 2083 1879 17206 921
Nano-signal-slot 17453 2080 1913 17187 925
Nano-signal-slot 17831 2160 1928 17764 959

Also I tried to use std::vector but connection was lower and emission the same. So I kept it to use std::list because the performance of emission/connect seemed more important to me. I would like to say I don't excel in c++, as I just do it for a hobby so, do let me know if this makes sense or not, let me know what you think.

Here is the diff: https://github.com/rvt/nano-signal-slot/commit/510b1c8aee07e30a27238f32bef2348623171599

NoAvailableAlias commented 9 years ago

Well that settles it, I have been trying to find time to create a separate branch devoted to additional features and performance, now it just has to be done. (programming as a job has been draining the motivation). However, now that some new motivation has been infused back into the system I will give it a go.

This issue will remain open for the time being until the branch and the readme information is pushed. Concerning your diff, I see no reason why you wouldn't be able to use a forward_list instead of std::list and would be interested in seeing the results against the other std structures.

rvt commented 9 years ago

Hey,

I must have done something wrong with my std::vector test but it seems like it does have a slight edge when -O2 is enabled.

For completeness, the tests with -O2 enabled

Library construct destruct connect emission combined
std::list -O2
Nano-signal-slot 190625 4843 8840 128532 3315
Nano-signal-slot 188133 4845 8873 130821 3259
Nano-signal-slot 189292 4883 8477 129740 3266
std::forward_list O2 with emplace_front, this is faster but 'reverses' the list during emit, this sounds awkward to me.
Nano-signal-slot 133842 3800 7017 128518 2638
Nano-signal-slot 144978 5477 9133 128984 3457
Nano-signal-slot 144775 5436 8681 130458 3514
std::vector -O2
Nano-signal-slot 135662 7471 9177 134810 4362
Nano-signal-slot 136231 7516 8953 132697 4372
Nano-signal-slot 143026 7708 9631 139162 4658
NoAvailableAlias commented 9 years ago

Ah yes because there is no emplace back on a forward list iterating will be "reversed" compared to the std::list. Thus a doubly linked list would be a better choice as you would be able to choose which direction to emit (front to back vs back to front). Using an std::vector will most likely be faster ("combined" score), however, this limits the potential capabilities of Nano-signal-slot (no disconnect objects).

lucijan commented 9 years ago

Another advantage of using std::list (or std::forward_list) over std::map is that it would allow the very slot to disconnect from a signal. At the moment that leads to a crash as std::map doesn't allow manipulating the structure while it's iterated. This sounds like a far-fetched use case but happens quite often in the wild. Think e.g. of a ui component that emits a signal for removing / deleting something.

NoAvailableAlias commented 9 years ago

So the master branch is crashing due to indirect disconnection, ain't nobody got time for that. The FT branch has been changed to a singly linked list structure and will have to be tested for this problem before the merge. Thank you for reporting this issue.

lucijan commented 9 years ago

cool! thanks for your fast reply! i'll give it a shot on monday.

have a nice weekend.

cheers — lucijan

On 05.06.2015, at 14:39, Matt M. notifications@github.com wrote:

So the master branch is crashing due to indirect disconnection, ain't nobody got time for that. The FT branch has been changed to a singly linked list structure and will have to be tested for this problem before the merge. Thank you for reporting this issue.

— Reply to this email directly or view it on GitHub.

NoAvailableAlias commented 9 years ago

Hmm, it would appear that the FT branch also suffers from this issue. Will create a new issue for this.

lucijan commented 9 years ago

hello,

i finally got around testing nano's FT branch in our code. it seems like the indirect observer deletion crash is finally fixed! thank you very much!

one thing i've come across is a template specialization that hadn't been updated to use emit() instead of operator(). I've included a patch.

also it might be worth debating to keep operator() for backwards compatibility (maybe using a preprocessor macro to enable it optionally). as the migration is somewhat hard since there is nothing good to grep for.

anyway … i'm looking forward to a stable release

cheers — lucijan

On 09.06.2015, at 01:22, Matt M. notifications@github.com wrote:

Hmm, it would appear that the FT branch also suffers from this issue. Will create a new issue for this.

— Reply to this email directly or view it on GitHub.

NoAvailableAlias commented 9 years ago

I decided to change the emit interface because of the difficulty I was running into with the accumulate overload of operator(). Since attempting perfect forwarding using universal references, I was not able to easily resolve overload resolution manually. Even then I would have had to change the signature of the accumulate operator() due to variadic template requirements.

In the meantime I will add preprocessor for backwards compatibility but will default to disabled. This interface will then be marked [[deprecated]] once c++14 is comfortably supported and will be completely removed some release after that.

I'm not sure when the FT will be ready. Still got to make sure that it will indeed be stable.

Matt M.