[HOLD MERGE PLEASE] ThreadPool compile time template parameter; CMake + CI cross platform work #9

headupinclouds · 2017-01-07T18:57:50Z

@inkooboo , @SuperV1234 : Please see below and share any comments. Once this looks good, I can run some cross platform tests, we can merge, and I will look at adding basic CI tests (this can be run by hunter for each release by default if the package is accompanied with an example build test).

add TSettings template parameter to ThreadPool to specify task_size at compile time, and for forward API stability, updated internal code...
include updates from discussion in Make library easy to install, put everything into tp namespace, add optional boost::future support #5
add explicit project(thread-pool-cpp VERSION 1.1.0) : 1.0.0 used in previous hunter release
add cmake platform checks for thread_local and various fallbacks (FATAL_ERROR if no found)
use internal ATTRIBUTE_TLS macro for portability (see above): this allows thread-pool-cpp to be functional on many incomplete c++11 platforms w/ lambda initialization extensions
add THREAD_POOL_CPP_BUILD_{TEST,BENCHMARKS} to manage tests
minor compiler error/warning fixes
bump cmake_minimum_required(VERSION 3.3) for more modern cmake policy defaults
add cmake package config installation for clean find_package use (see example below)
remove boost dependency from example, per discussion with @inkooboo
add ThreadPool process() api back for packaged_task interface
manage tests with CTest (can deploy-on-success, etc)

Test project /Users/dhirvonen/devel/elucideye/drishti/src/3rdparty/thread-pool-cpp/_builds/xcode
    Start 1: FixedFunctionTest
1/2 Test #1: FixedFunctionTest ................   Passed    0.00 sec
    Start 2: ThreadPoolTest
2/2 Test #2: ThreadPoolTest ...................   Passed    0.01 sec

The current installation path and package config files look like this:

tree _install/xcode/  <== `i.e., /usr/local, etc`
_install/xcode/
├── include
│   └── thread_pool
│       ├── fixed_function.hpp
│       ├── mpsc_bounded_queue.hpp
│       ├── thread_pool.hpp
│       └── worker.hpp
└── lib
    └── cmake
        └── thread-pool-cpp
            ├── thread-pool-cppConfig.cmake
            ├── thread-pool-cppConfigVersion.cmake
            └── thread-pool-cppTargets.cmake

With this configuration, the project can be installed and included using:

find_package(thread-pool-cpp CONFIG REQUIRED)
target_link_libraries(my_exe thread-pool-cpp::thread-pool-cpp)

It will also support automatic package management through hunter if used in a project w/ a top level HunterGate() command with one additional line. The previous include path addition allows both direct/submodule use and post install (package) use with similar #include syntax:

if(MY_USE_LOCAL_THREAD_POOL_CPP)
  add_subdirectory(src/3rdparty/thread-pool-cpp)
  add_library(thread-pool-cpp::thread-pool-cpp ALIAS thread-pool-cpp)
else() # use hunter
  hunter_add_package(thread-pool-cpp)
  find_package(thread-pool-cpp CONFIG REQUIRED)
endif()

target_link_libraries(my_exe thread-pool-cpp::thread-pool-cpp)

…atform specific fallbacks

… add TASK_SIZE template parameter to Worker for flexibility

* add package config installation * use cmake submodules for `thread_local` platform checks * add options for building tests and benchmarks (default off for lib builds) Note: Version 1.0.0 is used in previous hunter-packages release; imcrementing to 1.1.0 based on API mods Use INTERFACE library build

error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]

http://stackoverflow.com/a/777359

* include(CTest) (optional) * replace add_custom_command() with add_test() Replace relative path syntax for COMMAND with cmake executable name to support default installation paths

error: 'test.hpp' file not found with <angled> include; use "quotes" instead

include_directories("${THREAD_POOL_CPP_INC_DIR}"): provided in top cmake

provide better default cmake policy settings (visibility, etc)

vittorioromeo

Minor nitpicks

vittorioromeo · 2017-01-07T19:41:32Z

benchmark/benchmark.cpp

