Improve Type Checker performance in SyntaxVisitor, SyntaxRewriter, TokenKind, SyntaxUtils

[art-divin]

Description

This PR optimizes swift code to accommodate slowness of Type Checker.

Context

The following two methods are currently slow to compile in swift-syntax package:

1. TokenKind.fromRaw
2. SyntaxVisitor.visitationFunc
3. SyntaxRewriter.visitationFunc
4. SyntaxUtils.init?( combining syntax1: some UnexpectedNodesCombinable, _ syntax2: some UnexpectedNodesCombinable, _ syntax3: some UnexpectedNodesCombinable, _ syntax4: some UnexpectedNodesCombinable, arena: __shared SyntaxArena )

Optimization in this PR includes the following:

1. change in SyntaxVisitorFile template in code-generation package to include the name of the type in visit....() methods
2. change in SyntaxRewriterFile template in code-generation package to include the name of the type in visit...() methods
3. change in TokenKindFile template in code-generation package to add a private extension of RawTokenKind
4. manual change in SyntaxUtils.init

Measurements

I have measured the performance of the following command:

time xcodebuild -scheme swift-syntax-Package ONLY_ACTIVE_ARCH=YES -destination 'platform=macos' clean build > /dev/null

Comparison of time output

Before	After
2.24s user 0.61s system 20% cpu 13.775 total	2.21s user 0.57s system 20% cpu 13.510 total
2.25s user 0.59s system 20% cpu 13.994 total	2.25s user 0.60s system 20% cpu 13.858 total
2.23s user 0.59s system 20% cpu 13.989 total	2.21s user 0.58s system 20% cpu 13.556 total
2.24s user 0.58s system 20% cpu 13.837 total	2.18s user 0.57s system 20% cpu 13.745 total
2.26s user 0.59s system 20% cpu 13.812 total	2.21s user 0.60s system 20% cpu 13.686 total
2.15s user 0.63s system 20% cpu 13.721 total	2.10s user 0.60s system 19% cpu 13.909 total

Comparison of build operation duration in Xcode

Before (seconds)	After (seconds)
12.225	11.923
12.068	11.745
12.087	11.872
12.358	11.929
12.323	12.199
12.307	12.178

Overall decrease in compilation (on average): 2.07%

Comparison of SwiftCompilationTimingParser output

Before (ms)	After (ms)
SUM 6,128.71	SUM 5,415.44
AVERAGE 11.476985019	AVERAGE 9.900255941
MIN 4	MIN 4
MAX 485.9	MAX 353.48
COUNT 535	COUNT 548

Overall, decrease in compilation of slow symbols: ~12%

More Details

I have used https://github.com/qonto/SwiftCompilationTimingParser to measure slow compiling code. The following were the top candidates for optimization:

location	symbol	ms
Sources/SwiftSyntax/generated/SyntaxVisitor.swift:3438:16	instance method visitationFunc(for:)	485.9
Sources/SwiftSyntax/generated/SyntaxRewriter.swift:2113:16	instance method visitationFunc(for:)	315.72
Sources/SwiftSyntax/generated/TokenKind.swift:730:22	static method fromRaw(kind:text:)	113.12
Sources/SwiftSyntax/generated/ChildNameForKeyPath.swift:18:13	global function childName(_:)	371.07
Sources/SwiftSyntax/generated/SyntaxEnum.swift:306:8	instance method as(_:)	102.14

Out of these, only the mentioned 3 were optimizable, whereas SyntaxEnum and ChildNameForKeyPath are slow due to use of AnyKeyPath and keypath operator \, which I was not able to optimize.
After code optimization, the modified methods got the following timing:

location	symbol	ms	improvement
SyntaxVisitor.swift:3438:16	instance method visitationFunc(for:)	42.98	91.16%
SyntaxRewriter.swift:2113:16	instance method visitationFunc(for:)	33.33	89.44%
TokenKind.swift:739:22	static method fromRaw(kind:text:)	33.83	70.09%

Follow-up of #2308

[art-divin]

Including type name in method signature disables the slow typechecker operations within the SyntaxVisitor file (same goes for SyntaxRewriter).

With concrete methods on callee side, there is a drawback that the user of the code needs to think about the type of these methods. However, since performance is more important in my personal opinion than syntax sugar in the generated and library code, here I suggest this change.

[ahoppen]

The problem is that this is an API-breaking change and all clients of swift-syntax would need to update their SyntaxVisitor and SyntaxRewriter subclasses and we don’t have transition path using deprecations. Given that SyntaxVisitor and SyntaxRewriter are used fairly widely, unfortunately I don’t think that’s a change that we can make anymore.

I don’t suppose we can get similar performance benefits by just adding some type annotations in the visitationFunc implementation?

[art-divin]

Hey @ahoppen ,

I have tried using:

