Enhance -assume-single-threaded option (SR-3945) #7557
Conversation
mtake
changed the title
|
To integrate the flag with the build system, you might want to take a look at how -enable-resilience is plumbed through; starting with the -swift-stdlib-enable-resilience=1 flag passed to build-script-impl. |
|
@mtake Thanks for re-submitting this PR with only your changes! Do you plan to integrate it with the build system by adding an option (e.g. -enable-assume-single-threaded) to the build script and adding a preset, so that it can be tested with the continuous build system? |
include/swift/AST/SILOptions.h
Outdated
| bool AssumeSingleThreaded = false; | ||
| /// TODO: return to false when we pass "-Xfrontend -assume-single-threaded" | ||
| /// to swift compiler while building standard library. | ||
| bool AssumeSingleThreaded = std::getenv("SWIFT_ASSUME_SINGLE_THREADED"); |
There was a problem hiding this comment.
It is a bit unorthodox to initialize the value of a compiler option from the environment variable. Once we have a better integration with the build system, we should better pass proper -assume-single-threaded options to the swift compiler rather than doing this trick.
There was a problem hiding this comment.
I added -swift-stdlib-use-nonatomic-rc option to build-script-impl in 102ee35. I will remove std::getenv once I confirm that the option has the same effect with it.
There was a problem hiding this comment.
Removed std::getenv in 8f8fd35. We can address the issue in a separate PR as needed.
|
@swiftix Although it is not integrated with the build system, I am testing with the environment variable set. As shown in FAIL.txt, some tests are failing and I think most if not all of them are testing race condition. I believe you have experienced the same problem when you introduced the |
|
@mtake What you want to do is add a flag to the build system. I would do this by looking at where resilience is added to the build system. Specifically if you look at line 269 in ./CMakeLists.txt, you will see: I would copy that and add right beneath it a new option called: And then edit test/lit.site.cfg.in and validation-test/lit.site.cfg.in and add a lit feature in both places for your cmake option. To add a lit feature, i.e. assuming the build system flag is SWIFT_STDLIB_USE_NONATOMIC_RC, you add some python code that looks like this: Then in each one of the tests that you want to disable when nonatomic_rc is enabled, you add in the file this: |
|
@mtake If you are reading via email, I just updated my previous post with more comprehensive instructions. For some reason email doesn't mail out the updates. But I basically told you completely how to do it. |
|
@mtake BTW, your PR needs to be rebased, because there is a conflict according to GitHub. To rebase and resolve the conflicts, use the following commands: |
mtake
force-pushed
the
enhance-assume-single-threaded
branch
from
166a492 to
a93d595
Compare
|
@gottesmm Thank you for the detailed instruction on how to add an option to build-script-impl! I just added With the option: With std::getenv: It looks like, for some type of swift code, the option doesn't pass Do you have any idea about it? |
|
@mtake I was talking with @jrose-apple today. He mentioned to me that lit supports the UNSUPPORTED check. With that you can get rid of the no_nonatomic_rc flag. So in other words, make this change: |
stdlib/public/runtime/MetadataImpl.h
Outdated
| @@ -188,6 +189,7 @@ template <class Impl, class T> struct RetainableBoxBase { | |||
| static constexpr size_t stride = sizeof(T); | |||
| static constexpr bool isPOD = false; | |||
| static constexpr bool isBitwiseTakable = true; | |||
| static const bool isAtomic; | |||
There was a problem hiding this comment.
This will impose a cost for normal atomic code. Can you instead either use a cpp flag or us a constexpr that is set by a cpp flag. In the later case, the optimizer should be able to completely eliminate the false path but limit the amount of places the preprocessor code will touch. Your choice.
stdlib/public/SwiftShims/RefCount.h
Outdated
| @@ -471,6 +512,19 @@ class WeakRefCount { | |||
| return (oldval & RC_COUNT_MASK) == subval; | |||
| } | |||
|
|
|||
| /// Decrement the weak reference count. | |||
| /// Return true if the caller should deallocate the object. | |||
| bool decrementShouldDeallocateNNNonAtomic(uint32_t n) { | |||
There was a problem hiding this comment.
Shouldn't the name here have 2 ns? (i.e. decrementShouldDeallocateNNonAtomic).
|
@mtake I think the issue is that you probably need to thread the option through to the benchmark suite. It uses different cmake code. Give me a second to refresh my memory. |
|
@mtake This is one reason why I wish that the benchmark suite was separate from the main repo and used its own cmake stuff sigh. The hacky way to do this is to go to: AddSwiftBenchmarkSuite.cmake. Do a check if that cmake flag is set and if so, add your flag to both common_options and common_driver_options. It is a bit of a hack, but most of that code is a hack anyways... |
|
@gottesmm I am running benchmark on Linux. Since the benchmark doesn't compile on Linux, building Swift on Linux doesn't build the benchmark. I apply my local patch to the benchmark then build it with my shell script which is independent from the existing build mechanism for the benchmark. Details are described in https://github.com/mtake/swift-for-linux/blob/master/README-benchmark.txt. Anyway I wonder that there is a case (presumably related to -wmo) in the current swift compiler where a different instance of SILOptions is created and used without parsing arguments. |
mtake
changed the title
|
@swift-ci please test with buildbot_incremental,tools=RA,stdlib=RD,smoketest=macosx,single-thread |
|
preset=buildbot_incremental,tools=RA,stdlib=RD,smoketest=macosx,single-thread |
|
preset=buildbot_incremental,tools=RA,stdlib=RD,smoketest=macosx,single-thread |
|
preset=buildbot_incremental,tools=RA,stdlib=RD,smoketest=macosx,single-thread |
|
Build failed |
|
@mtake I've run your PR using the commands you can see above. It was running on our internal bots. Overall it look good and breaks only one of our internal tests, which probably needs to be updated. But it is impossible at the moment to benchmark your PR on our CI, because it does not support custom presets yet. Question: Have you benchmarked your latest version locally? Does it remove all the atomic ref-counting operations as good as it did before the changes related to the integration with the build system? I.e. is using of the your new build-system flag leads to expected results or do you see that some atomic reference counting operations are not removed yet, though they should be? |
utils/build-presets.ini
Outdated
|
|
||
| # Don't build the benchmarks | ||
| skip-build-benchmarks | ||
|
|
There was a problem hiding this comment.
@mtake You don't need an incremental build I guess. I think it could be shorter and more future proof to take preset: buildbot,tools=RA,stdlib=RD (based on mixin_buildbot_tools_RA_stdlib_RD ) as a starting point and slightly change it:
[preset: buildbot,tools=RA,stdlib=RD,single-thread]
mixin-preset=mixin_buildbot_tools_RA_stdlib_RD
build-subdir=buildbot_single_thread
dash-dash
# Enable non-atomic build of the stdlib
swift-stdlib-use-nonatomic-rc=true
# Don't run host tests for iOS, tvOS and watchOS platforms to make the build
# faster.
skip-test-ios-host
skip-test-tvos-host
skip-test-watchos-host
This way you don't need to repeat a lot of options. You also do not exclude the build of benchmarks.
@gottesmm What do you think? Does it make sense to take mixin_buildbot_tools_RA_stdlib_RD as a basis preset? Or should it be something else?
There was a problem hiding this comment.
Hmmm... If it is for benchmarking, then use stdlib_RD, if you want to use it for correctness, you probably want stdlib_RDA so you have asserts.
There was a problem hiding this comment.
*or you could even use stdlib_R if you want.
There was a problem hiding this comment.
Actually, since this preset for benchmarking purposes, we probably don't even need the debug info emission for the stdlib, as it only increases the build time.
So, one could even add:
swift-stdlib-build-type=Release after dash-dash
|
@swiftix Results with the new build-system flag (buildpreset only) are close to the results with the old build-system flag (buildpreset and envvar), but there are measurable difference in a few tests. If you compare ChartByARC graph, you will see the difference in SevenBoom, NSError and IterateData. Raw profiling data show high percentage for For your reference, the graph with default build (atomic) is also attached. |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please benchmark |
Build comment file:Optimized (O) Regression (0)Improvement (1)
No Changes (171)
Regression (0)Improvement (2)
No Changes (170)
|
|
Created another graph which shows improvements with the new build-system flag over the default build (improvement). Up to 87% of CPU samples have been reduced. |
|
@mtake Nice improvements. I think we are going to merge the PR this week. Could you please rebase it on the top of the tree? It seems to have some conflicts at moment. |
…getenv is still used in SILOptions.h since it has a wider coverage
… of reference counting in native runtime
mtake
force-pushed
the
enhance-assume-single-threaded
branch
from
68c4bd1 to
69768ad
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please benchmark |
1 similar comment
|
@swift-ci please benchmark |
Build comment file:Optimized (O) Regression (4)
Improvement (1)
No Changes (167)
Regression (1)
Improvement (1)
No Changes (170)
|
|
@mtake We are good to go! Thanks again for working on this!!! |
|
@swiftix @gottesmm @slavapestov Thank you all for taking time to review and feedback! |
This PR is part of SR-3945 which extends the scope of
-assume-single-threadedoption to value types.To create a special build of swift standard library and native runtime with non-atomic reference counting for single-threaded applications, pass
-swift-stdlib-use-nonatomic-rc=trueto build-script-impl.To build your single-threaded application with non-atomic reference counting, pass
-Xfrontend -assume-single-threadedto swift compiler.(Note that this PR replaces #7421 in favor of not including unrelated merged commits from other people.)