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.

Sign up for GitHub

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

Jump to bottom

Unneeded lock in cache? #1550

Closed
marcelofabri opened this issue May 23, 2017 · 8 comments
Closed

Unneeded lock in cache? #1550

marcelofabri opened this issue May 23, 2017 · 8 comments
Labels
enhancement Ideas for improvements of existing features and rules.

Comments

@marcelofabri
Copy link
Collaborator

I think this lock is unneeded: https://github.com/realm/SwiftLint/blob/master/Source/SwiftLintFramework/Models/LinterCache.swift#L68

Removing this could bring major performance benefits for the use case of running without any modifications, as it's slower than what I'd expect:

$ time Pods/SwiftLint/swiftlint lint --quiet
Pods/SwiftLint/swiftlint lint --quiet  5.96s user 0.26s system 122% cpu 5.065 total
@marcelofabri marcelofabri added the enhancement Ideas for improvements of existing features and rules. label May 23, 2017
@marcelofabri
Copy link
Collaborator Author

I've tested it locally and now I get ~2.0s total

@norio-nomura
Copy link
Collaborator

norio-nomura commented May 23, 2017 •
edited
Loading

On checking this, Thread Santizer detects following in violations(forFile:configuration:):

==================
WARNING: ThreadSanitizer: data race (pid=14103)
  Read of size 8 at 0x7d18000083d8 by thread T5:
    #0 LinterCache.fileManager.getter LinterCache.swift (SwiftLintFramework:x86_64+0x2d2d17)
    #1 LinterCache.violations(forFile : String, configuration : Configuration) -> [StyleViolation]? LinterCache.swift:62 (SwiftLintFramework:x86_64+0x2d9729)
    #2 Linter.cachedStyleViolations(benchmark : Bool) -> ([StyleViolation], [(id : String, time : Double)])? Linter.swift:103 (SwiftLintFramework:x86_64+0x81a55)
    #3 Linter.getStyleViolations(benchmark : Bool) -> ([StyleViolation], [(id : String, time : Double)]) Linter.swift:70 (SwiftLintFramework:x86_64+0x7f287)
    #4 Linter.styleViolations.getter Linter.swift:61 (SwiftLintFramework:x86_64+0x7eced)
    #5 LintCommand.(run(LintOptions) -> Result<(), CommandantError<()>>).(closure #1) LintCommand.swift:40 (swiftlint:x86_64+0x10001c8a6)
    #6 partial apply for LintCommand.(run(LintOptions) -> Result<(), CommandantError<()>>).(closure #1) LintCommand.swift (swiftlint:x86_64+0x10001d0a5)
    #7 Configuration.(visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>>).(closure #2).(closure #1).(closure #2) Configuration+CommandLine.swift:89 (swiftlint:x86_64+0x10000b4b4)
    #8 partial apply for Configuration.(visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>>).(closure #2).(closure #1).(closure #2) Configuration+CommandLine.swift (swiftlint:x86_64+0x10000e671)
    #9 thunk Configuration+CommandLine.swift (swiftlint:x86_64+0x10000b5f1)
    #10 partial apply for thunk Configuration+CommandLine.swift (swiftlint:x86_64+0x10000e760)
    #11 _TF10ObjectiveC15autoreleasepoolurFzT8invokingFzT_x_x <null>:3197024 (libswiftObjectiveC.dylib:x86_64+0x35d7)
    #12 partial apply for Configuration.(visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>>).(closure #2).(closure #1) Configuration+CommandLine.swift (swiftlint:x86_64+0x10000df29)
    #13 Configuration.(visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>>).(closure #2).(closure #2) Configuration+CommandLine.swift:94 (swiftlint:x86_64+0x10000b704)
    #14 partial apply for Configuration.(visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>>).(closure #2).(closure #2) Configuration+CommandLine.swift (swiftlint:x86_64+0x10000e18a)
    #15 __wrap_dispatch_apply_block_invoke <null>:3197024 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x65371)
    #16 _dispatch_client_callout2 <null>:3197024 (libdispatch.dylib:x86_64+0xe8f3)

  Previous write of size 8 at 0x7d18000083d8 by thread T1:
    #0 LinterCache.fileManager.getter LinterCache.swift:20 (SwiftLintFramework:x86_64+0x2d3663)
    #1 LinterCache.violations(forFile : String, configuration : Configuration) -> [StyleViolation]? LinterCache.swift:62 (SwiftLintFramework:x86_64+0x2d9729)
    #2 Linter.cachedStyleViolations(benchmark : Bool) -> ([StyleViolation], [(id : String, time : Double)])? Linter.swift:103 (SwiftLintFramework:x86_64+0x81a55)
    #3 Linter.getStyleViolations(benchmark : Bool) -> ([StyleViolation], [(id : String, time : Double)]) Linter.swift:70 (SwiftLintFramework:x86_64+0x7f287)
    #4 Linter.styleViolations.getter Linter.swift:61 (SwiftLintFramework:x86_64+0x7eced)
    #5 LintCommand.(run(LintOptions) -> Result<(), CommandantError<()>>).(closure #1) LintCommand.swift:40 (swiftlint:x86_64+0x10001c8a6)
    #6 partial apply for LintCommand.(run(LintOptions) -> Result<(), CommandantError<()>>).(closure #1) LintCommand.swift (swiftlint:x86_64+0x10001d0a5)
    #7 Configuration.(visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>>).(closure #2).(closure #1).(closure #2) Configuration+CommandLine.swift:89 (swiftlint:x86_64+0x10000b4b4)
    #8 partial apply for Configuration.(visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>>).(closure #2).(closure #1).(closure #2) Configuration+CommandLine.swift (swiftlint:x86_64+0x10000e671)
    #9 thunk Configuration+CommandLine.swift (swiftlint:x86_64+0x10000b5f1)
    #10 partial apply for thunk Configuration+CommandLine.swift (swiftlint:x86_64+0x10000e760)
    #11 _TF10ObjectiveC15autoreleasepoolurFzT8invokingFzT_x_x <null>:3197024 (libswiftObjectiveC.dylib:x86_64+0x35d7)
    #12 partial apply for Configuration.(visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>>).(closure #2).(closure #1) Configuration+CommandLine.swift (swiftlint:x86_64+0x10000df29)
    #13 Configuration.(visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>>).(closure #2).(closure #2) Configuration+CommandLine.swift:94 (swiftlint:x86_64+0x10000b704)
    #14 partial apply for Configuration.(visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>>).(closure #2).(closure #2) Configuration+CommandLine.swift (swiftlint:x86_64+0x10000e18a)
    #15 __wrap_dispatch_apply_block_invoke <null>:3197024 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x65371)
    #16 _dispatch_client_callout2 <null>:3197024 (libdispatch.dylib:x86_64+0xe8f3)
    #17 _TZFE8DispatchCSo13DispatchQueue17concurrentPerformfT10iterationsSi7executeFSiT__T_ <null>:3197024 (libswiftDispatch.dylib:x86_64+0xb118)
    #18 partial apply for Configuration.(visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>>).(closure #2) Configuration+CommandLine.swift (swiftlint:x86_64+0x1000091d0)
    #19 thunk Configuration+CommandLine.swift (swiftlint:x86_64+0x1000078ab)
    #20 partial apply for thunk Configuration+CommandLine.swift (swiftlint:x86_64+0x1000092d0)
    #21 Result.analysis<A> (ifSuccess : (A) -> A1, ifFailure : (B) -> A1) -> A1 Result.swift:57 (Result:x86_64+0x9b2c)
    #22 protocol witness for ResultProtocol.analysis<A> (ifSuccess : (A.Value) -> A1, ifFailure : (A.Error) -> A1) -> A1 in conformance <A, B where ...> Result<A, B> Result.swift (Result:x86_64+0xcaa4)
    #23 ResultProtocol.flatMap<A> ((A.Value) -> Result<A1, A.Error>) -> Result<A1, A.Error> ResultProtocol.swift:51 (Result:x86_64+0x26bf)
    #24 Configuration.visitLintableFiles(path : String, action : String, useSTDIN : Bool, quiet : Bool, useScriptInputFiles : Bool, cache : LinterCache?, parallel : Bool, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>> Configuration+CommandLine.swift:100 (swiftlint:x86_64+0x100005d6e)
    #25 Configuration.visitLintableFiles(options : LintOptions, cache : LinterCache?, visitorBlock : (Linter) -> ()) -> Result<[File], CommandantError<()>> Configuration+CommandLine.swift:133 (swiftlint:x86_64+0x10000cdfd)
    #26 LintCommand.run(LintOptions) -> Result<(), CommandantError<()>> LintCommand.swift:47 (swiftlint:x86_64+0x10001a90e)
    #27 protocol witness for CommandProtocol.run(A.Options) -> Result<(), A.ClientError> in conformance LintCommand LintCommand.swift (swiftlint:x86_64+0x100025e20)
    #28 CommandWrapper.(init<A where ...> (A1) -> CommandWrapper<A>).(closure #1) Command.swift:55 (Commandant:x86_64+0x24537)
    #29 partial apply for CommandWrapper.(init<A where ...> (A1) -> CommandWrapper<A>).(closure #1) Command.swift (Commandant:x86_64+0x2a330)
    #30 CommandRegistry.run(command : String, arguments : [String]) -> Result<(), CommandantError<A>>? Command.swift:102 (Commandant:x86_64+0x26494)
    #31 CommandRegistry.main(arguments : [String], defaultVerb : String, errorHandler : (A) -> ()) -> Never Command.swift:167 (Commandant:x86_64+0x271f8)
    #32 CommandRegistry.main(defaultVerb : String, errorHandler : (A) -> ()) -> Never Command.swift:131 (Commandant:x86_64+0x26b84)
    #33 (closure #1) main.swift:24 (swiftlint:x86_64+0x1000334d0)
    #34 thunk Configuration+CommandLine.swift (swiftlint:x86_64+0x10000ab3a)
    #35 __tsan::invoke_and_release_block(void*) <null>:3197024 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x631bb)
    #36 _dispatch_client_callout <null>:3197024 (libdispatch.dylib:x86_64+0x178b)

  Location is heap block of size 88 at 0x7d18000083a0 allocated by thread T1:
    #0 malloc <null>:3197056 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x4628a)
    #1 swift_slowAlloc <null>:3197056 (libswiftCore.dylib:x86_64+0x22b7c8)
    #2 LinterCache.__allocating_init(configuration : Configuration) -> LinterCache LinterCache.swift (SwiftLintFramework:x86_64+0x2d695e)
    #3 LintCommand.run(LintOptions) -> Result<(), CommandantError<()>> LintCommand.swift:26 (swiftlint:x86_64+0x10001a0ea)
    #4 protocol witness for CommandProtocol.run(A.Options) -> Result<(), A.ClientError> in conformance LintCommand LintCommand.swift (swiftlint:x86_64+0x100025e20)
    #5 CommandWrapper.(init<A where ...> (A1) -> CommandWrapper<A>).(closure #1) Command.swift:55 (Commandant:x86_64+0x24537)
    #6 partial apply for CommandWrapper.(init<A where ...> (A1) -> CommandWrapper<A>).(closure #1) Command.swift (Commandant:x86_64+0x2a330)
    #7 CommandRegistry.run(command : String, arguments : [String]) -> Result<(), CommandantError<A>>? Command.swift:102 (Commandant:x86_64+0x26494)
    #8 CommandRegistry.main(arguments : [String], defaultVerb : String, errorHandler : (A) -> ()) -> Never Command.swift:167 (Commandant:x86_64+0x271f8)
    #9 CommandRegistry.main(defaultVerb : String, errorHandler : (A) -> ()) -> Never Command.swift:131 (Commandant:x86_64+0x26b84)
    #10 (closure #1) main.swift:24 (swiftlint:x86_64+0x1000334d0)
    #11 thunk Configuration+CommandLine.swift (swiftlint:x86_64+0x10000ab3a)
    #12 __tsan::invoke_and_release_block(void*) <null>:3197056 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x631bb)
    #13 _dispatch_client_callout <null>:3197056 (libdispatch.dylib:x86_64+0x178b)

  Thread T5 (tid=9023462, running) created by thread T-1
    [failed to restore the stack]

  Thread T1 (tid=9023457, running) created by thread T-1
    [failed to restore the stack]

