[Perfomance] Significantly reduce amount of copies/moves inside operators

[victimsnino]

Significantly reduce amount of copies/moves inside operators. Each operator checked and fixed.. In most cases via forwarding/universal reference, in some cases via const reference (if operator is read-only). Each fix covered by unit test

[victimsnino]

Thank you for your time for reviewing and comments. Waiting for any future comments or merge =)

P.S. I have plans to do something similar for observable part. But I believe, it will be a bit more complicated

[kirkshoop]

I tried this out and I cannot compile my project. https://github.com/kirkshoop/twitter

/Users/kirk/source/twitter/main.cpp:1115:18: note: candidate function not viable: expects an lvalue for 1st argument
        rxo::map([](Model& m){
                 ^
/Users/kirk/source/twitter/main.cpp:1115:18: note: conversion candidate of type 'model::ViewModel (*)(model::Model &)'

The issue seems to be forcing const& value passing. Forwarding would be fine, but const breaks this pattern:

https://github.com/kirkshoop/twitter/blob/master/main.cpp#L1089..L1118

scan owns the Model instance and ViewModel is expected to mutate the instance in scan.

[victimsnino]

Is it expected behavior? I mean, from the point of "functional programming" view it is unexpected to change "original" variable in map, isn't it? For example, if i do something like this

auto observable = rxcpp::observable<>::just(1).publish();
// subscriber 1
observable.map([](int& v) {v += 2; return v;}).subscribe();
// subscriber 2
observable.subscribe(); 

observable.connect();

then subscriber 2 obtains incorrect/unexpected value, isn't it?

Also, your example contains this lines:

  start_with(Model{}) |
        rxo::map([](Model& m){
            return ViewModel{m};
        }) |

In case of "start_with" ViewModel contains reference to object inside start_with. Depending on implementation it can has as reference to temporal object (if start_with do copy for each new subscriber) or modify original value inside start_with (if no copy). Not sure if it is expected

[kirkshoop]

It is arguable. reduce and scan explicitly mutate the accumulator and scan intentionally emits the accumulator each time it changes. should subsequent operators be allowed to mutate the accumulator? It is safe for scan. the next value will not be accumulated until the stack unwinds from the on_next() that reports the current accumulator.

In practice, I had already made the tradeoff such that Model has a single member that is a shared_ptr. This made it fairly easy to get it to compile with your changes. However, this is a breaking change.

I think that I will create a new branch to hold the current state of rxcpp before I apply this PR. This will leave a path for others that are dependent on the previous design.

https://github.com/kirkshoop/twitter/tree/PR562

[kirkshoop]

The goal would be to allow just to control mutability.

it either can always pass const& to on_next, or take a copy of value before calling on_next or delegate to the caller. Delegation would support passing in a const int that would be stored as a const int and passed to on_next as a const int &.

The best world (from a C++ perspective) would keep the types directly under the users control - so if the user wanted a value to move or forward or copy or mutable&, through an expression the user would be able to specify the proper type decorations and the operators would preserve those decorations.

That is a lot more work. It was the intent of the original design, but it got lost and ignored too many times and devolved to the inconsistent treatments for values that exist today. If you wanted to take on that task, I would be pleased. I have expectation on you and it will not block this PR.

Thank you! Kirk

[victimsnino]

I would try to think about it after this PR

[victimsnino]

@kirkshoop, i have some ideas about providing ability to use mutable values, but i need some clarifications from you: Current implementation of rxcpp makes copy inside each operator. so, when you do map([](int& v){...}) actually you obtain reference to local copy (inside map). As a result, yes, you can use int& but actually it do nothing in terms of reference (you only avoid copy to pass value inside lambda), but from the meaning of control of value: reference == copy.

Suggested there solution has ability to pass real const reference (or rvalue reference) on original object till final observer or any operator (if no any operator with caching mechanism). As a result, each operator operates at each time const lvalue reference or rvalue reference. From this point of view, what does mean "non-const" reference? I mean, non-const reference from const reference -> impossible. non-const reference from rvalue reference - okay, possible, but does it make sense? Looks like you have next options: 1) You don't need any copies and your operator "read-only" -> use const referenece 2) You want to do some "non-const" operations -> use int in lambda and expect copy or move of original object. 3) You want to do some "non-const reference" operation: a) In old implementation it was reference on copy (actually, something like point 2) b) If original value was reference -> okay (but it will be rare case even in case of modifying implementation) c) If original value was const value, or const lvalue reference, how to deal with it? Looks like we need to provide copy of object inside operator's code. But does it make sense?

So, final question is: Does it correct, that you want: if original value was some "non-convertible-to-reference" type (const T & or const T), then we do copy for passing it to reference argument. If it is true, then ok, i can try to implement it

[victimsnino]

If it is applicable, we can do something like this: https://godbolt.org/z/eGcK9nKf7

TypeDeducer - is special wrapper to resolve all situations. It will be argument in on_next. Template parameter - type of argument in lambda. Several specializations handles all possible variants

call_lambda - imitation of operator (for example, map) where first param - lambda, second - gotten value. Template parameter is equal to argument of lambda

Check output of execution to understand if it is expected behavior. In some combintations we do extra copy/move, but anyway it is better than conditionalless copies as before

UPD: Updated version with reducing moves/copies https://godbolt.org/z/6bx1azK4a (std::optional can be chaned to maybe)

[kirkshoop]

Current implementation of rxcpp makes copy inside each operator. so, when you do map([](int& v){...}) actually you obtain reference to local copy (inside map).

I am not sure what you mean..

rx-map.hpp

        template<class Value>
        void on_next(Value&& v) const
        {
            on_exception_no_return([&]()
                                   {
                                       dest.on_next(this->selector(std::forward<Value>(v)));
                                   },
                                   dest);
        }

map() makes no copies of v.

[kirkshoop]

3. If original value was const value, or const lvalue reference, how to deal with it? Looks like we need to provide copy of object inside operator's code. But does it make sense?

I would expect a compile failure if a source value type was const T& and the lambda arg was T&.

I don't like magic :) Conversions should be explicit - either in the definition of the operator (via has to take copies of the value) or in the users code (a lambda taking T instead of T&/const T&

if original value was some "non-convertible-to-reference" type (const T & or const T), then we do copy for passing it to reference argument.

Thus my answer is that I would like this to fail at compile time. the way to choose the type of the Source Value is to set the type of the Source Value.

• If the Source Value is T or T&& then that should be maintained (even after an internal copy, as via() does).
• If the Source Value is T& then that should be maintained - unless there is an internal copy - in that case the type passed along should be T&& ideally.
• If the Source Value is const T&, then that should be maintained - even if the operator makes a copy internally. Note: it is possible that the correct type to pass along after an internal copy of a const T& is T&& just like the T& case. I am open to either.

The lambda arg type is not going to impact the implementation of any operator. If there is no conversion from the Source Value, then there will be a compile error.

[kirkshoop]

Check output of execution to understand if it is expected behavior.

While that code is cool, it is not the behaviour I would prefer. It is more magical than I like :)

