ByteBuffer: take ownership / relinquish storage

[weissi]

WRONG, INCOMPLETE, AND UNTESTED

This is literally just the description of an idea really. If we want it, I can make it nice, if we don't that's cool too.

The reason to bring it up now is because this would stop working if we switched to tail-allocated storage.

---

Motivation:

Maybe it'd be nice if we could take ownership & relinquish the storage
pretty cheaply.

Modification:

Allow ByteBuffer to take ownership of foreign storage & relinquish its
storage.

Result:

Better performance in certain special scenarios?

[weissi]

@Lukasa wdyt of this idea?

[Lukasa]

I don't think this is really worth doing: to my eye it seems like it's solving a problem we don't really have.

[weissi]

I don't think this is really worth doing: to my eye it seems like it's solving a problem we don't really have.

Probably agreed (which is also why we've never done it before). Only real thing I can think of is speeding up the send path for TS because we could (Dispatch)Data-ise a ByteBuffer for free more often.

[Lukasa]

We can do that anyway, I think, by just using the existing ByteBuffer constructors.

[weissi]

We can do that anyway, I think, by just using the existing ByteBuffer constructors.

No. What we can do is to give Data access to our storage by retaining it at the beginning and releasing it in Data's destructor. But unfortunately, that's Data's slow path and we need to allocate for the closure which is the destructor. So handing something to Data actually allocates... With this PR we could donate the storage to Data without allocs.

[Lukasa]

We can also do that any way using withVeryUnsafeBytesWithStorageManagement.

[weissi]

We can also do that any way using withVeryUnsafeBytesWithStorageManagement.

That's what we do today (if more than X bytes). But it will allocate because you need to give Data hold of the storage management token in a closure. That closure will then allocate.

        return self.withUnsafeReadableBytesWithStorageManagement { ptr, storageRef in
            if doCopy {
                return Data(bytes: UnsafeMutableRawPointer(mutating: ptr.baseAddress!.advanced(by: index)),
                            count: Int(length))
            } else {
                _ = storageRef.retain()
                return Data(bytesNoCopy: UnsafeMutableRawPointer(mutating: ptr.baseAddress!.advanced(by: index)),
                            count: Int(length),
                            deallocator: .custom { _, _ in storageRef.release() })
                                              /// ^^^^ alloc here for that closure because it captures the `storageRef`
            }
        }

Swift could be smarter and use an Unmanaged<AnyObject> as the closure context pointer (because it fits in one word) but it doesn't today.

[Lukasa]

Swift could be smarter and use an Unmanaged as the closure context pointer (because it fits in one word) but it doesn't today.

What if instead of using the explicit Unmanaged<AnyObject> we just stuffed ByteBuffer._Storage as AnyObject into the closure, and used withExtendedLifetime? That is:

        return self.withUnsafeReadableBytesWithOwner { (ptr: UnsafeRawBufferPointer, owner: AnyObject) in
            if doCopy {
                return Data(bytes: UnsafeMutableRawPointer(mutating: ptr.baseAddress!.advanced(by: index)),
                            count: Int(length))
            } else {
                _ = storageRef.retain()
                return Data(bytesNoCopy: UnsafeMutableRawPointer(mutating: ptr.baseAddress!.advanced(by: index)),
                            count: Int(length),
                            deallocator: .custom { _, _ in withExtendedLifetime(owner) })
            }
        }

That might not capture, right?

[weissi]

That might not capture, right?

Yes. We could possibly even do it today, the Unmanaged<AnyObject> lets us get hold of the AnyObject which we can then retain withExtendedLifetime(...). And then we may be able to piggyback on existing compiler smartness.

[Lukasa]

We should confirm that this does in fact not allocate though.

[weissi]

We should confirm that this does in fact not allocate though.

--- a/Sources/NIOFoundationCompat/ByteBuffer-foundation.swift
+++ b/Sources/NIOFoundationCompat/ByteBuffer-foundation.swift
@@ -126,10 +126,10 @@ extension ByteBuffer {
                 return Data(bytes: UnsafeMutableRawPointer(mutating: ptr.baseAddress!.advanced(by: index)),
                             count: Int(length))
             } else {
-                _ = storageRef.retain()
+                let storage = storageRef.takeUnretainedValue()
                 return Data(bytesNoCopy: UnsafeMutableRawPointer(mutating: ptr.baseAddress!.advanced(by: index)),
                             count: Int(length),
-                            deallocator: .custom { _, _ in storageRef.release() })
+                            deallocator: .custom { _, _ in withExtendedLifetime(storage) {} })
             }
         }
     }

should be all we need.

Yes, we do have an alloc test for that already I think.