Skip to content

Optimize SyntaxVisitor and make it a protocol #77

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
Feb 11, 2019

Conversation

akyrtzi
Copy link
Contributor

@akyrtzi akyrtzi commented Feb 10, 2019

  • Remove the deferred visitation shouldVisit checks. They are unnecessary now.
  • Change SyntaxVisitor to a protocol. SyntaxVisitor was a pure abstract class, a protocol is more appropriate for it.
  • Minimize casts to Syntax/_SyntaxBase during visitation to improve performance
  • Introduce SyntaxAnyVisitor as separate protocol for clients that want to use a visitAny(_:Syntax) function. It is a separate protocol because this kind of visitation is 1.6x slower.
  • Removed SyntaxVisitor.visitPre() because it doesn't seem useful, a client can put related code into visit() instead.
  • Move SyntaxVerifier to a separate source file

After these changes, performance of visition has improved significantly. Depending on whether the SyntaxVisitor client was visiting only token nodes or all nodes, the speed-up is 5x or 6x respectively.

* Remove the deferred visitation `shouldVisit` checks. They are unnecessary now.
* Change `SyntaxVisitor` to a protocol. `SyntaxVisitor` was a pure abstract class, a protocol is more appropriate for it.
* Minimize casts to `Syntax`/'_SyntaxBase' during visitation to improve performance
* Introduce `SyntaxAnyVisitor` as separate protocol for clients that want to use a `visitAny(_:Syntax)` function. It is a separate protocol because this kind of visitation is 1.6x slower.
* Move `SyntaxVerifier` to a separate source file

After these changes, performance of visition has improved significantly. Depending on whether the `SyntaxVisitor` client was visiting only token nodes or all nodes, the speed-up is 5x or 6x respectively.
@akyrtzi
Copy link
Contributor Author

akyrtzi commented Feb 10, 2019

@swift-ci Please test

Copy link
Contributor

@harlanhaskins harlanhaskins left a comment

Choose a reason for hiding this comment

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

🙌

How do these changes affect existing clients, beyond them needing to remove override?

@akyrtzi
Copy link
Contributor Author

akyrtzi commented Feb 10, 2019

  • They have to pass the visitor as inout, like tree.walk(&visitor)
  • If they use the generic visitPre/visitPost methods instead of type-specific ones they should use SyntaxAnyVisitor, see NodePrinter changes in this PR.

akyrtzi added a commit to akyrtzi/swift-stress-tester that referenced this pull request Feb 11, 2019
@akyrtzi akyrtzi merged commit bc3bf11 into swiftlang:master Feb 11, 2019
@akyrtzi akyrtzi deleted the opt4-visitor branch February 11, 2019 22:12
adevress pushed a commit to adevress/swift-syntax that referenced this pull request Jan 14, 2024
…-and-accessors

Fix formatting for vars with default values and accessors.
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