Split cache into read cache and write cache #1551

marcelofabri · 2017-05-23T15:47:07Z

Alternative fix for #1550 as suggested by @norio-nomura

This makes things slightly more complex, but it reduced by 50-70% the execution time when there's a cache available.

jpsim · 2017-05-23T15:57:45Z

I like the general idea, but flushWriteCache() mutates readCache but reads to readCache aren't locked, so the mutation could happen in the middle of reading.

SwiftLintBot · 2017-05-23T16:00:10Z

	12 Messages
📖	Linting Aerial with this PR took 0.52s vs 0.36s on master (44% slower)
📖	Linting Alamofire with this PR took 2.74s vs 2.46s on master (11% slower)
📖	Linting Firefox with this PR took 12.47s vs 10.38s on master (20% slower)
📖	Linting Kickstarter with this PR took 14.7s vs 14.11s on master (4% slower)
📖	Linting Moya with this PR took 0.34s vs 0.34s on master (0% slower)
📖	Linting Nimble with this PR took 1.33s vs 1.36s on master (2% faster)
📖	Linting Quick with this PR took 0.43s vs 0.47s on master (8% faster)
📖	Linting Realm with this PR took 2.04s vs 2.16s on master (5% faster)
📖	Linting SourceKitten with this PR took 0.89s vs 0.91s on master (2% faster)
📖	Linting Sourcery with this PR took 2.14s vs 2.24s on master (4% faster)
📖	Linting Swift with this PR took 10.08s vs 9.87s on master (2% slower)
📖	Linting WordPress with this PR took 10.43s vs 10.51s on master (0% faster)

Generated by 🚫 Danger

marcelofabri · 2017-05-23T16:00:10Z

Yes, but this flushWriteCache should only be called after the violations were processed. Everything was originally on save, but I had to expose it for unit tests 😅

jpsim · 2017-05-23T16:00:33Z

Source/SwiftLintFramework/Models/LinterCache.swift

        lock.unlock()
        try json.write(to: url, atomically: true, encoding: .utf8)
    }

