Support indexing of mulitple Swift files within the same compiler invocation #2293
Conversation
ahoppen
requested review from
bnbarham,
hamishknight and
rintaro
as code owners
ahoppen
force-pushed
the
multi-file-indexing
branch
from
ed5cf4f to
231214d
Compare
|
@swift-ci Please test |
| // the batch can be indexed by a single compiler invocation. However, we might discover that two Swift files within | ||
| // the same target have different build settings in the build server. In that case, the best thing we can do is | ||
| // trigger two compiler invocations. |
There was a problem hiding this comment.
How would this happen 🤔? For C-family, sure. But I would expect all files within a target to have the same build settings for Swift.
It's super unfortunate we have to look through the arguments to find -index-unit-output-path for every settings per file, even though they should all be the same for every batch (except for the output path).
There was a problem hiding this comment.
We don’t control what the BSP server gives us for the build settings, so a BSP server may return different build settings for different Swift files within the same target. It wouldn’t make much sense from a build server’s perspective in practice but I’ve learned that it’s better to design SourceKit-LSP in a way that makes as few implicit assumptions about the BSP server responses as possible and assuming that all Swift files within the same target have the same build settings would be one of them.
It's super unfortunate we have to look through the arguments to find -index-unit-output-path for every settings per file, even though they should all be the same for every batch (except for the output path).
Do you think from a performance perspective. If we launch a compiler to index the file, I doubt that scanning for a command line argument makes much of a difference. And really, we should be getting the output path from the buildTarget/sources response (fileInfo.outputPath), in which case the extraction of -index-unit-output-path is just a validity check to ensure the two match.
| // Since Swift files within the same target should build a single module, it is reasonable to assume that they all | ||
| // share the same build settings. |
There was a problem hiding this comment.
I'm not sure I'd even call this an assumption TBH - if it wasn't the case, things would be very broken.
There was a problem hiding this comment.
Same reply as #2293 (comment). I guess it’s just nice to see that SourceKit-LSP continues to function even if the build server gives us unreasonable responses.
| // hard to quanify the performance characteristics of different batch sizes. 5 seems like a good trade-off to | ||
| // share work between files within the same target without overloading a single job with too many files and | ||
| // thus loosing parallelism. | ||
| files.partition(intoBatchesOfSize: 5).map { (targetAndLanguage.target, targetAndLanguage.language, $0) } |
There was a problem hiding this comment.
Could make configurable? What's the driver choose for its batch sizes?
There was a problem hiding this comment.
Good idea to check with the driver. It schedules as many concurrent batches as the machine has CPU cores but limits the batch size to 25 (https://github.com/swiftlang/swift-driver/blob/main/Sources/SwiftDriver/Jobs/Planning.swift#L917-L1022). I think copying that behavior is as reasonable an ideas as I have. But I’d prefer to do that in a follow-up PR to keep this one focused on the actual implementation of batch indexing instead of introducing complications due to configuration options.
| let newTask = Task<Bool, Never> { () -> Bool in | ||
| #if compiler(>=6.4) | ||
| #warning( | ||
| "Once we no longer Swift 6.2 toolchains, we can assume that the compiler has https://github.com/swiftlang/swift-driver/pull/1979" |
There was a problem hiding this comment.
| "Once we no longer Swift 6.2 toolchains, we can assume that the compiler has https://github.com/swiftlang/swift-driver/pull/1979" | |
| "Once we no longer support Swift 6.3 toolchains, we can assume that the compiler has https://github.com/swiftlang/swift-driver/pull/1979" |
There was a problem hiding this comment.
By the time that we stop supporting 6.2 we don’t need to worry about the early Swift 6.3 toolchain snapshots that didn’t have multi-file indexing support anymore.
…files We should not take the number of files in an `UpdateIndexStoreTaskDescription` as an indication on how important the task is. If we do need this functionality, eg. because we want to update the index of files with syntactic matches for a rename term, this should be communicated using a specific purpose similar to `TargetPreparationPurpose`. Since the only reason we update the index store for a file right now is background indexing, such a check is not needed.
ahoppen
force-pushed
the
multi-file-indexing
branch
from
231214d to
d1981de
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
Since the indexing performance of most projects is bound by target preparation instead of actual indexing right now, I wasn’t able to measure any performance benefits of this. But it seems like a good thing to add regardless, because it might save CPU cycles.
Fixes #1268