Property Wrapper causing excessive array copying

Using Swift
#1

I have a property wrapper around an array property like this:

struct StateSurface {
	var info: String = ""
}

struct State {
	@Archive
	var surfaces: [StateSurface] = []
}

My Archive wrapper was pretty complicated, but in the interest of trying to figure out what was going on, it is now only this:

@propertyWrapper
public struct Archive<ValueType> {
	public init(wrappedValue initialValue: ValueType) {
		wrappedValue = initialValue
	}
	
	public var wrappedValue: ValueType
}

What I'm seeing is that when I change values in the surfaces property, the entire array is being allocated and copied every time. If I remove the property wrapper, this doesn't happen. The behavior is the same in both debug and release builds. Using the following code will trigger it. Simply commenting out the @Archive wrapper causes this code to run nearly instantly:

var state = State()
state.surfaces = Array(repeating: StateSurface(), count: 20000)
		
for i in 0..<20000 {
	state.surfaces[i].info = String(i)
}

My guess is that there is some kind of in-place mutating optimization that is being missed when going through the wrapper and confusing the copy-on-write built into Array. I have no idea if this can be avoided, but it is totally killing my performance. :frowning:

4 Likes
#2

I can simplify the test case to the following:

@propertyWrapper
struct TestWrapper<ValueType> {
	var wrappedValue: ValueType
}
		
struct State {
	@TestWrapper
	var values: [String] = []
}

var state = State()
state.values = Array(repeating: "", count: 20000)
		
for i in 0..<20000 {
	state.values[i] = String(i)
}

Running this code is slow as it copies the array 20,000 times or something.

If I change the code to this (note the missing property wrapper and instead using the very same structure directly), the loop is just as fast as not using the wrapper at all:

@propertyWrapper
struct TestWrapper<ValueType> {
	var wrappedValue: ValueType
}
		
struct State {
	var values: TestWrapper<[String]> = TestWrapper(wrappedValue: [])
}

var state = State()
state.values.wrappedValue = Array(repeating: "", count: 20000)
		
for i in 0..<20000 {
	state.values.wrappedValue[i] = String(i)
}

I don't know what (if anything) I can do about this.

#3

I‘d say this is expected behavior, but I could be wrong. I don‘t know if property wrappers would support this but can you make wrappedValue a computed property and instead of get/set use _read/_modify and yield or whatever those modifiers are. Keep in mind that these are not officially public yet.

Here is a video for reference from @Ben_Cohen :

1 Like
#4

Interesting. I just tried it and it is seemingly allowed, but unfortunately it made no difference:

@propertyWrapper
struct TestWrapper<ValueType> {
	init(wrappedValue initialValue: ValueType) {
		actualValue = initialValue
	}
	
	private var actualValue: ValueType
	
	var wrappedValue: ValueType {
		get { actualValue }
		_modify { yield &actualValue }
	}
}
#5

For completeness, also using _read on the wrappedValue getter doesn't change the performance either.

#6

I ended up spending my Sunday totally rewriting my project to not use property wrappers at all because of this problem. :frowning:

I don't know if this would be considered a bug or not, but I filed it anyway: https://bugs.swift.org/browse/SR-11762

1 Like
#7

_read and _modify would have to be used at the use site, not inside the definition of the wrapper type.

I think this is what's going on:

When you use a property wrapper, the compiler rewrites the declaration into a pair of a backing storage property of the wrapper type and a computed property of the wrapped type.

In other words, the compiler turns this:

@TestWrapper var values: [String] = []

into this:

var _values: TestWrapper<[String]> = TestWrapper(wrappedValue: [])
var values: [String] {
    get { _values.wrappedValue }
    set { _values.wrappedValue = newValue }
}

This apparently defeats the copy-on-write mechanism because the computed setter works on a copy of the array.

I tried rewriting this code with _read and _modify:

struct State {
    var _values: TestWrapper<[String]> = TestWrapper(wrappedValue: [])
    var values: [String] {
        _read { yield _values.wrappedValue }
        _modify { yield &_values.wrappedValue }
    }
}

This is as fast in my brief test as the fast version you posted.

Whether the synthesized code could use _read and _modify instead of get and set, I don't know.

I believe the relevant code where the compiler synthesizes the getter and setter for a property wrapper is synthesizePropertyWrapperGetterBody and synthesizePropertyWrapperSetterBody in TypeCheckStorage.cpp.

Complete code sample

Edit: Here's a full version of the modified code:

@propertyWrapper
struct TestWrapper<ValueType> {
    var wrappedValue: ValueType
}
		
struct State {
    var _values: TestWrapper<[String]> = TestWrapper(wrappedValue: [])
    var values: [String] {
        // get { _values.wrappedValue }
        // set { _values.wrappedValue = newValue }
        _read { yield _values.wrappedValue }
        _modify { yield &_values.wrappedValue }
    }
}

var state = State()
state.values = Array(repeating: "", count: 20000)

for i in 0..<20000 {
    state.values[i] = String(i)
}
7 Likes
#8

For context, here's an earlier thread about the same underlying problem: How does Swift COW manage to avoid a dictionary copy in this simple computed property example?.

Quoting @John_McCall (post):

Yes [confirming the issue]. Eliminating that overhead is a large part of the purpose of _read and _modify . I would also like to make it possible to conveniently declare storage as an alias for some other piece of storage, which would amount to defining _read and _modify automatically, but that's separable.

1 Like
#9

It would be very straightforward (and would not require Evolution approval) for the compiler to synthesize a _modify for the synthesized property. Arguably the compiler should have the knowledge to treat the synthesized property as an alias to the property of the wrapper, and thus to emit code to directly access the wrapper, but that'd take a little more compiler hacking.

16 Likes
#10

Thanks @ole and @John_McCall for confirming that it's likely something that should be considered a bug - so I'm glad I filed it. Fixing the compiler is a bit outside of my wheelhouse, so hopefully someone else can do that. :slight_smile: I would think in any property wrapper-heavy code, this would make a surprising performance difference (even in SwiftUI, perhaps?).

5 Likes
#11

I can probably take a look at doing that

12 Likes
#12

That was my initial idea all about, I wanted the author to find out if property wrappers also do synthesize new (not officially public) accessors or not!

This should apply to wrappedValue and projectedValue + probably wrapper nesting as well.

#13

While the issue isn‘t fixed you could write the property wrapper code that otherwise the compiler would create manually. Later you can just delete the boilerplate and it should work as expected. :)

#14

Okay, it works. I have tweaked the synthesis of the coroutines so it yields the wrappedValue in case we have a property wrapper and the code now runs as fast as it did without using a wrapper.

I also had to change the property's ReadWriteImplKind to Modify. Does this approach look okay @John_McCall?

(oops, just realised I posted in the wrong thread, I should've posted here. Sorry)

1 Like
#15

If ReadWriteImplKind wasn't Modify, that's the main problem.

2 Likes
#16

Opened a PR here: https://github.com/apple/swift/pull/28216

6 Likes
#17

So great to see a fix for this already! Does everything still work when the property wrapper is non-trivial and the wrappedValue is a getter/setter pair instead? (Or, I guess, the wrappedValue property would probably need to be a _read/_modify pair to really work properly in this regard.)

#18

If you manually add a get & set to the wrappedValue then you also need to add _modify as well.

1 Like
#19

Ok, makes sense. Thanks for your work on this.

1 Like
#20

This has now been resolved on master: https://github.com/apple/swift/pull/28216

5 Likes
Terms of Service

Privacy Policy

Cookie Policy