implement ManagerBuffer.reallocated to allow realloc'ing the storage #19421

weissi · 2018-09-20T17:54:57Z

what am I trying to achieve

The end goal of this PR and a bunch of follow-ups is to implement support for reallocating Swift objects (to be used for tail-allocated storage) after confirming with a Swift builtin that the type is 'bit-wise takable'. See also the Swift Forums discussion swift_tailRealloc.

what is PR doing?

This pull request adds

tryReallocateUniquelyReferenced which tries to realloc a uniquely referenced ManagedBuffer
(might be removed) _hasSideTable (stdlib-only) runtime function to check if an object has a side table or not
_swift_reallocObject runtime function which tries to reallocate a HeapObject if it's safe. Does not work on Darwin right now as we can't efficiently check if it's safe or not

weissi · 2018-09-20T17:56:18Z

stdlib/public/core/ManagedBuffer.swift

+      let newBufferPtr = _stdlib_realloc(oldBufferPtr, newSizeInBytes)
+      // ref counts: realloc worked: 2 (old ptr is new ptr)
+      //             realloc failed: old ptr: 2, new ptr: 2
+      buffer = Unmanaged.fromOpaque(newBufferPtr).takeRetainedValue()


so I think this is just wrong in the case that realloc fails (ie. old and new buffer have different addresses): assigning to buffer will then decrement the reference count old the old buffer (which is already free'd 🔥 --> use after free) and increment the reference count of the new buffer (okay but unnecessary).

could I just write into buffer's memory without doing any ref count stuff? Then I would also not need to manually adjust them.

Given that ManagedBuffer is a Swift class, you shouldn't need (or want) to do any refcounting, since the refcount is stored in the object and will be copied over. Instead, treat the old and new pointers as opaque pointers and assign like that. You may want to take buffer as an UnsafeMutablePointer<Buffer> to have more control over the refcounting. You could probably do it with withUnsafeMutablePointer(to:) but taking the parameter as a UMP seems like less magic.

This brings up another fun case: if the object has a side table (currently, if it has ever had a weak reference to it, possibly more scenarios in the future) then you'll end up with the side table pointing back to the old location. You'll either need to update it as well, or require (and preferably precondition) that the object has no side table.

@mikeash Thanks, that sounds like a good plan!

Regarding the side table: Yes, so the initial idea was to change isKnownUniquelyReferenced to only return true iff refcount == 1 && no side table. I filed SR-8538 for that. But then @lorentey previously had a use-case for exactly the opposite behaviour SR-5633. Someone then proposed to deprecate isKnownUniquelyReferenced and introduce two new functions with better names that make it clear what happens with weak/unowned references.
I'm happy to file an SR about that if you think that's a good plan.

For now (ie. this patch) I think it would be good enough to precondition that there's no side-table. Is there a way to do that already?

RefCount.h has a hasSideTable method, but I don't see anywhere that exposes it to Swift. Your best bet there is probably to add a new runtime function that calls through to that. isKnownUniquelyReferencedSeriouslyEvenIncludingTheSideTableYesIMeanIt

weissi · 2018-09-20T17:57:44Z

stdlib/public/core/ManagedBuffer.swift

@@ -136,6 +136,38 @@ open class ManagedBuffer<Header, Element> {
  }
 }

