Skip to content

GSB: Lazy requirement signature #24591

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 6 commits into from
May 10, 2019
Merged

GSB: Lazy requirement signature #24591

merged 6 commits into from
May 10, 2019

Conversation

xymus
Copy link
Contributor

@xymus xymus commented May 7, 2019 •
edited
Loading

Make getRequirementSignature lazy for the signature to be computed only on its first use. The requirement signature is still explicitly computed on the primary files to preserve diagnostics.

slavapestov
slavapestov reviewed May 7, 2019
View reviewed changes
Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

This looks like a good start. Can you rip out all the existing explicit calls to computeRequirementSignature() too?

@slavapestov
Copy link
Contributor

... also, any callers of ProtocolDecl::isRequirementSignatureComputed() should be removed or refactored. There's a tricky one in GenericSignatureBuilder; there, the right check you want is not "have I computed a requirement signature yet", but "am I currently not already computing a requirement signature". @DougGregor can help you implement that in terms of the request evaluator itself

@xymus
Explicitly request to createGenericParamsIfMissing as it will be lazy
862432f 
Computing the requirement signature created the generic params as
a side effect. Making getRequirementSignature lazy means that users
of the generic params must make sure they are created before use.
@xymus xymus force-pushed the req-sig-request branch 2 times, most recently from d0873b4 to e25c812 Compare May 8, 2019 21:12
xymus added 2 commits May 8, 2019 14:28
@xymus
Intro a request to compute the requirement signature of a ProtocolDecl
6c92927 
Replaces the explicit call to computeRequirementSignature from
validateDecl with a lazy getRequirementSignature. A side effect is that
the generic params of a ProtocolDecl are no longer computed from
validateDecl and must be computed lazily too.
@xymus
Remove explicit calls to computeRequirementSignature
3dd54c2 
We can rely on lazy calls to getRequirementSignature instead.
@xymus xymus force-pushed the req-sig-request branch from e25c812 to 2a93b9d Compare May 8, 2019 21:35
@xymus
Copy link
Contributor Author

xymus commented May 8, 2019

@swift-ci Please smoke test

@xymus
Copy link
Contributor Author

xymus commented May 8, 2019

@swift-ci Please test source compatibility

slavapestov
slavapestov approved these changes May 9, 2019
View reviewed changes
Copy link
Contributor

@slavapestov slavapestov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great! You can address the nitpicks in a followup.

lib/Serialization/Serialization.cpp
@@ -3284,6 +3284,7 @@ void Serializer::writeDecl(const Decl *D) {
rawAccessLevel,
inherited);

const_cast<ProtocolDecl*>(proto)->createGenericParamsIfMissing();
writeGenericParams(proto->getGenericParams());
Copy link
Contributor

Choose a reason for hiding this comment

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

In a separate PR can you try moving the createGenericParamsIfMissing() call into getGenericParams() itself?

@slavapestov
Copy link
Contributor

I think the next step is to try to remove TypeChecker::DelayedRequirementSignatures. It should not be needed any more.

@xymus xymus force-pushed the req-sig-request branch from 2a93b9d to 8037be0 Compare May 9, 2019 16:22
@xymus
Copy link
Contributor Author

xymus commented May 9, 2019

@swift-ci Please smoke test

@xymus
Copy link
Contributor Author

xymus commented May 9, 2019

@swift-ci Please test source compatibility

DougGregor
DougGregor approved these changes May 9, 2019
View reviewed changes
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This looks great! Just the one comment about the AST dumper.

xymus added 3 commits May 9, 2019 10:45
@xymus
Don’t use isRequirementSignatureComputed because it is lazy
be4ac2c
@xymus
Intro Evaluator::hasActiveRequest to prevent request cycles
3cb1b6a
@xymus
Avoid cycles when computing a requirement signature
cfae7bb 
Replace most remaining uses of isRequirementSignatureComputed by
isRequirementSignatureComputing which uses Evaluator::hasActiveRequest
to detect if the requirements are currently being computed.
@xymus xymus force-pushed the req-sig-request branch from 8037be0 to cfae7bb Compare May 9, 2019 18:00
@xymus
Copy link
Contributor Author

xymus commented May 9, 2019

@swift-ci Please smoke test

@xymus
Copy link
Contributor Author

xymus commented May 9, 2019

@swift-ci Please test source compatibility

@xymus xymus merged commit 9c4c304 into swiftlang:master May 10, 2019
@xymus xymus deleted the req-sig-request branch May 13, 2019 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slavapestov slavapestov slavapestov approved these changes

@DougGregor DougGregor DougGregor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xymus @slavapestov @DougGregor