Handle multiple awaits and suspend-on-exit for async let tasks. #37938

jckarter · 2021-06-16T01:15:41Z

Change the code generation patterns for async let bindings to use an ABI based on the following
functions:

swift_asyncLet_start, which as before, starts an async let child task, but which additionally
now associates the async let with a caller-owned buffer to receive the result of the task.
This is intended to allow the task to emplace its result in caller-owned memory, allowing the
child task to be deallocated after completion without invalidating the result buffer.
swift_asyncLet_get[_throwing], which replaces swift_asyncLet_wait[_throwing]. Instead of
returning a copy of the value, this entry point concerns itself with populating the local buffer.
If the buffer hasn't been populated, then it awaits completion of the task and emplaces the
result in the buffer; otherwise, it simply returns. The caller can then read the result out of
its owned memory. These entry points are intended to be used before every read from the
async let binding, after which point the local buffer is guaranteed to contain an initialized
value.
swift_asyncLet_finish, which replaces swift_asyncLet_end. Unlike _end, this variant
is async and will suspend the parent task after cancelling the child to ensure it finishes
before cleaning up. The local buffer will also be deinitialized if necessary. This is intended
to be used on exit from an async let scope, to handle cleaning up the local buffer if necessary
as well as cancelling, awaiting, and deallocating the child task.

To avoid an epoch break with existing swiftinterfaces and ABI clients, the old builtins and entry
points are kept intact for now, but SILGen now only generates code using the new interface.

This new interface fixes several issues with the old async let codegen, including use-after-free
crashes if the async let was never awaited, and the inability to read from an async let variable
more than once.

rdar://77855176

jckarter · 2021-07-07T23:40:05Z

@swift-ci Please test

swift-ci · 2021-07-08T01:01:56Z

Build failed
Swift Test Linux Platform
Git Sha - 5b8d0e98339c46cf541e83995e5cf23ef15ce196

jckarter · 2021-07-08T20:21:28Z

@swift-ci Please test

jckarter · 2021-07-09T21:10:17Z

@swift-ci Please test

swift-ci · 2021-07-09T21:43:39Z

Build failed
Swift Test Linux Platform
Git Sha - 050f511e140febf6be66289f46bb0fc6b8a4f929

swift-ci · 2021-07-09T22:55:38Z

Build failed
Swift Test OS X Platform
Git Sha - 050f511e140febf6be66289f46bb0fc6b8a4f929

jckarter · 2021-07-09T23:16:13Z

@swift-ci Please test

swift-ci · 2021-07-10T00:40:35Z

Build failed
Swift Test Linux Platform
Git Sha - 1fccb4bb6cfe5b2f7dd51a56d772464422c65dd4

swift-ci · 2021-07-10T00:54:50Z

Build failed
Swift Test OS X Platform
Git Sha - 1fccb4bb6cfe5b2f7dd51a56d772464422c65dd4

jckarter · 2021-07-10T01:14:15Z

@swift-ci Please test

swift-ci · 2021-07-10T02:41:39Z

Build failed
Swift Test Linux Platform
Git Sha - 2ab4829e35b7f1d4553487c3faa23cc50e95113d

ktoso

Read through in depth and looks fantastic as far as I can tell :-)

ktoso · 2021-07-12T08:38:27Z

include/swift/ABI/MetadataValues.h

-  /// The number of words in an AsyncLet (flags + task pointer)
-  NumWords_AsyncLet = 8, // TODO: not sure how much is enough, these likely could be pretty small
+  /// The number of words in an AsyncLet (flags + child task context & allocation)
+  NumWords_AsyncLet = 80, // 640 bytes ought to be enough for anyone


Curious why so much, is it because the task context and place where we'll store the result basically ends up statically as large as the result struct as well, so we effectively have a lot of bytes to use without additional alloc?

We use the same block for the child task's initial slab, and the previous code gave the child 512 bytes of slab to begin with. 640 seemed like a nice round number to allow for some growth in the task context header overhead and still ensure at least 512 bytes still remain for the child context to start with.

ktoso · 2021-07-12T08:38:49Z

include/swift/ABI/AsyncLet.h

