Mac platform support

[yfinkelstein]

I tried to compile this library on Mac using the latest clang/c++ and cmake. After excluding file io and win32 stuff similarly to how it's done for Linux, the only module that does not compile is lightweight_manual_reset_event.cpp for the reason that futex does not exist on Mac. A natural solve for this is to use std condition variable and mutex. It's not particularly "lightweight" but at least there won't be any compatibility issues which may be useful for other platforms.

I can contribute this code if this is seen useful. But the other part of this puzzle is making cake build work on Mac. Btw, any particular reason cake is used for this project instead of cmake or say scons which is similar ideologically as far as I can tell?

[yfinkelstein]

And btw, adaptive (http://lkml.iu.edu/hypermail/linux/kernel/0103.1/0030.html) type of mutex in pthread on Linux uses Futex under the covers. So, direct linking with futex API is probably not the best solution.

[lewissbaker]

I think a fallback for lightweight_manual_reset_event that uses std::mutex and std::condition_variable would be useful to have for those platforms that don't have a futex-equivalent. If you want to take a crack at it, I'm open to pull-requests ;)

With regards to using an adaptive mutex on Linux, it seems like a lot of overhead to get at the same underlying feature. A futex is pretty much exactly what we want for this class. We'd effectively have to reconstruct a futex out of a mutex and condition-variable which seems unnecessarily complicated.

Are you suggesting that by using an adaptive mutex that the code would become more portable?

[lewissbaker]

With regards to using the cake build system:

The short story: It is the build system I'm most familiar with, having written much of it myself, so it was just easier to get up and running than trying to learn CMake.

The long story: We originally wrote cake out of frustration with the lack of scalability of Scons. Incremental builds under SCons were taking >100s on a large code-base - under cake incremental builds were <4s). SCons was too difficult to extend to do some of the more complicated things I wanted to do. Although I did appreciate the benefits of having a first-class language like Python to write build-scripts in and extend the build system.

I also felt that CMake didn't support my needs for cross-compiling at the time. I was working in game-development where we often needed to be able to target multiple platforms in a single build and have dependencies between targets built for different platforms (eg. build a tool for PC that then is used to compile some asset for Xbox). Maybe CMake has improved since I last looked at it or maybe I just didn't invest enough effort in learning how to do these things in CMake...

We also tried out Waf, Jam, Boost.Build and a couple of others before building Cake, but none of them quite seemed to meet our needs.

Cake has good support for cross-compilation, build variants and dependencies between targets of different build-variants. Cake also has a good system for specifying dependencies and requirements of c++ libraries in large c++ projects (I use a fork of Cake at work with a 2M+ line code-base and many hundreds of libraries, DLLs and executables).

A major downside of Cake is the lack of documentation. There used to be some documentation for it on sourceforge but that was lost when sourceforge discontinued support for Trac. Adding documentation for Cake was this year's project until I got distracted on cppcoro ;)

As far as I know, Cake should be able to work on a Mac. It did work at one point, although I don't have access to a Mac to test the latest version and it may well have broken with more recent work.

Adding Mac support for cppcoro should hopefully just be a matter of adding an extra section to the config.cake that initialises the Mac build-variants, including finding locations of compilers, libraries, etc. Let me know if you need any help.

[lewissbaker]

I'm open to suggestions if there are things I can do for the build system to aid the ability to use cppcoro in other projects.

[yfinkelstein]

I'have a generic brain-dead implementation ready. It's sort of hard to get wrong given c++ native support.

Header: add section for CPPCORO_OS_GENERIC

///////////////////////////////////////////////////////////////////////////////
// Copyright (c) Lewis Baker
// Licenced under MIT license. See LICENSE.txt for details.
///////////////////////////////////////////////////////////////////////////////
#ifndef CPPCORO_DETAIL_LIGHTWEIGHT_MANUAL_RESET_EVENT_HPP_INCLUDED
#define CPPCORO_DETAIL_LIGHTWEIGHT_MANUAL_RESET_EVENT_HPP_INCLUDED

#include <cppcoro/config.hpp>

#if CPPCORO_OS_LINUX || (CPPCORO_OS_WINNT >= 0x0602)
# include <atomic>
# include <cstdint>
#elif CPPCORO_OS_WINNT
# include <cppcoro/detail/win32.hpp>
#elif CPPCORO_OS_GENERIC
# include <mutex>
# include <condition_variable>
#endif

namespace cppcoro
{
	namespace detail
	{
		class lightweight_manual_reset_event
		{
		public:

			lightweight_manual_reset_event(bool initiallySet = false);

			~lightweight_manual_reset_event();

			void set() noexcept;

			void reset() noexcept;

			void wait() noexcept;

