[stdlib] AutoreleasingUnsafeMutablePointer: eliminate questionable pointer use #26760

lorentey · 2019-08-21T03:20:37Z

AutoreleasingUnsafeMutablePointer is pointing to a +0 reference, but in its pointee property’s getter/setter implementations, it is loading the pointer into regular Unsafe[Mutable]Pointers. Those are assuming that the addressed memory contains a +1 reference, which can mislead the compiler into doing optimizations that aren’t valid.

Change the getter/setter implementations so that they use UnsafeRawPointer and load/store Unmanaged values instead. As long as Unmanaged.passUnretained(_:) and Unmanaged.takeUnretainedValue() do the right thing, then AutoreleasingUnsafeMutablePointer won’t have issues, either. (This boils down to ensuring that loading a strong reference out of an unmanaged(unsafe) value works correctly.)

…inter use AutoreleasingUnsafeMutablePointer is pointing to a +0 reference, but in its pointee property’s getter/setter implementations, it is loading the pointer into regular Unsafe[Mutable]Pointers. Those are assuming that the addressed memory contain a +1 reference, which can mislead the compiler into doing optimizations that aren’t valid. Change the getter/setter implementations so that they use UnsafeRawPointer and load/store Unmanaged values instead. As long as Unmanaged.passUnretained(_:) and Unmanaged.takeUnretainedValue() do the right thing, then AutoreleasingUnsafeMutablePointer won’t have issues, either. (This boils down to ensuring that loading a strong reference out of an unmanaged(unsafe) value works correctly.)

lorentey · 2019-08-21T03:22:45Z

This is still somewhat of a WIP - AutoreleasingUnsafeMutablePointer doesn't seem to have much test coverage yet, so perhaps this is the right PR to add those.

lorentey · 2019-08-21T03:24:55Z

@swift-ci benchmark

swift-ci · 2019-08-21T03:55:52Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenListFlatMap	6815	10537	+54.6%	0.65x (?)
FlattenListLoop	3974	5187	+30.5%	0.77x (?)
ObjectiveCBridgeStringHash	73	85	+16.4%	0.86x
RandomShuffleLCG2	704	768	+9.1%	0.92x
Array2D	6912	7520	+8.8%	0.92x (?)
Set.isDisjoint.Seq.Box.Empty	117	127	+8.5%	0.92x
RemoveWhereSwapInts	62	67	+8.1%	0.93x
MapReduce	369	398	+7.9%	0.93x (?)
MapReduceAnyCollection	370	399	+7.8%	0.93x

Improvement	OLD	NEW	DELTA	RATIO
LessSubstringSubstring	45	40	-11.1%	1.12x
EqualStringSubstring	45	40	-11.1%	1.12x
EqualSubstringString	45	40	-11.1%	1.12x
LessSubstringSubstringGenericComparable	45	40	-11.1%	1.12x
EqualSubstringSubstringGenericEquatable	44	40	-9.1%	1.10x
EqualSubstringSubstring	45	41	-8.9%	1.10x
ObjectiveCBridgeStubToNSDate2	1390	1290	-7.2%	1.08x (?)
SubstringEqualString	461	429	-6.9%	1.07x

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
ObjectiveCBridgeStringHash	73	85	+16.4%	0.86x
IterateData	1319	1452	+10.1%	0.91x
FlattenListLoop	4066	4437	+9.1%	0.92x (?)
RemoveWhereMoveInts	34	37	+8.8%	0.92x
Array2D	6928	7520	+8.5%	0.92x
RemoveWhereSwapInts	60	65	+8.3%	0.92x
Set.isDisjoint.Seq.Box.Empty	158	170	+7.6%	0.93x

