[Concurrency] Implement basic runtime support for task futures. #34703

DougGregor · 2020-11-12T06:19:43Z

Extend AsyncTask and the concurrency runtime with basic support for
task futures. AsyncTasks with futures contain a future fragment with
information about the type produced by the future, and where the
future will put the result value or the thrown error in the initial
context.

We still don't have the ability to schedule the waiting tasks on an
executor when the future completes, so this isn't useful for anything
just test, and we can only test limited code paths.

include/swift/ABI/Task.h

ktoso · 2020-11-12T06:39:20Z

include/swift/ABI/Task.h

+  /// wait queue and will be scheduled when the future completes or
+  /// is cancelled. Otherwise, the future has completed and can be
+  /// queried.
+  FutureFragment::Status waitFuture(AsyncTask *waitingTask);


is this more like pollFuture or peekFutureStatus/peekStatus?
wait to me implies waiting but we don't really do that, just check what we should do next

If the future is still executing, it puts waitingTask on the queue to be scheduled once the future has completed. We then get to yield the thread back to the scheduler to do more work (but don't have the way to do that yet)

include/swift/ABI/Task.h

ktoso · 2020-11-12T06:53:26Z

stdlib/public/Concurrency/Task.cpp

+  auto storagePtr = fragment->getStoragePtr();
+
+  // Check for an error.
+  if (unsigned errorOffset = fragment->errorOffset) {


for my understanding... when would this be 0? or just general pattern to avoid a nullptr if somehow(?) it would happen?

It could be zero for a non-throwing function. We should probably turn that into a JobFlag.

Ah I see, thanks! Yeah may be a job flag would be nicer.

stdlib/public/Concurrency/Task.cpp

DougGregor · 2020-11-12T07:46:53Z

@swift-ci please smoke test

include/swift/ABI/Task.h

rjmccall · 2020-11-12T23:15:42Z

include/swift/ABI/Task.h

+    unsigned resultOffset;
+
+    /// The offset of the error in the initial asynchronous context.
+    unsigned errorOffset;


What's the purpose of these?

This is me not knowing how to get a Swift function to put result values into the future.

include/swift/ABI/Task.h

rjmccall · 2020-11-12T23:24:32Z

stdlib/public/Concurrency/Task.cpp

+    break;
+
+  case Status::Error:
+    swift_unknownObjectRelease(getStoragePtr());


Please do not release an interior pointer into the future fragment. :)

Also, please use swift_errorRelease.

swift_errorRelease is only defined when Objective-C interoperability is enabled. But yeah, the other thing is bad ;)

rjmccall · 2020-11-12T23:35:07Z

stdlib/public/Concurrency/Task.cpp

