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

Improve Linux performance #1577

Closed
jpsim opened this Issue May 27, 2017 · 8 comments

Comments

Projects
None yet
2 participants
@jpsim
Collaborator

jpsim commented May 27, 2017

86% of the time spent linting the SwiftLint repo on Linux is in CFStringFindCharacterFromSet.

image

Interactive version (open in a web browser): flamegraph.svg.zip

@jpsim jpsim added the enhancement label May 27, 2017

@norio-nomura

This comment has been minimized.

Show comment
Hide comment
Collaborator

norio-nomura commented May 27, 2017

@jpsim

This comment has been minimized.

Show comment
Hide comment
@jpsim

jpsim May 27, 2017

Collaborator

Actually, enabling caching on Linux has a great impact:

image

flamegraph.svg.zip

Just a naive implementation:

diff --git a/Source/SourceKittenFramework/String+SourceKitten.swift b/Source/SourceKittenFramework/String+SourceKitten.swift
index c40cffe..14163a1 100644
--- a/Source/SourceKittenFramework/String+SourceKitten.swift
+++ b/Source/SourceKittenFramework/String+SourceKitten.swift
@@ -35,6 +35,10 @@ private let commentLinePrefixCharacterSet: CharacterSet = {
 private var keyCacheContainer = 0
 
 extension NSString {
+
+    static private var stringCache = [NSString: CacheContainer]()
+    static private var stringCacheLock = NSLock()
+
     /**
     CacheContainer caches:
 
@@ -181,7 +185,14 @@ extension NSString {
     */
     private var cacheContainer: CacheContainer {
         #if os(Linux)
-        return CacheContainer(self)
+        NSString.stringCacheLock.lock()
+        defer { NSString.stringCacheLock.unlock() }
+        if let cache = NSString.stringCache[self] {
+            return cache
+        }
+        let cache = CacheContainer(self)
+        NSString.stringCache[self] = cache
+        return cache
         #else
         if let cache = objc_getAssociatedObject(self, &keyCacheContainer) as? CacheContainer {
             return cache
Collaborator

jpsim commented May 27, 2017

Actually, enabling caching on Linux has a great impact:

image

flamegraph.svg.zip

Just a naive implementation:

diff --git a/Source/SourceKittenFramework/String+SourceKitten.swift b/Source/SourceKittenFramework/String+SourceKitten.swift
index c40cffe..14163a1 100644
--- a/Source/SourceKittenFramework/String+SourceKitten.swift
+++ b/Source/SourceKittenFramework/String+SourceKitten.swift
@@ -35,6 +35,10 @@ private let commentLinePrefixCharacterSet: CharacterSet = {
 private var keyCacheContainer = 0
 
 extension NSString {
+
+    static private var stringCache = [NSString: CacheContainer]()
+    static private var stringCacheLock = NSLock()
+
     /**
     CacheContainer caches:
 
@@ -181,7 +185,14 @@ extension NSString {
     */
     private var cacheContainer: CacheContainer {
         #if os(Linux)
-        return CacheContainer(self)
+        NSString.stringCacheLock.lock()
+        defer { NSString.stringCacheLock.unlock() }
+        if let cache = NSString.stringCache[self] {
+            return cache
+        }
+        let cache = CacheContainer(self)
+        NSString.stringCache[self] = cache
+        return cache
         #else
         if let cache = objc_getAssociatedObject(self, &keyCacheContainer) as? CacheContainer {
             return cache
@jpsim

This comment has been minimized.

Show comment
Hide comment
@jpsim

jpsim May 27, 2017

Collaborator

With the caching above, linting this repo takes 15.04s rather than the previous 3m23s.

Collaborator

jpsim commented May 27, 2017

With the caching above, linting this repo takes 15.04s rather than the previous 3m23s.

@norio-nomura

This comment has been minimized.

Show comment
Hide comment
@norio-nomura

norio-nomura May 27, 2017

Collaborator

Yup,. The disadvantage of that implementation is that "CacheContainer and String are never freed".

Collaborator

norio-nomura commented May 27, 2017

Yup,. The disadvantage of that implementation is that "CacheContainer and String are never freed".

@jpsim

This comment has been minimized.

Show comment
Hide comment
@jpsim

jpsim May 28, 2017

Collaborator

True, but we already make that trade off on macOS. Linting this repo in debug mode with a dictionary cache above takes the same amount of memory as with ObjC associated objects (46MB).

Collaborator

jpsim commented May 28, 2017

True, but we already make that trade off on macOS. Linting this repo in debug mode with a dictionary cache above takes the same amount of memory as with ObjC associated objects (46MB).

@norio-nomura

This comment has been minimized.

Show comment
Hide comment
@norio-nomura

norio-nomura May 29, 2017

Collaborator

Certainly File will persist until the end of the SwiftLint process, so neither String nor CacheContainer will be freed until the end. 🤔
I don't know about another consumer of SourceKitten. Are memory leaks due to intentional circular references an issue for other SourceKittenFramework users?

Collaborator

norio-nomura commented May 29, 2017

Certainly File will persist until the end of the SwiftLint process, so neither String nor CacheContainer will be freed until the end. 🤔
I don't know about another consumer of SourceKitten. Are memory leaks due to intentional circular references an issue for other SourceKittenFramework users?

@jpsim

This comment has been minimized.

Show comment
Hide comment
@jpsim

jpsim May 29, 2017

Collaborator

It probably is an issue, but not at the expense of performance. I'd say we should implement the dictionary based caching approach for now and eventually move that to File.

Collaborator

jpsim commented May 29, 2017

It probably is an issue, but not at the expense of performance. I'd say we should implement the dictionary based caching approach for now and eventually move that to File.

@norio-nomura

This comment has been minimized.

Show comment
Hide comment
@norio-nomura

norio-nomura May 29, 2017

Collaborator

I'd say we should implement the dictionary based caching approach for now and eventually move that to File.

I fully agree. 👍

Collaborator

norio-nomura commented May 29, 2017

I'd say we should implement the dictionary based caching approach for now and eventually move that to File.

I fully agree. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment