swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.58k stars 10.36k forks source link

[SR-10708] switch over Dictionary lookup retains ref count #53105

Open drexin opened 5 years ago

drexin commented 5 years ago
Previous ID SR-10708
Radar None
Original Reporter @drexin
Type Bug

Attachment: Download

Environment Apple Swift version 5.0 (swift-5.0-RELEASE) Target: x86_64-apple-darwin18.5.0
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 1 | |Component/s | Compiler | |Labels | Bug, 5.0Regression, ARC, Optimizer | |Assignee | None | |Priority | Medium | md5: c994e96b09804105727b513a51ebc355

relates to:

Issue Description:

When switching over the result of a dictionary lookup directly in Swift 5 and then modifying that dictionary it will be copied.

class Foo {
    var dict: [String: Int] = ["x": 1]

    func test(_ x: String) {
        switch dict[x] {
        case .some(let y):
            dict[x] = y
        default: ()
        }
    }
}

let foo = Foo()

for _ in 1 ... 1000 {
    foo.test("x")
}

The above code allocates 1092 times in total.

When assigning the result to a variable first, it does not copy.

class Foo {
    var dict: [String: Int] = ["x": 1]

    func test(_ x: String) {
        let value = dict[x]
        switch value {
        case .some(let y):
            dict[x] = y
        default: ()
        }
    }
}

let foo = Foo()

for _ in 1 ... 1000 {
    foo.test("x")
}

The above code allocates 91 times in total.

In Swift 4.2 both versions do not copy, so this seems to be a regression in Swift 5.

weissi commented 5 years ago

attached screenshots of the assembly and SIL diffs between the the good ({{ let value = dict[x]}}) and bad (switch dict[x]) versions. In both cases we see that the switch appears to retain the whole dictionary the the whole switch and in the good version it doesn't...

weissi commented 5 years ago

and it's still and issue in a very recent 5.1 snapshot and also in swift-DEVELOPMENT-SNAPSHOT-2019-05-12-a.xctoolchain.

belkadan commented 5 years ago

cc @gottesmm (possibly "fixed" by your redoing of SILGenPattern?)