Annotate Foundation overlay with shared/owned #19143

itaiferber · 2018-09-05T20:40:32Z

What's in this pull request?
Annotates portions of the Foundation overlay with the appropriate __shared/__owned annotations following the patterns we use (resolving rdar://problem/34989766). I looked at every declared func and init within our overlay and annotated all methods as appropriate. ~~~A search of all stored vars shows no necessary changes AFAICT as we don't expose reference types from our Swift APIs.~~~ (Since var types cannot be annotated for ownership, there's nothing for us to do in the cases where vars should be __shared.)

Overall the changes mostly revolve around bridging:

We often copy() the passed NS type before holding onto it as a reference to preserve value semantics, and in those cases, the parameter should be __shared as we don't hold on to the original. (AFAIK, copy() should help enforce the right refcounting in the case where copy() returns self.)
In other cases, we create an NS type out of the Swift type via bridging (which copies as well) and hold on to the NS type

There are a few cases where init parameters go unused (such as in String.init?(contentsOf:)), and some cases around KeyPath where we appear to not store the given KeyPath but it actually ends up stored in a global table.

itaiferber · 2018-09-05T20:41:08Z

@swift-ci Please test macOS

itaiferber · 2018-09-05T20:41:16Z

@swift-ci Please smoke test Linux

itaiferber · 2018-09-05T20:43:58Z

@eeckstein As far as I'm aware, our actual static bridging functions (_forceBridgeFromObjectiveC, _conditionallyBridgeFromObjectiveC, and _unconditionallyBridgeFromObjectiveC) should not require annotations as:

They're static funcs which have no local storage (or write out to inout-params)
Since they call into the shared reference variants changed above, the default for funcs applies here — the params should not be owned

Is my understanding here correct?

jrose-apple · 2018-09-05T21:09:33Z

Sorry, why is this specific to reference types? cc @airspeedswift

itaiferber · 2018-09-05T21:13:50Z

@jrose-apple Sorry, have I missed something here? Is this not inherently related to values which require reference counting?

jrose-apple · 2018-09-05T21:16:58Z

"Values which require reference counting" isn't quite the same as "reference types". String and Array and Data all require reference counting too, right?

itaiferber · 2018-09-05T21:27:37Z

Ah, therein lies my misunderstanding about what was necessary here. I'll re-audit for those types as well.

itaiferber · 2018-09-05T21:30:18Z

Closing for now.

swift-ci · 2018-09-05T21:53:44Z

Build failed
Swift Test OS X Platform
Git Sha - 402457b

itaiferber · 2018-09-11T21:39:41Z

Made another pass here to add annotations to cover other non-trivial types. Will be doing another pass to confirm there's nothing I missed.

itaiferber · 2018-09-11T21:39:55Z

@swift-ci Please test macOS

itaiferber · 2018-09-11T21:40:03Z

@swift-ci Please smoke test Linux

swift-ci · 2018-09-11T21:43:53Z

Build failed
Swift Test OS X Platform
Git Sha - 402457b

itaiferber · 2018-09-19T20:17:20Z

@jrose-apple @eeckstein Do the changes here look correct? Does anything stand out as being out of place?

airspeedswift · 2018-09-19T20:43:09Z

stdlib/public/SDK/Foundation/IndexPath.swift

@@ -156,7 +156,7 @@ public struct IndexPath : ReferenceConvertible, Equatable, Hashable, MutableColl
            }
        }

