unittests/runtime: Migrate tests from pthreads to std::thread #14598

AndrewSB · 2018-02-13T19:59:00Z

This is required for Windows, which has std::threads, but no pthreads.
This is probably completely broken, working on running the tests locally right now.

If a member could ask @swift-ci to test, that would be much appreciated ❤️

compnerd · 2018-02-13T23:57:58Z

CC: @gparker42

compnerd · 2018-02-14T01:04:53Z

@swift-ci please smoke test

gparker42 · 2018-02-16T00:12:25Z

unittests/runtime/Metadata.cpp

+  unsigned numThreads;
+  unsigned *numThreadsReady;
+  std::mutex *sharedMutex;
+  std::condition_variable *start_condition;


Can these fields be references instead of pointers?

gparker42 · 2018-02-16T00:12:34Z

unittests/runtime/Metadata.cpp

@@ -249,7 +185,7 @@ TEST(Concurrent, ConcurrentList) {
  }

  // Check that the length of the list is correct.
-  EXPECT_EQ(ListLen, results.size() * numElem);
+  EXPECT_EQ(ListLen, 0 * numElem);


This looks wrong. Why are we verifying the contents of List and then asserting that List is empty?

AndrewSB · 2018-02-16T00:51:47Z

Should have said this: The patch is currently in progress, I know that's broken. I'll ping you when its ready to be reviewed.

AndrewSB · 2018-02-20T22:01:22Z

I think this is the patch I want. I'm testing it locally, but CI will probably go faster @compnerd / @gparker42 could you ask CI to test?

@gparker42 I might have changes to fix this, but this is more or less the change I was thinking. Took your references instead of pointer comment into account.

compnerd · 2018-02-21T21:40:12Z

unittests/runtime/Metadata.cpp

