Add _modify accessor to Atomic.value

[p4checo]

Motivation

With Swift 5.0 the new _modify accessor became available, which allows a more efficient access to underlying storage when an in place mutation is made (i.e. via inout, like value += 1), by yielding
a reference to the storage itself, instead of performing a copy (get) followed by a write (set).

This is particularly useful in containers where memory operations can become expensive (like collections), but also in containers wrapping expensive operations (like locking/unlocking locks).

Such is the case of Atomic, where besides optimizing memory access to the underlying storage, it can save a lock/unlock when the .value is modified in place (i.e. via inout). In this particular case, one can even claim that it’s more correct, because the whole mutation is truly made in a single atomic operation instead of two (as is already the case in the modify(_:) API).

Changes

• Add _modify accessor to Atomic.value

Checklist

• Updated CHANGELOG.md.

[p4checo]

While I was making this change, I tried to do the same on PropertyBox.value and PropertyStorage.value, but the compiler segfaults with code 11 💥. I believe that it's because those types are private and internal respectively, and the compiler somehow can't yet handle those "visibility" limitations. Since documentation on this is very limited, I'll look it up and possibly open a bug report.

I did however manage to implement _modify on MutableProperty.value (as it's public API), but had to open up PropertyBox.lock's access level to fileprivate(which I didn't particularly enjoy 😅), as it was the only way I found to preserve correctness given the observer.send(value:) and box.isModifying side effects. Since I'm not sure if you agree with that approach, I didn't open a PR with it. Will gladly do so if you agree.

Essentially, it would end up like this:

public var value: Value {
	get { return box.value }
	set { modify { $0 = newValue } }
	_modify {
		box.lock.lock()
		defer { box.lock.unlock() }

		guard !box.isModifying else { fatalError("Nested modifications violate exclusivity of access.") }
		box.isModifying = true
		defer { box.isModifying = false }

		defer { observer.send(value: box._value) }
		yield &box._value
	}
}

[leonid-s-usov]

I was trying to find info on the _modify accessor, but the official Swift 5 documentation is not saying anything about it.
I had also no luck finding anything about this modifier on the Apple documentation

Would you mind posting a link to some official documentation about this accessor?

[p4checo]

Unfortunately, I think we're a bit out of luck in regards to official documentation, at least for now 😆

From what I've gathered, this is one of two new generalized accessors (the other one is _read) that has been unofficially introduced in Swift 5.0 here. From the PR's description, a proposal should eventually be made to formalize their introduction (and possibly rename them, hence the _).

I can however share some more useful links about this new _modify accessor:

• Generalized accessors information on the Swift docs' Ownership Manifesto
• Ben Cohen‘s Functional Swift talk, Fast Safe Mutable State.
• stdlib's Array's _modify usage
• stdlib's Dictionary's _modify usage
• Nice StackOverflow answer explaining the Dictionary access semantics evolution

Hope this helps! 🍻

[mdiep]

Thanks for the PR! I wasn't actually aware that this was a thing.

I'm not a fan of using these not-really-public-API APIs. If modify becomes a thing, then great. But the underscore is clearly meant to denote that it's not ready for broad usage.

[p4checo]

I'm pretty certain that this will go forward, as it was merged into master and is a part of the Ownership Manifesto. My guess is that they "rushed" this in probably because of ABI stability or some other high priority issue, or simply to fix the performance issues in Dictionary, for instance. However, the fact that they're already available in Swift 5 and for all matters public (despite the _), gives me a good level of confidence on this being a thing.

Some revision/renaming will most certainly happen before these are truly "official", but I want to believe the compiler will throw new custom errors and possibly contain fix-its guiding developers to the new accessor name(s) when that happens, because the usage of these new accessors is already spreading in the community.

All things considered, I still think your point is fair enough, even though it's not the outcome I would've hoped for 😄. That being said, if you and the rest of RS's core team agree that it's too soon to add this, feel free to close the PR.

Cheers!

[leonid-s-usov]

public var value: Value {
	get { return box.value }
	set { modify { $0 = newValue } }
	_modify {
		box.lock.lock()
		defer { box.lock.unlock() }

		guard !box.isModifying else { fatalError("Nested modifications violate exclusivity of access.") }
		box.isModifying = true
		defer { box.isModifying = false }

		defer { observer.send(value: box._value) }
		yield &box._value
	}
}

shouldn't the observer.send happen after the yield?

[p4checo]

shouldn't the observer.send happen after the yield?

The defer accomplishes that, although in this case it's a bit silly to use a defer for the observer.send as it's already the last statement before the yield. So yes, it could be after the yield.

Guess I got carried away copy-pasting modify's internals and forgot to clean up. 😅 Well spotted!

[leonid-s-usov]

Right, I haven't realized that it's inside the defer :/ Well that's a convoluted way to maintain instruction order, anyway. Also, had to double check that swift actually guarantees to execute defers in reverse order cause the send must happen before the lock is released.

Anyway, this modify accessor is a neat trick, but unless there are evidently bad performance cases around this particular point in ReactiveSwift, I also think that we should at least wait for the official release of the language feature.

[andersio]

Imagine this basic data race example:

@Atomic var tweets: [Tweet] = [Tweet(id: "tweet1")]

// Thread 0
if let index =  tweets.index(where: { $0.id == "tweet1" }) {
    tweets.remove(at: index)
}

// Thread 1
tweets.append(Tweet(id: "tweet0"))

While using the _modify coroutine accessor does help improving performance of the CoW mutating expressions, it does not help improve or serve as a safeguard of the correctness of the program, as the scope requiring mutual exclusion is beyond one single (mutating or not) expression. This is partly why we provide Atomic.modify and Atomic.withValue from the beginning.

I won't dispute the utility value of it for simple use cases, e.g. a head-tail queue. But it is not as great to the magnitude that we should include it even before it being formalised as part of the language.

[p4checo]

Maybe I wasn't clear on the motivation behind this PR, but this change isn't meant to replace Atomic.modify or Atomic.withValue.

The example you show is clearly not solved by this change, nor is it the goal of this change to solve it. The critical section on Thread 0 being composed of 2 operations (index(where:) and remove(at:)) on the shared state requires the locking to include them both, which makes Atomic.modify the correct helper in this case.

From the snippet you shared it seems (and please correct me if I'm wrong 😅) that you are trying to make a property wrapper that is backed by an Atomic<T>, and somehow expected that _modify could help you avoid using Atomic.modify in "compound operations" like the ones in Thread 0. Sadly, it can't AFAIK because the access semantics for _modify to be used by the compiler are more specific.

As a consequence of being more specific, the utility is also more "limited", and most likely won't be used in most real use cases. However, there are some use cases where it can be used and in these cases it brings clear performance and most importantly correctness benefits when mutating the underlying storage. Performance by saving one unnecessary lock/unlock (get + set vs _modify), and correctness by not allowing any change between the get and the set, which is currently possible when mutating the underlying storage directly (i.e. via .value).

I understand your concerns about this still not being "final" API (as was already mentioned before in the thread), and I am conscious that it's "usefulness" or "magnitude" is not very significant for most cases. I opened this PR because it's a simple change that brings a bit of performance and correctness for some use cases, that's all.

That being said, I'm not quite sure about the motivation behind your comment, in the sense that IMO nothing new was brought to the discussion. I will gladly close the PR if you want, or feel free to close it if you don't see any value at this point. 🍻

[andersio]

I guess the main point I am trying to get across is that:

But it (the benefit) is not as great to the magnitude that we should include it even before it being formalised as part of the language

I do reckon that there are plenty of use cases, and personally encountered plenty at work which I wish to have this. But as a public package, we've also been trying to avoid the use of compiler private features, given the stability and support are not guaranteed. For instance, the Apple-led, performance-critical SwiftNIO has made a decision not to do so on a similar ground. So in this regard, I do hesitate to accept this.

[p4checo]

I understand, @andersio. Thanks for your feedback.

Do you think I should close this PR, or wait for these accessors to be formally added to Swift and then update it?

[RuiAAPeres]

Closing this PR for now. @p4checo thank you for the contribution.