Turn on 'as' bridging for Linux (2) #16736
Conversation
|
Please test with the following: @swift-ci please smoke test |
|
@millenomi just a quick check: did you use the eager bridging stub? At some point we are going to want to try to enable that. (TBH I didn't look through the PR I just wanted to make sure that you were aware of this and if it was necessary used it). |
|
I’m not sure what you mean by that, unless that’s how |
|
@gottesmm for the question above. cc for review: @jckarter @jrose-apple @rjmccall @rudkx — Thanks for your time taking a look at this; some feedback from #16022 has been applied. I would prefer if possible to tackle moving the Swift bits to C++ in a follow-up patch and keep the class hierarchy the same, but we can discuss options on this. |
|
Please test with the following: @swift-ci please smoke test |
|
cc @itaiferber for changes to Codable.swift.gyb. |
| @@ -1180,7 +1180,7 @@ public enum EncodingError : Error { | |||
| public var _userInfo: AnyObject? { | |||
| // The error dictionary must be returned as an AnyObject. We can do this | |||
| // only on platforms with bridging, unfortunately. | |||
| #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS) | |||
| #if _runtime(_ObjC) | |||
There was a problem hiding this comment.
We could actually remove this #if now, right? We should be able to return [String : Any] as AnyObject on all platforms, yeah?
|
@millenomi Sorry I missed your question. Here is the code: Context: rdar://35943342 |
|
I'm still concerned about dynamically changing the bridging behavior based on the presence of Foundation. That would create a schism between Foundation-using and non-Foundation-using libraries, and mean that the former can't be dynamically loaded. If value boxes need to be NSCopying, then can you extend the class to be NSCopying from Foundation? We never really wanted |
| @@ -1134,22 +1134,19 @@ bool swift::canUseScalarCheckedCastInstructions(SILModule &M, | |||
| if (!objectType.isAnyClassReferenceType()) | |||
| return false; | |||
|
|
|||
| if (M.getASTContext().LangOpts.EnableObjCInterop) { | |||
| auto super = archetype->getSuperclass(); | |||
There was a problem hiding this comment.
The indentation here should be fixed to match the surrounding code.
|
|
||
|
|
||
| var x: Any = 1 | ||
| var y = x as AnyObject // expected-error{{not convertible}} |
There was a problem hiding this comment.
We should keep this test, but change the expected output.
test/stmt/errors_nonobjc.swift
Outdated
| @@ -1,18 +0,0 @@ | |||
| // RUN: %empty-directory(%t) | |||
There was a problem hiding this comment.
Similarly I think it makes sense to keep this test and update the comment and expected output.
test/Interpreter/generic_casts.swift
Outdated
| @@ -158,16 +158,6 @@ print(allToAll(type(of: C()), AnyObject.self)) // CHECK-NEXT: true | |||
| // Bridging | |||
| print(allToAll(0, AnyObject.self)) // CHECK-NEXT: true | |||
|
|
|||
| // This will get bridged using _SwiftValue. | |||
There was a problem hiding this comment.
What's the behavioral change here? It looks like it applies to macOS as well?
There was a problem hiding this comment.
My bad; this is an artifact of development where _SwiftValue was not yet available. It will be restored as-is.
|
Question. @millenomi do you think it would make sense to just enable eager bridging straight up now on Linux? It would make optimizing arrays significantly easier and prevent the lazy behavior that really hurts the optimizer on macOS. I would rather us stay away from those legacy problems on Linux if we can. |
|
@millenomi Just to be complete: these are design flaws from the optimizer's perspective.
|
There was a problem hiding this comment.
I agree with @gottesmm. There are potentially considerable performance implication. I suggest that you at least run the swift benchmarks on linux before and after the change. Unfortunately the linux benchmarks cannot be run in the CI. But it is easy to run the benchmarks locally on a linux machine. Can you please then copy the results into this PR?
|
@gottesmm The code modified by 3214627 is not compiled on Linux. On Linux, there is Core Foundation but no ObjC runtime. This means that there is no need to consider invocations to ObjC classes; it is my understanding that those are the reason the bridging paths are not on Darwin. The use case here is instead for interop with Core Foundation, which contains the ultimate implementation of many basic algorithms and is used by swift-corelibs-foundation to ensure behavior remains close to Darwin, and source and behavior interop when bridging is invoked by user code, which unfortunately is necessary for use of some Foundation API that return Any (like Thus, we have decided to not use the stdlib potentially-lazy bridging approach. Instead, we use the current behavior of the bridging primitives in s-c-f, which bridge eagerly at the transition point. The code you point to is guarded by Insofar as the other points you mention:
|
|
What that function does is just use a stub that we can use to add eager bridging in an ABI preserving way when/if we enable eager bridging on macOS. Those 4 things were more things to keep in mind when messing with stuff like this. We do not want anything like any of those 4 things to make the Linux implementation more conservative than necessary. I was not saying that this patch does all of those things. That being said, as long as the bridging is eager, I am ok with this. One unfortunate aspect of this is that since the benchmarks do not build on Linux I do not think that you can run these and validate the performance. Maybe we should fix that first so we can actually see how this is effecting the benchmarks? |
|
@jckarter One of the targets of this patch is to prevent silent dependency scenarios. A typical example is class Problem: NSSecureCoding { … }
class ProblemHolder: NSSecureCoding {
…
required init?(coder: NSCoder) {
guard let problems = coder.decode(of: [NSArray.self, Problem.self], forKey: "MaybeNinetyNineProblems") as? [Problem] else { … }
…
}
…
}the The misgivings you have re: NSObject are of a similar silent nature. On Darwin, I'd rather be thorough here than not. I understand this prevents an application from dlopen()ing Foundation-linking libraries unless it itself links Foundation — I'll note tho that the supported API for loading libraries that apps are likely to be using is in Foundation (the Bundle class). Still, yours is a concern that I tried to work around several times in this patch to little avail (including, at one point, moving NSObject down to the stdlib, breaking several method signatures on the base class that require Foundation types). Any solution I can come up with so far has significant drawbacks, and this looked like a sweet spot in behavior and drawback. Any improvement suggestion is welcome. |
|
@gottesmm I'll look into running benchmarks on Linux and see if there's anything I can do to fix or have them fixed. |
|
(To be clear: we do have a way that we share between Darwin and Linux to (de)serialize value types, |
Predictability is all well and good within reason, but I think the solution here trades one impredictability for worse and harder-to-manage ones. I would expect
Using |
|
I am very worried for the churn I see in trying to implement your proposal, in particular:
(Silly idea: is it possible for us to 'fake' reparenting a stdlib |
It's not silly at all. Since |
Note that |
There was a problem hiding this comment.
Lily, Tony, and I met in person, and I'm ok with taking this patch as is for now and pursuing cleanups as followup tasks:
https://bugs.swift.org/browse/SR-7767
https://bugs.swift.org/browse/SR-7768
https://bugs.swift.org/browse/SR-7769
millenomi
force-pushed
the
LinuxAsBridging_2
branch
from
2afb116 to
94f8470
Compare
|
@rudkx I have readded (and where necessary, reworked) the tests you pointed to. How does this look? |
|
Please test with the following: @swift-ci please smoke test |
|
@eeckstein @gottesmm How do we want to proceed with benchmarks? After landing? |
|
I am pretty close. Let me see what I can do tonight. |
|
@gottesmm Thank you so much for the after hours help! |
|
Got it to work. I am going to go grab a coffee/upstream. It is slightly manual (it is an external build), but I have a script that you just need to pass in the args and it will just work. |
|
@gottesmm Thanks! I’ll sync up with you after the holiday. |
|
Please test with the following: @swift-ci please smoke benchmark |
…ion type, remove assertions.
millenomi
force-pushed
the
LinuxAsBridging_2
branch
from
94f8470 to
f7a28a8
Compare
|
Please test with the following: @swift-ci please smoke test |
|
@eeckstein @gottesmm Benchmarks have been taken: perf-patch-applied.txt These benchmarks include the fixes I just pushed, which prevent a large slowdown (22x) in AnyHashable tests. Some slowdowns are expected, given that we are enabling paths that do more work during casting; as a summary, the following tests have a slowdown (ratio of means) over 10%:
Numbers have been generated by merging #16882 and running: utils/build-script --preset="buildbot_linux,no_test" install_destdir=~/Install/root installable_package=~/Install/archive
benchmark/scripts/build_linux.py `which cmake` ~/Developer/Sources/Swift/swift ~/Developer/Sources/Swift/build/buildbot_linux/llvm-linux-x86_64/bin/clang ~/Install/root/usr ~/Install/benchmark --clean
~/Install/benchmark/bin/Benchmark_O --num-samples=5I am at this point really inclined to investigate these as a follow-up; is that ok with you? |
Build comment file:Optimized (O)Regression (9)
Improvement (12)
No Changes (417)
Hardware Overview |
| return archetype->requiresClass(); | ||
| // A base class constraint that isn't NSError rules out the archetype being | ||
| // bound to NSError. | ||
| if (M.getASTContext().LangOpts.EnableObjCInterop) { |
There was a problem hiding this comment.
every other change here seems to be about removing such calls, why does this remain?
There was a problem hiding this comment.
I've been introducing the minimum change that supports the subset we need in s-c-f by elimination. A follow-up will be to add tests, and revisit paths here if we find corner cases that I missed.
Fine with me |
|
Follow-ups I promised beyond @jckarter's: |
|
Simultaneous merging with swiftlang/swift-corelibs-foundation#1526 imminent… |
|
Standing by for CI now. |
|
@benlangmuir FYI, this has been merged; standing by for CI. |
Turn on 'as' bridging for Linux (2) (cherry picked from commit 63348bc)
(This is a second attempt at #16022.)
Turn on some of the same kinds of bridging on Linux as Swift already allows on Darwin, reducing language differences between platforms.
The following bridging situations will now work the same from a developer's perspective on Linux as they do on Darwin:
NSString(…) as String,"abc" as NSString,1.23 as NSNumber)._SwiftValue) reference types and vice versa (withstruct X {}:([X(), X(), X()] as NSArray) as! [X]will work).Optional<_>.noneto an appropriate object and back. (If Foundation is available, we will useNSNullas the object, to match Darwin behavior. For example,(([nil, nil, nil] as [Int?]) as NSArray) as! [Int?]will work, and theNSArraywill be[NSNull(), NSNull(), NSNull()]like it is on Darwin.)Just like on Darwin, these objects are all laid out identically to CFTypeRefs, meaning for example that an array of these values, including opaque boxed values, can be casted to CFArrayRef and back correctly just like it can on Darwin. Likewise, if Foundation is linked, these opaque objects will be castable to
Foundation.NSObjectand implementFoundation.NSCopyingjust like they can on Darwin; while this isn't part of the contract for these objects, code that unwittingly relies on this will continue to work on Linux as well.This does not target enabling other
as{,?,!}casts that work on Darwin but not on Linux that strictly require the ObjC runtime:X.selffrom/toAnyObjectAnyObjectThis patch must land in lockstep with a separate Foundation patch (at swiftlang/swift-corelibs-foundation#1526). This patch doesn't require that applications link Foundation; apps that do not link it will still be able to use
as AnyObjectcasts for any value to turn it into a bridged object, if they implement_ObjectiveCBridgeable(which still remains an implementation detail), or an opaque boxed value; this also reflects Swift language behavior for Darwin applications that do not import Foundation. One difference with Darwin is that in this case the opaque box returned by casting will not be aNSObject, since that's only available if Foundation is.