[math] Enable move semantics for polynomials
Hello all The polynomial class in boost.math does not support move sematics. I tried to implement them and found that they immediately lead to significant improvements in performance (benchmarking using google-benchmark). I would like to get the changes merged into the main repository. I understand that boost accepts very high quality C++ code and since this will be my first time contributing to boost I am looking forward to assistance from other experienced contributors. Could someone please advise me on how to move forward and get the changes merged. Regards Lakshay
On 8/13/17 4:31 AM, Lakshay Garg via Boost wrote:
Hello all
The polynomial class in boost.math does not support move sematics. I tried to implement them and found that they immediately lead to significant improvements in performance (benchmarking using google-benchmark).
I would like to get the changes merged into the main repository. I understand that boost accepts very high quality C++ code and since this will be my first time contributing to boost I am looking forward to assistance from other experienced contributors.
Could someone please advise me on how to move forward and get the changes merged.
Sounds worthwhile. While you're at it, how about considering making all the functions constexpr ? Robert Ramey
On 13/08/2017 22:05, Robert Ramey via Boost wrote:
On 8/13/17 4:31 AM, Lakshay Garg via Boost wrote:
Hello all
The polynomial class in boost.math does not support move sematics. I tried to implement them and found that they immediately lead to significant improvements in performance (benchmarking using google-benchmark).
I would like to get the changes merged into the main repository. I understand that boost accepts very high quality C++ code and since this will be my first time contributing to boost I am looking forward to assistance from other experienced contributors.
Could someone please advise me on how to move forward and get the changes merged.
Sounds worthwhile. While you're at it, how about considering making all the functions constexpr ?
constexpr support would require the underlying container (std::vector) to be constexpr as well, in fact since new (variable sized) storage is required, I don't see how that would be possible at all..... you could as someone suggested have a fixed storage polynomial class with multiplication yielding a different type... any non-trivial manipulation would lead to a complete explosion of template instantiations though. John. --- This email has been checked for viruses by AVG. http://www.avg.com
On 14 August 2017 at 12:37, John Maddock via Boost <boost@lists.boost.org> wrote:
On 13/08/2017 22:05, Robert Ramey via Boost wrote:
On 8/13/17 4:31 AM, Lakshay Garg via Boost wrote:
Hello all
The polynomial class in boost.math does not support move sematics. I tried to implement them and found that they immediately lead to significant improvements in performance (benchmarking using google-benchmark).
I would like to get the changes merged into the main repository. I understand that boost accepts very high quality C++ code and since this will be my first time contributing to boost I am looking forward to assistance from other experienced contributors.
Could someone please advise me on how to move forward and get the changes merged.
Sounds worthwhile. While you're at it, how about considering making all the functions constexpr ?
constexpr support would require the underlying container (std::vector) to be constexpr as well, in fact since new (variable sized) storage is required, I don't see how that would be possible at all..... you could as someone suggested have a fixed storage polynomial class with multiplication yielding a different type... any non-trivial manipulation would lead to a complete explosion of template instantiations though.
Yes exactly. That is what I'm concerned about too. I'm not sure about constexpr or fixed storage polynomials but I think having a class which supports sparse polynomials (eg: 1 + x^4 + x^100) by using a linked list would be useful. I would like to implement the functionality if I receive a positive response. Comments are welcome.
On 8/14/17 12:07 AM, John Maddock via Boost wrote:
Sounds worthwhile. While you're at it, how about considering making all the functions constexpr ?
constexpr support would require the underlying container (std::vector) to be constexpr as well, in fact since new (variable sized) storage is required, I don't see how that would be possible at all..... you could as someone suggested have a fixed storage polynomial class with multiplication yielding a different type...
I think that someone was me.
any non-trivial manipulation would lead to a complete explosion of template instantiations though.
This doesn't actually bother me. It's happened with the safe numerics library and it hasn't created a problem - at least in my tests and examples. I suggested this idea without really thinking it through. Hmmm - not an uncommon occurrence. I see now that it would require a totally new library based on tuple rather than on vector. Robert Ramey
John.
--- This email has been checked for viruses by AVG. http://www.avg.com
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 8/13/2017 7:31 AM, Lakshay Garg via Boost wrote:
Hello all
The polynomial class in boost.math does not support move sematics. I tried to implement them and found that they immediately lead to significant improvements in performance (benchmarking using google-benchmark).
I would like to get the changes merged into the main repository. I understand that boost accepts very high quality C++ code and since this will be my first time contributing to boost I am looking forward to assistance from other experienced contributors.
Could someone please advise me on how to move forward and get the changes merged.
Make a Github PR ( pull request ) against the library in Github. All PRs should be against the 'develop' branch of a library. If the PR is merged, it will update the 'develop' branch of the library. After appropriate testing the library's maintainer(s) will then merge the 'develop' change to the 'master' branch and the changes will then subsequently make its way into a Boost release.
Regards Lakshay
Hello all
The polynomial class in boost.math does not support move sematics. I tried to implement them and found that they immediately lead to significant improvements in performance (benchmarking using google-benchmark).
Here are some benchmarking results: Run on (1 X 2394.45 MHz CPU ) Run on (1 X 2394.45 MHz CPU ) 2017-08-14 05:20:24 2017-08-14 05:21:31 ------------------------------------- ------------------------------------ Benchmark Time Iterations Benchmark Time Iterations ------------------------------------- ------------------------------------ [[ move construction ]] [[ copy construction ]] constr/1 2 ns 343554733 constr/1 31 ns 22353034 constr/8 2 ns 343350386 constr/8 52 ns 10000000 constr/64 3 ns 344426801 constr/64 114 ns 6089098 constr/512 2 ns 207412914 constr/512 122 ns 5712918 constr/4096 2 ns 338440949 constr/4096 2076 ns 467209 [[ unmodified from boost version ]] mult/1 92 ns 10456937 mult/1 111 ns 6406373 mult/8 202 ns 3296293 mult/8 126 ns 5272682 mult/64 1845 ns 377277 mult/64 2069 ns 375123 mult/512 111874 ns 7214 mult/512 159189 ns 4393 mult/4096 12454282 ns 56 mult/4096 12539754 ns 56 [[ addition using move semantics ]] [[ boost addition w/ const lvalue ref ]] add/1 63 ns 8470126 add/1 256 ns 2726596 add/8 64 ns 10869408 add/8 318 ns 2203414 add/64 247 ns 3707535 add/64 617 ns 1105630 add/512 1352 ns 512563 add/512 1330 ns 526513 add/4096 8146 ns 81454 add/4096 15984 ns 49450 [[ unmodified from boost version ]] inplaceAdd/1 8 ns 105715477 inplaceAdd/1 11 ns 64811794 inplaceAdd/8 16 ns 42666675 inplaceAdd/8 12 ns 44115677 inplaceAdd/64 33 ns 17414051 inplaceAdd/64 34 ns 20628203 inplaceAdd/512 181 ns 4483653 inplaceAdd/512 246 ns 4431873 inplaceAdd/4096 2737 ns 255393 inplaceAdd/4096 2766 ns 254879 [[ total heap usage ]] 308,365,390 bytes allocated 1,940,239,990 bytes allocated
Make a Github PR ( pull request ) against the library in Github. All PRs should be against the 'develop' branch of a library. If the PR is merged, it will update the 'develop' branch of the library. After appropriate testing the library's maintainer(s) will then merge the 'develop' change to the 'master' branch and the changes will then subsequently make its way into a Boost release.
I have submitted a patch to the boost.math github repo. It would be great if people can look into the PR and provide suggestions. https://github.com/boostorg/math/pull/79 Regards Lakshay
On 14/08/2017 07:19, Lakshay Garg via Boost wrote:
Make a Github PR ( pull request ) against the library in Github. All PRs should be against the 'develop' branch of a library. If the PR is merged, it will update the 'develop' branch of the library. After appropriate testing the library's maintainer(s) will then merge the 'develop' change to the 'master' branch and the changes will then subsequently make its way into a Boost release.
I have submitted a patch to the boost.math github repo. It would be great if people can look into the PR and provide suggestions. https://github.com/boostorg/math/pull/79
Many thanks for this! I'll look into it shortly. John. --- This email has been checked for viruses by AVG. http://www.avg.com
participants (4)
-
Edward Diener -
John Maddock -
Lakshay Garg -
Robert Ramey