@@ -88,50 +82,25 @@ struct RepostJob
    // Heavy heavy;

    ThreadPoolStd* thread_pool;
-#ifndef WITHOUT_ASIO
-    AsioThreadPool* asio_thread_pool;
-#endif

    volatile size_t counter;


All occurrences of size_t should be properly qualified as std::size_t

Agreed. I can replace size_t -> std::size_t in this PR.

vittorioromeo · 2017-01-07T19:42:52Z

include/thread_pool/thread_pool.hpp

 #include "./worker.hpp"

+#define _tp_unused(x) ((void)(x))


I'm not a fan of this macro. I think that (void) something; is idiomatic C++ and doesn't require a macro.

vittorioromeo · 2017-01-07T19:43:56Z

include/thread_pool/thread_pool.hpp

    }

    template <typename TSettings>
-    inline Worker& ThreadPool<TSettings>::getWorker()
+    inline typename ThreadPool<TSettings>::FixedWorker& ThreadPool<TSettings>::getWorker()


inline auto& would be nicer IMHO.

I realized this is a pure C++14 feature, so I will revert this and move the inline method inside the of the class to avoid the syntax issue:

FixedWorker & getWorker() { auto id = FixedWorker::getWorkerIdForCurrentThread(); if(id > m_workers.size()) { id = m_next_worker.fetch_add(1, std::memory_order_relaxed) % m_workers.size(); } return *m_workers[id]; }

Aren't we targeting C++14 though? If you feel like this should target C++11 (to reach more people) we should discuss about that and eventually change the README. I like targeting the newest standard because it drives people to update their toolchains and to learn the new features/idioms. Thoughts, @inkooboo?

I can understand that. If you both feel these changes aren't compatible with the project objectives, I can maintain a c++1y fork as needed.

vittorioromeo · 2017-01-07T19:49:06Z

LGTM apart from some minor nitpicks. I'm not sure about the value of ATTRIBUTE_TLS, as I was hoping to target C++14-compliant platforms only, where it seems unlikely that thread_local would be missing.

headupinclouds · 2017-01-07T21:04:57Z

LGTM apart from some minor nitpicks. I'm not sure about the value of ATTRIBUTE_TLS, as I was hoping to target C++14-compliant platforms only, where it seems unlikely that thread_local would be missing.

Thanks for the review.

This ATTRIBUTE_TLS is necessary for my builds, and the mods support a fairly wide range of compilers (various flavors of iOS, Android, OS X, MSVC). If these backwards compatibility changes can be merged, and don't get in the way, that would be quite useful. For the current thread-pool-cpp functionality, this seems reasonable, and may support a larger initial audience. If this gets in the way at some point, and some core functionality requiring C++14 is added that can't be made optional (I can imagine such cases), then I can work in a fork until I can upgrade. The README does clearly state this is a C++14 thread pool, so I have no good argument for the C++11-ish support, other than the fact that I need it 😄, and I think this is still a practical limitation for a fairly wide range of applications (even in 2017).

@inkooboo , @SuperV1234 : Let me know if you are both okay with this for now.

headupinclouds · 2017-01-07T21:48:00Z

Another small change...

target_compile_definitions() is preferred over add_definitions(). I can add an option for full c++14 support (default TRUE) and can encapsulated c++1y changes as needed (for now).

I think this should be maintainable:

option(THREAD_POOL_CPP_HAS_CPP14 "Has true C++14 support." ON)
if(THREAD_POOL_CPP_HAS_CPP14)
  target_compile_definitions(thread-pool-cpp INTERFACE -std=c++14 -Wall -Werror -O3)
else()
  target_compile_definitions(thread-pool-cpp INTERFACE -std=c++1y -Wall -Werror -O3)
endif()

* add option for c++14 support (w/ c++1y fallback) top top level CMakeLists.txt * add initial appveyor.yml and .travis.yml * add per toolchain HAS_CPP14 variable in CI files * formatting * support MSVC flags in top level CMakeLists.txt * avoid running tests for IOS and ANDROID platforms * use ATTRIBUTE_TLS definition * detect and link threads (as needed) portably ``` terminate called after throwing an instance of 'std::system_error' what(): Enable multithreading to use std::thread: Operation not permitted ```