-std::vector<T> 
-RaceTest(std::function<T()> code)
-{
+std::vector<T> RaceTest(std::function<T()> code) {
  const unsigned threadCount = NumThreads;


Im not sure I understand the point of the threadCount variable. Why not just use NumThreads everywhere?

That was in the original implementation, I think it was because pthreads required an unsigned.
I'll remove it

compnerd · 2018-02-21T21:41:39Z

@swift-ci please test

swift-ci · 2018-02-21T21:42:11Z

Build failed
Swift Test OS X Platform
Git Sha - 668970310fc13b37adcaec0342b6e301601c3b0b

swift-ci · 2018-02-21T21:43:32Z

Build failed
Swift Test Linux Platform
Git Sha - 668970310fc13b37adcaec0342b6e301601c3b0b

AndrewSB · 2018-02-21T22:17:42Z

@compnerd I removed threadCount, it looks like the CI failed with an Infra issue. Could you ask it to try again?

gparker42 · 2018-02-21T22:54:19Z

unittests/runtime/Metadata.cpp

+
+  // Create all the threads
+  for (unsigned i = 0; i < NumThreads; i++) {
+    threads.emplace_back(std::thread(RaceThunk<T>, contexts[i]));


This combination of sized constructor and emplace_back() looks wrong. I think it creates a vector of 2*NumThreads elements, with the first half being default-initialized and the second half being the desired contents.

I'm sorry! I'm still learning cpp, my bad 😬
How should I update the vector initializer to not fill in default values? I looked at http://en.cppreference.com/w/cpp/container/vector/vector but it doesn't seem easy to have the Allocator not insert values. Should I switch from emplace_backing to assigning to threads[i]?

For this code simply assigning to threads[i] should be fine. Or create an empty vector and push_back or emplace_back onto it, like the old pthreads code did. Both of these might have performance downsides, but we don't care in this code.

This stackoverflow answer has a good overview of the combinations that work and their performance effects.
https://stackoverflow.com/a/32200517/881456

AndrewSB · 2018-02-23T21:28:36Z

@gparker42 that was an interesting read. Thank you 😄
Switched to using reserving and emplacing, instead of initializing with with default-initialized

compnerd · 2018-02-28T20:57:47Z

@swift-ci please test

swift-ci · 2018-02-28T20:59:15Z

Build failed
Swift Test OS X Platform
Git Sha - af0ec973d924343742d96a822df8d7cc09b8a1bd

swift-ci · 2018-02-28T22:34:31Z

Build failed
Swift Test Linux Platform
Git Sha - 7a31e1ad3582dc4d98bb1765b3da1360a77f3170

shahmishal · 2018-03-01T01:31:23Z

Xcode updated

@swift-ci please test

swift-ci · 2018-03-01T02:40:39Z

Build failed
Swift Test Linux Platform
Git Sha - 7a31e1ad3582dc4d98bb1765b3da1360a77f3170

swift-ci · 2018-03-01T02:59:37Z

Build failed
Swift Test OS X Platform
Git Sha - 7a31e1ad3582dc4d98bb1765b3da1360a77f3170

AndrewSB · 2018-03-01T19:07:44Z

This looks like a legitimate failure, not just an infra issue. Investigating today

compnerd · 2018-03-07T17:43:08Z

unittests/runtime/Metadata.cpp

+  std::vector<std::thread> threads;
+  threads.reserve(NumThreads);
+  for (unsigned i = 0; i < NumThreads; i++)
+    threads.emplace_back(std::bind(RaceThunk<T>, contexts[i]));


Needs a std::cref on the contexts[i].

Oh right, thank you 🙏

compnerd · 2018-03-07T18:54:12Z

@swift-ci please test

swift-ci · 2018-03-07T18:54:36Z

Build failed
Swift Test OS X Platform
Git Sha - 7a31e1ad3582dc4d98bb1765b3da1360a77f3170

swift-ci · 2018-03-07T18:54:37Z

Build failed
Swift Test Linux Platform
Git Sha - 7a31e1ad3582dc4d98bb1765b3da1360a77f3170

compnerd · 2018-03-19T20:58:13Z

@swift-ci please test

swift-ci · 2018-03-19T21:43:19Z

Build failed
Swift Test Linux Platform
Git Sha - 31ebf68f32736edcc83825273f792ad81e1cd797

swift-ci · 2018-03-19T22:10:12Z

Build failed
Swift Test OS X Platform
Git Sha - 31ebf68f32736edcc83825273f792ad81e1cd797

swift-ci · 2018-03-20T21:30:49Z

Build failed
Swift Test OS X Platform
Git Sha - 31ebf68f32736edcc83825273f792ad81e1cd797

swift-ci · 2018-03-20T21:32:08Z

Build failed
Swift Test Linux Platform
Git Sha - 31ebf68f32736edcc83825273f792ad81e1cd797

AndrewSB · 2018-03-21T00:03:05Z

@gparker42 can you take a look last this? After your review it should be good to merge

AndrewSB force-pushed the pthreads= branch 2 times, most recently from 4801d7f to 6689703 Compare February 13, 2018 23:51

AndrewSB force-pushed the pthreads= branch 3 times, most recently from c21d7ed to 7c1f63d Compare February 14, 2018 00:58

AndrewSB force-pushed the pthreads= branch 2 times, most recently from afd0e45 to 70603fd Compare February 15, 2018 19:38

gparker42 reviewed Feb 16, 2018

View reviewed changes

AndrewSB force-pushed the pthreads= branch 2 times, most recently from ab1a0d7 to 3b948bf Compare February 20, 2018 21:48

AndrewSB force-pushed the pthreads= branch 4 times, most recently from 3f3077e to 9dacfa2 Compare February 20, 2018 23:14

AndrewSB changed the title ~~[WIP] unittests/runtime: Migrate tests from pthreads to std::thread~~ unittests/runtime: Migrate tests from pthreads to std::thread Feb 20, 2018

AndrewSB force-pushed the pthreads= branch 2 times, most recently from b7a846a to af0ec97 Compare February 21, 2018 00:09

compnerd reviewed Feb 21, 2018

View reviewed changes

AndrewSB force-pushed the pthreads= branch from af0ec97 to 6dbe319 Compare February 21, 2018 22:12

AndrewSB force-pushed the pthreads= branch from 6dbe319 to 5401cfc Compare February 21, 2018 22:18

gparker42 reviewed Feb 21, 2018

View reviewed changes

AndrewSB force-pushed the pthreads= branch from 5401cfc to 7a31e1a Compare February 23, 2018 21:24

AndrewSB force-pushed the pthreads= branch 2 times, most recently from 95ab994 to 99056d4 Compare March 7, 2018 17:35

compnerd requested changes Mar 7, 2018

View reviewed changes

AndrewSB force-pushed the pthreads= branch from 99056d4 to 5200830 Compare March 7, 2018 17:58

AndrewSB force-pushed the pthreads= branch from 5200830 to 31ebf68 Compare March 7, 2018 22:57

unittests/runtime: Migrate race tests from pthreads to std::thread

4308104

AndrewSB force-pushed the pthreads= branch from 31ebf68 to 4308104 Compare March 20, 2018 21:30

compnerd approved these changes Mar 20, 2018

View reviewed changes

compnerd merged commit 8695224 into swiftlang:master Mar 29, 2018

AndrewSB deleted the pthreads=😥 branch March 31, 2018 00:13

unittests/runtime: Migrate tests from pthreads to std::thread #14598

unittests/runtime: Migrate tests from pthreads to std::thread #14598

Uh oh!

Conversation

AndrewSB commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compnerd commented Feb 13, 2018

Uh oh!

compnerd commented Feb 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewSB commented Feb 16, 2018

Uh oh!

AndrewSB commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Feb 21, 2018

Uh oh!

swift-ci commented Feb 21, 2018

Uh oh!

swift-ci commented Feb 21, 2018

Uh oh!

AndrewSB commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewSB commented Feb 23, 2018

Uh oh!

compnerd commented Feb 28, 2018

Uh oh!

swift-ci commented Feb 28, 2018

Uh oh!

swift-ci commented Feb 28, 2018

Uh oh!

shahmishal commented Mar 1, 2018

Uh oh!

swift-ci commented Mar 1, 2018

Uh oh!

swift-ci commented Mar 1, 2018

Uh oh!

AndrewSB commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Mar 7, 2018

Uh oh!

swift-ci commented Mar 7, 2018

Uh oh!

swift-ci commented Mar 7, 2018

Uh oh!

compnerd commented Mar 19, 2018

Uh oh!

swift-ci commented Mar 19, 2018

Uh oh!

swift-ci commented Mar 19, 2018

Uh oh!

swift-ci commented Mar 20, 2018

Uh oh!

swift-ci commented Mar 20, 2018

Uh oh!

AndrewSB commented Mar 21, 2018

Uh oh!

Uh oh!

AndrewSB commented Feb 13, 2018 •

edited

Loading

AndrewSB commented Feb 21, 2018 •

edited

Loading