Rawtherapee: Review idle_register.add() calls

[heckflosse]

There are a lot of idle_register.add() calls in rtgui which are coded a bit overcomplicated.
This issue is to review them. I created a new branch and already added fixes for wavelets, colortoning and ciecam02.

[Floessie]

@heckflosse Great idea. I'll join you on this branch to review the IdleRegister class for the already mentioned deleter and another thing I have in mind. Okay? You can still continue to simplify the code, I'll take care of the necessary adjustments.

[heckflosse]

Currently I gave priority to other issues. Are you ok for adding this to 5.6 milestone?

[Floessie]

I have the slight feeling that 5.5 is due when #4893 is done as a final additional feature for this round. Changing such basic things like the IdleRegister so late seems risky to me, so tagging this 5.6 appears opportune.

Nevertheless, this is something we must tackle soon, because overly complicated code is never nice to maintain and the leaks are real. This weekend I'm focused on another project but I promise to come up with my IdleRegister revision in this branch during the next week.

Best,
Flössie

[Floessie]

@heckflosse I've started working on this.

[Floessie]

@heckflosse Yet another mass commit. This was done in a hurry, I hope I didn't break a thing. Please review the diff. Thanks.

[Floessie]

BTW: Tested also with Clang on Linux. Adds 60k to the release binary built with GCC on AMD64.

[gaaned92]

W10/MSYS2/GCC8.2.0

--      Commit description:     5.4-995-g590632948
--      Branch:                 review-idle_register-calls
--      Commit:                 590632948
--      Commit date:            2018-10-28

D:/RAWTHERAPEE/RTSOURCE/rawtherapee/rtgui/dirbrowser.cc: In member function 'virtual void DirBrowser::winDirChanged()':
D:/RAWTHERAPEE/RTSOURCE/rawtherapee/rtgui/dirbrowser.cc:257:33: error: no matching function for call to 'IdleRegister::add(const DirBrowser::winDirChanged()::<lambda(gpointer)>&, DirBrowser*)'
     idle_register.add(func, this);
                                 ^
In file included from D:/RAWTHERAPEE/RTSOURCE/rawtherapee/rtgui/dirbrowser.h:28,
                 from D:/RAWTHERAPEE/RTSOURCE/rawtherapee/rtgui/dirbrowser.cc:19:
D:/RAWTHERAPEE/RTSOURCE/rawtherapee/rtgui/guiutils.h:54:10: note: candidate: 'template<class DATA> void IdleRegister::add(bool (*)(DATA*), DATA*, bool, gint)'
     void add(bool (*function)(DATA*), DATA* data, bool delete_data, gint priority = G_PRIORITY_DEFAULT_IDLE)
          ^~~
D:/RAWTHERAPEE/RTSOURCE/rawtherapee/rtgui/guiutils.h:54:10: note:   template argument deduction/substitution failed:
D:/RAWTHERAPEE/RTSOURCE/rawtherapee/rtgui/dirbrowser.cc:257:33: note:   mismatched types 'bool (*)(DATA*)' and 'DirBrowser::winDirChanged()::<lambda(gpointer)>'
     idle_register.add(func, this);
                                 ^

[Floessie]

@gaaned92 André, thanks for testing. I've tried to fix the Windows build, but did so blindly.

[heckflosse]

@Floessie builds fine now on Windows

[heckflosse]

@Floessie

Adds 60k to the release binary built with GCC on AMD64.

We could get some of this back by marking all functions in rtgui/preferences.* as 'cold' ;-)

[Floessie]

@heckflosse

We could get some of this back by marking all functions in rtgui/preferences.* as 'cold' ;-)

Or defaulting to -Os. :grin:

[thirtythreeforty]

I ran into this function when I was experimenting with the queueing code. I think a cleaner solution would be to add support for adding a capturing lambda. This is essentially what all the little callback structs do; they could all go away. I have an experimental implementation I hacked together if you're interested in seeing it (it builds a std::function around the lambda and defers to the current add() implementation, which should probably be reversed in a real refactoring IMO).

