Contrib separation & ftl::Trampoline

[martty]

Please find the initial implementation of Trampoline, along with a hypothetical split from the mainline ftl library.

This is not intended to be merged in this form, but I wanted to initiate discussion on this implementation approach.

[cwfitzgerald]

Frankly, I'm not really a big fan of this interface. The code itself looks perfectly fine for what it does, though I haven't been terribly thorough.

My comments in no particular order:

• Forcing a bump up to c++17 for the functionality achievable with std::function doesn't seem worth it to me. There shouldn't be a massive disclaimer "we're a c++11 library unless you want the convenient stuff". I understand that the std::invoke stuff is really nice, but I really can't justify to myself why it is truly necessary.
• I understand that the purpose of having the Trampoline type is to allow you to control the scope as you wish, but I think this is just making things unnecessarily complicated. The whole purpose of interface is to make things as simple as possible, and i can't see the benefit of all the machinery you have to do with this (construct a trampoline, bind arguments, deal with the future etc.).
• I personally think that the best interface would be very similar to a std::async call and how I designed my prototype interface:

  template<class F, class... Args>
  future<rettype> TaskScheduler::AddTask(F& function, Args&&... args);
  

  Obviously you need to add the other arguments in there, but you get the general point. All the nasty stuff can be handled internally, and you are allowed arbitrary functions to be called (including member functions as long as you delegate off to std::bind).
• Because there is a vector of all the current BoundTrampolines you have to hold a mutex in order to modify it. This seems counterproductive over the alternative is just calling off to malloc/new/user provided allocator. The function that gets called can do the deletion. Holding mutexes within a fiber isn't good, and as of right now, ftl::Futex doesn't allow non-fiber threads to hold it.
• Adding an addTask to AtomicCounter seems to be violating separation of concerns. There isn't a convincing reason why it should be there. There can be an overload of addTask or an additional member function that also blocks on the AtomicCounter.
• FTL_CONTRIB_CHANGES really should be specific about what it is enabling. If we want things to be able to be compile time enabled and disabled (which I agree with), each optional feature should have its own descriptive macro on what it is enabling and disabling. While contrib is accurate, all the file names should be descriptive of what they are and what part they are tied to.

All in all, I think the code is fine, but the interface isn't as friendly as I hoped, and there is a lot of excess machinery that I don't think is needed.

[martty]

Hey, thanks for the comments! No worries, I knew that this is a controversial proposal ;). Your comments were reasonable, so I went ahead and took most of the changes you found questionable.

template<class F, class... Args> future<rettype> TaskScheduler::AddTask(F& function, Args&&... args);

I avoided making this change, because I felt it would be too invasive, since it would require all users of the TS to be aware of *Trampoline, but I agree that this would be a better interface.

[RichieSams]

Overall, it's looking very nice.

With the recent changes pulling it back down to C++11, my first question is: do we really need a contrib folder / #ifdef? In my opinion, just put trampoline.h in with all the other files. And get rid of the #ifdef for the tests, and always call them.

[RichieSams]

I think trampoline was just chosen because that's what the internal stuff is called in std::thread. If we do end up going with the TaskScheduler::AddTask(F& function, Args&&... args) style interface, all this would be hidden from the user anyhow.

[cwfitzgerald]

Are we still planning on going with that interface? I somehow got the impression that this would be the interface itself.

[RichieSams]

I think the AddTasks() interface is cleaner, and helps to prevent potential memory issues. For example, when the user uses Trampoline directly, they must not use the same function returned from make_trampoline in more than one Task. Otherwise you'll get a double free. With the AddTasks(), this is all hidden from the user, so they don't need to worry about it.

What are your thoughts? Pros? Cons?

[cwfitzgerald]

I definitely agree wholeheartedly. I also really don't know what the advantage of something like this is versus just using std::bind/std::function. That's what I used in my implementation and it was something on the order of 20 loc for the whole thing.