		private:

#if CPPCORO_OS_LINUX
			std::atomic<int> m_value;
#elif CPPCORO_OS_WINNT >= 0x0602
			// Windows 8 or newer we can use WaitOnAddress()
			std::atomic<std::uint8_t> m_value;
#elif CPPCORO_OS_WINNT
			// Before Windows 8 we need to use a WIN32 manual reset event.
			cppcoro::detail::win32::handle_t m_eventHandle;
#elif CPPCORO_OS_GENERIC
            // On a generic platform, we resort to native C++ which is not as optimal for this case as futex on Linux
			std::mutex m_mutex;
			std::condition_variable m_cv;
			bool m_is_set;
#endif
		};
	}
}

#endif

A new section is appended to lightweight_manual_reset_event.cpp at the bottom:

#elif CPPCORO_OS_GENERIC

cppcoro::detail::lightweight_manual_reset_event::lightweight_manual_reset_event(bool initiallySet)
		: m_is_set(initiallySet)
{
}

cppcoro::detail::lightweight_manual_reset_event::~lightweight_manual_reset_event()
{
}

void cppcoro::detail::lightweight_manual_reset_event::set() noexcept
{
	std::unique_lock<std::mutex> lk(m_mutex);
	m_is_set = true;
	lk.unlock();
	m_cv.notify_all();
}

void cppcoro::detail::lightweight_manual_reset_event::reset() noexcept
{
	std::unique_lock<std::mutex> lk(m_mutex);
	m_is_set = false;
}

void cppcoro::detail::lightweight_manual_reset_event::wait() noexcept
{
	std::unique_lock<std::mutex> lk(m_mutex);
        if(!m_is_set) 
	  m_cv.wait(lk, [this]{return m_is_set;});
}


#endif

[yfinkelstein]

Re build, I obviously need something to build the above on Mac. So I put together a basic cmakelist file. I'm also new to cmake but it feels a good match for this type of project where support for a generic platform is a must-have. I now need to to invoke the tests as a part of this build. After that, I will be able to link this library with the application I'm eyeing for this use case.

[yfinkelstein]

I will fork and submit a PR once I have the tests passing.

[yfinkelstein]

As an afterthought, this case is a good candidate for a reader/writer lock. There is a single writer invoking set flag and multiple readers checking and waiting for the set flag that can do it without mutual exclusion. Agree? The only potential consideration is whether C++ shared_mutex does not have extra overhead compared to regular mutex as single waiter is probably the most likely case and deserves to be optimized for. Facebook has put quite a bit of effort into this: https://github.com/facebook/folly/blob/master/folly/SharedMutex.h

[tlanc007]

I would be willing to help out with CMake support, too. Although I wouldn't call myself an expert.

Also, I have hacked the config.cake to all me to build on the Mac by adding a elif isDarwin() block.

[yfinkelstein]

I have checked in initial version along with cmake file here: https://github.com/yfinkelstein/cppcoro

still need to invoke tests build from cmake.

[tlanc007]

Cool. I'll give it try under MacOs, linux, Windows and see how it goes. As I recall MSVS doesn't support std=c++1z. For now, I think you need /std:c++latest.

So you may want a conditional like so:

if(NOT DEFINED MSVC)
    add_definitions(-std=c++1z)
else()
    add_definitions(/std:c++latest)

In case you didn't already know, tests are added via add_test() and you may need to proceed it with an add_executable().

[yfinkelstein]

My goal was very minimalistic: have a way to build this project on mac with clang. If this cmake file can be extended to support linux and windows - this is great, but it's an extension of the original goal :) Plus, cake already does it on windows and linux anyway.

[yfinkelstein]

Just added cmake script for tests. Wow, cmake exceeded my expectations. It's pretty easy to put it together. The good news is that the tests seem to be passing in debug build:

$ ./test/cppcoro_tests
[doctest] doctest version is "1.2.1"
[doctest] run with "--help" for options
===============================================================================
/Users/yfinkelstein/work/cppcoro/test/cancellation_token_tests.cpp(260)
TEST SUITE: cancellation_token tests
TEST CASE:  cancellation registration single-threaded performance

/Users/yfinkelstein/work/cppcoro/test/cancellation_token_tests.cpp(333) MESSAGE!
  Individual took 21447us (214.47 ns/item)

/Users/yfinkelstein/work/cppcoro/test/cancellation_token_tests.cpp(333) MESSAGE!
  Batch10 took 212628us (212.628 ns/item)

/Users/yfinkelstein/work/cppcoro/test/cancellation_token_tests.cpp(333) MESSAGE!
  Batch50 took 1071389us (214.278 ns/item)

===============================================================================
/Users/yfinkelstein/work/cppcoro/test/recursive_generator_tests.cpp(343)
TEST SUITE: recursive_generator
TEST CASE:  recursive iteration performance

/Users/yfinkelstein/work/cppcoro/test/recursive_generator_tests.cpp(360) MESSAGE!
  Range iteration of 100000elements took 17216us

===============================================================================
[doctest] test cases:     91 |     91 passed |      0 failed |      0 skipped
[doctest] assertions: 1000348 | 1000348 passed |      0 failed |
[doctest] Status: SUCCESS!