Improvement	OLD	NEW	DELTA	RATIO
EqualSubstringSubstring	45	40	-11.1%	1.12x
EqualSubstringSubstringGenericEquatable	45	40	-11.1%	1.12x
LessSubstringSubstring	45	41	-8.9%	1.10x (?)
EqualStringSubstring	45	41	-8.9%	1.10x (?)
EqualSubstringString	45	41	-8.9%	1.10x (?)
LessSubstringSubstringGenericComparable	45	41	-8.9%	1.10x
Data.hash.Empty	74	68	-8.1%	1.09x (?)
SubstringEquatable	771	715	-7.3%	1.08x (?)
ObjectiveCBridgeFromNSArrayAnyObjectForced	5100	4760	-6.7%	1.07x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
ObjectiveCBridgeStringHash	73	85	+16.4%	0.86x

Improvement	OLD	NEW	DELTA	RATIO
ArrayOfPOD	1144	1061	-7.3%	1.08x (?)

Code size: -swiftlibs

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

Catfish-Man · 2019-08-21T22:39:14Z

stdlib/public/core/BridgeObjectiveC.swift

+      // The memory addressed by this pointer contains a non-owning reference,
+      // therefore we *must not* point an `UnsafePointer<AnyObject>` to
+      // it---otherwise we would allow the compiler to assume it has a +1
+      // refcount, enabling some optimizations that wouldn't be valid.


This makes sense but is kinda terrifying. I need to think carefully about some of the NSDictionary bridging stuff…

All +0 references in memory are hazards; they must only be handled through the Unmanaged glovebox. E.g., we'll need to audit any and all NSFastEnumeration implementations & usages from Swift code.

