Skip to content

[Build][Runtime] Add a new threading library. #42447

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.

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

Merged
merged 37 commits into from
Jun 1, 2022

Conversation

al45tair
Copy link
Contributor

@al45tair al45tair commented Apr 19, 2022

A fair bit of this is really shuffling things around — that's largely true for the swift::Mutex, which was an existing thing. swift::Thread is really a placeholder for when we need more support — right now, we don't need any of the functionality that e.g. std::thread has, but we do need some functionality it doesn't have. It's also desirable to be able to choose which threading package to use, especially for embedded use or for testing purposes, and if we make everything defer to the C++ library we'll only be able to use whatever the C++ library is using.

The main new things are:

  • You can ask to use a specific threading package.
  • The threading code is in its own library, so no more #includes of implementation files in order to bring them in to the standard library or concurrency code.
  • swift::Lazy now uses swift::once rather than having its own implementation that uses a macro.
  • New swift::once implementations for Linux and Windows that should be somewhat faster on those platforms.
  • Improved error handling, so the threading code raising errors can hook into the error handling used in the places it's being used.
  • Adjusted the TLS support code slightly, to avoid trying to put TLS keys in template arguments, as that isn't guaranteed to work.

@al45tair al45tair force-pushed the eng/PR-90776105 branch 4 times, most recently from f0a627b to 0e9c955 Compare April 26, 2022 15:05
@al45tair al45tair force-pushed the eng/PR-90776105 branch 9 times, most recently from d6495f8 to 56c7210 Compare May 4, 2022 10:09
@al45tair al45tair force-pushed the eng/PR-90776105 branch 4 times, most recently from c09e832 to 054b957 Compare May 6, 2022 12:18
@al45tair
Copy link
Contributor Author

al45tair commented May 6, 2022

@swift-ci Please test

@al45tair al45tair marked this pull request as ready for review May 6, 2022 15:19
@grynspan
Copy link
Contributor

grynspan commented May 6, 2022

I haven't taken a deep dive yet, but I'm wondering about the necessity of the platform-specific implementations. Since Linux and Windows both have C++'s std::thread (and std::mutex and and and…), would it make sense to just… use that for every non-Apple platform? I can buy that Darwin may need to be specialized.

Given Swift's lack of official support for platforms other than Darwin and Linux, it seems to me like we should lean on the STL as much as possible rather than rolling our own equivalent implementations.

@al45tair
Copy link
Contributor Author

al45tair commented May 6, 2022

I've updated the comment at the top with some notes on the changes that are hopefully useful for reviewers.

After rebasing, a few things broke.  Fix them.

rdar://90776105
…d special.

While most systems aren't going to have their stack bottom at zero,
using llvm::Optional<> here is cleaner.

rdar://90776105
@al45tair
Copy link
Contributor Author

@swift-ci Please test

@gottesmm
Copy link
Contributor

@al45tair can you make sure the standalone minimal test still passes? I think it uses the single threaded flag.

@al45tair
Copy link
Contributor Author

preset=stdlib_S_standalone_minimal_macho_x86_64,build,test
@swift-ci please test with toolchain and preset

@al45tair
Copy link
Contributor Author

al45tair commented May 25, 2022

Hmmm. "Can't find CMake" seems like not my fault. I'll give it another go, in case it's just a host issue, then go look what the problem might be.

@al45tair
Copy link
Contributor Author

preset=stdlib_S_standalone_minimal_macho_x86_64,build,test
@swift-ci please test with toolchain and preset

@kubamracek
Copy link
Contributor

LGTM (sorry, it took a while to go over the large diff).

Given that the PR testing for "stdlib minimal" seems to have some infrastructure issues, I am going to run the "preset testing" locally to ensure we're not breaking it, I'll report back.

When we're building static libraries, we don't need to include the
threading objects in both Concurrency and Core, and we also don't need
two copies of the threading library's fatal error handler.

rdar://90776105
@al45tair
Copy link
Contributor Author

@swift-ci Please test

@al45tair
Copy link
Contributor Author

preset=stdlib_S_standalone_minimal_macho_x86_64,build,test
@swift-ci please test with toolchain and preset

@al45tair
Copy link
Contributor Author

This passed local testing of the minimal preset just now, FWIW:

Testing Time: 456.15s
  Excluded         : 14471
  Unsupported      :   230
  Passed           :   289
  Expectedly Failed:     1

49 warning(s) in tests
-- check-swift-validation-only_executable-freestanding-x86_64 finished --
--- Finished tests for swift ---

@al45tair
Copy link
Contributor Author

Looks like preset testing in CI is still broken.

@gottesmm
Copy link
Contributor

@shahmishal can you prioritize fixing preset testing in CI? We need it to work to test PRs like this?

@gottesmm
Copy link
Contributor

If the preset testing worked locally... I am happy.

@al45tair
Copy link
Contributor Author

al45tair commented Jun 1, 2022

@swift-ci Please test Linux platform

@al45tair
Copy link
Contributor Author

al45tair commented Jun 1, 2022

@swift-ci Please test

`SWIFT_BUILD_STATIC_STDLIB` is not mutually exclusive with
`SWIFT_BUILD_DYNAMIC_STDLIB`, and Linux sets both, so we can't use
`SWIFT_BUILD_STATIC_STDLIB` to conditionalise things.

The linker error about duplicate definitions for the threading error
handler was happening because we were forced to include the object
containing that symbol; moving it to another object should fix that.

And it turns out there's a way to get CMake to include the threading
object library only when building a shared object, so use that too.

rdar://90776105
@al45tair
Copy link
Contributor Author

al45tair commented Jun 1, 2022

@swift-ci Please test

@al45tair
Copy link
Contributor Author

al45tair commented Jun 1, 2022

The Linux issue has to do with the fact that I'd not appreciated that SWIFT_BUILD_STATIC_STDLIB and SWIFT_BUILD_DYNAMIC_STDLIB weren't mutually exclusive. Hopefully the latest change fixes that.

Local testing of the preset passes still:

Testing Time: 452.37s
  Excluded         : 14471
  Unsupported      :   230
  Passed           :   289
  Expectedly Failed:     1

49 warning(s) in tests
-- check-swift-validation-only_executable-freestanding-x86_64 finished --
--- Finished tests for swift ---

@al45tair al45tair merged commit 8bcb711 into swiftlang:main Jun 1, 2022
@kateinoigakukun
Copy link
Member

Thank you @al45tair for your great work!! The none thread variant is really helpful for us Wasm support!

ahoppen added a commit to ahoppen/swift that referenced this pull request Jun 2, 2022
…105"

This reverts commit 8bcb711, reversing
changes made to c4dd271.
@al45tair
Copy link
Contributor Author

al45tair commented Jun 2, 2022

@kateinoigakukun Glad to know it's helpful. It's been reverted for now because of a few CI failures (the Linux problem you sent a PR for, plus some simulator related problems). I'll get it re-submitted ASAP.

ahoppen added a commit to ahoppen/swift that referenced this pull request Jun 2, 2022
…105"

This reverts commit 8bcb711, reversing
changes made to c4dd271.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants