Add a flag to parse serially #1063
Conversation
Parsing in parallel is leading to unexpected `EXC_BAD_ACCESS`es in our codebase. By switching to `compactMap`, the crashes go away. The specific crash is deep in the internals of `SwiftSyntax`, which leads me to believe that there's some internal global thread-unsafe state that's being shared bewteen parsing instances. Here's an example (abbreviated) stacktrace: ``` #0 0x00000001001209e4 in protocol witness for SyntaxProtocol._syntaxNode.getter in conformance Syntax () #1 0x0000000100121ec3 in SyntaxProtocol.raw.getter at /Users/eric_horacek/Library/Developer/Xcode/DerivedData/Sourcery-hibchsafsxkdcjaxxaylrbeyoydh/SourcePackages/checkouts/swift-syntax/Sources/SwiftSyntax/Syntax.swift:129 #2 0x0000000100b85c32 in SimpleTypeIdentifierSyntax.init(_:) at /Users/eric_horacek/Library/Developer/Xcode/DerivedData/Sourcery-hibchsafsxkdcjaxxaylrbeyoydh/SourcePackages/checkouts/swift-syntax/Sources/SwiftSyntax/gyb_generated/syntax_nodes/SyntaxTypeNodes.swift:68 #3 0x0000000100b89979 in protocol witness for SyntaxProtocol.init(_:) in conformance SimpleTypeIdentifierSyntax () #4 0x00000001001acab7 in TypeSyntax.as<τ_0_0>(_:) at /Users/eric_horacek/Library/Developer/Xcode/DerivedData/Sourcery-hibchsafsxkdcjaxxaylrbeyoydh/SourcePackages/checkouts/swift-syntax/Sources/SwiftSyntax/gyb_generated/SyntaxBaseNodes.swift:421 ``` I was able to reproduce this in the debugger, but there was no information available with either TSAN or ASAN. Fixes #1009. I know that a lot of folks rely on the performance of parsing in parallel and it only seems to be crashing on our codebase, so perhaps we could put this behind an argument?
Sourcery/Sourcery.swift
Outdated
| func incrementFileParsedCount() { | ||
| OSAtomicIncrement32(&numberOfFilesThatHadToBeParsed) |
There was a problem hiding this comment.
When I ran TSAN on the codebase this increment was causing some violations to be printed, and they went away when I switched to a DispatchQueue.sync as a barrier. However even with that change I still encountered the crashes.
There was a problem hiding this comment.
oh that's really interesting, would not expect that for sure
|
This change matches my expectations re working with SwiftSyntax. We currently do all SwiftSyntax work on I do not believe that SwiftSyntax intends to be thread-unsafe (see forum post). But in my experience it is. Probably worth filing an issue on the repo |
|
@dfed @erichoracek yeah I talked to @akyrtzi and he said the intention is to be thread-safe, and suggested using the debug build of the parser library to debug. In the meantime could we add a flag for single threaded processing @erichoracek ? I can merge that as optional one but don't want to move whole threading model to single threaded since this crash doesn't happen often and it's a significant perf regression |
Is there any easy way to run Sourcery with a debug build of SwiftSyntax?
Sure, let me update the PR to include a flag for serial parsing |
|
https://github.com/apple/swift-syntax/tree/main/Documentation mentions instructions how to build
|
Parsing in parallel is leading to unexpected
EXC_BAD_ACCESSes in our codebase. By switching tocompactMap, the crashes go away.The specific crash is deep in the internals of
SwiftSyntaxmid-parse, which leads me to believe that there's some internal global thread-unsafe state that's being shared between parser instances. I'm not sure if there's any guarantees about thread safety inSwiftSyntax, but I couldn't find any. Here's an example (abbreviated) stacktrace:I was able to reproduce this in the debugger, but there was no information available with either TSAN or ASAN.
Fixes #1009. With this change Sourcery 1.8.1 now works on our very large codebase where we run one instance of Sourcery per module. However it's important to note that this crash was occurring consistently on a single module—even if no other Sourcery processes were running concurrently.
I know that a lot of folks rely on the performance of parsing in parallel and it only seems to be crashing on our codebase, so perhaps we could put this behind an argument?