headupinclouds · 2017-01-07T23:36:19Z

@inkooboo , @SuperV1234 : The above commit adds initial Travis and ASppveyor CI testing, and fixes a few issues I uncovered during testing. These are modified from the current CI tests in Hunter. We can add additional toolchains as needed, but this would at least provide some stability for new PR's. I ran this locally in my fork and all toolchains are currently passing.

@inkooboo : I believe you will need to enable hooks for these in your account. For now we can commit them at least.

I'm going to remove the "DO NOT MERGE" tag.

Here is a summary:

Here are the Linux + OS X platform host builds (TARGET: iOS, Android, MSVC, OS X and Linux)

      env: CONFIG=Release TOOLCHAIN=gcc-4-8-pic-hid-sections HAS_CPP14=OFF
    - os: linux
      env: CONFIG=Debug TOOLCHAIN=gcc-4-8-pic-hid-sections HAS_CPP14=OFF
    - os: linux
      env: CONFIG=Release TOOLCHAIN=android-ndk-r10e-api-19-armeabi-v7a-neon-hid-sections HAS_CPP14=OFF
    - os: linux
      env: CONFIG=Debug TOOLCHAIN=android-ndk-r10e-api-19-armeabi-v7a-neon-hid-sections HAS_CPP14=OFF
    - os: osx
      env: CONFIG=Release TOOLCHAIN=osx-10-11-hid-sections HAS_CPP14=OFF
    - os: osx
      env: CONFIG=Debug TOOLCHAIN=osx-10-11-hid-sections HAS_CPP14=OFF
    - os: osx
      env: CONFIG=Release TOOLCHAIN=ios-nocodesign-9-3-device-hid-sections HAS_CPP14=OFF
    - os: osx
      env: CONFIG=Debug TOOLCHAIN=ios-nocodesign-9-3-device-hid-sections HAS_CPP14=OFF
    - os: osx
      osx_image: xcode8.1
      env: CONFIG=Release TOOLCHAIN=osx-10-12-sanitize-address-hid-sections HAS_CPP14=OFF
    - os: osx
      osx_image: xcode8.1
      env: CONFIG=Debug TOOLCHAIN=osx-10-12-sanitize-address-hid-sections HAS_CPP14=OFF
    - os: osx
      env: CONFIG=Release TOOLCHAIN=android-ndk-r10e-api-19-armeabi-v7a-neon-hid-sections HAS_CPP14=OFF
    - os: osx
      env: CONFIG=Debug TOOLCHAIN=android-ndk-r10e-api-19-armeabi-v7a-neon-hid-sections HAS_CPP14=OFF

Here are the Appveyor MSCV builds:

    - TOOLCHAIN: "vs-14-2015-sdk-8-1"
      CONFIG: Release
      HAS_CPP14: OFF

    - TOOLCHAIN: "vs-14-2015-sdk-8-1"
      CONFIG: Debug
      HAS_CPP14: OFF      

    - TOOLCHAIN: "vs-14-2015-win64-sdk-8-1"
      CONFIG: Release
      HAS_CPP14: OFF      

    - TOOLCHAIN: "vs-14-2015-win64-sdk-8-1"
      CONFIG: Debug

https://travis-ci.org/headupinclouds/thread-pool-cpp/builds/189887074
https://ci.appveyor.com/project/headupinclouds/thread-pool-cpp/build/1.0.11

vittorioromeo · 2017-01-08T14:04:08Z

CMakeLists.txt

+
+option(THREAD_POOL_CPP_HAS_CPP14 "Has true C++14 support." ON)
+
+if(NOT MSVC)


Could this work to avoid the explicit branching? https://cmake.org/cmake/help/v3.1/prop_tgt/CXX_STANDARD.html

I'm not sure. I will take a look.

Could this work to avoid the explicit branching

It looks like this isn't supported for interface libraries: http://stackoverflow.com/q/29687331

