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.

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?

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?

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

👋🏻 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 👍🏻

@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: apple/swift#69621

@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)
}

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.

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 ?

@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?