Wip/shake queue #51

mpickering · 2020-05-30T11:19:51Z

Putting this up here so we can comment about what needs to be tidied up.

mpickering · 2020-05-30T11:20:31Z

src/Development/IDE/Core/OfInterest.hs

    files <- modifyVar var $ pure . dupe . f
    logDebug (ideLogger state) $ "Set files of interest to: " <> T.pack (show $ HashSet.toList files)
-    void $ shakeRun state []
+    let das = map (\nfp -> mkDelayedAction "OfInterest" (GetSpanInfo, nfp) Debug (use GetSpanInfo nfp)) (HashSet.toList files)


Do we want this or not?

What is the point supposed to be?

On this branch there isn't the parent typechecking logic so if you edit a file A which is depended on by a file B then B won't get recompiled if you make a change in A without this logic.

src/Development/IDE/Import/DependencyInformation.hs

mpickering · 2020-05-30T11:21:31Z

src/Development/IDE/Plugin/CodeAction.hs

    let text = Rope.toText . (_text :: VirtualFile -> Rope.Rope) <$> contents
        mbFile = toNormalizedFilePath' <$> uriToFilePath uri
-    (ideOptions, parsedModule, join -> env) <- runAction state $
+    (ideOptions, parsedModule, join -> env) <- runAction "CodeAction" state $


CodeActions are still really slow on GHC because of this.

src/Development/IDE/Plugin/Completions.hs

mpickering · 2020-05-30T11:22:45Z

src/Development/IDE/Core/Shake.hs

    }


+data QPriority = QPriority { retries :: Int


Move all the ShakeQueue stuff into another module

src/Development/IDE/Core/Shake.hs

mpickering · 2020-05-30T11:24:57Z

src/Development/IDE/Spans/AtPoint.hs

-        eqName n n' = nameOccName n == nameOccName n' && nameModule_maybe n == nameModule_maybe n'
-        setFileName f (RealSrcSpan span) = RealSrcSpan (span { srcSpanFile = mkFastString f })
-        setFileName _ span@(UnhelpfulSpan _) = span
+        getSpan (Named name) = nameToLocation getHieFile name


THis refactoring is from goto type definition patch which is still not merged.

mpickering · 2020-05-30T11:25:18Z

Patch overall looks quite clean once Shake.hs is sorted out.

@mpickering

Currently we start a new Shake session for every interaction with the Shake database, including type checking, hovers, code actions, completions, etc. Since only one Shake session can ever exist, we abort the active session if any in order to execute the new command in a responsive manner. This is suboptimal in many, many ways: - A hover in module M aborts the typechecking of module M, only to start over! - Read-only commands (hover, code action, completion) need to typecheck all the modules! (or rather, ask Shake to check that the typechecks are current) - There is no way to run non-interfering commands concurrently This is an experiment inspired by the 'ShakeQueue' of @mpickering, and the follow-up discussion in #7 We introduce the concept of the 'ShakeSession' as part of the IDE state. The 'ShakeSession' is initialized by a call to 'shakeRun', and survives until the next call to 'shakeRun'. It is important that the session is restarted as soon as the filesystem changes, to ensure that the database is current. The 'ShakeSession' enables a new command 'shakeRunGently', which appends work to the existing 'ShakeSession'. This command can be called in parallel without any restriction.

shakeRun is not correct as it never returns anymore

The previous approach reused the shakeProgress thread, which doesn't work anymore as ShakeSession keeps the ShakeDatabase open until the next edit

Dropping the 0.1s sleep to ensure that progress messages during tests are deterministic

mpickering · 2020-06-03T09:09:56Z

My plan here now is to rebase this onto @pepeiborra's shake enqueue patch and wait for that to land before making a PR.

This is required for progress reporting to work, see notes in shakeRun As to whether this is the right thing to do: 1. Less magic, more explicit 2. There's only 2 places where kick is actually used

A Barrier is a smaller abstraction than an MVar, and the next version of the extra package will come with a suitably small implementation: ndmitchell/extra@98c2a83

The action returned by shakeRun now blocks until another call to shakeRun is made, which is a change in behaviour,. but all the current uses of shakeRun ignore this action. Since the new behaviour is not useful, this change simplifies and updates the docs and name accordingly

This introduces a new function `useWithStaleFast` which returns with stale information WITHOUT checking freshness like `use` and `useWithStale`. Greatly improve debug logging All actions triggered by shakeRun now also pass an identifier which means that the debug logging shows which actions are starting/finishing We also distinguish between internal and external events. By default external events are ones triggered by runAction and the debug output is displayed to the user in command line and --lsp mode. In order to see internal logging statements, there is a new flag called --verbose which also prints out internal events such as file modification flushes. Cleaner variant using runAfter Step 1: Do not run actions with shakeRun Queue implementation, living, breathing Use a priority queue to schedule shake actions. Most user actions are answered immediately with a cache but also spawn a shake action to check the cached value we consulted was up to date.