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.

[martty]

I left this in here accidentally.

[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]

Instead of the cout, can you do a GTEST_ASSERT_*() ?

[RichieSams]

Missing newline

[cwfitzgerald]

Code is looking better. I would agree that we shouldn't have the contrib split as it frankly seems a bit silly.

Code comments/questions:

• Could we call it something other than trampoline. It isn't a very descriptive name. ftl::BoundFunction, ftl::FunctionCall, ftl::Callee would all be fine in my book.
• What is the purpose of allowing late binding? In every use case I can think of, you construct the trampoline then immediately bind them. Why not just construct the trampoline already pre-bound. It would eliminate most of the problems with forgetting to bind it.
• I'm not a big fan of having to dereference the trampoline in order to use it and by extension the abuse of operator Task to get a task out of it. There's too much type crap going on that isn't easy to reason about. I would prefer there be overloads that accept a trampoline/array of trampoline and that a ready to use trampoline doesn't expose the pointer directly. The thing you handle should be a wrapper around a pointer that disallows you from fucking with the allocation. This wrapper would also allow us to deal with the possible memory leaks from creating a trampoline anywhere other than the call itself.

[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.