[CodeCompletion] Give up fast-completion if dependent files are modified #31388

rintaro · 2020-04-29T01:35:50Z

Check if dependencies are modified since the last checking.
Dependencies:

Other source files in the current module
Dependent files collected by the dependency tracker

When:

If the last dependency check was over N (defaults to 5) seconds ago

Invalidate if:

The dependency file is missing
The modification time of the dependecy is greater than the last check
If the modification time is zero, compare the contents using the file system from the previous completion and the current completion (in-memory filesystem)

rdar://problem/62336432

rintaro · 2020-04-29T01:36:12Z

@swift-ci Please build toolchain macOS

swift-ci · 2020-04-29T03:26:17Z

macOS Toolchain
Download Toolchain
Git Sha - 8c8849b4916e5531493849f78d9d19d13c76808b

Install command
tar -zxf swift-PR-31388-505-osx.tar.gz --directory ~/

rintaro · 2020-04-29T05:37:55Z

@swift-ci Please build toolchain macOS

rintaro · 2020-04-29T05:43:31Z

@swift-ci Please build toolchain macOS

rintaro · 2020-04-30T00:04:06Z

@swift-ci Please build toolchain macOS

rintaro · 2020-05-01T00:02:15Z

@swift-ci Please smoke test

rintaro · 2020-05-01T17:04:53Z

@swift-ci Please test

swift-ci · 2020-05-01T17:07:10Z

Build failed
Swift Test Linux Platform
Git Sha - 8c8849b4916e5531493849f78d9d19d13c76808b

swift-ci · 2020-05-01T17:08:14Z

Build failed
Swift Test OS X Platform
Git Sha - 8c8849b4916e5531493849f78d9d19d13c76808b

rintaro · 2020-05-01T17:22:19Z

@swift-ci Please test

swift-ci · 2020-05-01T17:25:02Z

Build failed
Swift Test Linux Platform
Git Sha - 95528a433c64742ab80725936b4308ea0229a94a

swift-ci · 2020-05-01T17:25:48Z

Build failed
Swift Test OS X Platform
Git Sha - 95528a433c64742ab80725936b4308ea0229a94a

lib/IDE/CompletionInstance.cpp

akyrtzi · 2020-05-01T17:50:32Z

lib/IDE/CompletionInstance.cpp

+    // If the last modification time is zero, this file is probably from a
+    // virtual file system. We need to check the content.
+    if (lastModTime == llvm::sys::TimePoint<>()) {
+      if (&CI.getFileSystem() == &FS)


A couple of questions:

Are these ever going to be equal, or in practice we'll always get a new VFS object?

Even if the pointers are equal, is it correct to assume the contents did not change? The VFS object could be mutable.

Are these ever going to be equal, or in practice we'll always get a new VFS object?

I don't think so. I believe this is unreachable.

Even if the pointers are equal, is it correct to assume the contents did not change? The VFS object could be mutable.

You are right. We perhaps should retain all buffers of the VFS dependencies. But I think it's good enough for now because the VFS objects should be always different.

I would say keep a hash not the whole buffer.

OK. I updated the PR to keep hashes of the dependencies.

lib/IDE/CompletionInstance.cpp

akyrtzi · 2020-05-01T17:55:56Z

test/SourceKit/CodeComplete/complete_checkdeps_bridged.swift

+  /*HERE*/
+}
+
+// FIXME: Windows CI doesn't support bash array syntax i.e. 'ARRY=( .... )'


The tests probably need

// REQUIRES: shell

I didn't know shell. Thank you!

include/swift/IDE/CompletionInstance.h

rintaro · 2020-05-01T19:06:36Z

@swift-ci Please test

swift-ci · 2020-05-01T19:07:01Z

Build failed
Swift Test Linux Platform
Git Sha - 0f66247db4c5215f4dc63da70028332aacf3451b

swift-ci · 2020-05-01T19:11:02Z

Build failed
Swift Test OS X Platform
Git Sha - 0f66247db4c5215f4dc63da70028332aacf3451b

benlangmuir · 2020-05-01T20:42:45Z

