[swift-evolution] [swift-evolution-announce] [Review] SE-0184: Unsafe[Mutable][Raw][Buffer]Pointer: add missing methods, adjust existing labels for clarity, and remove deallocation size

[Taylor Swift]

not a fan of this. A lot of data structures including buffer pointers
already store their capacity in a property, storing an allocation token
would be redundant and waste space. It also forces allocate(capacity:) to
return a tuple which is a big turn off for me.

On Thu, Sep 7, 2017 at 10:56 AM, Joe Groff <jgroff at apple.com> wrote:

>
> On Sep 7, 2017, at 8:06 AM, Taylor Swift <kelvin13ma at gmail.com> wrote:
>
> I don’t see any source for this claim in the documentation
> <https://developer.apple.com/documentation/swift/unsafemutablepointer/2295090-deallocate>,
> or the source code
> <https://github.com/apple/swift/blob/master/stdlib/public/core/UnsafePointer.swift.gyb#L432>.
> As far as I can tell the expected behavior is that partial deallocation
> “should” work.
>
>
> We should fix the documentation bug then.
>
> Another way we could make these APIs easier to use correctly is to
> leverage the type system. allocate could return the allocation size wrapped
> up in an opaque AllocationToken type, and deallocate could take an
> AllocationToken:
>
> static func allocate(capacity: Int) -> (UnsafeMutableBufferPointer<T>,
> AllocationToken)
>
> func deallocate(token: AllocationToken)
>
> That would make it harder for user code to pass an arbitrary size in, and
> make the relationship between the allocate and deallocate calls more
> explicit in their signatures. If we get an ABI break and a bold new idea
> for an allocator design with different context needs in the future, it'd
> also insulate source code from the exact contents of the information that
> needs to be carried from allocation to deallocation.
>
> -Joe
>
> On Thu, Sep 7, 2017 at 8:59 AM, Joe Groff <jgroff at apple.com> wrote:
>
>>
>> > On Sep 6, 2017, at 4:46 PM, Taylor Swift <kelvin13ma at gmail.com> wrote:
>> >
>> >
>> >
>> >> On Sep 6, 2017, at 5:12 PM, Joe Groff <jgroff at apple.com> wrote:
>> >>
>> >>
>> >>> On Sep 6, 2017, at 3:07 PM, Taylor Swift <kelvin13ma at gmail.com>
>> wrote:
>> >>>
>> >>>
>> >>>
>> >>>> On Sep 6, 2017, at 4:31 PM, Joe Groff <jgroff at apple.com> wrote:
>> >>>>
>> >>>>
>> >>>>> On Sep 6, 2017, at 2:28 PM, Andrew Trick <atrick at apple.com> wrote:
>> >>>>>
>> >>>>>
>> >>>>>>> On Sep 6, 2017, at 1:12 PM, Joe Groff via swift-evolution <
>> swift-evolution at swift.org> wrote:
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> On Sep 6, 2017, at 1:06 PM, Taylor Swift <kelvin13ma at gmail.com>
>> wrote:
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>> On Wed, Sep 6, 2017 at 12:41 PM, Joe Groff via swift-evolution <
>> swift-evolution at swift.org> wrote:
>> >>>>>>>> Currently, memory is deallocated by an instance method on
>> UnsafeMutablePointer, deallocate(count:). Like much of the Swift pointer
>> API, performing this operation on a buffer pointer requires extracting
>> baseAddress! and count. It is very common for the allocation code above to
>> be immediately followed by:
>> >>>>>>>>
>> >>>>>>>> defer
>> >>>>>>>>
>> >>>>>>>> {
>> >>>>>>>> buffer.
>> >>>>>>>> baseAddress?.deallocate(capacity: buffer.count
>> >>>>>>>> )
>> >>>>>>>> }
>> >>>>>>>>
>> >>>>>>>> This method is extremely problematic because nearly all users,
>> on first seeing the signature of deallocate(capacity:), will naturally
>> conclude from the capacity label that deallocate(capacity:) is equivalent
>> to some kind of realloc()that can only shrink the buffer. However this is
>> not the actual behavior — deallocate(capacity:) actually ignores the
>> capacity argument and just calls free() on self. The current API is not
>> only awkward and suboptimal, it is misleading. You can write perfectly
>> legal Swift code that shouldn’t segfault, but still can, for example
>> >>>>>>>>
>> >>>>>>>> var ptr = UnsafeMutablePointer<UInt8>.allocate(capacity: 1000000
>> >>>>>>>> )
>> >>>>>>>> ptr.
>> >>>>>>>> initialize(to: 13, count: 1000000
>> >>>>>>>> )
>> >>>>>>>> ptr.
>> >>>>>>>> deallocate(capacity: 500000) // deallocate the second half of
>> the memory block
>> >>>>>>>> ptr[0] // segmentation fault
>> >>>>>>>> where the first 500000 addresses should still be valid if the
>> documentation is to be read literally.
>> >>>>>>>
>> >>>>>>> The fact that the Swift runtime currently uses malloc/free is an
>> implementation detail. Tracking deallocation size is a very useful
>> optimization for better allocator backends, and C++ underwent an ABI break
>> to make it so that sized delete can be supported. Maybe we can change the
>> name to `deallocate(allocatedCapacity:)` to make it clear that it isn't
>> resizing the memory, and/or make the capacity argument optional so that you
>> can pay for the overhead of the allocator deriving the size if it's
>> inconvenient for the calling code to carry the size around, but we
>> shouldn't remove the functionality altogether.
>> >>>>>>>
>> >>>>>>> -Joe
>> >>>>>>> _______________________________________________
>> >>>>>>> swift-evolution mailing list
>> >>>>>>> swift-evolution at swift.org
>> >>>>>>> https://lists.swift.org/mailman/listinfo/swift-evolution
>> >>>>>>>
>> >>>>>>> The idea is to get the house in order by removing all parameters
>> from deallocate(), since that’s what it really does right now. Then, in the
>> future, if Swift gets a more sophisticated allocator backend, a new method
>> like deallocate(capacity:) or reallocate(toCapacity:) could be added
>> without conflicting with the currently named deallocate(capacity:).
>> However, using the function signature to pretend that it does something it
>> can’t actually do right now is extremely dangerous.
>> >>>>>>
>> >>>>>> I don't think that's a good idea in this case, because it's not
>> unlikely we would explore an optimized allocator soon after ABI stability,
>> and retrofitting these interfaces in a future version of Swift would put a
>> deployment target limit on when they can be used, and mean that a lot of
>> user code would need to be retrofitted to carry allocated capacities where
>> needed to see any benefit.
>> >>>>>>
>> >>>>>> -Joe
>> >>>>>
>> >>>>> The fact that we’re using malloc and free is already part of the
>> ABI because old libraries need to be able to deallocate memory allocated by
>> newer libraries.
>> >>>>
>> >>>> The compiler doesn't ever generate calls directly to malloc and
>> free, and the runtime entry points we do use already take size and
>> alignment on both allocation and deallocation.
>> >>>>
>> >>>>> Within the standard library we could make use of some new
>> deallocation fast path in the future without worrying about backward
>> deployment.
>> >>>>>
>> >>>>> Outside of the standard library, clients will get the benefits of
>> whatever allocator is available on their deployed platform because we now
>> encourage them to use UnsafeBufferPointer.deallocate(). We can change
>> the implementation inside UnsafeBufferPointer all we want, as long as it’s
>> still malloc-compatible.
>> >>>>>
>> >>>>> I’m sure we’ll want to provide a better allocation/deallocation API
>> in the future for systems programmers based on move-only types. That will
>> already be deployment-limited.
>> >>>>>
>> >>>>> Absolute worst case, we introduce a sized UnsafePointer.deallocate
>> in the future. Any new code outside the stdlib that wants the performance
>> advantage would either need to
>> >>>>> - trivially wrap deallocation using UnsafeBufferPointer
>> >>>>> - create a trivial UnsafePointer.deallocate thunk under an
>> availability flag
>> >>>>
>> >>>> Since we already have sized deallocate, why would we take it away?
>> If the name is misleading, we can change the name.
>> >>>>
>> >>>> -Joe
>> >>>
>> >>> Exactly, we are changing the name to `deallocate()`. As for the old
>> `deallocate(capacity:)` method that *needs* to be removed because it is
>> actively harmful. As I’ve explained it’s not enough to just drop in a sized
>> backend later as an “implementation detail” because it’s not an
>> implementation detail, you’re changing the fundamental behavior of that
>> method.
>> >>
>> >> It would remove a mode of "holding it wrong", but the specified
>> behavior will not change; it has and will always fully deallocate the
>> object being referenced in the cases where the call has defined behavior.
>> >
>> > idk what you mean by it holding it wrong, but  while the specified
>> behavior won’t change the actual behavior *will* change, because right now
>> the two don’t agree. We can’t just treat it as a bug to fix because it’s
>> been around since the beginning of Swift and some people treat it as part
>> of expected behavior, using the function like `free()`. This isn’t using it
>> incorrectly, in fact in terms of behavior, passing a meaningful capacity
>> argument to `deallocate(capacity:)` is *incorrect usage*, as demonstrated
>> in the segfaulting example in the proposal.
>>
>> The segfaulting example is an incorrect usage. The only valid parameters
>> to deallocate(capacity:) are the base address of an allocation, and the
>> original capacity passed into allocate(); it has never been intended to
>> support partial deallocation of allocated blocks. It seems to me like this
>> proposal is based on a misunderstanding of how the API works. The
>> documentation and/or name should be clarified.
>>
>> -Joe
>>
>> > “fixing” this bug will cause programs that once operated on previously
>> valid assumptions of “free()” semantics to behave differently, without any
>> warnings ever being generated. Conversely incorrect code will suddenly
>> become “correct” though this is less of a problem.
>> >
>> >> A sized implementation may fail more obviously when you violate the
>> contract in the future. Not having sized deallocation is a known deficiency
>> of the C model we've been fairly diligent about avoiding in Swift's
>> allocation interfaces, and it would be extremely unfortunate for us to
>> backpedal from it.
>> >>
>> >> -Joe
>> >
>> > Which is worse, an active gaping hole in Swift’s memory system, or
>> potentially discouraging users from using a hypothetical future allocation
>> API?
>> >
>> > Making sure the existing allocation API is working properly is a
>> prerequisite to introducing a future more advanced allocation API.
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20170907/4d4e433d/attachment.html>