[yfinkelstein]

Event better news: it works with release build too.

yfinkelstein@LM-SJL-11000508:~/work/cppcoro/Release
$ ./test/cppcoro_tests 
[doctest] doctest version is "1.2.1"
[doctest] run with "--help" for options
===============================================================================
/Users/yfinkelstein/work/cppcoro/test/cancellation_token_tests.cpp(260)
TEST SUITE: cancellation_token tests
TEST CASE:  cancellation registration single-threaded performance

/Users/yfinkelstein/work/cppcoro/test/cancellation_token_tests.cpp(333) MESSAGE!
  Individual took 19712us (197.12 ns/item)

/Users/yfinkelstein/work/cppcoro/test/cancellation_token_tests.cpp(333) MESSAGE!
  Batch10 took 210890us (210.89 ns/item)

/Users/yfinkelstein/work/cppcoro/test/cancellation_token_tests.cpp(333) MESSAGE!
  Batch50 took 1081916us (216.383 ns/item)

===============================================================================
/Users/yfinkelstein/work/cppcoro/test/recursive_generator_tests.cpp(343)
TEST SUITE: recursive_generator
TEST CASE:  recursive iteration performance

/Users/yfinkelstein/work/cppcoro/test/recursive_generator_tests.cpp(360) MESSAGE!
  Range iteration of 100000elements took 16725us

===============================================================================
[doctest] test cases:     91 |     91 passed |      0 failed |      0 skipped
[doctest] assertions: 1000348 | 1000348 passed |      0 failed |
[doctest] Status: SUCCESS!

[yfinkelstein]

@lewissbaker please review my fork as I'm ready to submit a PR. Btw, do you have .clang-format or something like this? If more than 1 person is going to contribute to this project you might want to assert a formatting style. With clang-format it is trivial as most c++ editors can enforce this formatting on file save. I pushed .clang-format and .editorconfig that I used somewhere else but you might want to edit/overwrite with whatever style you like.

[tlanc007]

@yfinkelstein in CMakeLists.txt you set and make use of LLVM_HOME:

#override via command line, for instance: cmake -DLLVM_HOME=/Library/Developer/CommandLineTools/usr
set(LLVM_HOME /usr/local/opt/llvm/Toolchains/LLVM6.0.0svn.xctoolchain/usr CACHE PATH "path to LLVM distribution")
set(CMAKE_CXX_COMPILER ${LLVM_HOME}/bin/clang++)
set(CMAKE_C_COMPILER ${LLVM_HOME}/bin/clang)

Might I suggest deleting those lines and instead rely on CC/CXX from the command line with say something like:

CC=clang CXX=clang++ cmake ..  

This is a more common convention.

Of course, the cmake example assumes that the shell path points to the desired version of clang. Otherwise one would have to give the full path to the CC/CXX.

[yfinkelstein]

yes, in fact I did it this way first. But exactly because of what you said - the ugly long path to cc and cxx has to be specified twice, - I put that plug in. I honestly thought this is only for Mac, and today you can't build this project without trunk llvm build of the clang. So, everbody will have this long and ugly path to type twice. We can revert if cmake effort takes off.

[yfinkelstein]

@tlanc007

In case you didn't already know, tests are added via add_test() and you may need to proceed it with an add_executable().

I did not know :) Does it work with any framework, with doctest specifically? Perhaps you can improve and submit a PR directly either to me or to the origin. Or even paste here.

[lewissbaker]

As an afterthought, this case is a good candidate for a reader/writer lock. There is a single writer invoking set flag and multiple readers checking and waiting for the set flag that can do it without mutual exclusion. Agree?

@yfinkelstein The manual-reset event concept can theoretically support concurrent calls to set() as well as concurrent calls to wait(). However, the use-case we're currently using it for is in sync_wait() where there is only a single thread calling set() and a single thread calling wait(). So I'm not sure the added overhead/complexity of a reader/writer lock is needed here.

[yfinkelstein]

ok. I've implemented both options just in case. They can be switched with a single define statement which should be set to 0 for now.

[lewissbaker]

@yfinkelstein I've added some comments to your fork.

I've added a .editorconfig and .clang-format config file to the upstream master branch in bdd7ccc88d8fce7c4543d3c153df430b6dc789a9.

Do you mind if I just manually apply some of your changes to lightweight_manual_reset_event? You can then rebase your branch off that and submit a PR for the CMake files.

[lewissbaker]

I have hacked the config.cake to all me to build on the Mac by adding a elif isDarwin() block.

@tlanc007 Do you mind sharing your changes to the config.cake? I'd also like to work towards getting support for building on Mac using cake if possible.

[lewissbaker]

I've pushed the changes to lightweight_manual_reset_event in 396d0271d1a335546b3dd68b036a66b5576c8165

[lunastorm]

@lewissbaker I've created a PR #48 which includes the config.cake I used for your reference.