"Indeed, it is a deliberate design choice not to support a INTERFACE_CXX_STANDARD target property. See also the CMAKE_CXX_STANDARD variable. "

But the target_compile_features() are.... That may not necessarily help too much.

I think something like this might work (and is a little cleaner):

if(THREAD_POOL_HAS_CPP14) set(CMAKE_CXX_STANDARD 14) else() set(CMAKE_CXX_STANDARD 11) endif() target_compile_features( thread-pool-cpp INTERFACE cxx_auto_type cxx_nullptr cxx_lambdas cxx_lambda_init_captures )

headupinclouds · 2017-01-08T21:52:22Z

I'm testing these in my current projects and am thinking about the compiler flags, in addition to the CXX_STANDARD suggestion.

I realized we should use target_compile_options(... INTERFACE ...) instead of target_compile_definitions() for compiler flags.

Also, since this is a header only library, and we are using INTERFACE options, all specified compiler options will be inherited by the parent project, and decisions like -Wall -Werror should probably not be set here in the top level CMakeLists.txt, but these strict settings could be applied to local tests and benchmarks. When they are inherited by top level projects, they can trigger build failures in 3rd party packages due to -Wunused-value -Wunused-variable and related strict checks.

inkooboo

I'm going to merge it offline, leaving all code improvements and part of build system improvements.
Also CI integration to be added but with lighter setup.

headupinclouds · 2017-05-16T20:20:47Z

Ok. This one is long overdue (from indecision as much as anything else). Sorry about that.

In my last testing I recall messing with:

CMAKE_CXX_STANDARD

had the some unintended consequences (which may vary based on the CMake version). For example, this ended up setting -std=gnu++11 for me in a case where I wanted -std=c++11. Currently, I feel it may be better not to set such language options in the project's CMakeLists.txt (which could lead to conflict), but to leave that for a top level toolchain. The project CMakeLists.txt may be better off detecting compatibility and reporting a message + FATAL_ERROR if the required language features aren't available.

Here is the latest one I've been using (w/ C++11 compatibility):

https://github.com/hunter-packages/thread-pool-cpp/commits/hunter

This is some logic I had added to test for thread_local availability (preference given to true thread_local if available):

https://github.com/hunter-packages/thread-pool-cpp/blob/hunter/CMakeLists.txt#L20-L28

Let me know if there is something I can help with. I will have some time this weekend.

headupinclouds · 2017-05-16T20:25:25Z

The full compiler logic is provided here (I've been testing this with a fairly broad range of toolchains in CI without issue):

# If true C++11 thread_local support exists, we will use it:
include(thread_pool_has_thread_local_storage)
thread_pool_has_thread_local_storage(THREAD_POOL_HAS_THREAD_LOCAL_STORAGE)
message("THREAD_POOL_HAS_THREAD_LOCAL_STORAGE: ${THREAD_POOL_HAS_THREAD_LOCAL_STORAGE}")

if(NOT THREAD_POOL_HAS_THREAD_LOCAL_STORAGE)
  # Else, we will check for backups
  include(thread_pool_has_thread_storage)
  thread_pool_has_thread_storage(THREAD_POOL_HAS_THREAD_STORAGE)
  message("THREAD_POOL_HAS_THREAD_STORAGE: ${THREAD_POOL_HAS_THREAD_STORAGE}")
  if(NOT THREAD_POOL_HAS_THREAD_STORAGE)
    message(FATAL_ERROR "Compiler does not support: thread_local, __thread, or declspec(thread)")
  endif()
endif()

Thanks again for a great lib!

headupinclouds added 15 commits January 7, 2017 11:48

add cmake package config template

bd7d94c

add cmake platform checks for: (1) truethread_local storage; (2) pl…

57dbc3c

…atform specific fallbacks

add thread_local attribute abstraction ATTRIBUTE_TLS for portability;…

edcfe98

… add TASK_SIZE template parameter to Worker for flexibility

remove TSettings template paramter; add TASK_SIZE template parameter

d32d21d

remove std::move() for temporary object

d28d5b6

error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]

