Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Distributed] Implement distributed method accessors #40270

Merged
merged 38 commits into from
Dec 18, 2021

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Nov 20, 2021

Make it possible to reach and call distributed methods via runtime mechanism
that would look them up by originating method name at runtime and apply with
the given arguments.

@ktoso ktoso added the distributed Feature → concurrency: distributed actor label Nov 21, 2021
Copy link
Contributor

@aschwaighofer aschwaighofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an IRGen test that checks that we emit the distributed thunk, the underlying function , and the remote accessor? And check that the remote accessor calls the distributed thunk.

@xedin
Copy link
Contributor Author

xedin commented Nov 30, 2021

@aschwaighofer Yeah, that's the next step for me here, adding IRGen tests for combinations of arguments/result to make sure that everything is generated correctly. Just wanted to figure out whether I'm on the right track here before I continue...

@aschwaighofer
Copy link
Contributor

Ok. This looks good to me

distributed func complex(_: [Int], _: Obj, _: String?, _: LargeStruct) -> LargeStruct {
fatalError()
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DougGregor and @aschwaighofer Please take a look and let me know what else would be useful to add for the initial concrete-type only implementation. What ever it is has to be able to conform to Codable though...

@xedin xedin force-pushed the thunks-for-dist-methods branch 3 times, most recently from 1fe2207 to 06f8315 Compare December 10, 2021 23:38
@xedin
Copy link
Contributor Author

xedin commented Dec 14, 2021

@swift-ci please clean test

@xedin xedin marked this pull request as ready for review December 14, 2021 22:12
@xedin
Copy link
Contributor Author

xedin commented Dec 14, 2021

@swift-ci please clean test macOS platform

@swiftlang swiftlang deleted a comment from swift-ci Dec 14, 2021
@xedin
Copy link
Contributor Author

xedin commented Dec 14, 2021

@swift-ci please clean test macOS platform

@swiftlang swiftlang deleted a comment from swift-ci Dec 14, 2021
@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - dd3bd6d392bc9fa1d226475192acffb5849843f3

@xedin
Copy link
Contributor Author

xedin commented Dec 14, 2021

@swift-ci please clean test macOS platform

Copy link
Contributor

@aschwaighofer aschwaighofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - dd3bd6d392bc9fa1d226475192acffb5849843f3

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - dd3bd6d392bc9fa1d226475192acffb5849843f3

@xedin
Copy link
Contributor Author

xedin commented Dec 15, 2021

@swift-ci please clean test

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reviewing this but don't have much to add other than that it looks good and I hope to connect it all up shortly :-)

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

SWIFT_RUNTIME_STDLIB_SPI const AccessibleFunctionRecord *
swift_findAccessibleFunction(const char *targetNameStart,
size_t targetNameLength);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok... so this is good since it just says "name" and not exactly what shape of name it is (mangled, or just full name of the base etc). I remain a little worried worried about how we'll know what name to register methods under (today we do mangled names, but what if we want to do the other scheme -- we'd have to annotate methods I guess), but I guess it'll work out since we'd need annotations for the other scheme anyway perhaps 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might use flags to detect new scheme of naming if we switch to something other than mangled names.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My worry is about both "sides" having to use the same scheme so they'd have to be compiled using the same flags, but I guess that's just what we'll live with 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the unfortunate reality of the situation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh yeah... that's what I kept bringing up as "we change it later" being a problem... so we'll be forever stuck emitting the mangled type names as identifiers, and may be able to–in addition to those–add some shorter names... Unless we know "all sides" of the peers and can decide on a scheme to use with a compilation flag... I guess there's ways out of this corner but it'll be annoying :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think if we just wanted to drop all of the type information then it would be pretty easy to handle at runtime because we could just read mangled name from the record, remove the type information and match against the name provided by the caller. We'd only have to do that transformation once per method and cache it, but for other schemes it would be tricker.

Copy link
Contributor

@ktoso ktoso Dec 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I wonder if we could get away with recording using the "full name" and then compare the mangled name if we have it to disambiguate any potential stores with the same name... 🤔

Not a blocker for merging this, but just something to continue thinking about :-)

addUsedGlobal(var);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool 👍

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - b4f2c6575d8feeb9f46f1afd434ef870f9854cba

xedin and others added 26 commits December 17, 2021 10:52
Instead of using the distributed method directly, let's always
call distributed thunk instead which is safe option because
actor could not be local (e.g. issue with routing) but only
distributed thunk would know that and take appropriate action.
…tive schema

If parameter type's size is `0` then we don't want to perform a load
at the current position in the buffer and have to move to the next
non-empty element.
…sors

Test multiple scenarios - value types, reference types, optionals,
arrays, direct and indirect returns.
A "accessible" function that can be looked up based on a string key,
and then called through a fully-abstracted entry point whose arguments
can be constructed in code.
Uses a dedicated section in the binary to emit records about
functions that can be looked up by name at the runtime, and
then called through a fully-abstracted entry point whose
arguments can be constructed in code.
…g to load

If underlying function does not have any parameter, then let's
exit early and avoid stack allocating the argument buffer cursor.
`findAccessibleFunction` has to be accessible from Concurrency
and Distributed modules to be able to lookup distributed accessors.
…ssor

Direct pointer to the accessor cannot be called at runtime,
so here is how everything is stored:

- For `distributed` and `async` functions -> async function pointer;
- For regular functions -> function pointer.
The implementation is as follows:

- Looks up distributed accessor by the given target name
- Extracts information required to setup async context from
  async function pointer stored in accessor record
- Allocates context and calls accessor
…lt into a provided buffer

Instead of trying to return result from distributed thunk directly,
modify accessor to store result into the caller-provided buffer.
Doing so helps us avoid boxing the result into `Any`.
…et` from `DistributedActor.swift`

The function should be (and is) declared in `DistributedActorSystem.swift`
It's incorrect to load from the pointer directly, instead accessor
should use `loadAsCopy` for loadable types.
…ave correct ownership

Use parameter convention information to determine ownership of the extracted arguments.
I don't have access to Windows machine so there is no way for
me to debug and fix them.
@xedin
Copy link
Contributor Author

xedin commented Dec 17, 2021

@swift-ci please clean test

Copy link
Contributor

@aschwaighofer aschwaighofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@xedin xedin merged commit 9965df7 into swiftlang:main Dec 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed Feature → concurrency: distributed actor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants