Skip to content
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

[Speedup] [Proof Of Concept] Use unique file Id for each SiwftLintFile for caching #2929

Merged
merged 4 commits into from
Nov 7, 2019

Conversation

PaulTaykalo
Copy link
Collaborator

@PaulTaykalo PaulTaykalo commented Oct 29, 2019

In short

How about having unique key per file?
This can be done in a lot of ways, one of these. is below.
A suggested solution is to have own wrapper over File which will allow storing additional data per file.

What

Change the way how files information is cached in Swiftlint

Why

Speedup mostly


Note

In general, swiftlint is using contents or path as cacheKey. Unfortunately using keys so big is slow.
Using simple, fast, unique cacheKey, allows speed up running time.

On the Image. Most of the time spent in Dictionary.getter is actually calling Cache.get(file.cacheKey)


The worst-case scenario for this one is to have multiple files with identical contents. which is not-really li


Alternatives

  • Having unique key per file in SourceKittent framework
  • making File overrideable. (this actually can decrease speedup)

@PaulTaykalo PaulTaykalo added enhancement Ideas for improvements of existing features and rules. discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions. labels Oct 29, 2019
@@ -0,0 +1,56 @@
import Foundation
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one is mostly what this suggestion about

@@ -1,9 +1,10 @@
import Foundation
import SourceKittenFramework

private typealias FileCacheKey = Int
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And this one

@PaulTaykalo
Copy link
Collaborator Author

The main question here whether it is appropriate or allowable to make changes like this one :)
If YES - in general, but some additional things need to be done - then let's discuss this. If NO - then consider using alternative as suggested.

@SwiftLintBot
Copy link

SwiftLintBot commented Oct 29, 2019

1 Warning
⚠️ Big PR
12 Messages
📖 Linting Aerial with this PR took 1.49s vs 1.94s on master (23% faster)
📖 Linting Alamofire with this PR took 2.77s vs 3.2s on master (13% faster)
📖 Linting Firefox with this PR took 8.24s vs 11.04s on master (25% faster)
📖 Linting Kickstarter with this PR took 16.97s vs 26.79s on master (36% faster)
📖 Linting Moya with this PR took 1.61s vs 2.04s on master (21% faster)
📖 Linting Nimble with this PR took 1.55s vs 2.49s on master (37% faster)
📖 Linting Quick with this PR took 0.56s vs 1.08s on master (48% faster)
📖 Linting Realm with this PR took 2.45s vs 3.31s on master (25% faster)
📖 Linting SourceKitten with this PR took 1.02s vs 1.64s on master (37% faster)
📖 Linting Sourcery with this PR took 3.28s vs 3.88s on master (15% faster)
📖 Linting Swift with this PR took 13.11s vs 17.67s on master (25% faster)
📖 Linting WordPress with this PR took 17.96s vs 29.8s on master (39% faster)

Generated by 🚫 Danger

Copy link
Collaborator

@norio-nomura norio-nomura left a comment

Choose a reason for hiding this comment

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

If we introduce a new type called SwiftLintFile, SwiftLintFile should have them as lazy properties rather than using File.cacheKey to retrieve data from the caches. 🤔

@PaulTaykalo
Copy link
Collaborator Author

PaulTaykalo commented Oct 29, 2019

If we introduce a new type called SwiftLintFile, SwiftLintFile should have them as lazy properties rather than using File.cacheKey to retrieve data from the caches. 🤔

While this is true, it's really hard to clear all caches if we go that direction.

@PaulTaykalo PaulTaykalo force-pushed the speedup/test-use-cache-as-int branch 2 times, most recently from 8087127 to 4e8fa64 Compare October 29, 2019 15:48
@PaulTaykalo PaulTaykalo changed the title [Speedup] [Proof Of Concept] Use unique file Id for each File [Speedup] [Proof Of Concept] Use unique file Id for each SiwftLintFile for caching Oct 29, 2019
@PaulTaykalo
Copy link
Collaborator Author

This one is probably, the most we can take out in terms of speed from SwiftLint not touching regular expressions.

@jpsim @marcelofabri Please, review

@PaulTaykalo
Copy link
Collaborator Author

@jpsim ping?

@PaulTaykalo PaulTaykalo force-pushed the speedup/use-swift-declaration-kinds-enums-instead-of-rawValues branch from 6497350 to b1cdc11 Compare November 7, 2019 09:05
@PaulTaykalo PaulTaykalo changed the base branch from speedup/use-swift-declaration-kinds-enums-instead-of-rawValues to master November 7, 2019 11:13
@PaulTaykalo PaulTaykalo merged commit 4513190 into master Nov 7, 2019
@PaulTaykalo PaulTaykalo deleted the speedup/test-use-cache-as-int branch November 7, 2019 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions. enhancement Ideas for improvements of existing features and rules.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants