Expose VtArray zero-copy read/single-copy write

[furbytm]

Several of OpenUSD's VtArray types utilize internal copy-on-write semantics. Some operations like the non-const data() will trigger a deep array copy.

However, exposing the const-pointer via cdata() guarantees zero-copy read-only operations, bypassing copy overhead entirely.

Would it be possible to expose Overlay.cdata() overrides for some or all of the VtArray types in a similar fashion to this revision?

I would be happy to create a PR for this if this would be a value add to the project, thanks!

[maddyadams]

Sure. Can you expose this as an extension on the __Overlay.VtArray_Sequence protocol in source/SwiftOverlay/VtArrayProtocol.swift that adds public borrowing func withUnsafeBufferPointer<T, E>(_ code: (UnsafeBufferPointer<ElementType>) throws(E) -> T) throws(E) -> T? You'll probably need to add func __cdataUnsafe() -> UnsafePointer<ElementType>? and func size() -> Int as protocol requirements.

Also, it'd be good to test this with a few different instantiations of varying sizes (including a new empty VtArray and a VtArray that had some elements added and then had all of them removed).

[furbytm]

@maddyadams one more thing, though I can make a separate issue/PR for it if you would prefer...

It would be helpful to also expose VtArray's single-copy construction from a contiguous buffer, in a similar fashion to how I exposed those here.

Usage from Swift would look like:

// @{ client code begin, not SwiftUsd specific
struct MyFloat3 { var x: Float, y: Float, z: Float } // layout compatible

let view = [ MyFloat3(x: 1, y: 2, z: 3), MyFloat3(x: 4, y: 5, z: 6) ]
view.withUnsafeBufferPointer { buffer in
  guard let baseAddress = buffer.baseAddress else { return }

  // bind the client memory layout directly to the OpenUSD C++ type
  let vecs = UnsafeRawPointer(baseAddress).assumingMemoryBound(to: pxr.GfVec3f.self)
  // }@ client code end
  
  // @{ SwiftUsd code begin (Proposed API)
  // the new Overlay API call site (or however you'd best like to shape it).
  let vec3fArray: pxr.VtVec3fArray = Overlay.vtArray(vecs, buffer.count)
  
  // mock usage (one would have this logic scoped out of this closure)
  let stage = Overlay.Dereference(pxr.UsdStage.CreateInMemory())
  pxr.UsdGeomMesh.Define(Overlay.TfWeakPtr(stage), "/hello/world")
  let prim = stage.GetPrimAtPath("/hello/world")

  // the goal: Set the single-copy constructed VtArray type
  prim.GetAttribute(.UsdGeomTokens.Points).Set(vec3fArray, pxr.UsdTimeCode.Default())
  // }@ SwiftUsd code end
}

Which would be the write-side complement to VtArray::cdata()'s read-only operations.

I can edit this issue to reflect both of those additions, or I am happy to file that separately if that keeps things simpler to review/test accordingly.

[maddyadams]

I think that also makes sense to add. Your version ends up using this constructor (the one with LegacyInputIterator and std::enable_if_t), right? I haven't tried it myself, but I don't think that'll be imported into Swift because it uses dependent types (basically, the template is too complicated for Swift-Cxx interop at the moment), so it might be a bit of a challenge to get it working without having to copy-paste an implementation for each VtArray specialization.

[furbytm]

Yes, my version explicitly declares a non-templated overload for each individual VtArray specialization (VtBoolArray, VtIntArray, VtFloatArray, etc.) in the overlay header to make it visible to Swift. It is definitely a naive copy-paste maintenance burden to repeat these signatures for every underlying type.

Would it be preferable to remove that copy-paste maintenance burden by using the following C++ template (or if you have an alternative preferred shape) instead?

namespace Overlay {
    template <class T, typename ScalarType>
    inline T vtArray(const ScalarType *src, std::size_t count) {
        return count ? T(src, src + count) : T();
    }
}

No template specializations required

That template appears to compile and run cleanly with the following Swift code:

// note: the explicit type is necessary here otherwise you will get an error

// ok, explicit type resolves parameter 'T'
let vec3fArray: pxr.VtVec3fArray = Overlay.vtArray(vecs, buffer.count)

// error: Generic parameter 'T' could not be inferred
let vec3fArray = Overlay.vtArray(vecs, buffer.count)

