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 <jgr...@apple.com> wrote: > > On Sep 7, 2017, at 8:06 AM, Taylor Swift <kelvin1...@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 <jgr...@apple.com> wrote: > >> >> > On Sep 6, 2017, at 4:46 PM, Taylor Swift <kelvin1...@gmail.com> wrote: >> > >> > >> > >> >> On Sep 6, 2017, at 5:12 PM, Joe Groff <jgr...@apple.com> wrote: >> >> >> >> >> >>> On Sep 6, 2017, at 3:07 PM, Taylor Swift <kelvin1...@gmail.com> >> wrote: >> >>> >> >>> >> >>> >> >>>> On Sep 6, 2017, at 4:31 PM, Joe Groff <jgr...@apple.com> wrote: >> >>>> >> >>>> >> >>>>> On Sep 6, 2017, at 2:28 PM, Andrew Trick <atr...@apple.com> wrote: >> >>>>> >> >>>>> >> >>>>>>> On Sep 6, 2017, at 1:12 PM, Joe Groff via swift-evolution < >> swift-evolution@swift.org> wrote: >> >>>>>>> >> >>>>>>> >> >>>>>>> On Sep 6, 2017, at 1:06 PM, Taylor Swift <kelvin1...@gmail.com> >> wrote: >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>>> On Wed, Sep 6, 2017 at 12:41 PM, Joe Groff via swift-evolution < >> swift-evolution@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@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. >> >> > >
_______________________________________________ swift-evolution mailing list swift-evolution@swift.org https://lists.swift.org/mailman/listinfo/swift-evolution