[victimsnino]

Current implementation of rxcpp makes copy inside each operator. so, when you do map([](int& v){...}) actually you obtain reference to local copy (inside map).

I am not sure what you mean..

rx-map.hpp

        template<class Value>
        void on_next(Value&& v) const
        {
            on_exception_no_return([&]()
                                   {
                                       dest.on_next(this->selector(std::forward<Value>(v)));
                                   },
                                   dest);
        }

map() makes no copies of v.

Yes, but nextdetacher does:

template<class U>
        void operator()(U u) {
            .....
                that->destination.on_error(std::move(ex));
            ....
        }

[kirkshoop]

This is what I see in nextdetacher

template<class U>
        void operator()(U&& u) {
            trace_activity().on_next_enter(*that, u);
            RXCPP_TRY {
                that->destination.on_next(std::forward<U>(u));
                do_unsubscribe = false;
            } RXCPP_CATCH(...) {
                auto ex = rxu::current_exception();
                trace_activity().on_error_enter(*that, ex);
                that->destination.on_error(std::move(ex));
                trace_activity().on_error_return(*that);
            }
        }

I do not see a copy..

[victimsnino]

This is what I see in nextdetacher

template<class U>
        void operator()(U&& u) {
            trace_activity().on_next_enter(*that, u);
            RXCPP_TRY {
                that->destination.on_next(std::forward<U>(u));
                do_unsubscribe = false;
            } RXCPP_CATCH(...) {
                auto ex = rxu::current_exception();
                trace_activity().on_error_enter(*that, ex);
                that->destination.on_error(std::move(ex));
                trace_activity().on_error_return(*that);
            }
        }

I do not see a copy..

With merged changes -> yes =) i told about rxcpp without this merge

[kirkshoop]

Ah! Well thank you! That is much better 😳

[victimsnino]

I've tried to implement idea with reference and your requirements above about compilation errors: looks like compilator against this idea and wants to error everytime.

Problem is: due to now we actually forward values everywhere (instead of providing copies everytime), compilator started to look widely. For example, on_next inside map operator now researched for all possible types (const T&, T&& and T& with local changes) So, if i try to pass map([](int&){}, then it tries to satisfy ALL possible variants of values for on_next inside map operator. As a result, const int& and some other can't match for (int&) event if it is unused actually (but code exists). Theoretically it should be fixed by templating everything (and compilator would check only really used ways), but virtual_observer can not be replaced by this way. Any ideas probably???

[kirkshoop]

Would you push the new branch as it is so that I can take a look? Perhaps we can move this work into a new issue, do you mind creating an issue for this?