It also correctly errors if the the vecs parameter is say, a UnsafePointer<pxr.GfVec4f> that you are attempting to pass with an explicit T parameter of pxr.VtVec3fArray (the error comes from C++ stdlib's uninitialized_algorithms.h for those kinds of mismatches).

But it would probably be a good idea to setup a good test suite for this to ensure the template shape doesn't have any weird edge cases.

[maddyadams]

I think it'd be better if an explicit type annotation wasn't required. I've been looking at VtArray and trying to see if there's a way to get this to work with "empty" protocol conformances (e.g. what I already have for VtArrayProtocol et al, where requirements are fulfilled by the Swift importer importing C++ methods), but I don't think there is. In that case, I think that something kind of like this could work (just a sketch, open to bike-shedding and other designs):

// .h
namespace __Overlay {
    // expose `assign` in a way Swift will import
    template <typename VtArray, typename Element>
    void VtArray_assign(VtArray& array, const Element* first, const Element* last) {
        static_assert(std::is_same_v<VtArray, pxr::VtArray<Element>>);
        array.assign(first, last);
    }
}

// .swift
extension __Overlay {
    // add a way for generic code to call down to `VtArray_assign `
    public protocol VtArray_Sequence: Sequence {
        ... // everything VtArray_Sequence already has
        mutating func __assign(_ p: UnsafeMutableBufferPointer<Element>)
    }
}

extension __Overlay.VtArrayProtocol {
    // Same behavior as the `template <typename LegacyInputIterator>` constructor
    public init(initializingWith p: UnsafeMutableBufferPointer<Element>) {
        self.init()
        self.__assign(p)
    }
}


// Conformances
extension pxr.VtBoolArray: __Overlay.VtArrayProtocol {
    public mutating func __assign(_ p: UnsafeMutableBufferPoint<Element>) {
        __Overlay.VtArray_assign(&self, p.baseAddress, p.baseAddress.map { $0 + p.count })
    }
}

extension pxr.VtDoubleArray: __Overlay.VtArrayProtocol {
    // Exact same "protocol conformance body" as above, generic spelling lets it be copy-pastable but still typecheck
    public mutating func __assign(_ p: UnsafeMutableBufferPoint<Element>) {
        __Overlay.VtArray_assign(&self, p.baseAddress, p.baseAddress.map { $0 + p.count })
    }
}

[furbytm]

Sketch looks right. The per-conformance __assign seems unavoidable: VtArray_assign is a function template, so it needs a concrete element type at the call site, and inside a VtArrayProtocol extension ElementType is still an associated type - there's nothing concrete for the template to specialize on until you're in a per-type conformance.

Worth deciding which way we want this to fall. init<C: Collection> already accepts UnsafeBufferPointer currently and quietly does N push_backs, so the slow path is already in the API and already reachable. A concrete UnsafeBufferPointer<ElementType> overload would win resolution and silently upgrade those call sites to assign:

extension __Overlay.VtArrayProtocol {
    public init(_ buffer: UnsafeBufferPointer<ElementType>) {
        self.init()
        self.__assign(buffer)
    }
}

A distinctly-named init(initializingWith:) instead leaves the slow route as the one you get by writing the obvious thing. I'd lean toward the overload for that reason, though the explicit name does make the assign contract legible at the call site. I haven't confirmed the concrete overload actually beats init<C: Collection> rather than going ambiguous - worth checking before we commit to it.

One small thing: is UnsafeMutableBufferPointer in the requirement intentional? assign only reads the source range, and withUnsafeBufferPointer - the natural way to get a buffer out of a Swift array - hands back the immutable UnsafeBufferPointer, so the mutable spelling would make the most common call site cast away immutability just to satisfy the signature.

[maddyadams]

public init(_ buffer: UnsafeBufferPointer<ElementType>) is a better name/call site. (I was looking for prior art for if e.g. Array had an initializer that took an UnsafeBufferPointer, but the best thing I could find was Array(unsafeUninitializedCapacity:initializingWith:) because I'd missed that UnsafeBufferPointer conforms to Collection, so init(_: UnsafeBufferPointer) works via init<S>(_:).)

I haven't confirmed the concrete overload actually beats init<C: Collection> rather than going ambiguous - worth checking before we commit to it.

Yep. (I don't think that needs a unit test, a print statement within the initializer should be enough to confirm that the faster overload is usually selected unless you go out of your way to use the slower overload.)

One small thing: is UnsafeMutableBufferPointer in the requirement intentional?

Nope, good catch, it should be UnsafeBufferPointer. (I think the Mutable crept in while I was looking at this)