Le 25/07/14 11:10, Rob Stewart a écrit :

On July 24, 2014 9:57:39 PM EDT, Ian Forbes wrote:

...

Hello,

I am working on implementing scheduled executors as part of GSoC 2014 and am ready to announce my beta 1 release. For more information on my project please see my original proposal at www.cs.mcgill.ca/~iforbe All my work is currently on this branch of boost.thread. https://github.com/BoostGSoC14/boost.thread.executors . I have written a fairly extensive README about design and implementation on this page. Your queue is flawed: what if T's copy constructor is invoked when calling pull(), and it throws an exception? Also, pull() is conventionally called "pop". Hi,

what about http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3533.html#exception... The overload T pull(); must be available only if T move does't throws. Otherwise, there is the overload void pull(T&); which could take care of the exceptions thrown by the copy/move constructor. IMO, pop is not a good name as pull does more than pop an element, it moves the element. I have used pull in Boost.Thread instead of pop_value (n3533). I think that pull conveys quite clearly the intent. Vicente

The approach I have in mind for this library respect to scheduled work of the N3785 proposal is quite different. Instead of adding the scheduled operations to a specific scheduled_executor polymorphic interface, I opted by adding two member template functions to a class scheduled_executor that wraps an existing executor. This scheduled_executor will probably have a thread to take of the time scheduled operations, but this is not necessary. The semantic of the time scheduled operations is a little bit different. The task will be submitted to the associated executor at/after the given time_point/duration.

This describes the scheduling_adaptor class. Since there are no virtual methods the addition of template functions is possible.

Note that your base class scheduled_executor is not an executor at all.

Perhaps I should move this into detail then. I could also change the name to scheduled_executor_base and perhaps then change scheduling_adaptor to scheduled_executor. This however would rule out any possible use of the scheduled_executor class a polymorphic base class since it would be hidden in detail.

This has several advantages: * The time scheduled operations are available for all the executors.

This is provided via the scheduling_adaptor class.

* The template functions could accept any chrono::steady_clock::time_point or any chrono::duration.

I can add template functions to do this. Others have requested this.

* The non-time scheduled task don't pay for the additional cost to manage with a priority queue.

I can override (non-virtually) the submit(work) function in scheduling_adaptor to do this. This however will complicate to ability to use the scheduled_executor polymorphically as calls though scheduling_adaptor will avoid the queue while calls through a scheduled_executor& will put the task at the front/top. Both will have the same behaviour but one will go through the queue. Although it was never my intention of have this polymorphic behaviour it was a nice consequence of my design. Ian,

...

...

This has several advantages: * The time scheduled operations are available for all the executors. This is provided via the scheduling_adaptor class. This is not correct, as your schedulling_adaptor needs an executor that has a timed queue. With my design, only the scheduled_executor would need a timed_queue.

...

...

* The non-time scheduled task don't pay for the additional cost to manage with a priority queue. I can override (non-virtually) the submit(work) function in scheduling_adaptor to do this. See above. This doesn't change anything, as the queue is a timed queue and so you will pay for the non-timed operations.

I'm not sure I follow. The scheduling_adaptor takes a template parameter <Executor> that can be any type with an add(work) function. It then forwards the work from itself to Executor when the work comes off the front of the timed_queue. I can override the add(work) function for this adaptor to immediately forward the work to the wrapped Executor type and bypass the queue all together. The current behaviour is to place it in the queue with its timed point as clock::now(), generally placing it at the front. I have an example here: thread/test/test_scheduling_adaptor.cpp Here is the file on GitHub: https://github.com/BoostGSoC14/boost.thread.executors/blob/gsoc/executors/te... This example wraps a basic_thread_pool and adds timed semantics to to it. Ian,

Le 26/07/14 19:34, Ian Forbes a écrit :

...

Your queue is flawed: what if T's copy constructor is invoked when calling pull(), and it throws an exception? Also, pull() is conventionally called "pop". I can't see anyway around this as the queue needs to be locked to guarantee serial execution to top() and pop(). The convention of having top() and pop() to avoid this can't be used since a call to top() followed by a call to pop() on the same thread are no guaranteed to happen consecutively.

One solution would be have T top(mutex* p) that would accept a mutex pointer and store the internal queue's mutex address into this. The method would lock the queue but would not unlock it. Then we could have void pop(mutex* p) that would take that mutex pointer and unlock it after the pop. Users could either unlock mutex after top or pass it to pop to unlock for them. This however is a fairly poor solution since an uncaught copy ctor would cause the queue to be locked forever.

mutex* m; T copy = queue.top(m); //Queue is now locked. If T's copy ctor throws we have to unlock the mutex, otherwise deadlock. queue.pop(m); //A call to pop will unlock the mutex through m.

If you have another solution please provide it. Also see Vicente's comments on this as well.

