realm
Enable caching for collecting rules
Hi 👋
This PR enables caching for collecting rules
Main idea
The collection step required by collecting rules is usually a very time consuming process. Under some scenarios and making some assumptions, the result of such computation can be cached
Implementation details:
- Each collecting rule can opt in by conforming their
FileInfotype toCollectingCachable - The easiest way to conform to
CollectingCachableis makingFileInfoconform toCodable. If that is undesired, then creating a middleware Dto is also possible
Discussion
As an example, I enabled caching for the two collecting rules in the codebase: UnusedDeclarationRule and CaptureVariableRule. Both rules query SourceKitService during collection. The main assumption that allows caching is that if a swift file and its compiler arguments do not change => the values returned by SourceKitService do not change either, and can be cached and reused between SwiftLint executions.
This is specially useful for the UnusedDeclarationRule, as the process of removing all unused declarations is iterative: removing flagged unused declarations will uncover new unused declarations. In a big codebase where UnusedDeclarationRule takes long time to execute, caching the collection information dramatically speeds up the code removal process
| 12 Messages | |
|---|---|
| :book: | Linting Aerial with this PR took 0.96s vs 0.95s on master (1% slower) |
| :book: | Linting Alamofire with this PR took 1.01s vs 1.0s on master (1% slower) |
| :book: | Linting Firefox with this PR took 3.43s vs 3.4s on master (0% slower) |
| :book: | Linting Kickstarter with this PR took 6.76s vs 6.7s on master (0% slower) |
| :book: | Linting Moya with this PR took 4.35s vs 4.32s on master (0% slower) |
| :book: | Linting Nimble with this PR took 0.39s vs 0.38s on master (2% slower) |
| :book: | Linting Quick with this PR took 0.17s vs 0.17s on master (0% slower) |
| :book: | Linting Realm with this PR took 6.11s vs 6.11s on master (0% slower) |
| :book: | Linting SourceKitten with this PR took 0.31s vs 0.3s on master (3% slower) |
| :book: | Linting Sourcery with this PR took 1.86s vs 1.85s on master (0% slower) |
| :book: | Linting Swift with this PR took 2.85s vs 2.82s on master (1% slower) |
| :book: | Linting WordPress with this PR took 6.72s vs 6.64s on master (1% slower) |
Generated by :no_entry_sign: Danger
@jpsim Would appreciate some feedback, so I can understand if this is something you would accept
Thanks for the PR and the discussion! I think this is an important optimization to make analyzer rules significantly more useful. Were you able to measure a significant performance improvement on a real world codebase? I think it's important to be able to measure if this is having the expected impact you want.
Also if you rebase, hopefully OSSCheck will produce more reasonable results than what's currently posted above. Not that it matters much for this PR since OSSCheck doesn't run any analyzer rules.
Were you able to measure a significant performance improvement on a real world codebase? I think it's important to be able to measure if this is having the expected impact you want.
I did not start using this on a real world codebase yet. Will do that and come back with an answer
Thanks! I wonder if caching the result of cursor-info requests is the best solution here, since that's really the expensive part that all analyzer rules need.
Thanks! I wonder if caching the result of cursor-info requests is the best solution here, since that's really the expensive part that all analyzer rules need.
Mmm 🤔Seems like if file did not change (and compile arguments did not change - I will update the PR to take them into account - EDIT: done), caching only cursor-info or caching all the collection information should be the same. In such case I think makes sense to cache all the collection information for better performance