Skip to content

Parser: calculating interface hash only for primary files. #19196

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

Merged
merged 1 commit into from
Sep 10, 2018

Conversation

nkcsgexi
Copy link
Contributor

@nkcsgexi nkcsgexi commented Sep 7, 2018

Per discussion we had in #19192, we only need to collect interface hashes for primary files. This change allows parser to consume tokens out of order for non-primary files.

@nkcsgexi
Copy link
Contributor Author

nkcsgexi commented Sep 7, 2018

@swift-ci please smoke test

Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

Glad it's looking easy! I think I'd prefer a different implementation strategy though…it feels like you mixed two of my ideas.

@@ -1117,20 +1118,23 @@ class SourceFile final : public FileUnit {
/// Set the root refinement context for the file.
void setTypeRefinementContext(TypeRefinementContext *TRC);

void initInterfaceHashIfEmpty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right to me. I'd expect something like enableInterfaceHash(), which would assert if it were already enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense.

@@ -117,6 +117,7 @@ namespace swift {
///
/// \return true if the parser found code with side effects.
bool parseIntoSourceFile(SourceFile &SF, unsigned BufferID, bool *Done,
bool CollectInterfaceHash,
Copy link
Contributor

Choose a reason for hiding this comment

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

…and then you wouldn't need this.

@nkcsgexi
Copy link
Contributor Author

nkcsgexi commented Sep 7, 2018

@swift-ci please smoke test

@@ -745,7 +745,8 @@ void CompilerInstance::parseLibraryFile(
auto &Diags = NextInput->getASTContext().Diags;
auto DidSuppressWarnings = Diags.getSuppressWarnings();
Diags.setSuppressWarnings(DidSuppressWarnings || !IsPrimary);

if (IsPrimary)
NextInput->enableInterfaceHash();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can simplify this by doing it even earlier, in CompilerInstance::createSourceFileForMainModule.

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've put it in recordPrimarySourceFile.

@nkcsgexi
Copy link
Contributor Author

nkcsgexi commented Sep 7, 2018

@swift-ci please smoke test

Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

Now that's a nice, small diff! Thanks for working on this, Xi. Hopefully it still gives you the speedups you need. (And it'll be something you can refine later, I guess.)

@nkcsgexi
Copy link
Contributor Author

nkcsgexi commented Sep 7, 2018

@swift-ci please smoke test OS X Platform

1 similar comment
@nkcsgexi
Copy link
Contributor Author

nkcsgexi commented Sep 8, 2018

@swift-ci please smoke test OS X Platform

Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

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

Thank you Xi! This patch itself may speedups non-primary files parsing.

@nkcsgexi
Copy link
Contributor Author

nkcsgexi commented Sep 8, 2018

@swift-ci please smoke test

2 similar comments
@nkcsgexi
Copy link
Contributor Author

nkcsgexi commented Sep 9, 2018

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi nkcsgexi merged commit 59fc174 into swiftlang:master Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants