[fiber] ready for next review
As request, I'd like to announce that boost.fiber has addressed the requests of the last review. Especially migrating fibers between threads is supported - sub-directory 'examples' contains code demonstrating work-sharing and work-stealing. boost.fiber: github.com/olk/boost-fiber/tree/develop (branch develop) requires boost.context (branch develop) Oliver
On 12/2/2015 6:19 PM, Oliver Kowalke wrote:
As request, I'd like to announce that boost.fiber has addressed the requests of the last review. Especially migrating fibers between threads is supported - sub-directory 'examples' contains code demonstrating work-sharing and work-stealing.
boost.fiber: github.com/olk/boost-fiber/tree/develop (branch develop)
Where can we find the documentation that corresponds to this branch? Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
2015-12-03 13:26 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>: Where can we find the documentation that corresponds to this branch?
On 12/3/2015 3:49 PM, Oliver Kowalke wrote:
2015-12-03 13:26 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Where can we find the documentation that corresponds to this branch?
Is this the one that corresponds to the develop branch? I looked for three specific mistakes that I raised during the review, but nothing seems to have changed there... Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
2015-12-03 21:45 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
On 12/3/2015 3:49 PM, Oliver Kowalke wrote:
2015-12-03 13:26 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Where can we find the documentation that corresponds to this branch?
Is this the one that corresponds to the develop branch? I looked for three specific mistakes that I raised during the review, but nothing seems to have changed there...
in the documentation?
On 12/3/2015 5:50 PM, Oliver Kowalke wrote:
2015-12-03 21:45 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
On 12/3/2015 3:49 PM, Oliver Kowalke wrote:
2015-12-03 13:26 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Where can we find the documentation that corresponds to this branch?
Is this the one that corresponds to the develop branch? I looked for three specific mistakes that I raised during the review, but nothing seems to have changed there...
in the documentation?
Yes, in the documentation. During the re-review process I raised three conceptual mistakes in the documentation, important things and not simply just typos. I see they are still there in the documentation that corresponds to the develop branch, where review feedback has supposedly been addressed. So I went looking at the code, and while I do see some problems I reported have been addressed, others have not. Could you please just let us know exactly which pieces of feedback did you address and which did you decide to ignore, so that we can make sure nothing has been unintentionally fell through the cracks again? Thank you. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Yes, in the documentation.
documentation might need some updates - my announcement was primarly focused to the source code
During the re-review process I raised three conceptual mistakes in the documentation, important things and not simply just typos.
which one - I can't remember your concerns. maybe 'this_fiber::yield() (et al.) are valid from main()' issue?
I see they are still there in the documentation that corresponds to the develop branch, where review feedback has supposedly been addressed.
So I went looking at the code, and while I do see some problems I reported have been addressed, others have not. Could you please just let us know exactly which pieces of feedback did you address and which did you decide to ignore,
* addressed: - cross-thread fiber migration - suggestions related to scheduler structure - wait_until() accepts any supported time_point - mark_ready_and_notify_() accepts std::unique_lock<> - release mutex before condition_variable signals - async() does not return by std::move() - use of intrusive-lists - re-factoring of future<> and related classes - use of predicate-based condition_variable::wait() internally * ignored: - C++11 equivalent for deferred calls (was impossible to get right for all use cases in boost.fiber) - nested schedulers * todo: - update documentation
On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Yes, in the documentation.
documentation might need some updates - my announcement was primarly focused to the source code
Fair enough, I was misguided by the "ready for next review" subject as well as the announcement that requests from the review have been addressed. This is obviously not the case, but we can still make a lot of progress based on source code adjustments only.
During the re-review process I raised three conceptual mistakes in the documentation, important things and not simply just typos.
which one - I can't remember your concerns. maybe 'this_fiber::yield() (et al.) are valid from main()' issue?
Please go back to that thread where people put their time and effort at your disposal to provide feedback for your library. Take with you pen and paper, and write each piece of feedback down, even those you've already acted on. Then let us know which of them you have addressed, and how you have done so (accepted, discarded, reasons, etc). This will help us avoid wasting our time now looking for things that haven't yet been addressed, like documentation changes, and it will help you to get ready for the next round of review so that we don't waste everyone's time then. Note that I have provided extensive and detailed feedback during the first review, and a big chunk of it was ignored, lost, or dropped on the floor. I had to re-raise those concerns among the fresh ones during the re-review, and from the looks of it we are heading into the same situation yet again. I would very much like to avoid having to re-re-raise these concerns again... As for those three particular conceptual mistakes in the documentation I was talking about, those are: - Bad code in example leading to races in destruction, with a big bold incorrect explanation on how the code is "safe" when it isn't. - Inconsistent `condition_variable` / `condition_variable_any` definitions, where the interface of `condition_variable_any` is exposed twice. - Inconsistent and contradictory `condition_variable` / `condition_variable_any` requirements, which are even stricter than those of `std::condition_variable`.
I see they are still there in the documentation that corresponds to the develop branch, where review feedback has supposedly been addressed.
So I went looking at the code, and while I do see some problems I reported have been addressed, others have not. Could you please just let us know exactly which pieces of feedback did you address and which did you decide to ignore,
* addressed: - cross-thread fiber migration - suggestions related to scheduler structure - wait_until() accepts any supported time_point - mark_ready_and_notify_() accepts std::unique_lock<> - release mutex before condition_variable signals - async() does not return by std::move() - use of intrusive-lists - re-factoring of future<> and related classes - use of predicate-based condition_variable::wait() internally
The list of feedback you have received is considerably larger than this one, even without considering those concerns raised only during the first round of review which still need to be addressed.
* ignored: - C++11 equivalent for deferred calls (was impossible to get right for all use cases in boost.fiber)
Note that this was not an official part of my review, but a comment on the margin. It's a pitiful decision which renders the library useless to me for no good reason, since C++14 only brings about one and a half new features with no known workaround, neither of which is being exploited by the library. But since you bring it up, I would like to know which cases you found to be impossible to get right. Since this is supposed to be just a mechanical transformation, it would be good to know those corner cases where it falls short. Related to this was my also unofficial suggestion to reuse one of the many implementations of `index_sequence` within Boost implementation details when `std::index_sequence` is not available. I don't see it mentioned in these lists, and I can see no code changes were done in this area, so I would like to know whether this is missing from the ignored list or just missing from the to-do list. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
2015-12-04 15:48 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>: Please go back to that thread where people put their time and effort at
your disposal to provide feedback for your library. Take with you pen and paper, and write each piece of feedback down, even those you've already acted on. Then let us know which of them you have addressed, and how you have done so (accepted, discarded, reasons, etc). This will help us avoid wasting our time now looking for things that haven't yet been addressed, like documentation changes, and it will help you to get ready for the next round of review so that we don't waste everyone's time then.
I'm sorry that I waste your time
Note that I have provided extensive and detailed feedback during the first review, and a big chunk of it was ignored, lost, or dropped on the floor.
I used the summary of the last review
On 12/4/2015 2:25 PM, Oliver Kowalke wrote:
2015-12-04 15:48 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Please go back to that thread where people put their time and effort at
your disposal to provide feedback for your library. Take with you pen and paper, and write each piece of feedback down, even those you've already acted on. Then let us know which of them you have addressed, and how you have done so (accepted, discarded, reasons, etc). This will help us avoid wasting our time now looking for things that haven't yet been addressed, like documentation changes, and it will help you to get ready for the next round of review so that we don't waste everyone's time then.
I'm sorry that I waste your time
Let's just focus on addressing all that feedback that has thus far been ignored.
Note that I have provided extensive and detailed feedback during the first review, and a big chunk of it was ignored, lost, or dropped on the floor.
I used the summary of the last review
Of all the messages I sent during the review, only one of them was directed at the review manager. I believe the goal of the review summary is to present us, reviewers and casual bystanders, with a general view of what went on during the review. I might be wrong at that, but I don't think the goal of the summary is to exempt you, the library author, of having to read all the discussion and feedback meant for you to improve the quality of your library. And I happen to think Nat did a great job with it. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
On 12/4/2015 11:48 AM, Agustín K-ballo Bergé wrote:
On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Yes, in the documentation.
documentation might need some updates - my announcement was primarly focused to the source code
Fair enough, I was misguided by the "ready for next review" subject as well as the announcement that requests from the review have been addressed. This is obviously not the case, but we can still make a lot of progress based on source code adjustments only.
I found an unusual pattern in the code, where memory is allocated via an allocator's `allocate` but afterwards `construct` is side-stepped and placement new is used instead. This breaks proper allocator support. Curiously `destroy` is correctly used down the line. Furthermore, from a cursory look it seems C++11 allocators are not supported. The code assumes a C++03 allocator interface. If it is intentional then this requirement should be documented. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
2015-12-05 15:03 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
On 12/4/2015 11:48 AM, Agustín K-ballo Bergé wrote:
On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com
:
Yes, in the documentation.
documentation might need some updates - my announcement was primarly focused to the source code
Fair enough, I was misguided by the "ready for next review" subject as well as the announcement that requests from the review have been addressed. This is obviously not the case, but we can still make a lot of progress based on source code adjustments only.
I found an unusual pattern in the code, where memory is allocated via an allocator's `allocate` but afterwards `construct` is side-stepped and placement new is used instead. This breaks proper allocator support. Curiously `destroy` is correctly used down the line.
Furthermore, from a cursory look it seems C++11 allocators are not supported. The code assumes a C++03 allocator interface. If it is intentional then this requirement should be documented.
it is not an ordinary allocator - it's a 'stack allocator' placement new is used to allocate the control-structure on the fiber-stack (instead on using new -> heap)
On 12/5/2015 11:18 AM, Oliver Kowalke wrote:
2015-12-05 15:03 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
On 12/4/2015 11:48 AM, Agustín K-ballo Bergé wrote:
On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com
:
Yes, in the documentation.
documentation might need some updates - my announcement was primarly focused to the source code
Fair enough, I was misguided by the "ready for next review" subject as well as the announcement that requests from the review have been addressed. This is obviously not the case, but we can still make a lot of progress based on source code adjustments only.
I found an unusual pattern in the code, where memory is allocated via an allocator's `allocate` but afterwards `construct` is side-stepped and placement new is used instead. This breaks proper allocator support. Curiously `destroy` is correctly used down the line.
Furthermore, from a cursory look it seems C++11 allocators are not supported. The code assumes a C++03 allocator interface. If it is intentional then this requirement should be documented.
it is not an ordinary allocator - it's a 'stack allocator' placement new is used to allocate the control-structure on the fiber-stack (instead on using new -> heap)
I was actually referring to `promise` and `packaged_task`. Is that the case there too? If that means an argument whose type is `Allocator` has stronger requirements than those expected by the standard library it should be documented accordingly. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
2015-12-05 15:03 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
I found an unusual pattern in the code, where memory is allocated via an allocator's `allocate` but afterwards `construct` is side-stepped and placement new is used instead. This breaks proper allocator support. Curiously `destroy` is correctly used down the line.
allocators: classes fixedsize_stack/segmented_stack etc. are not allocators you know by the C++standard(s). those classes are used to allocate stack space. The classes make use of fucntionallity not common for allocators you have had in mind - for instance adding a protected page at the end or using splitstack-functions like __splitstack_makecontext).
Furthermore, from a cursory look it seems C++11 allocators are not supported. The code assumes a C++03 allocator interface. If it is intentional then this requirement should be documented.
In class promise and packaged_task probably simply copy&paste the code from the StackAllocator snippets. What's wrong with allocator::allocate() + placement new instead of allocator::allocate() + allocator::construct()?
On 12/5/2015 12:05 PM, Oliver Kowalke wrote:
2015-12-05 15:03 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Furthermore, from a cursory look it seems C++11 allocators are not supported. The code assumes a C++03 allocator interface. If it is intentional then this requirement should be documented.
In class promise and packaged_task probably simply copy&paste the code from the StackAllocator snippets. What's wrong with allocator::allocate() + placement new instead of allocator::allocate() + allocator::construct()?
An allocator might do other stuff in `construct` besides placement-new construction, which is what the default `std::allocator_traits` does. An allocator might, for instance, choose to trace all calls and log them, or it might choose to defer construction to some other allocator. Allocators like `scoped_allocator_adaptor` inject additional arguments and/or select different constructors to call (although I do not think this applies here). An allocator might even reject to construct certain types or use certain types of arguments, which they do so by failing to instantiate `construct` (17.6.3.5 [allocator.requirements]/9). Furthermore, keep in mind that `allocator::allocate` does not necessarily return a **raw** pointer. Using `std::allocator_traits` here would, besides giving some support for C++11 allocators, also pick the right placement-new to call in the case of nasty overloads (current code correctly qualifies `::new` but fails to secure a `void*` argument). Also, use of the replaceable standard library placement new requires `#include <new>`. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
On 12/5/2015 12:37 PM, Agustín K-ballo Bergé wrote:
On 12/5/2015 12:05 PM, Oliver Kowalke wrote:
2015-12-05 15:03 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Furthermore, from a cursory look it seems C++11 allocators are not supported. The code assumes a C++03 allocator interface. If it is intentional then this requirement should be documented.
In class promise and packaged_task probably simply copy&paste the code from the StackAllocator snippets. What's wrong with allocator::allocate() + placement new instead of allocator::allocate() + allocator::construct()?
An allocator might do other stuff in `construct` besides placement-new construction, which is what the default `std::allocator_traits` does. An allocator might, for instance, choose to trace all calls and log them, or it might choose to defer construction to some other allocator. Allocators like `scoped_allocator_adaptor` inject additional arguments and/or select different constructors to call (although I do not think this applies here). An allocator might even reject to construct certain types or use certain types of arguments, which they do so by failing to instantiate `construct` (17.6.3.5 [allocator.requirements]/9).
Furthermore, keep in mind that `allocator::allocate` does not necessarily return a **raw** pointer.
Using `std::allocator_traits` here would, besides giving some support for C++11 allocators, also pick the right placement-new to call in the case of nasty overloads (current code correctly qualifies `::new` but fails to secure a `void*` argument). Also, use of the replaceable standard library placement new requires `#include <new>`.
Some more notes on allocator usage: When constructing an object on allocated storage, there's a chance an exception is thrown. In that case, the allocated storage should not be leaked. This is at least a concern for `packaged_task`, that decay-copies user-defined types during construction. When destroying an object which might (or in this case will) have an embedded allocator, the allocator has to be copied out first. This is to avoid accessing a destroyed object, as it could happen if an allocator `destroy` member function decides to run code after performing the destruction (again, it could for instance decide to log the operation to some file handle it keeps as a member). Back to `packaged_task`, it's member function `reset` is not resetting the `packaged_task` but it resets the shared state instead. This is not how the standard defines this operation, and it is a particularly dangerous semantic to have. Once a shared state is observed as ready it cannot go back to not ready, and it's result cannot be overwritten. Resetting a `packaged_task` requires allocating a new shared state and moving the type-erased target callable from the old one to it. That said, if the shared state is known to be non-observable (the `packaged_task` itself holds the last reference to it), then reusing the shared state is a viable optimization. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
On 12/4/2015 11:48 AM, Agustín K-ballo Bergé wrote:
On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Yes, in the documentation.
documentation might need some updates - my announcement was primarly focused to the source code
Fair enough, I was misguided by the "ready for next review" subject as well as the announcement that requests from the review have been addressed. This is obviously not the case, but we can still make a lot of progress based on source code adjustments only.
* addressed: - re-factoring of future<> and related classes
I assume this ^ refers to the following piece of feedback: http://lists.boost.org/Archives/boost/2015/09/225289.php The refactoring does not appear to have happened for `promise`. Is this intentional, or just an oversight? In the futures, `wait_for` and `wait_until` stand out as they have been defined to forward to base instead of simply using `using`. Is there a reason for that? Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
2015-12-05 15:08 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>: The refactoring does not appear to have happened for `promise`. Is this
intentional, or just an oversight?
oversight
In the futures, `wait_for` and `wait_until` stand out as they have been defined to forward to base instead of simply using `using`. Is there a reason for that?
a false usage of directive usage together with templated member functions - fixed
On 12/4/2015 11:48 AM, Agustín K-ballo Bergé wrote:
On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Yes, in the documentation.
documentation might need some updates - my announcement was primarly focused to the source code
Fair enough, I was misguided by the "ready for next review" subject as well as the announcement that requests from the review have been addressed. This is obviously not the case, but we can still make a lot of progress based on source code adjustments only.
The future error category uses plain "future" as its name. This is not technically wrong, but given that it is used to identify the category that an error belongs to, it would be better to choose a name that does not clash with the standard library future error category. I would like to suggest "boost::fiber", or "boost::fiber::future" if there might be other error categories. The implementation of `default_error_condition` makes use of some alarming hard-coded magic constants in a switch statement. It appears that every case in the switch is actually doing the same thing, which is just what the overriden function already does. Since there is no mapping of error values to default error conditions happening here, I would suggest to drop the definition entirely. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
On 12/4/2015 11:48 AM, Agustín K-ballo Bergé wrote:
On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Yes, in the documentation.
documentation might need some updates - my announcement was primarly focused to the source code
Fair enough, I was misguided by the "ready for next review" subject as well as the announcement that requests from the review have been addressed. This is obviously not the case, but we can still make a lot of progress based on source code adjustments only.
The value of `future_status` enumerators is shifted when compared to the corresponding `std::` definition. This could lead to subtle mistakes for people familiarized with the standard definition. I would suggest to simply follow the normative definition from the standard here. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
On 12/4/2015 11:48 AM, Agustín K-ballo Bergé wrote:
On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Yes, in the documentation.
documentation might need some updates - my announcement was primarly focused to the source code
Fair enough, I was misguided by the "ready for next review" subject as well as the announcement that requests from the review have been addressed. This is obviously not the case, but we can still make a lot of progress based on source code adjustments only.
While tracing overload resolution inside the library, I noticed a regression in the way rvalues are handled (some of the intervening are not in Boost.Context). When an argument whose type is deduced as `T&&` is used in a function type, like with `result_of`, it has to be specified as `T&&`. If the rvalue reference is omitted then cv-qualifiers will be dropped, and arrays and functions will turn into pointers. Note this issue overlaps with already reported issues on incorrect result type computations, and invalid decay-copying. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
On 12/5/2015 11:25 AM, Agustín K-ballo Bergé wrote:
On 12/4/2015 11:48 AM, Agustín K-ballo Bergé wrote:
On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Yes, in the documentation.
documentation might need some updates - my announcement was primarly focused to the source code
Fair enough, I was misguided by the "ready for next review" subject as well as the announcement that requests from the review have been addressed. This is obviously not the case, but we can still make a lot of progress based on source code adjustments only.
While tracing overload resolution inside the library, I noticed a regression in the way rvalues are handled (some of the intervening are not in Boost.Context).
Ugh, I should double check before sending... What I meant here is that some of the intervening pieces (the INVOKE related stuff) are now in Boost.Context detail namespace, so this issue affects both libraries.
When an argument whose type is deduced as `T&&` is used in a function type, like with `result_of`, it has to be specified as `T&&`. If the rvalue reference is omitted then cv-qualifiers will be dropped, and arrays and functions will turn into pointers.
Note this issue overlaps with already reported issues on incorrect result type computations, and invalid decay-copying.
Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
On 12/4/2015 11:48 AM, Agustín K-ballo Bergé wrote:
On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86@hotmail.com>:
Yes, in the documentation.
documentation might need some updates - my announcement was primarly focused to the source code
Fair enough, I was misguided by the "ready for next review" subject as well as the announcement that requests from the review have been addressed. This is obviously not the case, but we can still make a lot of progress based on source code adjustments only.
The destructor for `shared_state` contains the following code: if ( ready_) { reinterpret_cast< R const* >( storage_)->~R(); } As far as I can see, `ready_` will be set both when the shared state holds a value or an exception. This would mean a shared state holding an exception would run arbitrary code on random garbage during destruction (sidenote: how did this manage to eluded unit tests??). The choice of a cast to `const` pointer for destruction is peculiar. Not wrong, just odd, since qualifiers stop being in effect when the destructor starts. The choice of `reinterpret_cast` is questionable. It would be safer to replace it with the two `static_cast`s this use case is supposed to synthesize. It'd probable be good too to replace the "redundant" artificial array of one storage this `reinterpret_cast` is relying on. ...or maybe just use `optional<R>`? Furthermore, the member function `reset` will simply set the `ready_` flag to `false`, leaking the result value if there is one. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
participants (2)
-
Agustín K-ballo Bergé -
Oliver Kowalke