[swift-dev] Thread safety of weak properties

[Mike Ash]

On Dec 12, 2015, at 3:29 AM, Kevin Ballard via swift-dev <swift-dev at swift.org> wrote:
> 
> On Sat, Dec 12, 2015, at 12:01 AM, Kevin Ballard wrote:
>> Another possible fix is to just atomically load/store the Value pointer itself (assuming all platforms Swift runs on supports lock-free atomic pointers). This way if the object is deallocating, it would attempt an atomic CAS on the pointer to null it out, and only release the object if it wasn't already null. This means all writes to weak pointers now require an atomic write, but the benefit over the activity count / spinlock is reads don't have to perform an atomic write while the object is still alive, only an atomic read (although I'm not sure offhand if there's any practical performance difference there). And with this approach, it now becomes easy to make read/write and write/write safe. Whether this approach is worth doing depends on how many weak writes there are compared to weak reads (and offhand I don't have any idea there, except I do know I see an awful lot of code that looks like `weakRef?.foo(); weakRef?.bar(); weakRef?.baz()`).
> 
> I take it back, this is still unsafe because the object can get free'd while another thread is in the process of checking its refcount.
> 
> At the moment I'm leaning towards the activity count idea, though I'm not sure why you need a 16-byte CAS for that.

Yes, I initially thought of the CAS idea as well, but it doesn't work, like you say.

For the activity count, I was thinking that you'd need to update it atomically along with the weak pointer. But thinking more, I believe that's unnecessary. In fact, the weak pointer doesn't even need to be updated at all.

Here's how I envision this one working. Let's say the weak pointer itself looks like `(ptr, count)`. Define a special value for `count` that means "the weak pointer has been zeroed out." 0xFFFFFFFFFFFFFFFF would be a good candidate. Then reading looks like:

read(ref):
	(obj, count) = *ref
	if count == 0xFFFFFFFFFFFFFFFF: return null
	CAS(ref->count, count, count + 1)
	if failed: restart from the top
	
	if obj.isDeallocating():
		CAS(ref->count, 1, 0xFFFFFFFFFFFFFFFF)
		if success: weakRelease(obj)
		return null
	else:
		result = tryRetain(obj)
		atomic_decrement(ref->count)
		return result

If an atomic increment is faster than a compare-and-swap loop (probably the case on x86-64?) then you could modify this to say that the top bit of count means "reference is null," then read can safely do an atomic increment and check the top bit of the result to know how to proceed.

For what it's worth, I implemented stealing the bottom bit for a spinlock yesterday and it's pretty nice and easy. I'm a bit conflicted on cutting down on concurrency but preserving weak references as a single pointer, versus improving concurrency but taking up more memory to store them. Concurrency is probably more important.

Mike