Performance regressions in latest 509.0.0 releases

[SimplyDanny]

This is not a bug. I just want to inform you about performance regressions we noticed while integrating the two latest 509.0.0 releases of SwiftSyntax into SwiftLint.

In numbers, we see a slowdown of 2% - 3% upgrading from 509.0.0-swift-DEVELOPMENT-SNAPSHOT-2023-07-09-a to 509.0.0-swift-DEVELOPMENT-SNAPSHOT-2023-08-07-a and another 2% - 3% drop when moving to 509.0.0-swift-DEVELOPMENT-SNAPSHOT-2023-08-28-a.

That's probably something you might be interested in.

[ahoppen]

Tracked in Apple’s issue tracker as rdar://115001578

[ahoppen]

Thanks for reporting this. That’s valuable information. The 2-3% performance regression when moving from 07-09 to 08-07 is probably not something that’s reasonable to investigate since it includes way too many commits (08-07 was the first one that we tagged of main again instead of release/5.9).

The regression from 08-07 to 08-28 is quite interesting though. Are you able to bisect which PR might have introduced the regression or is there an easy way to replicate the performance measurements so I can run them?

[SimplyDanny]

Unfortunately, I'm not able to boil this down to a specific commit. It's also hard to reproduce the results in any case.

I've done a few tests with the swift-parser-cli tool parsing the SwiftSyntax repository itself with the following results:

Tag	Runtime	Change
2023-07-09-a	488.95ms
2023-08-07-a	520.92ms	+6.5%
2023-08-28-a	516.07ms	-0.9%
509.0.0	520.59ms	+0.8%

As we see, there's actually been no change since 2023-08-07-a. But that's just parsing. SwiftLint uses a bit more of SwiftSyntax' infrastructure on top of parsing. Perhaps the regression happens there. But as said, it's difficult to analyze that.

Are there performance tests in SwiftSyntax running regularly or even on every PR build?

[ahoppen]

As we see, there's actually been no change since 2023-08-07-a. But that's just parsing. SwiftLint uses a bit more of SwiftSyntax' infrastructure on top of parsing. Perhaps the regression happens there. But as said, it's difficult to analyze that.

Are there performance tests in SwiftSyntax running regularly or even on every PR build?

We gather performance measurements when running SwiftParser as part of the SourceKit Stress Tester and I’m monitoring these changes. I also did ad-hoc performance measurements of swift-format for PRs that seemed like they could be risky but haven’t been monitoring that performance continuously.

I was thinking to set up some kind of performance monitoring and would be interested in using SwiftLint as a benchmark if there’s a stable way to get the performance numbers.

[SimplyDanny]

Sorry for the late reply. I've totally missed this thread. I come back to this now that we were about to upgrade to SwiftSyntax 510.0.0, but another performance regression of 9% - 13% is holding us back at the moment. Please see the PR for the update. Is there any obvious reason for the performance decrease, @ahoppen?

I was thinking to set up some kind of performance monitoring and would be interested in using SwiftLint as a benchmark if there’s a stable way to get the performance numbers.

Performance in PRs in SwiftLint is measured on dedicated real machines (no VMs) as follows:

• Two binaries are built with Bazel from both main and the branch that's requested to be merged into main.
• Both are run on a set of open source Swift repositories by enabling all rules available or only the ones that got changed.

Due to the use of non-virtual machines, the benchmark results are quite reliable.

Most of the rules are meanwhile based on SwiftSytax (parsing and visiting). But some still require SourceKit as well.

[ahoppen]

Do you have some kind of script/commands that I could use to reproduce the measurements? I would like to bisect this on swift-syntax because there really haven’t been many changes between 509 and 510.

I did just measure parsing performance using our swift-parser-cli performance-test script and that only showed a 1.5% performance decrease between the two releases, which is in a completely different ballpark.

[SimplyDanny]

Do you have some kind of script/commands that I could use to reproduce the measurements?

On the PR branch, you can run ./tools/oss-check. It uses Bazel to build. If you'd like to build with Swift Package Manager instead, you may adapt the command accordingly.

[ahoppen]

Oh, that’s great, thanks! I wasn’t expecting something that simple. I was able to reproduce the regression locally using the script. I’ll bisect it now, trying to find out what in swift-syntax might have caused it. I’ll post an update once I’ve got more information.

[ahoppen]

I think I have found the culprit: #2038 I’m profiling it now to see if there’s something we can do to mitigate the performance impact of that PR.

[ahoppen]

I knelt down into SyntaxVisitor yesterday and was able to optimize it quite a bit. Instead of a ~10% performance regression, I am seeing a ~10% performance improvement now when building SwiftLint with swift-syntax from #2537. More details are in the PR.

@SimplyDanny Would it be possible for you to test SwiftLint’s performance with that branch, just to double-check that my measurements are correct and that there isn’t another regression hiding somewhere?

I can’t make any statements on whether we can or will include the 510 release series of swift-syntax. Changing memory management is always risky and we need to assess the risk here.

[SimplyDanny]

I checked your branch in the same PR and indeed see an impressive performance improvement of 5% - 25% depending on the linted repository compared to 509. Well done! 🎉

[ahoppen]

Wohoo. That’s great. 🏎️

We deemed the risk of taking this change into a patch release for the 510 series too big because it’s modifying the memory management in a non-trivial way and it’ll thus be available in the 600 release series aligned with Swift 6.

[SimplyDanny]

We deemed the risk of taking this change into a patch release for the 510 series too big ...

As a side note: Apart from the performance improvements, all tests passed and no crashes appeared while scanning several open source repositories.

Any chance this change can be part of a beta version on the 600 line already?