+  while (true) {
+    waitingTask->NextWaitingTask = fragment->waitQueue.load();
+    if (fragment->waitQueue.compare_exchange_strong(
+            waitingTask->NextWaitingTask, waitingTask)) {


The reason you should unify the two atomic fields is that you have a race here against someone concurrently completing the task. The thread that completes the task should exchange the appropriate completion value into the queue and then process the waiting tasks they just loaded (via the exchange). Here you need to loop trying to add the task to the queue until you see success.

Always use memory orderings when working with std::atomic. This needs to be a std::memory_order_release on success; it can be std::memory_order_relaxed on failure, except actually it needs to be std::memory_order_acquire because you're going to check for success/error.

Don't repeatedly reload waitQueue. Load it (with an acquire) into a local, then use that local in the compare_exchange. When you see a pointer, copy it into NextWaitingTask (or really the SchedulerPrivate contents).

You should use compare_exchange_weak since you're already doing this in a loop.

Thank you! I've made all of these changes together. It looks much more reasonable now.

rjmccall · 2020-11-12T23:40:56Z

stdlib/public/Concurrency/Task.cpp

+  auto resultPtr = reinterpret_cast<OpaqueValue *>(
+      reinterpret_cast<char *>(context) + fragment->resultOffset);
+  fragment->resultType->vw_initializeWithTake(storagePtr, resultPtr);
+  fragment->status = FutureFragment::Status::Success;


The general ABI for indirect return values is that the address to fill would be passed as an argument, not that we'd make space for it in the initial context and that move out of that into the future.

I don't remember what convention we decided on for errors, but I doubt this it's this strange. Should there be a flag in the task reporting whether it can throw?

stdlib/public/Concurrency/Task.cpp

rjmccall

Thanks, looking a lot better.

stdlib/public/Concurrency/Task.cpp

include/swift/ABI/Task.h

DougGregor · 2020-11-13T20:21:17Z

@swift-ci please test

swift-ci · 2020-11-13T21:39:46Z

Build failed
Swift Test Linux Platform
Git Sha - ca83686ac2ad833f7bce039cf5f5396b2acd323a

DougGregor · 2020-11-14T00:08:41Z

@swift-ci please smoke test

rjmccall · 2020-11-14T01:13:01Z

include/swift/ABI/Task.h

+    /// fragment.
+    static size_t storageOffset(const Metadata *resultType)  {
+      size_t offset = sizeof(FutureFragment);
+      size_t alignment = std::max(resultType->vw_alignment(), alignof(void *));


What is this max about? The error value? Could you use SwiftError* to communicate that, here and below?

...should I overlook that this is subtly pessimal in really implausible cases, like a highly-aligned byte? I'm not sure it can practically happen.

Yes, it's the SwiftError*. I can use that.

rjmccall · 2020-11-14T01:17:55Z

include/swift/ABI/Task.h

+    ///
+    /// The low bits contain the status, the rest of the pointer is the
+    /// AsyncTask.
+    std::atomic<WaitQueueItem> waitQueue;


Technically I don't think that tasks need to coincide with any status but Executing, so the other statuses could be signal values rather than masked on. I don't think it really matters, though.

rjmccall · 2020-11-14T01:20:15Z

stdlib/public/Concurrency/Task.cpp

+    break;
+
+  case Status::Error:
+    swift_unknownObjectRelease(reinterpret_cast<OpaqueValue *>(getError()));


Is this the standard way we release errors in the runtime? Shouldn't we have an swift_errorRelease we can unconditionally call, even if it just compiles to swift_release in some configurations? Is it not important that we call swift_errorRelease in the configurations that provide it?

This is the standard way when we don't know whether we'll have a native Swift object or not. swift_errorRelease is implemented as objc_release where it's implemented.

rjmccall · 2020-11-14T01:23:20Z

stdlib/public/Concurrency/Task.cpp

+    auto nextWaitingTask = waitingTask->getNextWaitingTask();
+
+    // Remove this task from the list.
+    waitingTask->getNextWaitingTask() = nullptr;


You actually don't have to do this; the memory is invalidated once it's not waiting.

rjmccall · 2020-11-14T01:24:24Z

stdlib/public/Concurrency/Task.cpp

+    JobFlags flags, AsyncTask *parent, const Metadata *futureResultType,
+    AsyncFunctionType<void()> *function, size_t initialContextSize) {
+  assert((futureResultType != nullptr) == flags.task_isFuture());
+  assert((futureResultType != nullptr) == flags.task_isFuture());


Seeing undouble?

Should we assert here that initialContextSize >= sizeof(FutureAsyncContext)?

That's a good idea!

rjmccall · 2020-11-14T01:28:35Z

include/swift/ABI/Task.h

+/// task.
+///
+/// This type matches the ABI of a function `<T> () async throws -> T`, which
+/// is used to describe futures.


To be precise: it's the signature of the function taken by runDetached and Task.Group.add. We basically fully set up the call except for any formal arguments (e.g. closure-context arguments).

DougGregor · 2020-11-14T05:29:40Z

@swift-ci please smoke test

DougGregor · 2020-11-14T06:02:55Z

@swift-ci please smoke test

DougGregor · 2020-11-14T07:25:37Z

This makes no sense...

FAILED: unittests/runtime/SwiftRuntimeTests.exe 
cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E vs_link_exe --intdir=unittests\runtime\CMakeFiles\SwiftRuntimeTests.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100183~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100183~1.0\x64\mt.exe --manifests  -- C:\PROGRA~2\MICROS~1\2019\COMMUN~1\VC\Tools\MSVC\1424~1.283\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\SwiftRuntimeTests.rsp  /out:unittests\runtime\SwiftRuntimeTests.exe /implib:unittests\runtime\SwiftRuntimeTests.lib /pdb:unittests\runtime\SwiftRuntimeTests.pdb /version:0.0  /INCREMENTAL:NO /STACK:10000000 /INCREMENTAL:NO /subsystem:console  && cd ."
LINK: command "C:\PROGRA~2\MICROS~1\2019\COMMUN~1\VC\Tools\MSVC\1424~1.283\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\SwiftRuntimeTests.rsp /out:unittests\runtime\SwiftRuntimeTests.exe /implib:unittests\runtime\SwiftRuntimeTests.lib /pdb:unittests\runtime\SwiftRuntimeTests.pdb /version:0.0 /INCREMENTAL:NO /STACK:10000000 /INCREMENTAL:NO /subsystem:console /MANIFEST /MANIFESTFILE:unittests\runtime\SwiftRuntimeTests.exe.manifest" failed (exit code 1120) with the following output:
   Creating library unittests\runtime\SwiftRuntimeTests.lib and object unittests\runtime\SwiftRuntimeTests.exp
TaskFuture.cpp.obj : error LNK2019: unresolved external symbol "struct swift::FullMetadata<struct swift::TargetOpaqueMetadata<struct swift::InProcess> > const $sSiN" (?$sSiN@@3U?$FullMetadata@U?$TargetOpaqueMetadata@UInProcess@swift@@@swift@@@swift@@B) referenced in function "private: virtual void __cdecl TaskFutureTest_intFuture_Test::TestBody(void)" (?TestBody@TaskFutureTest_intFuture_Test@@EEAAXXZ)
unittests\runtime\SwiftRuntimeTests.exe : fatal error LNK1120: 1 unresolved externals

DougGregor · 2020-11-14T08:03:20Z

@swift-ci please smoke test Linux

Extend AsyncTask and the concurrency runtime with basic support for task futures. AsyncTasks with futures contain a future fragment with information about the type produced by the future, and where the future will put the result value or the thrown error in the initial context. We still don't have the ability to schedule the waiting tasks on an executor when the future completes, so this isn't useful for anything just test, and we can only test limited code paths.

…erly.

Use a single atomic for the wait queue that combines the status with the first task in the queue. Address race conditions in waiting and completing the future. Thanks to John for setting the direction here for me.

Reduce the size of AsyncTask by using the first slot of SchedulerPrivate for the next waiting task. Thanks, John!

There isn't a big advantage to keeping "simple" task as a separate concept from a task. Tasks may or may not have futures.

Introduce `FutureAsyncContext` to line up with the async context formed by IR generation for the type `<T> () async throws -> T`. When allocating a future task, set up the context with the address of the future's storage for the successful result and null out the error result, so the caller will directly fill in the result. This eliminates a bunch of extra complexity and a copy.

…mber. 'const' breaks compilation on newer compilers.

The objectFuture test is better anyway.

DougGregor · 2020-11-14T23:21:38Z

@swift-ci please test

DougGregor · 2020-11-14T23:22:16Z

Last commit should clear up the Windows failure. Rerunning all of the tests...

swift-ci · 2020-11-15T00:47:11Z

Build failed
Swift Test Linux Platform
Git Sha - ede5aa3

swift-ci · 2020-11-15T00:54:33Z

Build failed
Swift Test OS X Platform
Git Sha - ede5aa3

DougGregor · 2020-11-15T02:08:52Z

@swift-ci pleas test

DougGregor · 2020-11-15T02:10:07Z

Got tripped up by #34748, but otherwise everything was fine.

DougGregor · 2020-11-15T02:10:17Z

@swift-ci please smoke test

ktoso reviewed Nov 12, 2020

View reviewed changes

rjmccall requested changes Nov 12, 2020

View reviewed changes

rjmccall reviewed Nov 13, 2020

View reviewed changes

stdlib/public/Concurrency/Task.cpp Outdated Show resolved Hide resolved

include/swift/ABI/Task.h Show resolved Hide resolved

DougGregor marked this pull request as ready for review November 13, 2020 20:19

rjmccall reviewed Nov 14, 2020

View reviewed changes

DougGregor force-pushed the concurrency-task-future branch from 1b7fdea to 5d38e0c Compare November 14, 2020 06:00

DougGregor added 13 commits November 14, 2020 15:18

[Future] Add a test to make sure we're handling object lifetimes prop…

aa76672

…erly.

[Futures] Fix up some comments to address some comments.

0a07f18

[Concurrency] Use a single atomic for future wait queue.

4b92467

Use a single atomic for the wait queue that combines the status with the first task in the queue. Address race conditions in waiting and completing the future. Thanks to John for setting the direction here for me.

[Concurrency] Use SchedulerPrivate for the "next waiting task" link.

0ae6c63

Reduce the size of AsyncTask by using the first slot of SchedulerPrivate for the next waiting task. Thanks, John!

[Concurrency] Turn "simple task" into just "task".

91360f0

There isn't a big advantage to keeping "simple" task as a separate concept from a task. Tasks may or may not have futures.

[Concurrency] Inline AsyncTask::FutureFragment::fragmentSize.

5538aaf

[Concurrency] Clarify/fix error object handling in futures.

19d9e0f

[Concurrency] Minor cleanups to futures.

c0fd27f

[Concurrency] Remove an unnecessary 'const' from a non-static data me…

ab932db

…mber. 'const' breaks compilation on newer compilers.

[Concurrency] More cleanups for futures

4fb5f05

Remove test that depends on Swift.Int metadata, which isn't around.

ede5aa3

The objectFuture test is better anyway.

DougGregor force-pushed the concurrency-task-future branch from 5d38e0c to ede5aa3 Compare November 14, 2020 23:21

DougGregor merged commit 5dd0bce into swiftlang:main Nov 15, 2020

DougGregor deleted the concurrency-task-future branch November 15, 2020 04:50

[Concurrency] Implement basic runtime support for task futures. #34703

[Concurrency] Implement basic runtime support for task futures. #34703

Conversation

DougGregor commented Nov 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DougGregor commented Nov 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmccall left a comment

Choose a reason for hiding this comment

DougGregor commented Nov 13, 2020

swift-ci commented Nov 13, 2020

DougGregor commented Nov 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DougGregor commented Nov 14, 2020

DougGregor commented Nov 14, 2020

DougGregor commented Nov 14, 2020

DougGregor commented Nov 14, 2020

DougGregor commented Nov 14, 2020

DougGregor commented Nov 14, 2020

swift-ci commented Nov 15, 2020

swift-ci commented Nov 15, 2020

DougGregor commented Nov 15, 2020

DougGregor commented Nov 15, 2020

DougGregor commented Nov 15, 2020