add macro for default unused error in Xcode/clang

e019fac

http://stackoverflow.com/a/777359

add template syntax for test

3262699

manage tests w/ CTest if(THREAD_POOL_CPP_BUILD_TESTS)

00e7469

* include(CTest) (optional) * replace add_custom_command() with add_test() Replace relative path syntax for COMMAND with cmake executable name to support default installation paths

use "quotes" for local include

379432f

error: 'test.hpp' file not found with <angled> include; use "quotes" instead

remove pthread, remove include_directories()

03d47a9

include_directories("${THREAD_POOL_CPP_INC_DIR}"): provided in top cmake

bump cmake version to 3.3

1b969bc

provide better default cmake policy settings (visibility, etc)

remove boost and pthread dependencies in benchmark code

85d6312

Introduce TSettings template parameter wrapper for task_size

838a5b7

remove manual Findthread-pool-cpp.cmake

1c65c52

vittorioromeo requested changes Jan 7, 2017

View reviewed changes

size_t -> std::size_t; auto return type; remove/expand ((void)(x)) macro

4058471

headupinclouds changed the title ~~Discussion PR [PLEASE DO NOT MERGE]~~ ThreadPool compile time template parameter; CMake + CI cross platform work Jan 7, 2017

vittorioromeo approved these changes Jan 8, 2017

View reviewed changes

vittorioromeo reviewed Jan 8, 2017

View reviewed changes

vittorioromeo requested a review from inkooboo January 8, 2017 14:05

headupinclouds changed the title ~~ThreadPool compile time template parameter; CMake + CI cross platform work~~ [HOLD MERGE PLEASE] ThreadPool compile time template parameter; CMake + CI cross platform work Jan 8, 2017

inkooboo reviewed May 16, 2017

View reviewed changes

inkooboo closed this May 16, 2017

inkooboo added a commit that referenced this pull request May 16, 2017

refine code after merging of PR #9

2044ae0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD MERGE PLEASE] ThreadPool compile time template parameter; CMake + CI cross platform work #9

[HOLD MERGE PLEASE] ThreadPool compile time template parameter; CMake + CI cross platform work #9

headupinclouds commented Jan 7, 2017

vittorioromeo left a comment

vittorioromeo Jan 7, 2017

headupinclouds Jan 7, 2017

vittorioromeo Jan 7, 2017

headupinclouds Jan 7, 2017

vittorioromeo Jan 7, 2017

headupinclouds Jan 7, 2017

headupinclouds Jan 7, 2017

vittorioromeo Jan 7, 2017

headupinclouds Jan 8, 2017

vittorioromeo commented Jan 7, 2017

headupinclouds commented Jan 7, 2017

headupinclouds commented Jan 7, 2017

headupinclouds commented Jan 7, 2017 •

edited

Loading

vittorioromeo Jan 8, 2017

headupinclouds Jan 8, 2017

headupinclouds Jan 24, 2017

headupinclouds Jan 24, 2017

headupinclouds commented Jan 8, 2017

inkooboo left a comment

headupinclouds commented May 16, 2017 •

edited

Loading

headupinclouds commented May 16, 2017


		option(THREAD_POOL_CPP_HAS_CPP14 "Has true C++14 support." ON)

		if(NOT MSVC)

[HOLD MERGE PLEASE] ThreadPool compile time template parameter; CMake + CI cross platform work #9

[HOLD MERGE PLEASE] ThreadPool compile time template parameter; CMake + CI cross platform work #9

Conversation

headupinclouds commented Jan 7, 2017

vittorioromeo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vittorioromeo commented Jan 7, 2017

headupinclouds commented Jan 7, 2017

headupinclouds commented Jan 7, 2017

headupinclouds commented Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

headupinclouds commented Jan 8, 2017

inkooboo left a comment

Choose a reason for hiding this comment

headupinclouds commented May 16, 2017 • edited Loading

headupinclouds commented May 16, 2017

headupinclouds commented Jan 7, 2017 •

edited

Loading

headupinclouds commented May 16, 2017 •

edited

Loading