[SR-11706] Invoking computed properties on class-backed structures emits excessive retains/releases

[Lukasa]

Previous ID	SR-11706
Radar	rdar://problem/58667192
Original Reporter	@Lukasa
Type	Bug
Status	Reopened
Resolution

Attachment: Download

Environment

Apple Swift version 5.1.1 (swiftlang-1100.0.275.2 clang-1100.0.33.9)
Target: x86_64-apple-darwin19.0.0

Additional Detail from JIRA

Votes	1
Component/s	Compiler
Labels	Bug, Performance
Assignee	@gottesmm
Priority	Medium

md5: fe768e083fde0dd0bb93966e9182970c

Issue Description:

Consider a SwiftPM package that defines two modules. The first is called testlib, and contains the following code:

public struct Foo {
    private var _storage: _Storage

    public var bar: Int {
        return 5
    }

    public init() {
        self._storage = _Storage()
    }
}

extension Foo {
    fileprivate class _Storage { }
}

The second module is an executable called test, which is defined like so:

import Foundation
import testlib

@inline(never)
func main(_ f: inout Foo) -> Int {
    let result = f.bar
    return result
}

var f = Foo()
let result = main(&f)
if result != 5 {
    exit(1)
} else {
    exit(0)
}

This program creates a Foo, passes it inout to a function, and then calls a non mutating get accessor on Foo. This code performs some excessive retains/releases in test.main, which when disassembled looks like this:

_$s4test4mainySi7testlib3FooVzF:        // test.main(inout testlib.Foo) -> Swift.Int
    push       rbp                          ; CODE XREF=_main+55
    mov        rbp, rsp
    push       r14
    push       rbx
    mov        rbx, qword [rdi]
    mov        rdi, rbx                     ; argument "instance" for method imp___stubs__swift_retain
    call       imp___stubs__swift_retain    ; swift_retain
    mov        rdi, rbx
    call       _$s7testlib3FooV3barSivg     ; testlib.Foo.bar.getter : Swift.Int
    mov        r14, rax
    mov        rdi, rbx                     ; argument "instance" for method imp___stubs__swift_release
    call       imp___stubs__swift_release   ; swift_release
    mov        rax, r14
    pop        rbx
    pop        r14
    pop        rbp
    ret

In this case, a swift_retain is performed before the invocation of the computed property.

This seems excessive: it seems like the natural semantic for a nonmutating get accessor would be to assume that the reference currently held by the calling function ought to be sufficient to cover the computing needs of that accessor. I'd have assumed, then, that nonmutating calls should be passed self at +0 and can retain self if they need to.

The result of this is that repeated calls to computed properties incur substantial ARC traffic. For example, if we called Foo.bar in a loop, we will retain/release once per loop invocation.
In the case of a computed property as simple as Foo.bar the cost of these retain/release operations will strictly dominate the actual calculation of the computed property.

This is non-theoretical: SwiftNIO issue 1218 covers a feature request to have SwiftNIO add an "unsafe" equivalent of its ByteBuffer type to elide refcounting overhead in a parse-heavy loop. The ARC overhead here came entirely from accessing non-inlinable computed properties: making them inlinable halved the runtime of the program and elided the ARC traffic almost entirely. It would be good if we didn't have to make these properties inlinable to allow the compiler to see that they don't need self at +1.

I've attached a sample project.

[beccadax]

@swift-ci create

[weissi]

@swift-ci create

[gottesmm]

I have a feeling that we may support this now. Let me check. Just starting a build of swift on my side machine.

[gottesmm]

Yes. This is fixed. I just tried with snapshot from 03/04

// main(_:)                                                                                                        
sil hidden [noinline] @$s4test4mainySi7testlib3FooVzF : $@convention(thin) (@inout Foo) -> Int {                   
// %0                                             // users: %2, %1                                                 
bb0(%0 : $*Foo):                                                                                                   
  debug_value_addr %0 : $*Foo, var, name "f", argno 1 // id: %1                                                    
  %2 = load %0 : $*Foo                            // user: %4                                                      
  // function_ref Foo.bar.getter                                                                                   
  %3 = function_ref @$s7testlib3FooV3barSivg : $@convention(method) (@guaranteed Foo) -> Int // user: %4           
  %4 = apply %3(%2) : $@convention(method) (@guaranteed Foo) -> Int // users: %6, %5                               
  debug_value %4 : $Int, let, name "result"       // id: %5                                                        
  return %4 : $Int                                // id: %6                                                        
} // end sil function '$s4test4mainySi7testlib3FooVzF'

[gottesmm]

This was fixed by:

#29480

[gottesmm]

Hmmm... actually I just handled the case where there aren't any writes to the function at all. I have a plan for implementing the case with writes, but it will take more work. I think having the requirement of no writes is not predictable and unexpected for users. Going to unresolved this.

[gottesmm]

NOTE: I had a different radar internally for tracking this work (including the write case). I duped the original radar in this SR to the one I am actually using for the work, so I updated the radar in this SR to be the actual tracking radar.