swift_tailRealloc

yes I would be fine with that. The best part is we wrote together most of that builtin already!

@Andrew_Trick any further concerns?

I don’t have any concerns with that approach. For users who want to call the platform's realloc it seems perfectly natural to write a stdlib wrapper that actually calls realloc. We can always wrap it in some SIL construct later if we want to do something more sophisticated.

indeed :slight_smile:. I'll get a patch going tomorrow.

Ok, it wasn't quite 'tomorrow' but here we go. That's my first proposal for Builtin.isbitwisetakable.

Ok, here we got with part 2 of this work. This code now works (with a slightly adapted TestManagedBuffer from test/stdlib/ManagedBuffer.swift):

var tmb = TestManagedBuffer<Int>.create(10)
let oldCapacity = tmb.capacity
let newCapacity = oldCapacity * 2
ManagedBuffer.reallocate(buffer: &tmb,
                         count: tmb.count,
                         newMinimumCapacity: newCapacity)

But as I mention in the PR, there's still a few open question (besides the need for more tests/docs).

  1. Is there a better way to change the value of inout Buffer to the new instance? I started off using Unmanaged but that didn't work because assigning a new value would decrement the reference count of the old (currently assigned) object but that has already been free'd by realloc. @Mike_Ash then proposed to drop down to UnsafePointer land which worked a lot better. But unfortunately I was only able to reach the right pointer using an unsafeBitCast which feels like quite undefined behaviour in the way I use it.
  2. The API is still unclear (CC @Ben_Cohen, @lorentey, @Michael_Ilseman & @jrose here) for that too. Mike proposed to add a more higher-level API which also deals with things that are not uniquely referenced and just does the copy then which I think is a good idea, I'll add that as soon as we have a good low-level API. What are people thinking about the current low-level one?
