-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rvalue reduce #169
Rvalue reduce #169
Conversation
Hi Ben, thanks for the patch. Yes, we are interested, but - since this piece of code still has to work with C++98/03 - it needs to be improved. If you are comfortable with keeping working on it, we will provide some guidance; if not, thanks anyway, and we will let you know when the patch is integrated. |
@akukanov Thanks. I can take a look at it. What's the right way to selectively do move and to overload |
How relevant is it to make all those changes, including a voodoo magic rvalue ref-qualifier (the version with the lvalue ref-qualifier is not needed), just to also eliminate the copy for result(), which happens only once per invocation? Do you have measurements (total copy/move counts, performance)? I made an implementation including a test that counts the number of copy/move x construct/assign operations, and curiously the minimum number of copies is 2, presumably for passing init and returning the result, but it can also be higher, and it seems premature to worry about returning the result really really efficiently as long as those extra copies aren't eliminated as well. BTW, apparently Standard C++ has decided to pass init by value, so that's another problem: do the same, or take it by const reference instead? Here's some test output for what I've got so far:
Those high cc (copy constructor) counts apparently are all about initialising an internal accumulator value to the identity element, which might well be negligible in aggregate to a final copy of the result (copying an empty vector is very cheap indeed), but my hunch is still that a final copy is negligible to the total work by joining accumulator values in pairs, and perhaps it would be more useful to review that first, and perhaps assemble lists of accumulator values before they are joined. The contrapositive of that would tell you not to worry about one final copy. |
Can I publish my own solution in a different branch, or how does that work with github? It's mostly interesting for the counters that I've added to the test. |
(Comment removed: if nobody corrects me I'll do it myself...) |
@rfschtkt , thank you a lot for the contribution. We are aware about your patch and find it interesting. We just need some time to look it through... Sorry for the long respond. |
I should probably first add some counters to the operations and relate them to the current counters in hard ASSERTions to make sure that all move opportunities are exploited etc... BTW, I'm not totally against the use of ref-qualifiers, but it seems a bit contrived in this context where result() is always moved from and is not exposed as public API anyway, and I really would like to know how relevant it is in actual usage to move the final result (I'm just being an armchair critic here). I've also been thinking that it seems very strange indeed that there is an identity element at all. It is perfectly possible to parallelise a reduction on a semigroup that is not a monoid, and I would have reserved the name "reduce" for such usage, as in, e.g., Scala, which distinguishes "reduce" from "fold" and its variants. Of course it is possible to do this on top of the current inter-domain operation, but perhaps there should be an additional API to do this directly, maybe simply using the same name but with the identity parameter omitted? |
@rfschtkt, you are right, I probably didn't need to use ref qualifiers. I like your diff. The one change I'd suggest is instead of |
I've used RVALUE_REF_ELSE_LVALUE_REF in the test file, but it could/should obviously also be introduced in some form to the library itself, although preferably consistently and not just in this context, which seems like a project all by itself. (Added) But it would not make any difference to the user in this case, only perhaps make the library code somewhat easier to read by itself, although rvalue references were not introduced to the language to facilitate code comprehension but to differentiate two kinds of arguments for hard performance-related reasons, and here the argument is always expendable anyway, nothing "inadvertent" about it. |
I think I've reached the final limits of avoiding unnecessary copies now in #171. If it were ever to make a difference whether joins are performed from small to large or all together at the end (?), the user should probably implement this in a Body, and even a library-provided adapter might be more confusing than helpful in this fringe case, so I'm not going to explore that. However, I might have a look into supporting non-monoid semigroups next, primarily for the not-so-fringe benefits of the user's not having to provide an identity element and maybe also of the library's not having to copy it to construct each new internal value. |
As for identity element, we have experience with implementation of |
Well, you wouldn't have to use the version without identity element... but I also don't see the problem with SIMD if you implement the so-called RealBody with an identity value right there instead of one passed as an argument to The main idea is something like the following (probably using the TBB allocator instead, if not std::optional where available):
The biggest problem I'm seeing is all the annoying documentation that prevents generating the many versions automatically using macro expansion and/or templates, and adding another dimension would double the number of "lambda" versions... |
The proposed solution seems inefficient for heavy
The above expression prevents |
Does TBB have to support any compilers that don't provide (N)RVO? Evidently this approach would split up each Body "range extension" into a pair of so-called RealBody and Reduction calls (I would prefer to call these Reduction and Operation, respectively...), causing more calls to the latter, but how costly is such a call anyway if it can treat its arguments like C++11 rvalue references, and what percentage of Body invocations are "range extensions" (Amdahl)? And again, you wouldn't have to use the version without identity element, but why would all those functional languages provide reduce() as well as fold() if both didn't have their use? I might have some working code later this week. So far I've made some progress reducing the number of overloads using templates and SFINAE. (Added) First draft committed: about 100 extra lines, horrible! (Added) Well, first outline anyway: I still have to at least try to derive the type of Value (or else this should become the first template argument), my_value is leaked, etc. (Added) Now with first smoke test. I cheated by using explicit "reduce1" names, but maybe that can still be avoided by type tests on some of the arguments. I'm now thinking of either moving Value to the front in the template arguments list and requiring it to be specified, or requiring at least C++11 and deriving it from one of the operations (although I was hoping to also support C++98/03 functors, even though this is not currently tested with the other "lambda" versions either). (Added) BTW, it would also be possible to omit the so-called RealBody, and just iterate on the so-called Reduction, both with and without Identity element. Now think of the bloat that would cause! But it would be very user-friendly indeed if you couldn't vectorise the RealBody anyway. |
I am not sure that (N)RVO can solve array reduction concern. If |
Not sure I'm following you here. What I wrote about (N)RVO was to ask if your concern was that assigning to the intermediate variable would make a copy, which would indeed be the case for compilers that don't know how to do copy elision. Otherwise it doesn't seem to matter, because passing along the value is done in constant time anyway; with C++11 you could return There is of course the difference that non-initial |
"(although I was hoping to also support C++98/03 functors, even though this is not currently tested with the other "lambda" versions either)" Wait... what? Of course it is currently tested! Maybe my brain just had a glitch because of wondering earlier but not coming to a conclusion about why there is any lambda naming and testing going on at all, because a lambda expression is just a way to create a "closure object", which "behaves like a function object" (according to the Standard). So aren't these __TBB_CPP11_LAMBDAS_PRESENT tests following the functor tests merely testing the compiler rather than TBB, and wouldn't it be proper maintenance to just remove this clutter? Or what am I missing here? |
Sure, you can test lambda oriented interface with functors but it does not seem convenient and it almost does not have sense from the user perspective. As for array reduction, my only regret that we cannot reduce into
However, this concern relates only to lambda based interface, we can achieve in-place reduction with classic Body interface. |
"Sure, you can test lambda oriented interface with functors but it does not seem convenient and it almost does not have sense from the user perspective." I'm currently stuck with the no-Identity interface. I don't think I need a separate template name with enough type tests (although I have not fully worked that out yet), but it also doesn't solve the problem that remains: how to reconcile that C++98/03 needs a type for the return value (using an explicit template argument for Value seems to be the only way to do that?), and that C++11 can just get by with something like So it seems as if either C++98/03 should not get these new overloads, or both library and user have to differentiate, and most users probably don't have to support C++98/03 anymore anyway so it's not that much of a burden on the user side of the equation. With differentiation, the second question is whether the template parameters may be switched around so that only a Value has to be supplied rather than first Range and then Value. |
I believe it is acceptable if the new interface works only with C++11 (to avoid C++98/03 support |
I haven't done much yet, besides mulling over the design. Behind the scenes, the idea is fairly obvious, using either tbb::cache_aligned_allocator (I guess?) or perhaps differentiated for C++17 using std::optional for the current value, starting out as nullptr resp. nullopt (augmenting the destination domain with an "identity" value). Meanwhile, I've only added some counters to the pull request ("Yet more counters", reordered before the no-identity commits) to investigate the average reduction length (the longer I look at this, the more I think that the code should switch naming from RealBody&Reduction to Reduction&Operation, respectively), and it seems acceptable to use a lambda that creates a new value in the destination domain each time... but I'm also thinking of breaking the simple symmetry anyway and using a pointer parameter for the starting value instead, to take all concerns about destination domain value creation amortisation off the table, except that this would be weird if you just wanted to omit an argument for brevity even though you do have an identity value in the destination domain, which would probably be the main use case. Should the new parallel_reduce overloads accept both signatures to get out of this conundrum? That would be nice for the user, but the implementation would get even more complicated... Or else the implementation should only worry about the brevity-related use case, and then semigroup users should either accept limited amortisation or bring their own destination domain augmentation with the explicit-identity overloads, e.g., boost::optional or std::optional? (Added) Why did I write "source domain" 3 times when I meant "destination domain" (now corrected)? |
I pushed some plausible initial support for semigroups, only with symmetric interface (see above). Still wondering about the cost, both relative to ultra-cheap explicit identity elements and when destination domain values are expensive to generate de novo. (Added) Using aligned_space now, should be good enough to compete with the former, although the issue with the latter is not affected. (Added) Now with updated inspectCounters(). Next would be tests for new deterministic overloads. (Added) Tests completed. |
So that's about enough exploration, I would say. Did I overlook anything (other than some clean-up)? The main things are moving things around for performance (also before C++11 by using non-const rather than const lvalue references), and allowing usage without an identity element (for convenience and/or out of necessity, only from C++11). The implementation was condensed a bit before it got bloated again with the new overloads, and counters were added to validate what's going on with object life cycles. Plus some sneaky suggestions here and there of course. :-) |
I squashed all my commits in #171 and declared it ready for review. (Added) To make a review easier, I split it up again into thematic commits. (Added) Removed |
Does anything require further clarification? Any suggestions? |
Go for it. |
Maybe they're waiting for me to add some more overloads that synthesise the |
@BenFrantzDale can you re-target the path to the master branch ? (both tbb2020 and tbb2019 are no longer updated) |
7045af3
to
a92c3ea
Compare
@anton-potapov I just force-pushed. I went ahead and made the arg of |
Since #1307 was merged to master, this PR can be closed. Thanks to everyone for the productive discussion. |
Use rvalue references to allow reduction to more-efficiently handle operations like vector or list concatenation as the reduction operation.
Are you interested in this?