+    internal func flushWriteCache() {


flushWriteCacheToReadCache() might be a clearer (but more verbose) name?

Otherwise it isn't clear that this function is mutating readCache.

Sounds good to me - this method shouldn't be used often, so I'm cool with it being more verbose

jpsim · 2017-05-23T16:03:08Z

Yes, but this flushWriteCache should only be called after the violations were processed. Everything was originally on save, but I had to expose it for unit tests 😅

I guess this brings us to whether or not we want to properly support arbitrary uses of SwiftLintFramework outside of just our swiftlint CLI. We may not call save() while other linters are running, but the public API certainly allows you to do so.

marcelofabri · 2017-05-23T16:06:34Z

We may not call save() while other linters are running, but the public API certainly allows you to do so.

Agreed, but I think we should be fine if we document this limitation. I think the performance win for swiftlint is worth the less safety for other (potential) consumers of the public API.

jpsim · 2017-05-23T16:10:40Z

I agree, but it should be clearly documented.

marcelofabri · 2017-05-23T16:11:31Z

I agree, but it should be clearly documented.

Sure, will add that later and a changelog entry. Just wanted to get feedback on the general approach first 😊

jpsim · 2017-05-23T16:13:21Z

Source/SwiftLintFramework/Models/LinterCache.swift

+        guard !writeCache.isEmpty else {
+            return
+        }
+        flushWriteCacheToReadCache()
        lock.lock()


This lock() (and subsequent unlock()) can be removed if we maintain that:

readCache can only be mutated in flushWriteCacheToReadCache()

flushWriteCacheToReadCache() can only be called from save()

save() cannot be called concurrently

Actually, we can make save() thread-safe if we think about a way to support the unit tests. Maybe something like this

func flushWriteCacheToReadCache() { readCache = merge() } func merge() -> [String: Any] { // ... } func save() { // ... let cache = merge() let json = toJSON(cache) // ... }

(please ignore the bad names and lack of details)

I think that's doable. Give it a try and see! 😄

norio-nomura · 2017-05-24T03:58:20Z

Tests/SwiftLintFrameworkTests/LinterCacheTests.swift

@@ -89,6 +87,8 @@ class LinterCacheTests: XCTestCase {
    private func cacheAndValidate(violations: [StyleViolation], forFile: String, configuration: Configuration,
                                  file: StaticString = #file, line: UInt = #line) {
        cache.cache(violations: violations, forFile: forFile, configuration: configuration)
+        cache.flushWriteCacheToReadCache()


If this test is the only reason that readCache is mutable, how about making a method that returns a new LinterCache instance from mergeCaches() and using it here, and changing readCache to immutable?

norio-nomura · 2017-05-24T04:48:26Z

Source/SwiftLintFramework/Models/LinterCache.swift

@@ -15,30 +15,35 @@ internal enum LinterCacheError: Error {
 }

 public final class LinterCache {
-    private var cache = [String: Any]()
+    private var readCache = [String: Any]()
+    private var writeCache = [String: Any]()


Changing readCache and writeCache from [String: Any] to [String: [String: Any]] boosted performance on my test.

codecov-io · 2017-05-24T10:36:51Z

Codecov Report

Merging #1551 into master will increase coverage by 0.3%.
The diff coverage is 72.09%.

@@            Coverage Diff            @@
##           master    #1551     +/-   ##
=========================================
+ Coverage   83.75%   84.06%   +0.3%     
=========================================
  Files         188      189      +1     
  Lines        9597     9726    +129     
=========================================
+ Hits         8038     8176    +138     
+ Misses       1559     1550      -9

Impacted Files	Coverage Δ
Source/SwiftLintFramework/Models/LinterCache.swift	`77.31% <71.79%> (-1.35%)`	⬇️
...sts/SwiftLintFrameworkTests/LinterCacheTests.swift	`86.19% <75%> (+0.77%)`	⬆️
...tLintFramework/Rules/OverriddenSuperCallRule.swift	`100% <0%> (ø)`	⬆️
...SwiftLintFramework/Rules/ProhibitedSuperRule.swift	`100% <0%> (ø)`	⬆️
...wiftLintFramework/Models/Configuration+Cache.swift	`51.28% <0%> (ø)`
...intFramework/Extensions/Dictionary+SwiftLint.swift	`85.5% <0%> (+0.21%)`	⬆️
...urce/SwiftLintFramework/Models/Configuration.swift	`94.28% <0%> (+13.44%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ed4424...251f8b8. Read the comment docs.

norio-nomura

Looks good to me other than one comment.
On linting Carthage's repository and hitting all the cache,
rc1:

swiftlint lint --enable-all-rules --quiet 13.27s user 1.22s system 128% cpu 11.245 total

0ab8bfa:

swiftlint lint --enable-all-rules --quiet 14.30s user 1.27s system 197% cpu 7.900 total

98832ab:

swiftlint lint --enable-all-rules --quiet 5.72s user 1.06s system 143% cpu 4.737 total

👍

norio-nomura · 2017-05-24T09:43:46Z

Source/SwiftLintFramework/Models/LinterCache.swift

        for (key, value) in writeCache {
-            var filesCache = (cache[key] as? [String: Any]) ?? [:]
-            for (file, fileCache) in (value as? [String: Any]) ?? [:] {
+            var filesCache = cache[key] ?? [:]


If key does not exist, value can be inserted to cache[key] directly.
Sorry, I was aware of it before but I forgot to write a comment.

Yes, but is it worth the extra complexity?

for (key, value) in writeCache { if var filesCache = cache[key] { for (file, fileCache) in value { filesCache[file] = fileCache } cache[key] = filesCache } else { cache[key] = value } }

jpsim

Looks great!

jpsim · 2017-05-24T14:55:14Z

Tests/SwiftLintFrameworkTests/LinterCacheTests.swift

-        let cache = LinterCache()
-        cache.fileManager = TestFileManager()
-        return cache
+        return LinterCache(fileManager: TestFileManager())


This doesn't need to be in a closure anymore.

jpsim · 2017-05-24T14:56:10Z

Source/SwiftLintFramework/Models/LinterCache.swift

@@ -49,12 +66,12 @@ public final class LinterCache {
        let configurationDescription = configuration.cacheDescription

        lock.lock()
-        var filesCache = (cache[configurationDescription] as? [String: Any]) ?? [:]
+        var filesCache = (writeCache[configurationDescription]) ?? [:]


Parens are surperfluous now.

jpsim · 2017-05-24T14:57:54Z

CHANGELOG.md

@@ -93,6 +93,10 @@
  [Marcelo Fabri](https://github.com/marcelofabri)
  [#1504](https://github.com/realm/SwiftLint/issues/1504)

+* Improve cache performance by splitting it into read cache and write cache.  


Just add your name & issue to the item introducing caching? We don't usually include fixes or improvements to unreleased things in the changelog.

marcelofabri · 2017-05-24T15:38:24Z

🚢

marcelofabri requested review from norio-nomura and jpsim May 23, 2017 15:47

jpsim reviewed May 23, 2017

View reviewed changes

marcelofabri force-pushed the write-cache branch from 058ed2c to cb2a21c Compare May 23, 2017 16:05

jpsim reviewed May 23, 2017

View reviewed changes

marcelofabri force-pushed the write-cache branch from cb2a21c to 0ab8bfa Compare May 23, 2017 16:27

norio-nomura reviewed May 24, 2017

View reviewed changes

norio-nomura approved these changes May 24, 2017

View reviewed changes

jpsim approved these changes May 24, 2017

View reviewed changes

Split cache into read cache and write cache

9a5e30e

marcelofabri force-pushed the write-cache branch from 251f8b8 to 9a5e30e Compare May 24, 2017 15:03

marcelofabri merged commit bdcfe1b into master May 24, 2017

marcelofabri deleted the write-cache branch May 24, 2017 15:38

marcelofabri mentioned this pull request May 24, 2017

Unneeded lock in cache? #1550

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split cache into read cache and write cache #1551

Split cache into read cache and write cache #1551

marcelofabri commented May 23, 2017

jpsim commented May 23, 2017

SwiftLintBot commented May 23, 2017 •

edited

Loading

marcelofabri commented May 23, 2017

jpsim May 23, 2017

jpsim May 23, 2017

marcelofabri May 23, 2017

jpsim commented May 23, 2017

marcelofabri commented May 23, 2017

jpsim commented May 23, 2017

marcelofabri commented May 23, 2017

jpsim May 23, 2017

marcelofabri May 23, 2017

jpsim May 23, 2017

marcelofabri May 23, 2017

norio-nomura May 24, 2017

norio-nomura May 24, 2017

codecov-io commented May 24, 2017 •

edited

Loading

norio-nomura left a comment

norio-nomura May 24, 2017

marcelofabri May 24, 2017

jpsim left a comment

jpsim May 24, 2017

jpsim May 24, 2017

jpsim May 24, 2017

marcelofabri commented May 24, 2017

Split cache into read cache and write cache #1551

Split cache into read cache and write cache #1551

Conversation

marcelofabri commented May 23, 2017

jpsim commented May 23, 2017

SwiftLintBot commented May 23, 2017 • edited Loading

marcelofabri commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim commented May 23, 2017

marcelofabri commented May 23, 2017

jpsim commented May 23, 2017

marcelofabri commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 24, 2017 • edited Loading

Codecov Report

norio-nomura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri commented May 24, 2017

SwiftLintBot commented May 23, 2017 •

edited

Loading

codecov-io commented May 24, 2017 •

edited

Loading