Implement initial background indexing of a project #1216

ahoppen · 2024-05-03T04:01:56Z

Implements an initial background index when the project is opened.

The following will be implemented in follow-up PRs:

Resolving package dependencies
Preparing dependent modules
Watching for file updates

ahoppen · 2024-05-03T04:02:01Z

Implements an initial background index when the project is opened. The following will be implemented in follow-up PRs: - Resolving package dependencies - Preparing dependent modules - Watching for file updates

… different unit files

Windows has `ProcessResult.ExitStatus.abnormal` instead of ` `ProcessResult.ExitStatus.signalled`

ahoppen · 2024-05-05T21:54:28Z

@swift-ci Please test

ahoppen · 2024-05-05T21:56:00Z

@swift-ci Please test Windows

SolaWing · 2024-05-06T01:55:26Z

is it support other BSP or only Swift Package Manager?

ahoppen · 2024-05-06T15:44:13Z

To start off, I’m only aiming to support SwiftPM. I just opened #1226 from my local to-do list so we can share ideas there. If you could comment there which BSP server you are using with SourceKit-LSP, that would be valuable.

ahoppen · 2024-05-08T17:35:15Z

Merging this to unblock further work on background indexing. Will address review comments in follow-up PRs.

hamishknight · 2024-05-09T10:33:42Z

Sources/SemanticIndex/SemanticIndexManager.swift

+    let filesWithOutOfDateIndex = uris.filter { uri in
+      switch indexStatus[uri] {
+      case .inProgress, nil: return true
+      case .upToDate: return false
+      }
+    }


Isn't this filtering already done by index(files:priority:)?

Good catch 👍🏽

hamishknight · 2024-05-09T11:08:16Z

Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift

+      result.append(argument)
+    }
+    result.append(argument)


Not sure if this matters in practice, but this would append -Xfrontend twice if it's the last argument

Oh, yes. Looks like we should just remove the inner result.append(argument)

hamishknight · 2024-05-09T11:58:40Z

Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift

+    // Fake an output path so that we get a different unit file for every Swift file we background index
+    "-o", fileToIndex.pseudoPath + ".o",


I don't think it really matters, but I think you could use -index-unit-output-path here

Also just noticed this doesn't seem to respect IndexOptions's indexStorePath, is that intentional?

I don't think it really matters, but I think you could use -index-unit-output-path here.

Good idea. That does seem cleaner.

Also just noticed this doesn't seem to respect IndexOptions's indexStorePath, is that intentional?

Yes, that’s intentional. IndexOption.indexStorePath is an artifact from index-while-building. Background indexing stores all its artifacts independently of the build folder, which ensures that it doesn’t conflict with real builds for now. It should thus also use a different index store path. Unifying background indexing and index-while building is part of #1270

hamishknight · 2024-05-09T12:20:44Z

Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift

+      // We consider a file's index up-to-date if we have any up-to-date unit. Changing build settings does not
+      // invalidate the up-to-date status of the index.


I guess you have to touch the file or something if you want to index it with up-to-date build settings?

Still an open question: #1251

Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift

hamishknight · 2024-05-09T12:43:23Z

Sources/SemanticIndex/SemanticIndexManager.swift

+    // TODO (indexing): Group index operations by target when we support background preparation.
+    for files in outOfDateFiles.partition(intoNumberOfBatches: ProcessInfo.processInfo.processorCount * 5) {


Worth a comment explaining the batching size? Or does it not matter since this will change once we support preparation?

I am actually already changing this to one index file per task in #1273.

#1268 is when the batching really starts to make sense.

bnbarham · 2024-05-15T04:13:42Z

Sources/SKSupport/SwitchableProcessResultExitStatus.swift

+// We need to import all of TSCBasic because otherwise we can't refer to Process.ExitStatus (rdar://127577691)
+import struct TSCBasic.ProcessResult


Comment says we're importing all, but actually only importing ProcessResult 🤔?

Oh, yeah. I was just stupid when I wrote the comment.

bnbarham · 2024-05-15T04:52:29Z

Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift

+      return
+    }
+    guard !buildSettings.isFallback else {
+      // Only index with real build settings. Indexing with fallback arguments could result in worse results than not


When could this actually happen? We're not using index-while-building here, so it seems like an odd case. Presumably having something is better than nothing, but even if you added a "do we have anything in the index" check, that would still be odd as then we'd index it the first time and not again after that.

bnbarham · 2024-05-15T04:54:08Z

Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift

+/// Adjust compiler arguments that were created for building to compiler arguments that should be used for indexing.
+///
+/// This removes compiler arguments that produce output files and adds arguments to index the file.
+private func adjustSwiftCompilerArgumentsForIndexStoreUpdate(


This all makes me sad, we really need a "please don't output anything mode". Though... how that would then work with "oh but actually do output the index" is certainly a reasonable question 😅

Address review comment to #1216

ahoppen requested a review from benlangmuir as a code owner May 3, 2024 04:01

ahoppen added 2 commits May 5, 2024 14:26

Implement initial background indexing of a project

1a5d132

Implements an initial background index when the project is opened. The following will be implemented in follow-up PRs: - Resolving package dependencies - Preparing dependent modules - Watching for file updates

Conjure up an output file path so that indexing of two files produces…

a6a76c6

… different unit files

ahoppen mentioned this pull request May 5, 2024

Background Indexing #969

Closed

Fix Windows build failure

0ae3d80

Windows has `ProcessResult.ExitStatus.abnormal` instead of ` `ProcessResult.ExitStatus.signalled`

ahoppen force-pushed the background-indexing branch from 82964cf to 0ae3d80 Compare May 5, 2024 21:53

ahoppen changed the title ~~Implement initial background indexing of a project 🚥 #1214~~ Implement initial background indexing of a project May 5, 2024

ahoppen requested a review from bnbarham May 5, 2024 21:54

ahoppen merged commit efba8ce into swiftlang:main May 8, 2024

ahoppen deleted the background-indexing branch May 8, 2024 17:35

hamishknight reviewed May 9, 2024

View reviewed changes

ahoppen added the background indexing label May 9, 2024

ahoppen added a commit to ahoppen/sourcekit-lsp that referenced this pull request May 9, 2024

Address review comments to swiftlang#1216

7b4bbeb

ahoppen mentioned this pull request May 14, 2024

Merge main into release/6.0 #1301

Merged

bnbarham reviewed May 15, 2024

View reviewed changes

ahoppen added a commit to ahoppen/sourcekit-lsp that referenced this pull request May 15, 2024

Address review comment to swiftlang#1216

f10eac3

ahoppen added a commit that referenced this pull request May 15, 2024

Merge pull request #1303 from ahoppen/review-comments-1216

597932c

Address review comment to #1216

		// Fake an output path so that we get a different unit file for every Swift file we background index
		"-o", fileToIndex.pseudoPath + ".o",

		// We consider a file's index up-to-date if we have any up-to-date unit. Changing build settings does not
		// invalidate the up-to-date status of the index.

		// TODO (indexing): Group index operations by target when we support background preparation.
		for files in outOfDateFiles.partition(intoNumberOfBatches: ProcessInfo.processInfo.processorCount * 5) {

		// We need to import all of TSCBasic because otherwise we can't refer to Process.ExitStatus (rdar://127577691)
		import struct TSCBasic.ProcessResult

Implement initial background indexing of a project #1216

Implement initial background indexing of a project #1216

Uh oh!

Conversation

ahoppen commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented May 3, 2024

Uh oh!

ahoppen commented May 5, 2024

Uh oh!

ahoppen commented May 5, 2024

Uh oh!

SolaWing commented May 6, 2024

Uh oh!

ahoppen commented May 6, 2024

Uh oh!

ahoppen commented May 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen commented May 3, 2024 •

edited

Loading