{"action":"created","issue":{"url":"https://api.github.com/repos/stan-dev/math/issues/1085","repository_url":"https://api.github.com/repos/stan-dev/math","labels_url":"https://api.github.com/repos/stan-dev/math/issues/1085/labels{/name}","comments_url":"https://api.github.com/repos/stan-dev/math/issues/1085/comments","events_url":"https://api.github.com/repos/stan-dev/math/issues/1085/events","html_url":"https://github.com/stan-dev/math/pull/1085","id":391484397,"node_id":"MDExOlB1bGxSZXF1ZXN0MjM4OTc3MDE0","number":1085,"title":"map_rect_concurrent with parallel STL backends","user":{"login":"wds15","id":12070126,"node_id":"MDQ6VXNlcjEyMDcwMTI2","avatar_url":"https://avatars0.githubusercontent.com/u/12070126?v=4","gravatar_id":"","url":"https://api.github.com/users/wds15","html_url":"https://github.com/wds15","followers_url":"https://api.github.com/users/wds15/followers","following_url":"https://api.github.com/users/wds15/following{/other_user}","gists_url":"https://api.github.com/users/wds15/gists{/gist_id}","starred_url":"https://api.github.com/users/wds15/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wds15/subscriptions","organizations_url":"https://api.github.com/users/wds15/orgs","repos_url":"https://api.github.com/users/wds15/repos","events_url":"https://api.github.com/users/wds15/events{/privacy}","received_events_url":"https://api.github.com/users/wds15/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2018-12-16T16:47:19Z","updated_at":"2018-12-17T11:45:23Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/stan-dev/math/pulls/1085","html_url":"https://github.com/stan-dev/math/pull/1085","diff_url":"https://github.com/stan-dev/math/pull/1085.diff","patch_url":"https://github.com/stan-dev/math/pull/1085.patch"},"body":"## Summary\r\n\r\nThe intent is to allow for C++17 parallel STL backends to be used with `map_rect_concurrent` which offer speed improvements over the current existing C++11 based version. The C++17 backend is only used optionally if the user does one of:\r\n\r\n- the user defines `STAN_PSTL_CPP17` during compilation => the standard C++17 backend is used\r\n- the user defines `STAN_PSTL_INTEL` during compilation (and in addition supplies compile arguments to build and link against Intel PSTL) => the Intel Parallel STL is used\r\n- if none of the above is done, then a C++11 implementation is used instead\r\n\r\nWhat has been done in this PR is to implement a parallel version of `std::for_each` using C++11 language only. This implementation is used as a fall-back if neither the C++17 one is available nor the Intel Parallel STL is available. The respective code has been placed in `stan/math/parallel/`.\r\n\r\nIn order to test this PR with the Intel Parallel STL, please do (on a Mac):\r\n\r\n- download the binary edition of the Intel Parallel STL, from [here](https://github.com/intel/parallelstl)\r\n- unpack it where you like\r\n- put into your `make/local` these lines:\r\n```\r\n// you need a g++ compiler which support openMP SIMD... this one is g++ v7.0 from MacPorts\r\nCXX=g++-mp-7\r\nCC=g++-mp-7\r\n\r\n// adjust the TBBROOT & PSTLROOT path here (both are packaged together from Intel)\r\nTBBROOT = $(HOME)/work/tbb2019_20181203oss\r\nPSTLROOT = $(HOME)/work/pstl2019_20181203oss\r\nCXXFLAGS += -std=c++17 -I$(TBBROOT)/include -I$(PSTLROOT)/include -fopenmp-simd\r\nLDFLAGS += -L$(TBBROOT)/lib -Wl,-rpath,$(TBBROOT)/lib -ltbb -ltbbmalloc\r\n```\r\n\r\nThings should work on Linux as well... even Windows may be in scope with this as I saw somewhere that condor provides packages for it (at least this came up in one of my google searches, but I haven't found the link again). However, the Intel Threading Building Blocks on which the the Parallel STL is based, does support windows (but the binary edition for windows only includes builds for msvc).\r\n\r\n## Tests\r\n\r\nThis PR also fixes the recently discovered bug found in `map_rect_concurrent` whenever number of threads and jobs don't line up. The test for this is found in\r\n\r\n`test/unit/math/prim/mat/functor/map_rect_concurrent_threads_test.cpp`\r\n\r\nSince this test is OK on this branch, I have prepared a second branch which has `develop` unchanged, but includes this tests (` feature/map_rect-fail-develop `). On this branch the exact same test is failing (while it is fine on this branch).\r\n\r\nOther tests:\r\n\r\n`test/unit/math/parallel/for_each_test.cpp` to test the new `for_each` facility\r\n`test/unit/math/parallel/get_num_threads_test.cpp` for tests to read off the number of threads\r\n\r\n## Side Effects\r\n\r\nThe C++17 standard does not specify at all how the number of threads are controlled. For the Intel Parallel STL we will need to add code to cmdstan which initializes the backend of the Intel Parallel STL accordingly (which is the Intel Threading Building Blocks).\r\n\r\nThe signature of the function `get_num_threads` was changed to now not anymore take as argument the number of jobs which we want to compute. The intent of the function is to read the number of threads off the environment variable `STAN_NUM_THREADS` and not more. This is done as we will have to add code to cmdstan which will control the number of threads for the Intel Parallel STL. In cmdstan it does not make sense to pass in the number of jobs - rather we just want to read out the `STAN_NUM_THREADS`.\r\n\r\n## Checklist\r\n\r\n- [X] Math issue #1084 \r\n\r\n- [X] Copyright holder: Sebastian Weber\r\n\r\n    The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:\r\n      - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)\r\n      - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)\r\n\r\n- [X] the basic tests are passing\r\n\r\n    - unit tests pass (to run, use: `./runTests.py test/unit`)\r\n    - header checks pass, (`make test-headers`)\r\n    - docs build, (`make doxygen`)\r\n    - code passes the built in [C++ standards](https://github.com/stan-dev/stan/wiki/Code-Quality) checks (`make cpplint`)\r\n\r\n- [X] the code is written in idiomatic C++ and changes are documented in the doxygen\r\n\r\n- [X] the new changes are tested"},"comment":{"url":"https://api.github.com/repos/stan-dev/math/issues/comments/447817197","html_url":"https://github.com/stan-dev/math/pull/1085#issuecomment-447817197","issue_url":"https://api.github.com/repos/stan-dev/math/issues/1085","id":447817197,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0NzgxNzE5Nw==","user":{"login":"wds15","id":12070126,"node_id":"MDQ6VXNlcjEyMDcwMTI2","avatar_url":"https://avatars0.githubusercontent.com/u/12070126?v=4","gravatar_id":"","url":"https://api.github.com/users/wds15","html_url":"https://github.com/wds15","followers_url":"https://api.github.com/users/wds15/followers","following_url":"https://api.github.com/users/wds15/following{/other_user}","gists_url":"https://api.github.com/users/wds15/gists{/gist_id}","starred_url":"https://api.github.com/users/wds15/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wds15/subscriptions","organizations_url":"https://api.github.com/users/wds15/orgs","repos_url":"https://api.github.com/users/wds15/repos","events_url":"https://api.github.com/users/wds15/events{/privacy}","received_events_url":"https://api.github.com/users/wds15/received_events","type":"User","site_admin":false},"created_at":"2018-12-17T11:45:23Z","updated_at":"2018-12-17T11:45:23Z","author_association":"CONTRIBUTOR","body":"Ok... I updated the issue for this with a lengthy description for those interactions.\r\n\r\nThe key thing to take note of that I do not change anything in terms of how we deal with threading here. It is just a better (optional) backend which will give use massive speed gains. To see this, just follow the issue which first describes where we are now and where we go with this change.\r\n\r\n@bob-carpenter : I am glad you like the design as it sounds. Wrt to your concern of clashes between the parallel STL and the standard C++17: From going over the sources from the Parallel STL I think they do take care of this situation. So they are cautious about this problem as it appears to me. Now... I wanted to test this by including `execution` from the standard library just before the respective PSTL includes... but I can't as I do not have a compiler available to me which supports `execution`. The current open-source compiler just don't provide this. I could (maybe) try with Intel's compiler which seems to have support for C++17 parallel features.\r\n\r\nNow, there is one more parallel STL implementation out there which is HPX. It got my attention in this [post](https://cukic.co/2018/03/29/cxx17-and-parallel-algorithms-in-stl/). However, when I installed it and tried to use it the Stan program just segfaulted such that I gave up, but in theory this should be a very nice alternative for non-Intel systems. We just need to find someone who can compile it and run it (it would take me a moment).\r\n"}}
{"actor":{"display_login":"wds15"}}