return { (arg: Syntax) in
-  self.visitImpl(arg, AccessorBlockSyntax.self, self.visit, self.visitPost)
+  self.visitImpl(arg, AccessorBlockSyntax.self, self.visit as ((AccessorBlockSyntax) ->  SyntaxVisitorContinueKind), self.visitPost as ((AccessorBlockSyntax) -> Void))
}

and

- self.visit
+ { (arg: AccessorBlockSyntax) -> SyntaxVisitorContinueKind in return self.visit(arg) }

though it did not help. Do you have any proposal?

[ahoppen]

Hmm, that’s unfortunate. It’s what I would have tried as well.

[art-divin]

👋🏻 Hello @ahoppen ,

Good news: I have figured it out. Now I need to make a badge for myself that I have defeated Type Checker yet again in the most weirdest way I have ever came up with. Please see if the new solution can be merged, thank you.

I'll proceed with further changes based on your review after a few hours 🤝 Thank you for your review 👍🏻

[art-divin]

@ahoppen I can answer, the reason is the fact that func visitImpl declaration uses generics, and so, type specified for the arguments is "overridden" by the generic argument.

I have tried removing generics, but I hit a compiler bug, but also, I am unsure if it'd even work: swiftlang/swift#69621

[ahoppen]

@xedin and I just chatted about this. The difference seems to be that the constraint solve has optimizations for calls that don’t apply for unapplied function references.

@xedin also pointed out that we are probably hitting quadratic behavior in the number of nodes because both visit and visitPost. We should be able to eliminate that quadratic behavior by splitting the call to visitPost out of visitImpl. If that does indeed give similar improvements in compile time, I would prefer it because it blows up the code base a lot less.

Could you maybe try that? I.e. change it to something like

return {
  self.visitImpl($0, YieldStmtSyntax.self, self.visit)
  self.visitPost($0)
}

[xedin]

It all stems from the fact that there are performance optimizations for calls but not for conversions.

Consider this example which is a reduction of the problem:

class Visitor {
  func visit(_: A) {}
  func visit(_: B) {}
  func visit(_: C) {}
}

struct A {}
struct B {}
struct C {}

func test<T>(_: T.Type, _: (T) -> Void) {}

extension Visitor {
  func visitB() {
    test(B.self, self.visit)
  }
}

Here the solver would iterate over each overload of visit and check whether conversion to other function type holds or not.

The difference between that and a direct call to self.visit($0) is due to how the solver handles "applied" references (very simplified version) - it would match each argument to a corresponding parameter and if they matched exactly it would "favor" the overload.

[art-divin]

Hello @ahoppen , @xedin 👋

Thank you very much for the explanation. Now I know which part of swift compiler I need to debug in order to solve such issues 🤝 (I am trying to fix a bug I've reported in the compiler by myself).

Could you maybe try that? I.e. change it to something like
self.visitImpl($0, YieldStmtSyntax.self, self.visit)
self.visitPost($0)

This does not help, in fact, after applying all sorts of type annotations and refactoring of these methods, I got them down to:

let node = $0.cast(AccessorBlockSyntax.self)
self.visitImpl(node, self.visit)
self.visitPost(node)

... getting more duration than it was before any optimization. I even tried to remove visitImpl completely:

1 return {
2  let node: AccessorBlockSyntax = $0.cast(AccessorBlockSyntax.self)
3  let needsChildren = (self.visit(node) == .visitChildren)
4  if needsChildren && !node.raw.layoutView!.children.isEmpty {
5    self.visitChildren(node)
6  }
7  self.visitPost(node)
8}

and got one of the worst results. Line 3 from the above sample code would yield from 30 to 90 ms for each use case.

My proposal is to leave the private methods unless they cause runtime penalty which I would never will to introduce with such optimization. Since SyntaxVisitor is an open class, I suspect it just might be a no-go due to additional stack entry.

What do you think @ahoppen ?

[art-divin]

@ahoppen I have cleared up the diff, now it is very simple and just adds private methods, apart from other small improvements.

Do you think that it is worth merging this one?

[art-divin]

Hello, dear Swift team 👋🏻

I wish you all a great week ahead, and I hope you'll have a chance to review this PR. I have invested more than 1 month into figuring out how to speed up type checker for these use cases, and I could not speed it up more than this, next step would be (for me personally) to move to Swift repository and fix issues inside of the type checker itself.

Thank you for your work 🤝

[ahoppen]

Thanks for your continued effort to improve the compile times of swift-syntax. I know how tedious a process this is and I really appreciate it 🙏🏽

[ahoppen]

Hi @art-divin,

We just discussed your PR and decided that the added complexity in lines of code is not worth the marginal compile time improvement. I really appreciate the effort you put into this PR and respect the dedication with which you continued measuring the compile times.

[art-divin]

👋🏻 Hello @ahoppen ,

thank you very much for the update 🤝 My pleasure.