+public extension ManagedBuffer {
+  @inline(never)
+  final class func reallocate<Buffer: ManagedBuffer>(


should this be underscored?

weissi · 2018-09-20T17:57:58Z

stdlib/public/core/ManagedBuffer.swift

+    count: Int,
+    newMinimumCapacity: Int
+    ) {
+      precondition(_isBitwiseTakable(Header.self))


are we all fine with just preconditioning all of those?

I think that's fine. I'd like to also have a higher-level interface that falls back to manual copying for things that aren't bitwise takable, which should be doable if you require the count to be passed in like this.

that sounds like the best idea to me, will add later

weissi · 2018-09-20T17:58:13Z

stdlib/public/core/ManagedBuffer.swift

+      //                                       realloc failed: old ptr: 1, new ptr: 2
+      if oldBufferPtr != newBufferPtr {
+        // in case realloc failed, we need to decrease the reference count once more
+        Unmanaged<Buffer>.fromOpaque(newBufferPtr).release()


weissi · 2018-09-25T18:14:09Z

stdlib/public/core/ManagedBuffer.swift

+      precondition(_isBitwiseTakable(Header.self))
+      precondition(_isBitwiseTakable(Element.self))
+      precondition(isKnownUniquelyReferenced(&buffer))
+      precondition(!_hasSideTable(&buffer))


@mikeash wired that through now. Hope similar to how you thought.

weissi · 2018-09-25T18:14:48Z

stdlib/public/core/ManagedBuffer.swift

+      let newSizeInBytes = MemoryLayout<Header>.stride
+        + newMinimumCapacity * MemoryLayout<Element>.stride
+
+      withUnsafePointer(to: &buffer) { bufferPtr in


@mikeash 🙈. Is this really the best way of doing this? I think this is pretty wrong as it doesn't guarantee to give me the right pointer but it does work...

@mikeash checked some more but couldn't find anything that would allow me to rewrite the pointer of inout Buffer to a new value without messing the with ref counts (apart from the unsafeBitCast obvs)...

weissi · 2018-09-27T17:40:20Z

test/stdlib/Builtins.swift

+
+tests.test("_hasSideTable") {
+  var aS1 = X()
+  expectFalse(_hasSideTable(&aS1))


@mikeash are there any other ways for objects to get side-tables besides gaining weak refs?

Currently no, but I believe the side table is intended to expand to serve more needs in the future. For example, it would be the natural place to put stored properties declared in extensions, or other "associated object" like things.

@mikeash what about actual associated objects? Isn't that going to require a side table entry?

@mikeash Aren't side tables also generated in case of reference count overflows?

weissi · 2018-10-03T16:59:01Z

@swift-ci test this please

jrose-apple · 2018-10-03T17:37:16Z

Unfortunately being clever with swift-ci leads to disappointment

@swift-ci test

weissi · 2018-10-03T17:51:40Z

@jrose-apple haha, I can never remember which bots want what prose. The NIO one only wants 'test this please' I think :)

gottesmm · 2018-10-03T18:01:45Z

@weissi the ones for swift are documented here: https://github.com/apple/swift/blob/master/docs/ContinuousIntegration.md#swift-ci

weissi · 2018-10-08T16:24:10Z

rebased to latest master & added @atrick to check that this is along the right lines

weissi · 2018-10-08T16:24:37Z

@swift-ci test

swift-ci · 2018-10-08T16:26:38Z

Build failed
Swift Test OS X Platform
Git Sha - 6d37aa023e33f6ff820d26a64f48cf90321ded46

swift-ci · 2018-10-08T16:26:56Z

Build failed
Swift Test Linux Platform
Git Sha - 6d37aa023e33f6ff820d26a64f48cf90321ded46

weissi · 2018-10-10T12:56:54Z

@swift-ci test

swift-ci · 2018-10-10T13:33:04Z

Build failed
Swift Test Linux Platform
Git Sha - 6186eeacc074c525a4784401e3a436e4667199d4

atrick

The code looks good. I think I suggested just lowering to a runtime call in SILGen rather than adding a SIL instruction. But the HasSideTable primitive could be useful in for other features, and we could potentially decide to optimize it away, either by knowing that some types can never have a side table, or by doing some dominance-based analysis. So overall, no objections here.

atrick · 2018-10-10T21:38:40Z

test/stdlib/Builtins.swift

+
+tests.test("_hasSideTable") {
+  var aS1 = X()
+  expectFalse(_hasSideTable(&aS1))


@mikeash what about actual associated objects? Isn't that going to require a side table entry?

eeckstein · 2018-10-11T21:27:44Z

test/stdlib/Builtins.swift

+
+tests.test("_hasSideTable") {
+  var aS1 = X()
+  expectFalse(_hasSideTable(&aS1))


@mikeash Aren't side tables also generated in case of reference count overflows?

eeckstein · 2018-10-11T21:29:41Z

stdlib/public/core/ManagedBuffer.swift

+  precondition(_isBitwiseTakable(Header.self))
+  precondition(_isBitwiseTakable(Element.self))
+  precondition(isKnownUniquelyReferenced(&buffer))
+  precondition(!_hasSideTable(&buffer))


Is this the only place where hasSideTable is used?

It feels that adding a built-in + instruction for this is the wrong approach. Why not check this in the _swift_stdlib_realloc function?

Also, AFAIK side tables are also created for ref count overflows. So you cannot be sure if the precondition will hold, even if there are no weak references.

Took me a while to page this info back in, but the only refcount that overflows currently is the unowned count. If an object has never had weak or unowned references (which I think we can reasonably require for this use case) then it won't have a side table. At least as things stand now.

@weissi Any assumptions that we make about this use case at least need to be documented as part of the ManagedBuffer contract. e.g. weak/unowned references are disallowed. Otherwise, we can just get rid of the precondition and call the slow path for _hasSideTable.

@mikeash How are associated objects implemented? Don't they require some kind of side table as well?

@atrick associated objects go through the objective c runtime (and use a different side table mechanism there because plain swift objects don't use the non-pointer isa).

Relocating an object like this will break associated objects in interesting ways. That's another thing we should be OK to forbid here. I'm not sure if it's practical to assert it, though.

If Swift ever gets something like associated objects that doesn't go through the ObjC runtime, it will likely use the side table mechanism, so that would be another potential source of them in the future.

weissi · 2018-11-07T19:13:16Z

@swift-ci please test

swift-ci · 2018-11-07T19:13:52Z

Build failed
Swift Test OS X Platform
Git Sha - 8a2fbbba3f884200477d27b033bb858877e0cc2e

swift-ci · 2018-11-07T19:15:29Z

Build failed
Swift Test Linux Platform
Git Sha - 8a2fbbba3f884200477d27b033bb858877e0cc2e

atrick · 2018-11-08T02:25:47Z

stdlib/public/SwiftShims/LibcShims.h

@@ -70,6 +70,11 @@ __swift_ssize_t _swift_stdlib_write(int fd, const void *buf, __swift_size_t nbyt
 SWIFT_RUNTIME_STDLIB_SPI
 int _swift_stdlib_close(int fd);

+static inline void *_swift_stdlib_realloc(void *ptr, __swift_size_t size) {


We don't need this now that _swift_reallocObject is a runtime call right?

weissi · 2018-11-08T16:36:00Z

@swift-ci please test

swift-ci · 2018-11-08T16:37:54Z

Build failed
Swift Test Linux Platform
Git Sha - b111f3615b4877ea78991a6610151bec08e5e0aa

swift-ci · 2018-11-08T16:39:44Z

Build failed
Swift Test OS X Platform
Git Sha - b111f3615b4877ea78991a6610151bec08e5e0aa

weissi · 2018-11-08T18:19:16Z

@swift-ci please test

swift-ci · 2018-11-08T18:21:06Z

Build failed
Swift Test OS X Platform
Git Sha - 58dda8e0891f6e65ee4324c89ce2693215ba4063

swift-ci · 2018-11-08T18:21:11Z

Build failed
Swift Test Linux Platform
Git Sha - 58dda8e0891f6e65ee4324c89ce2693215ba4063

weissi · 2018-11-08T20:50:20Z

Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..Build timed out (after 45 minutes). Marking the build as failed.
14:28:04 Build was aborted

weissi · 2018-11-08T20:50:31Z

@swift-ci please test

weissi · 2018-11-08T21:21:08Z

hmm, timed out again. @shahmishal is there some issue? Got two timed out runs in a row at a seemingly random test.

weissi · 2018-11-09T17:13:07Z

@swift-ci please merge

This change makes count and capacity available in type-erased contexts and generates less code for these accessors (just one copy, instead of one per element type). Minor associated change: removed the ability to update capacity, since it's not needed until Swift has a realloc solution (swiftlang/swift#19421, which was reverted simply due to the lack of an evolution proposal!)

weissi requested review from mikeash, gottesmm, aschwaighofer and airspeedswift September 20, 2018 17:54

weissi commented Sep 20, 2018

View reviewed changes

weissi force-pushed the jw-managed-buffer-realloc branch from c9f29cf to eb911e3 Compare September 25, 2018 18:12

weissi commented Sep 25, 2018

View reviewed changes

weissi force-pushed the jw-managed-buffer-realloc branch from eb911e3 to 736b0a5 Compare September 27, 2018 17:12

weissi commented Sep 27, 2018

View reviewed changes

weissi force-pushed the jw-managed-buffer-realloc branch 2 times, most recently from 90d5e98 to 6d37aa0 Compare October 1, 2018 18:09

weissi force-pushed the jw-managed-buffer-realloc branch from 6d37aa0 to 6186eea Compare October 8, 2018 16:22

weissi requested a review from atrick October 8, 2018 16:23

atrick approved these changes Oct 10, 2018

View reviewed changes

eeckstein reviewed Oct 11, 2018

View reviewed changes

weissi force-pushed the jw-managed-buffer-realloc branch from 6186eea to 55f05d8 Compare October 12, 2018 15:31

weissi requested review from gparker42 and removed request for gottesmm, airspeedswift and aschwaighofer November 6, 2018 10:48

weissi force-pushed the jw-managed-buffer-realloc branch 2 times, most recently from 88d1a9d to b111f36 Compare November 7, 2018 17:39

atrick reviewed Nov 8, 2018

View reviewed changes

weissi force-pushed the jw-managed-buffer-realloc branch from b111f36 to 58dda8e Compare November 8, 2018 13:37

implement ManagerBuffer.reallocated to allow realloc'ing the storage

2d9fa20

weissi force-pushed the jw-managed-buffer-realloc branch from 58dda8e to 2d9fa20 Compare November 8, 2018 18:18

weissi merged commit 17e5fa3 into swiftlang:master Nov 9, 2018

weissi deleted the jw-managed-buffer-realloc branch November 23, 2018 15:58

airspeedswift mentioned this pull request Jan 15, 2019

Revert "implement ManagerBuffer.reallocated to allow realloc'ing the storage" #21874

Merged

weissi mentioned this pull request Nov 3, 2018

[SR-7545] Swift should support using realloc for tail allocated classes #50087

Closed

implement ManagerBuffer.reallocated to allow realloc'ing the storage #19421

implement ManagerBuffer.reallocated to allow realloc'ing the storage #19421

Uh oh!

Conversation

weissi commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

what am I trying to achieve

what is PR doing?

Uh oh!

weissi Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeash Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weissi commented Oct 3, 2018

Uh oh!

jrose-apple commented Oct 3, 2018

Uh oh!

weissi commented Oct 3, 2018

Uh oh!

gottesmm commented Oct 3, 2018

Uh oh!

weissi commented Oct 8, 2018

Uh oh!

weissi commented Oct 8, 2018

Uh oh!

swift-ci commented Oct 8, 2018

Uh oh!

swift-ci commented Oct 8, 2018

Uh oh!

weissi commented Oct 10, 2018

Uh oh!

swift-ci commented Oct 10, 2018

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weissi commented Sep 20, 2018 •

edited

Loading

weissi Sep 20, 2018 •

edited

Loading

mikeash Sep 20, 2018 •

edited

Loading