Track file versions accurately. #2735
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wz1000
requested review from
jneira,
berberman and
pepeiborra
as code owners
This patch does two things: 1. It allows us to track the versions of `Values` which don't come from the VFS, as long as those particular `Values` depended on the `GetModificationTime` rule This is necessary for the recompilation avoidance scheme implemented in #2316 2. It removes the VFSHandle type and instead relies on snapshots of the VFS state taken on every rebuild of the shake session to ensure that we see a consistent VFS state throughout each individual build. With regards to 2, this is necessary because the lsp library mutates its VFS file store as changes come in. This can lead to scenarios where the HLS build session can see inconsistent views of the VFS. One such scenario is. 1. HLS build starts, with VFS state A 2. LSP Change request comes in and lsp updates its internal VFS state to B 3. HLS build continues, now consulting VFS state B 4. lsp calls the HLS file change handler, interrupting the build and restarting it. However, the build might have completed, or cached results computed using an inconsistent VFS state.
wz1000
force-pushed
the
wip/file-versions
branch
from
44a7dda
to
52c7f7a
Compare
pepeiborra
reviewed
Feb 26, 2022
pepeiborra
approved these changes
Feb 26, 2022
michaelpj
reviewed
Feb 26, 2022
| -- time spent checking file modifications (which happens on every change) | ||
| -- from > 0.5s to ~0.15s. | ||
| -- We might also want to try speeding this up on Windows at some point. | ||
| -- TODO leverage DidChangeWatchedFile lsp notifications on clients that |
There was a problem hiding this comment.
Is this TODO in the right place?
| @@ -290,10 +290,12 @@ pattern GetModificationTime = GetModificationTime_ {missingFileDiagnostics=True} | |||
| -- | Get the modification time of a file. | |||
| type instance RuleResult GetModificationTime = FileVersion | |||
|
|
|||
| -- | Either athe mtime from disk or an LSP version | |||
There was a problem hiding this comment.
Suggested change
| -- | Either athe mtime from disk or an LSP version | |
| -- | Either the mtime from disk or an LSP version |
| @@ -290,10 +290,12 @@ pattern GetModificationTime = GetModificationTime_ {missingFileDiagnostics=True} | |||
| -- | Get the modification time of a file. | |||
| type instance RuleResult GetModificationTime = FileVersion | |||
|
|
|||
| -- | Either athe mtime from disk or an LSP version | |||
| -- LSP versions always compare as greater than on disk versions | |||
There was a problem hiding this comment.
Why? Is that important?
| Nothing -> (,Nothing) . T.decodeUtf8 <$> BS.readFile (fromNormalizedFilePath file) | ||
| Just vf -> pure (Rope.toText $ _text vf, Just $ _lsp_version vf) | ||
| vfsRef <- asks vfs | ||
| vfsData <- liftIO $ vfsMap <$> readTVarIO vfsRef |
There was a problem hiding this comment.
Can we use the getVirtualFile action? Or not because we're in a Rules?
| (bs, (diags, res)) <- actionCatch | ||
| (do v <- action; liftIO $ evaluate $ force v) $ | ||
| \(e :: SomeException) -> do | ||
| pure (Nothing, ([ideErrorText file $ T.pack $ show e | not $ isBadDependency e],Nothing)) | ||
| modTime <- liftIO $ (currentValue . fst =<<) <$> atomicallyNamed "define - read 2" (getValues state GetModificationTime file) | ||
|
|
||
| modTime <- case eqT @k @GetModificationTime of |
There was a problem hiding this comment.
Something clever is happening here, worth a comment.
| shared_change = mkDelta changes | ||
| actual_version = case _version of | ||
| Nothing -> error "Nothing version from server" -- This is a violation of the spec |
There was a problem hiding this comment.
This seems to be a bug in lsp, please make an issue.
There was a problem hiding this comment.
Hmm, or maybe we're using the same type to cover OptionalVersionedTextDocumentIdentifier also. I think you can get a missing version here, in fact.
| @@ -156,6 +155,8 @@ createExportsMapTc modIface = do | |||
| nonInternalModules :: ModuleName -> Bool | |||
| nonInternalModules = not . (".Internal" `isSuffixOf`) . moduleNameString | |||
|
|
|||
| type WithHieDb = forall a. (HieDb -> IO a) -> IO a | |||
There was a problem hiding this comment.
Why this change? now it's duplicated?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch does two things:
It allows us to track the versions of
Valueswhich don't come from the VFS, as long as thoseparticular
Valuesdepended on theGetModificationTimeruleThis is necessary for the recompilation avoidance scheme implemented in Improve recompilation avoidance in the presence of TH #2316
It removes the VFSHandle type and instead relies on snapshots of the VFS state taken on every rebuild
of the shake session to ensure that we see a consistent VFS state throughout each individual build.
With regards to 2, this is necessary because the lsp library mutates its VFS file store as changes come
in. This can lead to scenarios where the HLS build session can see inconsistent views of the VFS.
One such scenario is.
However, the build might have completed, or cached results computed using an
inconsistent VFS state.