Skip to content

[Distributed] Ensure _remote funcs synthesized before dynamic replacement #38974

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 23, 2021
Merged

[Distributed] Ensure _remote funcs synthesized before dynamic replacement #38974

merged 3 commits into from
Aug 23, 2021

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Aug 20, 2021

This enables a _remote function to be looked up even if we FIRST visit an extension, say in a different file, and notice a dynamic member replacement there. This will be the case basically always since member replacements are in generated sources, in other files.

This cleans up also some cached requests which don't need to be cached since they just check for the presence of an attribute which is pretty cheap.

More cleanups to follow because now we can move all protocol witness required synthesis to the DerivedMember style -- something we could not do before since we needed to synthesize initializers etc.

@ktoso ktoso added the distributed Feature → concurrency: distributed actor label Aug 20, 2021
@ktoso ktoso requested review from kavon, drexin and slavapestov August 20, 2021 14:35
ktoso
ktoso commented Aug 20, 2021
View reviewed changes
include/swift/AST/Decl.h
@@ -5995,6 +5995,10 @@ class AbstractFunctionDecl : public GenericContext, public ValueDecl {
/// Returns 'true' if the function is distributed.
bool isDistributed() const;

/// Get (or synthesize) the associated remote function for this one.
/// For example, for `distributed func hi()` get `func _remote_hi()`.
AbstractFunctionDecl *getDistributedActorRemoteFuncDecl() const;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm really happy with this approach -- it allows us to just get the remote func whenever we want, and if it wasn't synthesized yet, it becomes synthesized.

In some situations we may need for force synthesizing all _remote funcs, e.g. when the dynamic member replacement is triggered, since we don't know yet which exact function we'll be looking up -- I believe we can improve this in the future when we do some @remoteFuncReplacement since then we know exactly which one to "get" for, this way everything will be even more lazy.

ktoso
ktoso commented Aug 20, 2021
View reviewed changes
include/swift/AST/TypeCheckRequests.h
@@ -933,18 +933,18 @@ class IsDistributedActorRequest :
bool isCached() const { return true; }
};

/// Determine whether the given func is distributed.
class IsDistributedFuncRequest :
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This request is removed now since it is so cheap we dont need to cache it

@ktoso
Copy link
Contributor Author

ktoso commented Aug 20, 2021

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Aug 20, 2021

@swift-ci please build toolchain

ktoso
ktoso commented Aug 20, 2021
View reviewed changes
lib/Sema/TypeCheckAttr.cpp
//
// This request is cached, so it won't cause wasted/duplicate work.
TypeChecker::addImplicitDistributedActorRemoteFunctions(clazz);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the important bit (well, one of them); if we first visit an extension, and didn't yet synthesize remote functions, but the extension has an function dynamic member replacement, the function it wants to replace does not exist yet so we must trigger synthesis. We can be smarter about the synthesis later, by requesting the specific func, but realistically we need all of them anyway and this unblocks us for now.

@ktoso
Copy link
Contributor Author

ktoso commented Aug 20, 2021

@swift-ci please smoke test

1 similar comment
@ktoso
Copy link
Contributor Author

ktoso commented Aug 21, 2021

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Aug 21, 2021

@swift-ci please build toolchain

@swift-ci
Copy link
Contributor

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 13a25bd

Install command
tar zxf swift-PR-38974-647-ubuntu16.04.tar.gz
More info

@ktoso
Copy link
Contributor Author

ktoso commented Aug 21, 2021

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Aug 21, 2021

Weird macos test error

15:12:07 Archiving artifacts
15:12:09 ERROR: Build step failed with exception
15:12:14 java.lang.IllegalArgumentException: The tools section is required.
15:12:14 	at org.jenkinsci.plugins.xunit.XUnitProcessor.<init>(XUnitProcessor.java:141)
15:12:14 	at org.jenkinsci.plugins.xunit.XUnitPublisher.perform(XUnitPublisher.java:185)
15:12:14 	at jenkins.tasks.SimpleBuildStep.perform(SimpleBuildStep.java:123)
15:12:14 	at hudson.tasks.BuildStepCompatibilityLayer.perform(BuildStepCompatibilityLayer.java:80)
15:12:14 	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
15:12:14 	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:803)
15:12:14 	at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:752)
15:12:14 	at hudson.model.Build$BuildExecution.post2(Build.java:177)
15:12:14 	at hudson.model.AbstractBuild$AbstractBuildExecution.post(AbstractBuild.java:697)
15:12:14 	at hudson.model.Run.execute(Run.java:1931)
15:12:14 	at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
15:12:14 	at hudson.model.ResourceController.execute(ResourceController.java:97)
15:12:14 	at hudson.model.Executor.run(Executor.java:429)
15:12:14 Build step 'Publish xUnit test result report' marked build as failure

@ktoso
Copy link
Contributor Author

ktoso commented Aug 21, 2021

@swift-ci please smoke test macOS Platform

@ktoso
Copy link
Contributor Author

ktoso commented Aug 21, 2021

CI is having known issues re:

15:41:34 ERROR: Build step failed with exception
15:41:35 java.lang.IllegalArgumentException: The tools section is required.

@swift-ci
Copy link
Contributor

macOS Toolchain
Download Toolchain
Git Sha - 13a25bd

Install command
tar -zxf swift-PR-38974-1088-osx.tar.gz --directory ~/

@ktoso
Copy link
Contributor Author

ktoso commented Aug 21, 2021 •
edited
Loading

@swift-ci please smoke test macOS Platform

@ktoso
Copy link
Contributor Author

ktoso commented Aug 21, 2021

@swift-ci please smoke test linux Platform

@ktoso
Copy link
Contributor Author

ktoso commented Aug 22, 2021

MacOS had timed out...

@swift-ci please smoke test macOS Platform and merge

@ktoso
Copy link
Contributor Author

ktoso commented Aug 22, 2021

Build timeout again... 😢

@ktoso
Copy link
Contributor Author

ktoso commented Aug 22, 2021

@swift-ci please smoke test macOS Platform

@ktoso
Copy link
Contributor Author

ktoso commented Aug 23, 2021

CI really not cooperating this weekend...


18:29:04 The recommended git tool is: NONE
18:34:05 FATAL: java.io.IOException: Unexpected termination of the channel
18:34:05 
java.io.EOFException
18:34:05 	at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2798)
18:34:05 	at java.io.ObjectInputStream$BlockDataInputStream.readShort(ObjectInputStream.java:3273)
18:34:05 	at java.io.ObjectInputStream.readStreamHeader(ObjectInputStream.java:933)
18:34:05 	at java.io.ObjectInputStream.<init>(ObjectInputStream.java:395)

@ktoso
Copy link
Contributor Author

ktoso commented Aug 23, 2021

@swift-ci please smoke test macOS Platform

@ktoso ktoso merged commit fe4ba18 into swiftlang:main Aug 23, 2021
@ktoso ktoso deleted the wip-dynamic-replacement-even-on-late-visit-to-actor-decl branch August 23, 2021 02:40
ktoso added a commit to ktoso/swift that referenced this pull request Aug 23, 2021
@ktoso
Revert "[Distributed] Ensure _remote funcs synthesized before dynamic…
beaf8a3 
… replacement (swiftlang#38974)"

This reverts commit fe4ba18.
@ktoso ktoso mentioned this pull request Aug 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kavon kavon Awaiting requested review from kavon

@drexin drexin Awaiting requested review from drexin

@slavapestov slavapestov Awaiting requested review from slavapestov slavapestov is a code owner

Assignees
No one assigned
Labels
distributed Feature → concurrency: distributed actor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ktoso @swift-ci