(Currently Unmanaged doesn't quite get this right either, but Michael is already working on fixing that.)

lorentey · 2019-08-22T00:33:56Z

The benchmark results seem irrelevant; I don't see how any of these benchmarks would touch AutoreleasingUnsafeMutablePointer paths.

However, this new version does generate a little bit worse code:

// SOURCE:
class Foo {}

func foo(_ p: AutoreleasingUnsafeMutablePointer<Foo>) -> Foo {
  return p.pointee
}

// ORIGINAL SIL:
sil hidden [signature_optimized_thunk] [always_inline] @$s6output3fooyAA3FooCSAyADGF : $@convention(thin) (AutoreleasingUnsafeMutablePointer<Foo>) -> @owned Foo {
bb0(%0 : $AutoreleasingUnsafeMutablePointer<Foo>):
  debug_value %0 : $AutoreleasingUnsafeMutablePointer<Foo>, let, name "p", argno 1 // id: %1
  %2 = struct_extract %0 : $AutoreleasingUnsafeMutablePointer<Foo>, #AutoreleasingUnsafeMutablePointer._rawValue // user: %3
  %3 = pointer_to_address %2 : $Builtin.RawPointer to [strict] $*Foo // user: %4
  %4 = load %3 : $*Foo                            // users: %5, %6
  strong_retain %4 : $Foo                         // id: %5
  return %4 : $Foo                                // id: %6
} // end sil function '$s6output3fooyAA3FooCSAyADGF'

// NEW SIL:
sil hidden @$s6output3fooyAA3FooCSAyADGF : $@convention(thin) (AutoreleasingUnsafeMutablePointer<Foo>) -> @owned Foo {
bb0(%0 : $AutoreleasingUnsafeMutablePointer<Foo>):
  debug_value %0 : $AutoreleasingUnsafeMutablePointer<Foo>, let, name "p", argno 1 // id: %1
  %2 = struct_extract %0 : $AutoreleasingUnsafeMutablePointer<Foo>, #AutoreleasingUnsafeMutablePointer._rawValue // user: %3
  %3 = pointer_to_address %2 : $Builtin.RawPointer to $*Optional<Unmanaged<AnyObject>> // user: %4
  %4 = load %3 : $*Optional<Unmanaged<AnyObject>> // user: %5
  switch_enum %4 : $Optional<Unmanaged<AnyObject>>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2 // id: %5

bb1(%6 : $Unmanaged<AnyObject>):                  // Preds: bb0
  %7 = struct_extract %6 : $Unmanaged<AnyObject>, #Unmanaged._value // user: %8
  %8 = unmanaged_to_ref %7 : $@sil_unmanaged AnyObject to $AnyObject // users: %10, %9
  %9 = enum $Optional<AnyObject>, #Optional.some!enumelt.1, %8 : $AnyObject // user: %11
  strong_retain %8 : $AnyObject                   // id: %10
  br bb3(%9 : $Optional<AnyObject>)               // id: %11

bb2:                                              // Preds: bb0
  %12 = enum $Optional<AnyObject>, #Optional.none!enumelt // user: %13
  br bb3(%12 : $Optional<AnyObject>)              // id: %13

bb3(%14 : $Optional<AnyObject>):                  // Preds: bb2 bb1
  %15 = metatype $@thick Optional<AnyObject>.Type // user: %16
  %16 = builtin "sizeof"<Optional<AnyObject>>(%15 : $@thick Optional<AnyObject>.Type) : $Builtin.Word // user: %17
  %17 = builtin "sextOrBitCast_Word_Int64"(%16 : $Builtin.Word) : $Builtin.Int64 // user: %21
  %18 = metatype $@thick Foo.Type                 // user: %19
  %19 = builtin "sizeof"<Foo>(%18 : $@thick Foo.Type) : $Builtin.Word // user: %20
  %20 = builtin "sextOrBitCast_Word_Int64"(%19 : $Builtin.Word) : $Builtin.Int64 // user: %21
  %21 = builtin "cmp_eq_Int64"(%17 : $Builtin.Int64, %20 : $Builtin.Int64) : $Builtin.Int1 // user: %23
  %22 = integer_literal $Builtin.Int1, -1         // user: %23
  %23 = builtin "xor_Int1"(%21 : $Builtin.Int1, %22 : $Builtin.Int1) : $Builtin.Int1 // user: %24
  cond_fail %23 : $Builtin.Int1, "Can't unsafeBitCast between types of different sizes" // id: %24
  %25 = unchecked_ref_cast %14 : $Optional<AnyObject> to $Foo // user: %26
  return %25 : $Foo                               // id: %26
} // end sil function '$s6output3fooyAA3FooCSAyADGF'

// Original x86_64 assembly:
output.foo(Swift.AutoreleasingUnsafeMutablePointer<output.Foo>) -> output.Foo:
        push    rbp
        mov     rbp, rsp
        mov     rdi, qword ptr [rdi]
        pop     rbp
        jmp     _swift_retain

// New x86_64 assembly:
output.foo(Swift.AutoreleasingUnsafeMutablePointer<output.Foo>) -> output.Foo:
        push    rbp
        mov     rbp, rsp
        push    rbx
        push    rax
        mov     rbx, qword ptr [rdi]
        test    rbx, rbx
        je      LBB5_1
        mov     rdi, rbx
        call    _swift_unknownObjectRetain
        jmp     LBB5_3
LBB5_1:
        xor     ebx, ebx
LBB5_3:
        mov     rax, rbx
        add     rsp, 8
        pop     rbx
        pop     rbp
        ret

AutoreleasingUnsafeMutablePointer isn't particularly performance sensitive, though.

…parent This improves codegen in debug builds.

… than UnsafeRawPointer This improves code generation in -Onone mode. (UnsafeRawPointer.load/.storeBytes isn’t @_transparent and it doesn’t get specialized in unoptimized builds.)

lorentey · 2019-08-22T01:49:03Z

The latest commits get rid of unspecialized generic calls (to UnsafeRawPointer.load/.storeBytes and Unmanaged.takeUnretainedValue) in -Onone builds.

lorentey · 2019-08-22T01:50:16Z

(And they also use my new favorite type, UnsafePointer<Optional<Unmanaged<AnyObject>>>.)

atrick · 2019-08-22T21:22:21Z

@lorentey thanks for figuring out the semantics and documenting it. Just to limit the number of times unsafeBitCast appears in the code--since each occurrence is generally considered a "bug"--you can use _unsafeReferenceCast as long as the values are dynamically Optional<AnyObject>.

…eCast for extra validation

lorentey · 2019-08-22T21:36:35Z

@swift-ci test

lorentey · 2019-08-27T03:10:41Z

@swift-ci test

lorentey · 2019-08-27T04:44:41Z

The latest commit adds some rudimentary high-level functional tests for AutoreleasingUnsafePointer.

I think this is ready to land now.

lorentey requested review from gottesmm and Catfish-Man August 21, 2019 03:20

lorentey requested a review from atrick August 21, 2019 04:03

Catfish-Man approved these changes Aug 21, 2019

View reviewed changes

lorentey added 3 commits August 21, 2019 18:25

[stdlib] AutoreleasingUnsafeMutablePointer: update docs

61c5ac1

[stdlib] Make Unmanaged.takeUnretainedValue, .takeRetainedValue trans…

b9e3ada

…parent This improves codegen in debug builds.

[stdlib] AutoreleasingUnsafeMutablePointer: use typed pointers rather…

88630e8

… than UnsafeRawPointer This improves code generation in -Onone mode. (UnsafeRawPointer.load/.storeBytes isn’t @_transparent and it doesn’t get specialized in unoptimized builds.)

[stdlib] AutoreleasingMutableUnsafePointer: switch to _unsafeReferenc…

b8dc0f4

…eCast for extra validation

This comment has been minimized.

Sign in to view

[test] Add some high-level tests for AutoreleasingUnsafeMutablePointer

d5ea371

This comment has been minimized.

Sign in to view

lorentey merged commit 895f136 into swiftlang:master Aug 27, 2019

lorentey deleted the ♪-hold-me-use-me-autorelease-me-♬ branch August 27, 2019 17:22

lorentey mentioned this pull request Sep 24, 2019

Add an Array-based NSMutableArray subclass #27341

Merged

johnno1962 mentioned this pull request Apr 18, 2020

[stdlib] Fix for objc_copyClassList broken since snapshot 2019-08-27 #31128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[stdlib] AutoreleasingUnsafeMutablePointer: eliminate questionable pointer use #26760

[stdlib] AutoreleasingUnsafeMutablePointer: eliminate questionable pointer use #26760

Uh oh!

lorentey commented Aug 21, 2019 •

edited

Loading

Uh oh!

lorentey commented Aug 21, 2019

Uh oh!

lorentey commented Aug 21, 2019

Uh oh!

swift-ci commented Aug 21, 2019

Uh oh!

Catfish-Man Aug 21, 2019

Uh oh!

lorentey Aug 21, 2019 •

edited

Loading

Uh oh!

lorentey commented Aug 22, 2019

Uh oh!

lorentey commented Aug 22, 2019

Uh oh!

lorentey commented Aug 22, 2019

Uh oh!

atrick commented Aug 22, 2019

Uh oh!

lorentey commented Aug 22, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

lorentey commented Aug 27, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

lorentey commented Aug 27, 2019

Uh oh!

Uh oh!

[stdlib] AutoreleasingUnsafeMutablePointer: eliminate questionable pointer use #26760

[stdlib] AutoreleasingUnsafeMutablePointer: eliminate questionable pointer use #26760

Uh oh!

Conversation

lorentey commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lorentey commented Aug 21, 2019

Uh oh!

lorentey commented Aug 21, 2019

Uh oh!

swift-ci commented Aug 21, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

Catfish-Man Aug 21, 2019

Choose a reason for hiding this comment

Uh oh!

lorentey Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorentey commented Aug 22, 2019

Uh oh!

lorentey commented Aug 22, 2019

Uh oh!

lorentey commented Aug 22, 2019

Uh oh!

atrick commented Aug 22, 2019

Uh oh!

lorentey commented Aug 22, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

lorentey commented Aug 27, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

lorentey commented Aug 27, 2019

Uh oh!

Uh oh!

lorentey commented Aug 21, 2019 •

edited

Loading

lorentey Aug 21, 2019 •

edited

Loading