SUMMARY: ThreadSanitizer: data race LinterCache.swift in LinterCache.fileManager.getter
==================
ThreadSanitizer report breakpoint hit. Use 'thread info -s' to get extended information about the report.
(lldb)

The Swift Programming Language (Swift 3.1) said in Lazy Stored Properties:

If a property marked with the lazy modifier is accessed by multiple threads simultaneously and the property has not yet been initialized, there is no guarantee that the property will be initialized only once.

@marcelofabri
Copy link
Collaborator Author

I always forget about how awesome Thread Sanitizer is 💯

Luckily, if that's the only issue, it'd be pretty easy to fix: just making it a let and passed on init

@norio-nomura
Copy link
Collaborator

According to Thread Sanitizer, violations (forFile:) is called simultaneously from multiple threads, so is lock necessary?

@marcelofabri
Copy link
Collaborator Author

It does, but I don't know if it'd be fine to remove the lock since we only read from the Dictionary

@marcelofabri
Copy link
Collaborator Author

Thinking more about this, I think removing the lock might not be the right solution. Maybe in this case would be better to use this approach: https://www.objc.io/issues/2-concurrency/low-level-concurrency-apis/#one-resource-multiple-readers-and-a-single-writer?

@norio-nomura
Copy link
Collaborator

Since we don't need to read updated entry from cache, we may be able to separate read cache instance and write cache instance.

@marcelofabri marcelofabri mentioned this issue May 23, 2017
Merged
@marcelofabri
Copy link
Collaborator Author

Fixed in #1551

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Ideas for improvements of existing features and rules.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@norio-nomura @marcelofabri