public extension ManagedBuffer {
  @inline(never)
  final class func reallocate<Buffer: ManagedBuffer>(
    buffer: inout Buffer,
    count: Int,
    newMinimumCapacity: Int
  )

As far as (1) goes, I think withMemoryRebound will probably do what you want, though I always have to check with @Andrew_Trick to be sure. (We're going from a pointer-to-owned-object-reference to a pointer-to-pointer.)

I do notice you don't actually use count in the function, so you can at least drop that from the low-level entry point. I can't really think of a better API otherwise. This is a low-level operation, and it can't possibly do any checking because it doesn't have information about the subclass.

The one part that's wrong is the introduction of a new generic parameter; that means that Element and Buffer.Element could be different. I see that without it, though, we have no way to promise that you get the same subclass back (because we don't allow you to use Self in that position). That probably argues for the low-level interface being a free function today:

@inline(never)
public func reallocateUniquelyReferenced<Buffer: ManagedBuffer>(
  buffer: inout Buffer,
  newMinimumCapacity: Int
)

You can get around this by making a dummy protocol, but that's kind of unfortunate too because now that's in the ABI, and someone else could try to conform to it:

public protocol ManagedBufferReallocation: AnyObject {}
extension ManagedBufferReallocation where Self: ManagedBuffer {
  public static func reallocateUniquelyReferenced(
    buffer: inout Self,
    newMinimumCapacity: Int
  ) {
    // …
  }
}
extension ManagedBuffer: ManagedBufferReallocation {}

What you actually want is this:

extension ManagedBuffer {
  mutating func reallocateUniquelyReferenced(newMinimumCapacity: Int) {
    // …
  }
}

but we don't allow mutating functions to be defined directly on classes, because they usually don't really make sense if the class is not uniquely referenced.

(I've been using reallocateUniquelyReferenced here; another name that might make sense is reallocateInPlace, even though it's not always going to actually be the same memory location.)

1 Like

Hi Jordan,

Thanks very much, that's very helpful!

Hmm, my problem is getting to the pointer not changing its type if that makes sense. I need to rewrite the pointer value of the inout Buffer itself. Pretty much what buffer = newBuffer would do on an buffer: inout Buffer but without all the ref counting.

Good catch, thank you! Removed.

Thanks! That was all really useful. Apart from the unsupported mutating func on extension ManagedBuffer which I really like, my second preference is the free function so I went with that for now.

I updated the PR with all that, let me know what you think.

Here's the withMemoryRebound part spelled out:

withUnsafeMutablePointer(to: &buffer) {
  $0.withMemoryRebound(to: UnsafeMutableRawPointer.self, capacity: 1) {
    $0.pointee = _stdlib_realloc($0.pointee, newSizeInBytes)
  }
}

This is also making me wonder if the interface to _stdlib_realloc is wrong. Maybe it should be _stdlib_realloc($0, newSizeInBytes), with no return value. Then again, realloc is written that way so that you can handle the failure case; if we ever want to do that here, too, we'll need that flexibility.

Ok, apologies, that is much nicer and does work!

I'm happy changing the way _stdlib_realloc works as you suggested (no return value). We could also have it return if it was successful in changing the allocation in place or not. I just went for exactly what libc's realloc looks like without thinking much about it.

Making a patch to replace the unsafeBitCast right now...

1 Like

@Mike_Ash / @Andrew_Trick I've updated the PR which doesn't actually look terrible anymore thanks due @jrose. Would you mind having a look to see if that's basically what's needed to get this in? If yes, I'd write a few unit tests using that function as the final thing.
The higher-level APIs I'd propose as a follow-up PR, good?

Sorry about not getting around to reviewing earlier. This all looks great to me. The one thing that isn't obvious to me is why you needed a SIL instruction for HasSideTable instead of using a builtin, like "isBitwiseTakable".

Regarding the pointer casting problem, since you can't use Unmanaged, I don't see any better solution for avoiding refcounts than Jordan's withUnsafePointer/withMemoryRebound trick.

No worries and thanks for having a look! I’m working on a change to make it a runtime call swift_hasSideTable instead (through Builtin.hasSideTable). I followed how isUnique and friends work and they are instructions so I went for the same :slight_smile: .
isBitwiseTakable is slightly different because it’s evaluated at compile time because it depends on the type not the value right?. Don’t think it’ll be long, it’s already compiling but currently having an LLVMValue of the wrong type which makes it hit an unreachable... will hopefully fix that tomorrow

I think you just want to call directly into the runtime here, so you could just add the declaration of _swift_hasSideTable to SwiftShims/HeapObject.h. Then you don't even need to define a builtin.

@Joe_Groff Any thought on this?

The reason for a builtin I think would be if we want the compiler to recognize and lower it specially.

isUnique is special because SIL passes need to reason about it's semantics, and those semantics are slightly different than the builtin/runtime call.

I agree, calling into the runtime is the best option. I don't think the compiler has any special knowledge of when things have side tables, and moreover, it really shouldn't—the side table should be a private detail of the reference counting implementation, and we shouldn't export evidence of its existence from the runtime. We should just make it a Swift-callable function and @_silgen_name it into the standard library, IMO.

Ah, I see. You can just add SWIFT_CC(swift) SWIFT_RUNTIME_STDLIB_SPI to your definition in SwiftObject.mm, remove the exported declaration from HeapObject.h, then add an internal stdlib declaration in Builtin.swift using @_silgen_name("_swift_hasSideTable").

I suppose that way it's not part of the runtime's ABI and also might potentially use a better calling convention.

Why do we need hasSideTable at all? We can add this check in the _swift_stdlib_realloc.

1 Like

Thanks @Andrew_Trick & @Joe_Groff, calling into the runtime was a much better plan! Could delete most of my code and it still works :slight_smile:.

@Erik_Eckstein well, in the future I would like to support side-tables too so I guess reallocateUniquelyReferenced is a good place, no?

I'm jumping into this late, so pardon my ignorance, but why is the reference counting side table relevant to what you're trying to do? Shouldn't the "is uniquely referenced" check encapsulate that? The reference counting format is intended to be an implementation detail of the runtime.

I agree with Joe, we shouldn't expose the side tables.
We can handle side tables in _swift_stdlib_realloc if we want to.