Skip to content
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.

Sign up for GitHub

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

Fix io_service_fixture_with_threads: create given threads count #75

Merged
merged 6 commits into from
Apr 5, 2018
Merged

Fix io_service_fixture_with_threads: create given threads count #75

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c5fd167
Fix io_service_fixture_with_threads: create given threads count
grishavanika Mar 25, 2018
8467a21
Work-around for Optimized x86 build that avoids `out of range array a…
grishavanika Mar 25, 2018
a82c968
Revert work-around for Optimized x86 build
grishavanika Apr 5, 2018
88dfb36
Merge branch 'master' of https://github.com/grishavanika/cppcoro
grishavanika Apr 5, 2018
658f32e
Revert "Work-around for Optimized x86 build that avoids `out of range…
grishavanika Apr 5, 2018
6542e03
Merge branch 'master' into master
lewissbaker Apr 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions test/single_consumer_async_auto_reset_event_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ TEST_CASE("single waiter")

TEST_CASE_FIXTURE(io_service_fixture_with_threads<3>, "multi-threaded")
{
#if (CPPCORO_CPU_X86)
#if (1)
const int iterations = 10'000;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know how/why this change works around the issue with x86 optimised builds?

I can only assume that because it's now a variable that is captured by reference that this makes the code below slightly more complicated and less able to be optimised.

I have seen several different tests all fail under x86 optimised over various recent versions of msvc.
This mostly just seems like bad codegen for coroutines under x86, although I haven't looked into some of the more recent failures to identify the root cause yet. I've been largely just ignoring x86 optimised failures lately and am hoping that the codegen bugs will be fixed in the 15.7 update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I have no idea. I had no time to take a look into assembler output yet.

Also, bug disappear if iterations count will be less (lets say 6'000). In this case integer literal can be used.

recent versions of msvc

I'm using 15.6.4 and this fix helps me, but, as I said, appveyor build still fails. Since there are no linker errors (that I have for 15.6.4), appveyor has older MSVC version (15.6.3 ?). So (if this is codegen bug) something was changed in latest version.

Also, because less number of iteration helps, I thought that this is something relative to thread's stack size (?), but increasing stack size to 10 MB from default 1 MB does not help.

I can try to change iterations count to 2'000 and build again on appveyor if you think changes should be accepted.

Thank you for looking into the change :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have confirmed that this change also makes the test pass on MSVC 15.7 (preview 2).

That is very weird about the changing iteration causing issues. Perhaps reducing the iteration count reduces the contention on shared data-structures and thus is avoiding some race-conditions?

The AppVeyor tests are still running with an older version of msvc 15.6.2 but are failing on a different test (writing a file). This test only seems to fail on the AppVeyor CI machines. I haven't been able to reproduce it on a dev machine. My current working theory for that failing test is also bad x86 codegen but I don't have any evidence other than "other things are broken under x86 optimised due to compiler bugs so it's likely this is too".

I'll put through a change to disable the failing file test under x86 optimised for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lewissbaker, sorry for late response. Just to be sure: can I help you with this somehow ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be able to amend the PR to remove the change to test/single_consumer_async_auto_reset_event_tests.cpp ?

I'll put the PR through with just the fix to the io_service_fixture constructor.

I've put through a change on master that ignores test failures on x86 for now and will leave it as a known issue. There's no point in working around the compiler bug in the test - users of the library are just as likely to run into the bug in their own code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes reverted (not sure that was done right, I have few uneeded commits in the forked repository, but you have 1 file in this the pull request, finally)

#else
// Will cause "RangeChecks instrumentation code detected an out of range array access"
// exception for Optimized, x86 build
# define iterations 10'000
#endif
#else
const int iterations = 10'000;
#endif

cppcoro::sync_wait([&]() -> cppcoro::task<>
{
cppcoro::single_consumer_async_auto_reset_event valueChangedEvent;
Expand All @@ -65,7 +77,7 @@ TEST_CASE_FIXTURE(io_service_fixture_with_threads<3>, "multi-threaded")

auto consumer = [&]() -> cppcoro::task<int>
{
while (value.load(std::memory_order_relaxed) < 10'000)
while (value.load(std::memory_order_relaxed) < iterations)
{
co_await valueChangedEvent;
}
Expand All @@ -89,7 +101,7 @@ TEST_CASE_FIXTURE(io_service_fixture_with_threads<3>, "multi-threaded")
value.store(0, std::memory_order_relaxed);

// Really just checking that we don't deadlock here due to a missed wake-up.
(void)co_await cppcoro::when_all(consumer(), modifier(5'000), modifier(5'000));
(void)co_await cppcoro::when_all(consumer(), modifier(iterations / 2), modifier(iterations / 2));
}
}());
}
Expand Down