[Floessie]

@thirtythreeforty George, I'm interested. Please show us the code here.

[thirtythreeforty]

Certainly, see my initial attempt branch idleregister-lambdas. Of course the IdleRegister implementation could be cleaned up (see the commit msg), and perhaps it could support void-returning lambdas to get rid of the return 0 at the bottom of the lambdas. But the basic idea is that NLParams goes away.

Also, if we can use C++14, lambda capture initializers make:

        auto listener = this->listener;
        idle_register.add([listener, descr] {
            listener->queueSizeChanged(0, false, true, descr);
            return 0;
        });

into:

        idle_register.add([listener=this->listener, descr] {
            listener->queueSizeChanged(0, false, true, descr);
            return 0;
        });

If this feels like Android's runOnUiThread, it's because it is. :)

[Floessie]

@thirtythreeforty First of all, I really like the idea of adding capturing lambdas (and I'm a bit jealous I didn't come up with this first :stuck_out_tongue:).

    template<typename T>
    void add(T lambda, gint priority = G_PRIORITY_DEFAULT_IDLE) {
        add(std::function<gboolean()>(lambda), priority);
    }

What's the reason for this template function? std::function<> takes lambdas, so why add another forwarder to it?

void IdleRegister::add(std::function<gboolean()> function, gint priority)
{
    using callback_t = std::function<gboolean()>;
    auto source_func = [](void *data) {
        auto fn = std::unique_ptr<callback_t>(static_cast<callback_t*>(data));
        return (*fn)();
    };

    add(source_func, new callback_t(std::move(function)), priority);
}

void IdleRegister::destroy()
{
    mutex.lock();
    for (const auto& id : ids) {
        g_source_remove(id.second);
        delete id.first;
    }
    ids.clear();
    mutex.unlock();
}

This is still leaking in destroy(), that's why I added the deleter.

I think this is a nice add()-on to the current revision but not a replacement. With your consent, I'd like to integrate the std::function<> variant into IdleRegister here. Okay?

@heckflosse Thoughts?

[thirtythreeforty]

Ha, glad you like it. Feel free to take it and run with it.

What's the reason for this template function?

So I thought that's needed because a lambda isn't a std::function, and I wanted to be able to pass lambdas directly to add(). Upon checking the docs, I see the std::function constructor is not explicit so you may not need the overload at all. If you can delete it from my branch and everything still builds, it's unnecessary.

In destroy(), you should use a std::lock_guard for the mutex. Admittedly low-risk in this particular spot.

[Floessie]

@thirtythreeforty

Ha, glad you like it. Feel free to take it and run with it.

:+1:

Upon checking the docs, I see the std::function constructor is not explicit so you may not need the overload at all. If you can delete it from my branch and everything still builds, it's unnecessary.

I can tell from other code that it is indeed unnecessary. I was just curious if there was another reason for this forwarder.

In destroy(), you should use a std::lock_guard for the mutex. Admittedly low-risk in this particular spot.

Thanks for the pointer. I'm fond of explicit locking/unlocking* but I see the value in RAII which I often use (but not with locking).

Best,
Flössie

*) IMHO, lock guards are often used thoughtlessly and too broad. To cope with that, you have to use extra scopes which have their right to exist but look ugly. Those again lead to non-constness or require the lamba trick. Excessively simplified example:

mutex.lock();
const State easy_state = some_protected_state;
mutex.unlock();

... use easy_state copy ...

State non_const_state;
{
    LockGuard lock(mutex); // I've seen people writing `LockGuard(mutex);` instead
    non_const_state = some_protected_state;
}

... use non_const_state copy ...

const State hard_state =
    [&mutex, &some_protected_state]() -> State
    {
        LockGuard lock(mutex);
        return some_protected_state;
    }();

... use hard_state copy ...

[Floessie]

@heckflosse I assigned this issue to myself as I'm working on it. Due to the many places I have to convert this will take some time. But 5.6 isn't just around the corner...

[Floessie]

This was completed in #5131. Closing.