-        mutating func append(contentsOf other: [Int]) {
+        mutating func append(contentsOf other: __owned [Int]) {


this probably isn't necessary since it's just [Int]

No, Array of Int is not a trivial type, i.e. involves reference counting of the array buffer.

Right, but IndexPath doesn't gain anything by taking the array owned, since it's not going to be able to use that buffer for storage, and the type isn't generic.

You are right, assuming that the case where self is empty and size of other > 2 is not the common case, it's better to pass other as guaranteed.
(I misread airspeedswift's comment: I though he meant that it's not worth annotating because of the parameter type).

jrose-apple

Can you explain what the principles are for what is and isn't annotated? Since you're explicitly writing both __shared and __owned, I'd expect it to be

every (non-trivial) parameter
of all public/@usableFromInline functions and initializers

and perhaps some of the non-public stuff as optimization hints. But I see public functions where that hasn't been done, such as https://github.com/apple/swift/blob/9f042bc7c3452307b7929f5b76db5261b623d79a/stdlib/public/SDK/Foundation/Calendar.swift#L507.

jrose-apple · 2018-09-11T21:44:07Z

stdlib/public/SDK/Foundation/Calendar.swift

@@ -106,7 +106,7 @@ public struct Calendar : Hashable, Equatable, ReferenceConvertible, _MutableBoxi
    // MARK: -
    // MARK: Bridging

-    fileprivate init(reference : NSCalendar) {
+    fileprivate init(reference : __shared NSCalendar) {


This one should be owned, no? Since you're immediately going to store it.

Although it's not a public entry point anyway, so it doesn't really matter.

_MutableHandle makes a copy of the reference given to it so we never store the original — I've propagated __shared out to every init(reference:) which uses this, per @eeckstein's recommendation.

jrose-apple · 2018-09-11T21:48:54Z

stdlib/public/SDK/Foundation/JSONEncoder.swift

@@ -961,7 +961,7 @@ fileprivate class _JSONReferencingEncoder : _JSONEncoder {

    /// Initializes `self` by referencing the given dictionary container in the given encoder.
    fileprivate init(referencing encoder: _JSONEncoder,
-                     key: CodingKey, convertedKey: CodingKey, wrapping dictionary: NSMutableDictionary) {
+                     key: CodingKey, convertedKey: __shared CodingKey, wrapping dictionary: NSMutableDictionary) {


I don't see why only the one parameter changed here.

This parameter is not stored; all others are. Based on your comments, it sounds to me like I've misunderstood something critical here. Let me start an email thread so we can sort this out.

jrose-apple · 2018-09-19T21:32:32Z

stdlib/public/SDK/Foundation/IndexPath.swift

@@ -156,7 +156,7 @@ public struct IndexPath : ReferenceConvertible, Equatable, Hashable, MutableColl
            }
        }

-        mutating func append(contentsOf other: [Int]) {
+        mutating func append(contentsOf other: __owned [Int]) {


Right, but IndexPath doesn't gain anything by taking the array owned, since it's not going to be able to use that buffer for storage, and the type isn't generic.

jrose-apple · 2018-09-19T21:34:08Z

stdlib/public/SDK/Foundation/Data.swift

@@ -1178,7 +1178,7 @@ public struct Data : ReferenceConvertible, Equatable, Hashable, RandomAccessColl
    /// - parameter url: The `URL` to read.
    /// - parameter options: Options for the read operation. Default value is `[]`.
    /// - throws: An error in the Cocoa domain, if `url` cannot be read.
-    public init(contentsOf url: URL, options: Data.ReadingOptions = []) throws {
+    public init(contentsOf url: __shared URL, options: __shared Data.ReadingOptions = []) throws {


Data.ReadingOptions is an ObjC enum, right? So it shouldn't matter what ownership you use.

Sure — some of the changes here were mechanical, so I can pull this one out. I'm surprised the compiler didn't produce a warning/error in this case.

We haven't really talked about what form that'll take yet, but yeah, that's a reasonable idea. (You wouldn't necessarily want to do it to everything that's trivial, because a new version of the library might make it non-trivial, but that's not going to happen to an @objc enum.)

itaiferber · 2018-09-19T21:51:11Z

@jrose-apple Sure — based on the guidelines I was given, I went ahead and annotated every function parameter which deviated from the default rules that I understand are in place (__shared is assumed for all function parameters by default, __owned is assumed for all init parameters by default). Anything not annotated is meant to assume the default.

If it would be more helpful (or even necessary) to annotate all parameters regardless of the default, then I'm willing to go through and do that. Similarly, if I've misunderstood the guidelines (see rdar://problem/34989766 for discussion between between me and @eeckstein), happy to be corrected.

eeckstein · 2018-09-19T22:15:44Z

@itaiferber

If it would be more helpful (or even necessary) to annotate all parameters regardless of the default

No, that's not necessary

jrose-apple · 2018-09-19T22:16:00Z

I feel silly now. I didn't realize you were just annotating divergences from the default. Sorry for jumping in!

itaiferber · 2018-09-20T19:28:11Z

@swift-ci Please test macOS

itaiferber · 2018-09-20T19:28:20Z

@swift-ci Please smoke test Linux

swift-ci · 2018-09-20T19:28:41Z

Build failed
Swift Test OS X Platform
Git Sha - 9f042bc

itaiferber · 2018-09-20T19:28:57Z

Latest commit removes the unnecessary __shared annotations on Data.ReadingOptions and friends (which are Obj-C enums). If tests are green I'll merge.

itaiferber · 2018-09-20T19:29:42Z

@jrose-apple No worries at all, BTW — thanks for taking the time to review! Thanks @eeckstein for the help, too.

Annotate Foundation overlay with __shared/__owned

402457b

itaiferber requested review from phausler and eeckstein September 5, 2018 20:40

itaiferber closed this Sep 5, 2018

Apply annotations to further non-trivial types

9f042bc

itaiferber reopened this Sep 11, 2018

airspeedswift reviewed Sep 19, 2018

View reviewed changes

jrose-apple reviewed Sep 19, 2018

View reviewed changes

eeckstein approved these changes Sep 19, 2018

View reviewed changes

Remove unnecessary __shared annotations

832317c

itaiferber merged commit a28c9d6 into swiftlang:master Sep 24, 2018

itaiferber deleted the foundation-overlay-owned-annotations branch September 24, 2018 16:00

Annotate Foundation overlay with __shared/__owned #19143

Annotate Foundation overlay with __shared/__owned #19143

Uh oh!

Conversation

itaiferber commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itaiferber commented Sep 5, 2018

Uh oh!

itaiferber commented Sep 5, 2018

Uh oh!

itaiferber commented Sep 5, 2018

Uh oh!

jrose-apple commented Sep 5, 2018

Uh oh!

itaiferber commented Sep 5, 2018

Uh oh!

jrose-apple commented Sep 5, 2018

Uh oh!

itaiferber commented Sep 5, 2018

Uh oh!

itaiferber commented Sep 5, 2018

Uh oh!

swift-ci commented Sep 5, 2018

Uh oh!

itaiferber commented Sep 11, 2018

Uh oh!

itaiferber commented Sep 11, 2018

Uh oh!

itaiferber commented Sep 11, 2018

Uh oh!

swift-ci commented Sep 11, 2018

Uh oh!

itaiferber commented Sep 19, 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!

Choose a reason for hiding this comment

Uh oh!

jrose-apple left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

itaiferber commented Sep 19, 2018

Uh oh!

eeckstein commented Sep 19, 2018

Uh oh!

jrose-apple commented Sep 19, 2018

Uh oh!

itaiferber commented Sep 20, 2018

Uh oh!

itaiferber commented Sep 20, 2018

Uh oh!

swift-ci commented Sep 20, 2018

Uh oh!

itaiferber commented Sep 20, 2018

Uh oh!

itaiferber commented Sep 20, 2018

Uh oh!

Annotate Foundation overlay with shared/owned #19143

Annotate Foundation overlay with shared/owned #19143

itaiferber commented Sep 5, 2018 •

edited

Loading