Your solution is not as simple as it could be. A possible solution consists in adding functions having an additional lock as parameter uniqye_lock<mutex> lk(queue.mutex()); T copy = queue.top(lk); queue.pop(lk); An alternative is to provide a way to get an externally_locked of the underlying queue. externally_lockedstd::queue q(queue.get()); T copy = q.top(lk); q.pop(lk); You can see the section external locking in Boost.Thread documentation We could always provide an external locking interface on lock based queues, but not for lock-free ones. Best, Vicente

...

This defaults to `boost::chrono::steady_clock`. There is a static assert to ensure that `Clock` is steady.

Given that you restrict everything to steady clocks, What's the rationale for restricting to a single `time_point` instantiation only? Furthermore, restricting to a single `duration` makes absolutely no sense. Any `duration` can be mapped to a steady `time_point` and still meet the timing specifications.

This seems to be a popular request. I will look into it.

...

This is also satisfied by `high_resolution_clock` (...) It is not.

...

(...) is no longer a factor like with `steady_clock` and `high_resolution_clock` which are wall clocks.

They are not (or may not be). The only guarantees are that `system_clock` represents a wall clock, `steady_clock` represents a clock that may not be adjusted, `high_resolution_clock` represents a clock with the shortest tick period. In particular, `steady_clock` and `high_resolution_clock` may be aliases for other clocks.

It's not guaranteed but I would assume it is steady for most modern machines. For example: #include <iostream> #include using namespace boost::chrono; int main() { std::cout << system_clock::is_steady << std::endl; std::cout << steady_clock::is_steady << std::endl; std::cout << high_resolution_clock::is_steady << std::endl; std::cout << process_user_cpu_clock::is_steady << std::endl; std::cout << process_system_cpu_clock::is_steady << std::endl; std::cout << process_real_cpu_clock::is_steady << std::endl; std::cout << thread_clock::is_steady << std::endl; } Outputs this for me on Linux 3.13 with a Core2 cpu. 0 1 1 1 1 1 1 That means I can essentially pick from 6 clocks. And the cpu/thread clocks might be useful.

I've glanced at the code, and noticed that `time_point`s and `duration`s are taken by value. The standard takes them by `const&` so I think you should follow that.

This is an oversight on my part. I will fix this. Ian,

Le 26/07/14 22:09, Agustín K-ballo Bergé a écrit :

On 26/07/2014 03:02 p.m., Ian Forbes wrote:

...

...

...

This defaults to `boost::chrono::steady_clock`. There is a static assert to ensure that `Clock` is steady.

...

Given that you restrict everything to steady clocks, What's the rationale for restricting to a single `time_point` instantiation only? Furthermore, restricting to a single `duration` makes absolutely no sense. Any `duration` can be mapped to a steady `time_point` and still meet the timing specifications.

This seems to be a popular request. I will look into it.

I am not surprised. Note that proper Chrono support is trivial for any Clock for all your timed operations but some of those involving a `sync_timed_queue`.

You may wanna read the following http://talesofcpp.fusionfenix.com/post-15/rant-on-the-templated-nature-of-st... . It was partly motivated by the poor choices made by the executors proposal. Thanks for this pointer. Really very interesting.

There is a type here auto d = std::chrono::conversion_caststd::chrono::nanoseconds(rel_time);

...

...

...

This is also satisfied by `high_resolution_clock` (...) It is not.

...

...

(...) is no longer a factor like with `steady_clock` and `high_resolution_clock` which are wall clocks.

...

They are not (or may not be). The only guarantees are that `system_clock` represents a wall clock, `steady_clock` represents a clock that may not be adjusted, `high_resolution_clock` represents a clock with the shortest tick period. In particular, `steady_clock` and `high_resolution_clock` may be aliases for other clocks.

It's not guaranteed but I would assume it is steady for most modern machines.

Sorry, but that's not how it works. You can't base your work on assumptions nor observations, only guarantees. In particular, your observations don't hold for other platforms; and then there's the broken platforms where clocks pretend to be steady when in reality they are not.

You have already correctly identified that any implementation has to be based on a steady clock (kudos! the executors proposal got this wrong). Once you have a steady clock, you can support timed operations for all `duration`s, and all `time_point`s of any other steady clock. In order to support `time_point`s of non-steady clocks too, it would be enough to meet the standard timing specifications to wrap the closure into a helper task that checks if the timeout is already satisfied, and reschedules the closure otherwise (this may not be trivial, I haven't looked at your implementation in detail).

There is one and only one standard clock that is guaranteed to be steady, and that is the `steady_clock`. Build your implementation on top of it. Don't provide the user a way to specify the implementation Clock, provide the proper interface instead. If there happens to be a better choice than `steady_clock` for a particular platform (I'm not saying there is), and said clock is **guaranteed** to be steady, then the decision to use it should belong to the library and not the user.

+1 for all your remarks. Vicente