+  void *getPreallocatedSpace();
+
+  /// Return the size of the unused space within the async let block.
+  static size_t getSizeOfPreallocatedSpace();


ktoso · 2021-07-12T08:47:30Z

include/swift/ABI/AsyncLet.h

@@ -36,15 +36,23 @@ class alignas(Alignment_AsyncLet) AsyncLet {
  constexpr AsyncLet()
    : PrivateData{} {}

-  // FIXME: not sure how many words we should reserve
  void *PrivateData[NumWords_AsyncLet];

  // TODO: we could offer a "was awaited on" check here


This is solved now by this PR! :-)

specifically by: llvm::PointerIntPair<AsyncTask *, 1, bool> taskAndHasResult; that boolean is exactly the flag this was alluding to.

ktoso · 2021-07-12T08:49:02Z

include/swift/Runtime/Concurrency.h

+///
+/// \code
+/// func swift_asyncLet_finish(_ asyncLet: Builtin.RawPointer,
+///                            _ resultBuffer: Builtin.RawPointer) async


minor nitpick: use same formatting as other docs? (cut down at the parameters)

ktoso · 2021-07-12T08:58:08Z

include/swift/Runtime/RuntimeFunctions.def

         ARGS(SwiftAsyncLetPtrTy,         // AsyncLet*
              SwiftTaskOptionRecordPtrTy, // options
              TypeMetadataPtrTy,          // futureResultType
              Int8PtrTy,                  // closureEntry
              OpaquePtrTy,                // closureContext
+              Int8PtrTy


Suggested change

Int8PtrTy

Int8PtrTy // resultBuffer

?

ktoso · 2021-07-12T10:39:17Z

stdlib/public/Concurrency/AsyncLet.cpp

+                                 HeapObject *closureContext,
+                                 void *resultBuffer) {
+  auto flags = TaskCreateFlags();
+  flags.setEnqueueJob(true);


Also setIsChildTask I think...?

Hm, I followed the code that asyncLet_start already had for getting an async let task started. Is that not implied by being an async let task?

ktoso · 2021-07-12T10:39:52Z

stdlib/public/Concurrency/AsyncLet.cpp

+  // Don't need to do anything if the result buffer is already populated.
+  if (asImpl(alet)->hasResultInBuffer()) {
+    return resumeFunction(callerContext);
+  }


ktoso · 2021-07-12T11:34:24Z

stdlib/public/Concurrency/AsyncLet.cpp

+  asImpl(alet)->setHasResultInBuffer();
+  swift_task_future_wait(reinterpret_cast<OpaqueValue*>(resultBuffer),
+                         callerContext, alet->getTask(),
+                         resumeFunction, callContext);


Right, that should be safe to do 👍

jckarter · 2021-07-12T17:23:23Z

@swift-ci Please test

swift-ci · 2021-07-12T18:49:18Z

Build failed
Swift Test Linux Platform
Git Sha - a1df8165a42daa79587a70d6bb854c2292793b42

swift-ci · 2021-07-12T19:41:21Z

Build failed
Swift Test OS X Platform
Git Sha - a1df8165a42daa79587a70d6bb854c2292793b42

jckarter · 2021-07-12T21:13:06Z

@swift-ci Please test

swift-ci · 2021-07-12T22:36:55Z

Build failed
Swift Test Linux Platform
Git Sha - 8101b02c6d0a13ba390bb545d1404fac8c8b8730

swift-ci · 2021-07-16T02:05:12Z

Build failed
Swift Test Linux Platform
Git Sha - b8100389c4e9b825473d106e5120319b4b03c580

jckarter · 2021-07-16T04:35:21Z

@swift-ci Please test Linux

swift-ci · 2021-07-16T05:16:50Z

Build failed
Swift Test Linux Platform
Git Sha - 2a9f5c6f27e417ad2189e6735e0d3c20d7d71fb4

jckarter · 2021-07-16T16:06:09Z

@swift-ci Please test Linux

swift-ci · 2021-07-16T17:38:55Z

Build failed
Swift Test Linux Platform
Git Sha - eb587fbe1de48fb9e62a95f6529efa611d742bb7

jckarter · 2021-07-16T17:42:38Z

@swift-ci Please test Linux

swift-ci · 2021-07-16T19:09:36Z

Build failed
Swift Test Linux Platform
Git Sha - d68b80d5968d2e932c064407d13c7960673385c7

aschwaighofer · 2021-07-21T16:41:17Z

Please test with following PR:
swiftlang/llvm-project#3121

@swift-ci test linux

aschwaighofer · 2021-07-21T16:52:30Z

Please test with following PR:
swiftlang/llvm-project#3121

@swift-ci test macOS

swift-ci · 2021-07-21T18:17:09Z

Build failed
Swift Test Linux Platform
Git Sha - 2fb271eb37010838704918624e44807b330d8073

swift-ci · 2021-07-21T18:53:11Z

Build failed
Swift Test OS X Platform
Git Sha - 2fb271eb37010838704918624e44807b330d8073

jckarter · 2021-07-21T20:10:15Z

Please test with following PR:
swiftlang/llvm-project#3121

@swift-ci Please test

swift-ci · 2021-07-21T21:38:46Z

Build failed
Swift Test Linux Platform
Git Sha - 7cb9c432233354283eb7e442561c41eaacc9bac8

jckarter · 2021-07-21T23:24:37Z

Please test with following PR:
swiftlang/llvm-project#3121

@swift-ci Please test

swift-ci · 2021-07-22T00:30:13Z

Build failed
Swift Test Linux Platform
Git Sha - 90b7cb9902c1709624facedc79d5d8f4a7529e0d

Change the code generation patterns for `async let` bindings to use an ABI based on the following functions: - `swift_asyncLet_begin`, which starts an `async let` child task, but which additionally now associates the `async let` with a caller-owned buffer to receive the result of the task. This is intended to allow the task to emplace its result in caller-owned memory, allowing the child task to be deallocated after completion without invalidating the result buffer. - `swift_asyncLet_get[_throwing]`, which replaces `swift_asyncLet_wait[_throwing]`. Instead of returning a copy of the value, this entry point concerns itself with populating the local buffer. If the buffer hasn't been populated, then it awaits completion of the task and emplaces the result in the buffer; otherwise, it simply returns. The caller can then read the result out of its owned memory. These entry points are intended to be used before every read from the `async let` binding, after which point the local buffer is guaranteed to contain an initialized value. - `swift_asyncLet_finish`, which replaces `swift_asyncLet_end`. Unlike `_end`, this variant is async and will suspend the parent task after cancelling the child to ensure it finishes before cleaning up. The local buffer will also be deinitialized if necessary. This is intended to be used on exit from an `async let` scope, to handle cleaning up the local buffer if necessary as well as cancelling, awaiting, and deallocating the child task. - `swift_asyncLet_consume[_throwing]`, which combines `get` and `finish`. This will await completion of the task, leaving the result value in the result buffer (or propagating the error, if it throws), while destroying and deallocating the child task. This is intended as an optimization for reading `async let` variables that are read exactly once by their parent task. To avoid an epoch break with existing swiftinterfaces and ABI clients, the old builtins and entry points are kept intact for now, but SILGen now only generates code using the new interface. This new interface fixes several issues with the old async let codegen, including use-after-free crashes if the `async let` was never awaited, and the inability to read from an `async let` variable more than once. rdar://77855176

This isn't implemented. It would never make sense for escaping captures of `async let` values, because doing so would either involve an implicit awaiting of the value on capture, or else stretching the lifetime of the task past its scope. In nonescaping contexts, and in nested `async let` expressions, it might be implementable in the future. rdar://80043610

jckarter · 2021-07-22T17:20:04Z

Please test with following PR:
swiftlang/llvm-project#3121

@swift-ci Please test

swift-ci · 2021-07-22T19:13:07Z

Build failed
Swift Test OS X Platform
Git Sha - 7470155

jckarter · 2021-07-22T19:37:39Z

@swift-ci Please test OS X

jckarter · 2021-07-22T21:33:33Z

Testing if it tests cleanly with all of the other memory bugs fixed, but without the LLVM patch:

jckarter · 2021-07-22T21:33:39Z

@swift-ci Please test

jckarter · 2021-07-22T21:35:35Z

@swift-ci Please test

jckarter force-pushed the async-let-multi-suspend branch from 406cf71 to f91bfb3 Compare June 16, 2021 01:16

jckarter force-pushed the async-let-multi-suspend branch 2 times, most recently from b256a70 to 5b8d0e9 Compare July 7, 2021 23:38

jckarter requested review from ktoso, DougGregor and rjmccall July 7, 2021 23:39

jckarter changed the title ~~[WIP] Revise async let ABI to efficiently allow multiple reads.~~ Handle multiple awaits and suspend-on-exit for async let tasks. Jul 7, 2021

jckarter marked this pull request as ready for review July 7, 2021 23:39

jckarter force-pushed the async-let-multi-suspend branch from 5b8d0e9 to d53522f Compare July 8, 2021 20:09

jckarter force-pushed the async-let-multi-suspend branch from d53522f to 050f511 Compare July 9, 2021 21:10

jckarter force-pushed the async-let-multi-suspend branch from 050f511 to 1fccb4b Compare July 9, 2021 23:16

jckarter force-pushed the async-let-multi-suspend branch from 1fccb4b to 2ab4829 Compare July 10, 2021 01:14

ktoso approved these changes Jul 12, 2021

View reviewed changes

jckarter force-pushed the async-let-multi-suspend branch from 2ab4829 to a1df816 Compare July 12, 2021 17:23

jckarter force-pushed the async-let-multi-suspend branch from 2a9f5c6 to eb587fb Compare July 16, 2021 16:05

jckarter force-pushed the async-let-multi-suspend branch from c1242e3 to 2fb271e Compare July 19, 2021 22:07

jckarter force-pushed the async-let-multi-suspend branch from 2fb271e to 7cb9c43 Compare July 21, 2021 20:09

jckarter force-pushed the async-let-multi-suspend branch from 7cb9c43 to 90b7cb9 Compare July 21, 2021 23:23

jckarter added 2 commits July 22, 2021 10:19

jckarter force-pushed the async-let-multi-suspend branch from 90b7cb9 to 7470155 Compare July 22, 2021 17:19

jckarter merged commit fc67ba5 into swiftlang:main Jul 23, 2021

Handle multiple awaits and suspend-on-exit for async let tasks. #37938

Handle multiple awaits and suspend-on-exit for async let tasks. #37938

Uh oh!

Conversation

jckarter commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jckarter commented Jul 7, 2021

Uh oh!

swift-ci commented Jul 8, 2021

Uh oh!

jckarter commented Jul 8, 2021

Uh oh!

jckarter commented Jul 9, 2021

Uh oh!

swift-ci commented Jul 9, 2021

Uh oh!

swift-ci commented Jul 9, 2021

Uh oh!

jckarter commented Jul 9, 2021

Uh oh!

swift-ci commented Jul 10, 2021

Uh oh!

swift-ci commented Jul 10, 2021

Uh oh!

jckarter commented Jul 10, 2021

Uh oh!

swift-ci commented Jul 10, 2021

Uh oh!

ktoso 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!

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!

jckarter commented Jul 12, 2021

Uh oh!

swift-ci commented Jul 12, 2021

Uh oh!

swift-ci commented Jul 12, 2021

Uh oh!

jckarter commented Jul 12, 2021

Uh oh!

swift-ci commented Jul 12, 2021

Uh oh!

swift-ci commented Jul 16, 2021

Uh oh!

jckarter commented Jul 16, 2021

Uh oh!

swift-ci commented Jul 16, 2021

Uh oh!

jckarter commented Jul 16, 2021

Uh oh!

swift-ci commented Jul 16, 2021

Uh oh!

jckarter commented Jul 16, 2021

Uh oh!

swift-ci commented Jul 16, 2021

Uh oh!

aschwaighofer commented Jul 21, 2021

Uh oh!

aschwaighofer commented Jul 21, 2021

Uh oh!

jckarter commented Jun 16, 2021 •

edited

Loading