Builtin.isbitwisetakable

[weissi]

what am I trying to achieve

The end goal of this PR and a bunch of follow-ups is to implement support for reallocating Swift objects (to be used for tail-allocated storage) after confirming with a Swift builtin that the type is 'bit-wise takable'. See also the Swift Forums discussion swift_tailRealloc.

what is PR doing?

This pull request only adds Builtin.isbitwisetakable.

state of this PR

This is probably incomplete. This is my first more substantial Swift contribution and I only got here with a lot of @gottesmm's help, in fact most of this was written together with @gottesmm.

open questions/todo

• when used, this needs to be used in conjunction with isKnownUniquelyReferenced. Unfortunately, AFAIK isKnownUniquelyReferenced returns true for 1 strong + 1 weak reference. And having a weak reference to the buffer would be really bad with realloc used. Can we change isKnownUniquelyReferenced to return false as soon as there is any reference (including weaks)? As @atrick suggested I filed a bug fixing this.
• I should probably add the new builtin to SIL.rst?
• _isBitwiseTakable((() -> Void).self) is true which doesn't make sense to me

[atrick]

AFAIK isKnownUniquelyReferenced returns true for 1 strong + 1 weak reference. And having a weak reference to the buffer would be really bad with realloc used. Can we change isKnownUniquelyReferenced to return false as soon as there is any reference (including weaks)?

Correct. I believe the current isKnownUniquelyReferenced behavior is a bug. There was some unresolved debate about this a while ago. Anyone checking for a weak reference needs to obtain a strong reference first, but that check could occur in between a separate isKnownUniquelyReferenced check and the object mutation.

Even if it isn't a bug now, it certainly wouldn't hurt to make isKnownUniquelyReferenced return false if the object has weak or unowned references, other than making the runtime call a bit more expensive.

Could you file a bug against the Swift runtime and see what @gparker42 and @mikeash have to say about it?

[mikeash]

The typical pattern is that you have a struct presenting the public API, which has a property pointing to a class implementing the guts. You use isKnownUniquelyReferenced on the class to determine if it's safe to mutate in place. You'd never end up with a weak or unowned reference to that private object, so isKnownUniquelyReferenced can safely ignore them. I think the intent is that isKnownUniquelyReferenced is only for use in circumstances like this, and that it's not a general-purpose way to find out if anyone anywhere might have any kind of reference to it.

Of course, as @atrick says, it wouldn't really hurt to fix this. I'm just not sure how useful it is.

Does this change have any different requirements? It seems like it's still in the same general case as copy-on-write types in general, where you wouldn't have any weak or unowned references to the objects that this would be used for.

[aschwaighofer]

Can you also add runtime tests to test/stdlib/Builtins.swift?

Like the "_isPOD" test in that file.

[jrose-apple]

The isKnownUniquelyReferenced issue is SR-5633.

[jrose-apple]

Whoops, no, that's yet another issue. Hm.

[atrick]

@mikeash I think we're in the same undefined behavior territory regardless of whether the weakly referenced object is mutable collection storage or realloc'd collection storage. This is generally the scenario I'm concerned about:

if (isKnownUniquelyReferenced(&uniqueStrongRef)) {
  //...
  anotherStrongRef = weakRef
  //...
  uniqueStrongRef = new Buffer(...)
  //...
}

anotherStrongRef[i] = ...

I agree that for private storage classes, we can avoid the issue.

[atrick]

In the bug @jrose-apple pointed to, @lorentey points out that the current behavior is actually useful, contradicting what I said in my previous comment:
https://bugs.swift.org/browse/SR-5633

So, we should introduce a new runtime call that checks all refcounts and let the user decide which to use. The existing runtime call is sufficient for the stdlib (although stdlib authors will need to be very careful about thread safety!).

@weissi Do you have an example of the weak reference bug that we can use to motivate a change? Could you add your use case to the motivation in SR-5633?

I suspect that you can go ahead an use the existing isKnownUniquelyReferenced and comment that weak references aren't allowed on your buffer storage?

[weissi]

@atrick 1) thanks, I filed a bug, in fact I did that before seeing a bug
2) regarding the weak example: I'm still worried that something comes back as uniquely referenced, then we realloc it, it fails to resize the buffer so allocates a new one, frees the old one and now the weak reference is dangling which is bad.
3) very happy to go ahead with adding the comment that weak references aren't allowed. That's for the next PR that adds realloc functionality to ManagedBuffer, right?

@aschwaighofer thanks, added the test

all: I'm seeing that _isBitwiseTakable( (() -> Void).self ) == true but are really all () -> Void's bitwise takable? I doubt so.

[aschwaighofer]

I'm seeing that _isBitwiseTakable( (() -> Void).self ) == true but are really all () -> Void's bitwise takable? I doubt so.

Yes they are. The context of a closure is a reference to the context object. A reference is bitwise takable.

[weissi]

I'm seeing that _isBitwiseTakable( (() -> Void).self ) == true but are really all () -> Void's bitwise takable? I doubt so.

Yes they are. The context of a closure is a reference to the context object. A reference is bitwise takable.

Oh of course, thank you, I was one level of indirection off and thought about the stuff that could be captured 🙈

[weissi]

@aschwaighofer / @atrick / @gottesmm wondered what the next steps necessary are to move this along. Should I add docs to SIL.rst?

[aschwaighofer]

@swift-ci Please test

[aschwaighofer]

LGTM

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - c31ef9228ec8885877c472997062d6afd7e0e96e

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - c31ef9228ec8885877c472997062d6afd7e0e96e

[weissi]

fixed the merge conflict and will kick off tests again. @aschwaighofer can we merge after they go green?

[weissi]

@swift-ci please test