-
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
Tentative rvalue reference support for parallel_reduce #171
Tentative rvalue reference support for parallel_reduce #171
Conversation
6b3f665
to
0dc4b53
Compare
fd6be4e
to
c501689
Compare
204a8e6
to
302528b
Compare
398dfa7
to
2f283a1
Compare
70798a5
to
bad586d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding more expressive comments in the commit message describing the rationale for incorporating the changes would facilitate acceptance of the code changes.
The benefits of the proposed changes may be obvious to you, but they may require the reviewer to do extensive code hunting to understand the benefit of the changes.
Clearly stating the benefits of accepting the proposed changes makes reviewing tremendously easier.
Hmm, I would hope that the code and the comments there speak for themselves, without depending on any rationale hidden in a history that may not even remain accessible... Does it help if I characterise the current 9 commits as, respectively: trivial, frivolous (x4), preparation, material (x2), cleanup? Otherwise, please give an example of what you would like to see instead. (Added) I just force-pushed the same commits, but reworded, and with "Frivolous: alignment" first split off from "Material: rvalue support", then relocated next to the other frivolous commits, and finally edited with a variable renamed and a comment removed (I verified that these were also the only changes to the final result). (Added) Somebody stop me! Now with "Frivolous: whitespace". (Added) And function->operation. (Added) Applied final cleanup to previous commits instead. (Added) Corrected "body" -> "function object", renamed Accumulator*. (Added) Sorry, I did not maintain this, there are more changes after this. Maybe I should have tried to keep the history in git (if that is easily done and not too confusing)? |
cd69591
to
7c58e99
Compare
7c58e99
to
f58c370
Compare
c6f4417
to
3f37419
Compare
Some (trivial) tinkering with "Ancillary: Foo->Minimal" (only): MinimalBody member variables now always initialised, instead of assigned either inside or outside the constructor. Still wondering where std::optional, backported or not, causes additional move-construction to happen... but not enough to try to find out. |
What's needed to push this over the line? |
I am resigned to the distinct possibility that that might never happen, yet still available to assist in any serious attempt to integrate some of the ideas I had fun exploring. |
f662b2f
to
75808c3
Compare
Oops! I did it again... Moved a comment from the "Experimental: std::optional" commit to the "Material: semigroup support" where it belongs, then squashed the previous commits about "Experimental: std::optional" with an improved comment and some asserts added. BTW, I just noticed that my history rewriting seems to have discarded @hjmjohnson's review comments (sorry!)... maybe try again? |
75808c3
to
f7c1ff6
Compare
Moved some inline comments from "Experimental: std::optional" to "Material: rvalue support" where they belong, and minimally changed some others. But the outcome was negative anyway, so this commit can be disregarded. |
It seems like this adds a lot, going beyond the scope of adding rvalue support. Is there a reason not to do the simple change of just having I appreciate the desire for a more-elegant solution and for avoiding reconstructing default values, but it feels like that's something that callers could do if they want (using |
PS: There's no Issue to go with this. Should I create one? |
The main issue that we need to rebase the patch on onetbb_2021 because we are not planning to update TBB 2020. Unfortunately, the code base has changed over parallel_reduce and I expect a lot of merge conflicts. The good news is that we can rely on C++11 and do not need to provide compatibility with C++03. Sorry that I request additional work, can you reconsider (or at least rebase) the approach on the new code base? |
You certainly don't need all the ideas I had fun exploring here (although you also shouldn't forget to |
I'm not entirely convinced yet that git best practices were top of mind when branch onetbb_2021 was created... There are also lots of differences between the relevant corresponding files, so perhaps it would be best to first make firm decisions on what will be used, and to then reapply those changes manually to the new branch. BTW, I'm curious how |
The onetbb_2021 branch is not real branch. It is a completely new code base that partly was copied from TBB 2020 but the most of the code was reconsidered. With ranges, do you mean C++20 ranges support? |
What I meant was that TBB uses separate and redundant operations, one for reducing over a range of elements and the other for joining intermediate results: the former could most easily be implemented in terms of the latter, computationally speaking, but taking it as a parameter also offers an opportunity for explicit vectorisation or any other intervention for optimisation, so I was just wondering how often this is significant and to what extent. (2020-09-15 Added) To avoid misunderstanding, I think I'd rather be forced to always provide a range operation than not being able to customise one, but it might be even nicer to have it be an optional parameter. I think I gave up on the idea earlier because it probably requires C++11 (or even later?) and because there were already so many overloads, but if C++11 can now be assumed and the semigroup stuff could be handled separately by |
f7c1ff6
to
3edb858
Compare
Semigroup support now with (2020-09-18 Added) |
3edb858
to
e5b9d50
Compare
I don't think I'll be playing around with making range operations optional after all. The later operation-based functions entirely replace the original Body-based ones, but to provide the same functionality without range operations (in those cases where only performance-related things like explicit vectorisation can be omitted), there would also have to be I'll look into what's required to rebase this, but if it takes significant effort I would prefer the editorialising to occur prior to the effort with a firm commitment to integrate the result... (2020-09-20 Added, then corrected because somehow an intermediate version was saved) It seems that it would take significant effort, so I'm not going to rebase "at risk" (see COVID-19 vaccine development efforts)... Did anybody else notice the problem with how the aggregation type is (trivially) deduced from the identity argument (unless it is explicitly specified as a template argument, which is only possible by also specifying the type of the earlier Range parameter, which is quite cumbersome and therefore unlikely)? That seems quite the "design bug", apparently inherited from the accidental circumstance that (Added, then corrected a mistake) Alas, |
Since #1307 was merged to master, this PR can be closed. Thanks to everyone for the productive discussion. |
My own take on rvalue reference support for parallel_reduce (see comments in pull request #169), extended with support for semigroups, i.e., without using an identity element.
For a while, shameless history rewriting was going on here, then on 2019-09-09 I squashed everything, and now I intend to only do straight commits... or that was the intention before I rebased into thematic commits for easier review.