lib/IDE/CompletionInstance.cpp

+    // If the last modification time is zero, this file is probably from a
+    // virtual file system. We need to check the content.
+    if (lastModTime == llvm::sys::TimePoint<>()) {
+      if (&CI.getFileSystem() == &FS)


I would say keep a hash not the whole buffer.

benlangmuir · 2020-05-01T20:44:30Z

lib/IDE/CompletionInstance.cpp

+
+    // Ignore the current file and synthesized files.
+    if (filename.empty() || filename.front() == '<' ||
+        filename.equals(currentFileName))


Comparing filename seems potentially brittle - shouldn't we know which file is the completion buffer directly without looking at the name?

Updated the PR.

rintaro · 2020-05-02T00:07:07Z

@swift-ci Please test

swift-ci · 2020-05-02T00:07:31Z

Build failed
Swift Test Linux Platform
Git Sha - 4b2ad40d9e4b50a57d65b12c4a43e1d2e9c32217

swift-ci · 2020-05-02T01:28:22Z

Build failed
Swift Test OS X Platform
Git Sha - 4b2ad40d9e4b50a57d65b12c4a43e1d2e9c32217

benlangmuir · 2020-05-04T17:10:10Z

lib/IDE/CompletionInstance.cpp

+        // Stop processing if the modification time is not zero. Probably this
+        // is not a virtual file system.
+        if (stat->getLastModificationTime() != llvm::sys::TimePoint<>())
+          return true;


I think I'm not understanding something: why do we want to stop as soon as we see the first "normal file"? Why can't we have virtual mixed in with regular?

I just want to save the stat time if the filesystem is not in-memory.
AFAIU, in reality, we don't mix in-memory file with normal file.

The filesystem you get from our builtin FileSystemProvider in sourcekitd is an overlay, so if a call is not handled by the InMemoryFilesystem, it will fall through to the RealFilesystem.

Ah, thank you for notifying! Updated to just ignore the non-zero modification time here.

benlangmuir · 2020-05-04T17:11:13Z

lib/IDE/CompletionInstance.cpp

+/// For each dependency files in CI, run \p callback until the callback returns
+/// \c true. Returns \c true if any callback call returns \c true, \c false
+/// otherwise.
+bool forEachDependenciesUntilTrue(


Should be singular dependency in the name: forEachDependencyUntilTrue

Check if dependencies are modified since the last checking. Dependencies: - Other source files in the current module - Dependent files collected by the dependency tracker When: - If the last dependency check was over N (defaults to 5) seconds ago Invalidate if: - The dependency file is missing - The modification time of the dependecy is greater than the last check - If the modification time is zero, compare the content using the file system from the previous completion and the current completion rdar://problem/62336432

These test cases use shell variables etc.

benlangmuir · 2020-05-04T20:36:43Z

test/SourceKit/CodeComplete/complete_checkdeps_bridged.swift

+// RUN: %empty-directory(%t/Frameworks)
+// RUN: %empty-directory(%t/MyProject)
+
+// RUN: COMPILER_ARGS=( \


I'm not sure how supported it is to assume you can set shell variables and have them persist across run lines. This may work in practice, but are we sure lit will never run commands in a fresh environment?

I'm not 100% certain this is officially supported. But several test cases rely on the behavior e.g. https://github.com/apple/swift/blob/master/test/ModuleInterface/ModuleCache/module-cache-deployment-target-irrelevant.swift.

Okay, maybe we can follow up with people who know lit well, but no need to block this PR.

rintaro · 2020-05-04T20:56:46Z

@swift-ci Please test

swift-ci · 2020-05-04T20:59:09Z

Build failed
Swift Test OS X Platform
Git Sha - 42f79c195ae4caf862a06873428c6d291c4d1446

swift-ci · 2020-05-04T20:59:36Z

Build failed
Swift Test Linux Platform
Git Sha - 42f79c195ae4caf862a06873428c6d291c4d1446

- Detect same file with bufferID instead of the file name - Compare virtual in-memory filesystem content with hash value

rintaro · 2020-05-04T23:36:24Z

@swift-ci Please test

swift-ci · 2020-05-04T23:36:47Z

Build failed
Swift Test Linux Platform
Git Sha - 40ed3a436c542327795036559a4c6725e856b94e

swift-ci · 2020-05-05T00:15:44Z

Build failed
Swift Test OS X Platform
Git Sha - 40ed3a436c542327795036559a4c6725e856b94e

rintaro force-pushed the ide-completion-fastcheckdep-rdar62336432 branch from e0354c9 to 09c4f2c Compare April 29, 2020 05:43

rintaro force-pushed the ide-completion-fastcheckdep-rdar62336432 branch from 09c4f2c to 38c26cf Compare April 30, 2020 00:03

rintaro force-pushed the ide-completion-fastcheckdep-rdar62336432 branch from 9d4946e to 95528a4 Compare May 1, 2020 17:02

rintaro marked this pull request as ready for review May 1, 2020 17:04

rintaro force-pushed the ide-completion-fastcheckdep-rdar62336432 branch from 95528a4 to 0f66247 Compare May 1, 2020 17:20

rintaro requested review from benlangmuir and akyrtzi May 1, 2020 17:27

akyrtzi requested changes May 1, 2020

View reviewed changes

rintaro force-pushed the ide-completion-fastcheckdep-rdar62336432 branch from 0f66247 to 4b2ad40 Compare May 1, 2020 18:51

benlangmuir reviewed May 1, 2020

View reviewed changes

rintaro force-pushed the ide-completion-fastcheckdep-rdar62336432 branch from 4b2ad40 to be91880 Compare May 1, 2020 21:01

benlangmuir reviewed May 4, 2020

View reviewed changes

rintaro added 3 commits May 4, 2020 13:02

[CodeCompletion] Added check dependency test for complete.open

df8fa09

[Tests] Mark check dependency tests cases 'REQUIRES: shell'

e32de78

These test cases use shell variables etc.

rintaro force-pushed the ide-completion-fastcheckdep-rdar62336432 branch 2 times, most recently from 6210634 to 40ed3a4 Compare May 4, 2020 20:18

benlangmuir approved these changes May 4, 2020

View reviewed changes

akyrtzi approved these changes May 4, 2020

View reviewed changes

rintaro mentioned this pull request May 4, 2020

[5.3][CodeCompletion] Give up fast-completion if dependent files are modified #31547

Merged

[CodeCompletion] Tweak fast completion dependency checking

af5daed

- Detect same file with bufferID instead of the file name - Compare virtual in-memory filesystem content with hash value

rintaro force-pushed the ide-completion-fastcheckdep-rdar62336432 branch from 40ed3a4 to af5daed Compare May 4, 2020 23:36

rintaro merged commit 1f6c3be into swiftlang:master May 5, 2020

rintaro deleted the ide-completion-fastcheckdep-rdar62336432 branch May 5, 2020 03:18

[CodeCompletion] Give up fast-completion if dependent files are modified #31388

[CodeCompletion] Give up fast-completion if dependent files are modified #31388

Uh oh!

Conversation

rintaro commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Apr 29, 2020

Uh oh!

swift-ci commented Apr 29, 2020

Uh oh!

rintaro commented Apr 29, 2020

Uh oh!

rintaro commented Apr 29, 2020

Uh oh!

rintaro commented Apr 30, 2020

Uh oh!

rintaro commented May 1, 2020

Uh oh!

rintaro commented May 1, 2020

Uh oh!

swift-ci commented May 1, 2020

Uh oh!

swift-ci commented May 1, 2020

Uh oh!

rintaro commented May 1, 2020

Uh oh!

swift-ci commented May 1, 2020

Uh oh!

swift-ci commented May 1, 2020

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rintaro commented May 1, 2020

Uh oh!

swift-ci commented May 1, 2020

Uh oh!

swift-ci commented May 1, 2020

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!

rintaro commented May 2, 2020

Uh oh!

swift-ci commented May 2, 2020

Uh oh!

swift-ci commented May 2, 2020

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!

rintaro commented Apr 29, 2020 •

edited

Loading