Use stale information if it's available to answer requests quickly #624

mpickering · 2020-06-10T08:39:35Z

With this patch whenever any request made by the user is dealt with by an IdeAction which has the implicit guarantee to never block for an unbounded amount of time.

Stale information is used if it's available to answer all requests. Information should usually be up-to-date because the FileStore.hs rule requests up-to-date information whenver a file changes.

Do not review the changes to the hover/completions etc code too closely because that is all rewritten on another branch by Zubin anyway to use HieDb.

This patch also lays the way for removing kick and more fine grained control over which files are typechecked.

digitalasset-cla · 2020-06-10T08:39:42Z

All committers have signed the CLA.

ndmitchell

Looks reasonably plausible. I'll take a more detailed look after @cocreature weighs in on the concept of whether this is the right thing to do or not.

Splitting off the benchmark stuff would let us land this in two phases, and decouple things that aren't very related at all.

ndmitchell · 2020-06-10T11:25:42Z

bench/Main.hs

@@ -218,14 +218,16 @@ runBenchmarks benchmarks = do
    putStrLn $ intercalate ", " $ map (showDuration . snd) results


How big a deal would it be to send the benchmark changes in via a different PR? Seems totally unrelated to the rest of it, and one for @pepeiborra to review.

Agreed. I've got a number of changes for the benchmark suite myself, so a separate PR that can be reviewed and merged quickly would be much appreciated.

I've pushed my benchmarks changes in #625

ndmitchell · 2020-06-10T11:26:18Z

bench/Main.hs

@@ -218,14 +218,16 @@ runBenchmarks benchmarks = do
    putStrLn $ intercalate ", " $ map (showDuration . snd) results

 runBench :: HasConfig => Bench -> IO Seconds
-runBench Bench {..} = handleAny (\e -> print e >> return (-1))
+runBench Bench {..} = handleAny (\e -> print e >> return (-2))


Having -1 as a special case was odd, but just on the edge of tolerable. Now we're having two magic negative seconds I think an enum would be better.

Yes please!

ndmitchell · 2020-06-10T11:26:47Z

bench/Main.hs

    return $ if res then t else -1
  where
    cmd =
      unwords $
        [ "ghcide",
          "--lsp",
+          "--test",
+          "--verbose",


Why pass verbose? Why not take this as an argument to get passed onward?

ndmitchell · 2020-06-10T11:27:08Z

bench/Main.hs

    return $ if res then t else -1
  where
    cmd =
      unwords $
        [ "ghcide",
          "--lsp",
+          "--test",


Why is this required?

See https://github.com/digital-asset/ghcide/pull/624/files#diff-9f3c72df87c6e40a0ab15d33001d156cR739

ndmitchell · 2020-06-10T11:28:36Z

src/Development/IDE/Core/Service.hs

@@ -50,7 +47,7 @@ initialise :: LSP.ClientCapabilities
           -> IdeOptions
           -> VFSHandle
           -> IO IdeState
-initialise caps mainRule getLspId toDiags logger debouncer options vfs =
+initialise caps mainRule getLspId toDiags logger debouncer options vfs = do


What's this change for?

ndmitchell · 2020-06-10T11:29:14Z

src/Development/IDE/Core/Shake.hs

+data DelayedActionX a = DelayedActionX { actionName :: String -- Name we show to the user
+                                      -- The queue only contrains entries for
+                                      -- unique key values.
+                                      , actionPriority :: Logger.Priority


Why are we using log priorities here?

ndmitchell · 2020-06-10T11:30:08Z

src/Development/IDE/Core/Shake.hs

+    liftIO $ logPriority l (actionPriority d) $ T.pack $
+        "finish: " ++ (actionName d) ++ " (took " ++ showDuration runTime ++ ")"
+
+freshId :: Var Int -> IO Int


Why not just reuse Data.Unique?

ndmitchell · 2020-06-10T11:30:32Z

src/Development/IDE/Core/Shake.hs

+
+freshId :: Var Int -> IO Int
+freshId qcount = do
+    modifyVar qcount (\n -> return (n + 1, n))


Looks like a potential space leak.

ndmitchell · 2020-06-10T11:31:18Z

src/Development/IDE/Plugin/Completions/Logic.hs

@@ -395,7 +395,6 @@ getCompletions ideOpts CC { allModNamesAsNS, unqualCompls, qualCompls, importabl
        = filtModNameCompls ++ map (toggleSnippets caps withSnippets
                                      . mkCompl ideOpts . stripAutoGenerated) filtCompls
                            ++ filtKeywordCompls
-


Why delete one line in this file?

pepeiborra · 2020-06-10T11:41:25Z

Benchmarks please!

mpickering · 2020-06-10T11:44:00Z

Benchmarks are vacuous now but anyway.

TOTAL hover = 0.01s (10 repetitions)
TOTAL getDefinition = 0.03s (10 repetitions)
TOTAL documentSymbols = 0.65s (100 repetitions)
TOTAL documentSymbols after edit = 0.60s (100 repetitions)
TOTAL completions after edit = 0.16s (10 repetitions)
TOTAL code actions = 0.02s (10 repetitions)
TOTAL code actions after edit = 7.98s (10 repetitions)
Benchmark ghcide-bench: FINISH

pepeiborra

Summarizing my understanding, this change:

Introduces useWithStaleFast and IdeAction, which are instant access to possibly obsolete data
Moves completions, hover, get-definition and outline to IdeAction, but importantly not code actions
Moves some rules to IdeAction, concretely the ones for .hie files.
Makes kick ask for SpanInfo instead of just TypeCheck, and only on the file that changed
Introduces DelayedAction

To me 1,2, and 4 make sense, I have questions about 5 and concerns about 3.

About DelayedActions:

What in these actions is delayed?
Why are they needed?
I like that they add metadata, improving logging, but what are the ids used for?

~~About moving rules to IdeAction, I know it's only the .hie rules and you have plans down the road for those, but I would rather postpone the change until those plans materialize.~~ In fact, I would prefer if there was no reference to IdeAction in the Rules module, and instead all the IdeAction computations were moved to a separate module for clarity.

pepeiborra · 2020-06-10T12:05:15Z

src/Development/IDE/Core/Shake.hs

+-- been modified, basically any use of shakeRun which is not blocking (ie
+-- which is not called via runAction).
+shakeRunInternal :: IdeState -> [DelayedAction a] -> IO ()
+shakeRunInternal ide as = void $ shakeRunUser ide as


There's a lot of ways of running things now:

shakeRunUser

shakeRunInternal

delay

delayedAction

Assuming all are needed, is there a naming that makes things clearer? e.g.runUserIO, runUserAction, runInternalIO, runInternalAction ...

shakeRunUser and shakeRunInternal sound very different but then the latter just calls the former. Are you planning to differentiate them further in the future?

To make things more confusing, shakeRunUser is literally only called from shakeRunInternal.

pepeiborra · 2020-06-10T12:10:33Z

src/Development/IDE/Core/Shake.hs

+-- A (maybe) stale result now, and an up to date one later
+data FastResult a = FastResult { stale :: Maybe (a,PositionMapping), uptoDate :: IO (Maybe a)  }
+
+useWithStaleFast :: IdeRule k v => k -> NormalizedFilePath -> IdeAction (Maybe (v, PositionMapping))


doc comment needed

src/Development/IDE/Core/Shake.hs

pepeiborra · 2020-06-10T12:17:50Z

src/Development/IDE/Core/Service.hs

-runAction :: IdeState -> Action a -> IO a
-runAction ide action = join $ shakeEnqueue ide action
+runAction :: String -> IdeState -> Action a -> IO a
+runAction = runActionSync


Can we get rid of either runAction or runActionSync? They are the same...

pepeiborra · 2020-06-10T12:39:18Z

src/Development/IDE/Core/Rules.hs

+
+  mbHieTimestamp <- either (\(_ :: IOException) -> Nothing) Just <$> (liftIO $ try $ getModificationTime hie_f)
+  srcTimestamp   <- MaybeT (either (\(_ :: IOException) -> Nothing) Just <$> (liftIO $ try $ getModificationTime $ fromNormalizedFilePath f))
+  liftIO $ print (mbHieTimestamp, srcTimestamp, hie_f, normal_hie_f)


Moving rules out of the Shake graph doesn't seem like a good idea. Action and IdeAction sound alike but they have very different semantics, in that the former is backed by a recalculation graph and a cache, whereas the second is not.

For instance, the body of this new version getHomeHieFile will run in full every time it's called, hitting the disk for timestamps and reloading the .hie file every time. That doesn't seem right.

This was not a Rule in the first place, so the semantics should remain sufficiently similar, since it uses proper Rules just like before.

You're right, I was under the impression that it was a rule for some reason. Maybe it should be, but that's a question for another PR

pepeiborra · 2020-06-10T12:48:24Z

src/Development/IDE/Core/Shake.hs

+
+data DelayedActionX a = DelayedActionX { actionName :: String -- Name we show to the user
+                                      -- The queue only contrains entries for
+                                      -- unique key values.


This comment seems out of place

mpickering · 2020-06-11T18:44:53Z

I am taking a week break so probably won't get back to this until after that.

cocreature

Thanks a lot for the PR!

A couple of comments:

I’m broadly happy to make actions explicitly triggered by users like hover and completions rely on stale results. I still very much dislike the fact that hovering twice without making any change can now give you different results but I don’t see a way to avoid this.
I’m less happy to do the same for things not triggered by users even if they are triggered by their editor. This includes things like codelenses, document outline and so on. There are two reasons why I don’t like it here:
1. One I simply don’t think the benefits are as big. This is usually not something users are actively waiting for since they didn’t trigger it so they don’t need to be as fast.
2. As a user, I have no obvious way to trigger those actions again. Admittedly VSCode is fairly keen on sending these requests but e.g. I don’t know a nice way for refreshing code lenses without modifying my file which will again change those results so they are still not up2date.
The names of the various use* functions are a mess now and they behave pretty inconsistently.
I’m slightly worried about the fact that we switch to a different codepath for testing in useWithStaleFast'. I understand why but it still makes it easy for things to creep in that aren’t covered by testing.

cocreature · 2020-06-12T08:33:57Z

bench/Main.hs

@@ -218,14 +218,16 @@ runBenchmarks benchmarks = do
    putStrLn $ intercalate ", " $ map (showDuration . snd) results

 runBench :: HasConfig => Bench -> IO Seconds
-runBench Bench {..} = handleAny (\e -> print e >> return (-1))
+runBench Bench {..} = handleAny (\e -> print e >> return (-2))


Yes please!

cocreature · 2020-06-12T08:47:42Z

src/Development/IDE/Core/Shake.hs

+    GlobalIdeOptions x <- getIdeGlobalExtras ide
+    return x
+
+-- | The result of an IDE operation. Warnings and errors are in the Diagnostic,


This comment doesn’t really seem to belong to anything its surroundings. I think this got moved when IdeResult got moved and probably got messed up in a rebase.

cocreature · 2020-06-12T09:02:00Z

src/Development/IDE/Core/Shake.hs

+-- been modified, basically any use of shakeRun which is not blocking (ie
+-- which is not called via runAction).
+shakeRunInternal :: IdeState -> [DelayedAction a] -> IO ()
+shakeRunInternal ide as = void $ shakeRunUser ide as


shakeRunUser and shakeRunInternal sound very different but then the latter just calls the former. Are you planning to differentiate them further in the future?

cocreature · 2020-06-12T09:08:22Z

src/Development/IDE/Core/Shake.hs

+-- been modified, basically any use of shakeRun which is not blocking (ie
+-- which is not called via runAction).
+shakeRunInternal :: IdeState -> [DelayedAction a] -> IO ()
+shakeRunInternal ide as = void $ shakeRunUser ide as


To make things more confusing, shakeRunUser is literally only called from shakeRunInternal.

cocreature · 2020-06-12T09:17:02Z

src/Development/IDE/Core/Shake.hs

+  liftIO $ case r of
+    Nothing -> do
+      IdeTesting testing <- optTesting <$> getIdeOptionsIO s
+      if testing


I’m slightly worried about having this so deep in the internals. That makes it easy to mess up something and not have tests catch it. Not quite sure what a better solution is here.

cocreature · 2020-06-12T09:21:29Z

src/Development/IDE/Core/Shake.hs

+-- | Actions with an ID for tracing purposes
+data DelayedActionExtra
+  = DelayedActionExtra
+  { _actionInternalId :: Unique


You aren’t using this unique anywhere.

I believe it is meant to be used for tracing purposes, maybe it will be helpful with the work @mpardalos is going to do.

cocreature · 2020-06-12T09:22:05Z

src/Development/IDE/Core/Shake.hs

+type DelayedActionInternal = DelayedActionX DelayedActionExtra
+
+{-# COMPLETE DelayedActionInternal#-}
+pattern DelayedActionInternal :: String -> Logger.Priority -> Action () -> Unique -> DelayedActionX DelayedActionExtra


Might just be me but I find that those pattern synonyms here just make things more confusing. I’d prefer either having two separate types or actually pass around the polymorphic version.

cocreature · 2020-06-12T09:25:54Z

src/Development/IDE/Core/Shake.hs

+mkDelayedAction = DelayedAction
+
+data DelayedActionX a = DelayedActionX
+  { actionName :: String -- ^ Name we show to the user


user is the IDE dev right? I couldn’t find anything where this is used for non-debugging purposes.

Can we clarify the doc comment here please?

cocreature · 2020-06-12T09:43:35Z

src/Development/IDE/Core/Rules.hs

@@ -87,11 +91,11 @@ toIdeResult = either (, Nothing) (([],) . Just)

 -- | useE is useful to implement functions that aren’t rules but need shortcircuiting
 -- e.g. getDefinition.
-useE :: IdeRule k v => k -> NormalizedFilePath -> MaybeT Action v
-useE k = MaybeT . use k
+useE :: IdeRule k v => k -> NormalizedFilePath -> MaybeT IdeAction (v, PositionMapping)


Afaict use and useE are very different now. use will block until you get the result. useE will return without blocking. use on the other hand still block. useE also will give you stale results. useWithStale also blocks but gives you the last result if the rule fails. Currently you somewhat rely on the fact that useE is called outside of rules.
I think this could do with some new function names, e.g., (very happy if you come up with better names)

use

useWithStale

useWithStaleNonBlocking

Also usesE is still blocking afaict. That seems wrong.

wz1000 · 2020-06-12T10:18:16Z

4. I’m slightly worried about the fact that we switch to a different codepath for testing in `useWithStaleFast'`. I understand why but it still makes it easy for things to creep in that aren’t covered by testing.

I would like to follow the codepath we follow for tests even when not testing, but @mpickering disagrees.

I think it is OK to block on the first request when no results have been computed. Otherwise you get some very weird behavior.

For example, suppose you have an MaybeT IdeAction which uses n rules, where none of the rules have been computed before. When you run the action the first time, it will fail after trying to get the results of the first rule, and queue up a request to compute the rule. The next time, it will fail when useEing the second rule(if the first rule has finished being computed), and so on. In the best case scenario, the action will have to run at least n times before it succeeds.

wz1000 · 2020-06-12T10:57:38Z

I’m less happy to do the same for things not triggered by users even if they are triggered by their editor. This includes things like codelenses, document outline and so on. There are two reasons why I don’t like it here:

One problem with this is that vscode does not send any new requests until it gets a response for these. To the user, this looks the server blocking on hover, go to definition etc.

This means that in practice, a lot of the gains of this PR will be negated. For instance, on modification, vscode will send these requests. Then when the user tries to hover on something, vscode will refuse to send any new requests until the code lenses etc. triggered by the modification return.

I don't really have a problem with this though since I don't use vscode and my editor only sends outline requests when I ask it to.

pepeiborra · 2020-06-12T11:28:46Z

I’m less happy to do the same for things not triggered by users even if they are triggered by their editor. This includes things like codelenses, document outline and so on. There are two reasons why I don’t like it here:

One problem with this is that vscode does not send any new requests until it gets a response for these. To the user, this looks the server blocking on hover, go to definition etc.

This means that in practice, a lot of the gains of this PR will be negated. For instance, on modification, vscode will send these requests. Then when the user tries to hover on something, vscode will refuse to send any new requests until the code lenses etc. triggered by the modification return.

I don't really have a problem with this though since I don't use vscode and my editor only sends outline requests when I ask it to.

Are you positive that it is VSCode who doesn't send the requests concurrently?
I believe that ghcide currently deals with requests single-threadedly, so I don't this is VSCode specific.

wz1000 · 2020-06-12T12:22:52Z

Are you positive that it is VSCode who doesn't send the requests concurrently?
I believe that ghcide currently deals with requests single-threadedly, so I don't this is VSCode specific.

I think so, since I got frustrated by this once and checked the logs.

pepeiborra · 2020-06-12T12:25:52Z

Are you positive that it is VSCode who doesn't send the requests concurrently?
I believe that ghcide currently deals with requests single-threadedly, so I don't this is VSCode specific.

I think so, since I got frustrated by this once and checked the logs.

Do you mean the logs printed by ghcide? There's every chance that those would be single-threaded too

wz1000 · 2020-06-12T12:29:59Z

I'm not sure now, they might have been the haskell-lsp logs. I will check again when I'm at my computer.

As suggested in #617. Taken fron haskell#624

* Write a cabal.project file As suggested in #617. Taken fron #624 * Write a cabal.project.local Otherwise Cabal still errors out * Override default hie dir Otherwise .hi and .hie files end up in different locations, which causes the getDefinition experiment to fail the second time it's run. This is because we assume in ghcide that .hi and .hie files have the same lifetimes, which is not true when the ..hie files are wiped but the .hi files aren't.

As suggested in #617. Taken fron haskell#624

wz1000 · 2020-06-19T12:04:50Z

I have simplified the API somewhat by removing delay, shakeRunUser and shakeRunInternal.

Now we have shakeEnqueue and shakeRestart, along with delayedAction, which is essentially shakeEnqueue for the IdeAction monad.

wz1000 · 2020-06-23T09:18:13Z

I believe this is ready for another look. I think I've addressed most of the comments here.

pepeiborra · 2020-06-23T09:42:48Z

I'm not sure now, they might have been the haskell-lsp logs. I will check again when I'm at my computer.

Did you have a chance to check whether the lsp-client is able to send requests concurrently?

pepeiborra · 2020-06-23T09:46:17Z

Since this branch has performance impacting changes, could you run and share differential benchmarks following the guidelines below

https://github.com/digital-asset/ghcide/blame/master/README.md#L313-L321

alanz · 2020-06-23T11:54:35Z

Since this branch has performance impacting changes

Given that this PR splits requests into a fast path and a slow path, we must be careful in analysing any performance graphs we produce.

wz1000 · 2020-06-23T18:04:53Z

Ah, I meant code lens. Code action doesn't use Typecheck any way, so it shouldn't be much of a problem.

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.

wz1000 · 2020-06-29T12:12:35Z

Benchmark results:

name                       | success | samples | startup | setup | experiment | maxResidency
-------------------------- | ------- | ------- | ------- | ----- | ---------- | ------------
hover                      | True    | 10      | 6.98s   | 0.00s | 0.05s      | 57MB        
edit                       | True    | 10      | 6.73s   | 0.00s | 2.90s      | 192MB       
hover after edit           | True    | 10      | 6.77s   | 0.00s | 0.26s      | 57MB        
getDefinition              | True    | 10      | 6.95s   | 0.00s | 0.07s      | 56MB        
documentSymbols            | True    | 100     | 7.09s   | 0.00s | 0.58s      | 56MB        
documentSymbols after edit | True    | 100     | 6.74s   | 0.00s | 1.14s      | 197MB       
completions after edit     | True    | 10      | 6.67s   | 0.00s | 0.56s      | 57MB        
code actions               | True    | 10      | 6.91s   | 0.24s | 0.11s      | 57MB        
code actions after edit    | True    | 10      | 6.51s   | 0.00s | 3.61s      | 219MB

wz1000 · 2020-06-29T12:14:39Z

Here is the entire output of the benchmark tool:

Running hover benchmark
0.04s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
Running edit benchmark
0.24s
0.24s
0.59s
0.35s
0.22s
0.22s
0.38s
0.22s
0.22s
0.22s
Running hover after edit benchmark
0.24s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
Running getDefinition benchmark
0.05s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
Running documentSymbols benchmark
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.01s
0.01s
0.01s
0.02s
0.00s
0.01s
0.01s
0.01s
0.01s
0.00s
0.01s
0.01s
0.00s
0.01s
0.01s
0.00s
0.01s
0.00s
0.01s
0.01s
0.00s
0.01s
0.01s
0.02s
0.01s
0.01s
0.01s
0.01s
0.00s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.00s
0.01s
0.00s
0.01s
0.01s
0.00s
0.01s
0.00s
0.01s
0.01s
0.01s
0.00s
0.01s
0.00s
0.01s
0.01s
0.00s
0.01s
0.00s
0.00s
0.00s
0.01s
0.01s
0.00s
0.01s
0.01s
0.00s
0.00s
0.00s
0.02s
0.01s
0.01s
0.00s
0.01s
0.01s
0.00s
0.01s
0.00s
0.01s
0.01s
0.00s
0.01s
0.00s
0.01s
0.01s
0.00s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.02s
0.00s
0.00s
0.00s
0.00s
0.00s
Running documentSymbols after edit benchmark
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.02s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.16s
0.01s
0.01s
0.01s
0.01s
0.03s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.02s
0.01s
0.19s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.01s
0.03s
0.01s
0.01s
0.01s
Running completions after edit benchmark
0.26s
0.01s
0.02s
0.02s
0.02s
0.02s
0.02s
0.02s
0.18s
0.01s
Running code actions benchmark
0.08s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
0.00s
Running code actions after edit benchmark
0.30s
0.21s
0.38s
0.19s
0.17s
0.48s
0.59s
0.38s
0.53s

As we can see, for the results returning stale information, there is a pause for the initial run. Subsequent requests return instantaneously.

wz1000 · 2020-06-29T12:56:11Z

Here are the differential results: http://78.47.113.11/results/

wz1000 · 2020-06-29T13:02:35Z

I think the questions that remain are:

Should we block on the initial computation of a request (like we do when --testing) or fail until we have a cache?
Should we respond immediately with stale information for requests like codeLens

pepeiborra · 2020-06-29T18:36:11Z

I don't enjoy having to hover twice, go-to-def twice, etc, so I'd prefer blocking here.
It seems trivial to make ghcide handle requests concurrently, see https://github.com/pepeiborra/ghcide/tree/multi-threaded, so I'd prefer to be synchronous for code lenses and all other editor triggered requests

wz1000 · 2020-06-29T18:55:08Z

I don't enjoy having to hover twice, go-to-def twice, etc, so I'd prefer blocking here.

It seems trivial to make ghcide handle requests concurrently, see https://github.com/pepeiborra/ghcide/tree/multi-threaded, so I'd prefer to be synchronous for code lenses and all other editor triggered requests

Addressed both points.

wz1000 · 2020-06-29T18:55:57Z

From the differential benchmarks, this result seems to be the most important/striking one:

pepeiborra · 2020-06-29T19:04:57Z

From the differential benchmarks, this result seems to be the most important/striking one:

Very nice!! That's much better than the optimisation in #670, which is bottle-necked a bit by Shake

wz1000 · 2020-06-29T19:19:01Z

See also

cocreature

Nice! Sorry for the delay. Two small requests for doc fixes before merging.

cocreature · 2020-06-30T08:07:14Z

src/Development/IDE/Core/Shake.hs

+
+-- | Lookup value in the database and return with the stale value immediately
+-- Will queue an action to refresh the value.
+-- Never blocks.


This is no longer accurate. You block the first time (which is a good idea imho so just update the docs).

cocreature · 2020-06-30T08:08:13Z

src/Development/IDE/Core/Shake.hs

+mkDelayedAction = DelayedAction
+
+data DelayedActionX a = DelayedActionX
+  { actionName :: String -- ^ Name we show to the user


Can we clarify the doc comment here please?

wz1000 · 2020-06-30T08:14:05Z

Fixed!

* Write a cabal.project file As suggested in haskell/ghcide#617. Taken fron haskell/ghcide#624 * Write a cabal.project.local Otherwise Cabal still errors out * Override default hie dir Otherwise .hi and .hie files end up in different locations, which causes the getDefinition experiment to fail the second time it's run. This is because we assume in ghcide that .hi and .hie files have the same lifetimes, which is not true when the ..hie files are wiped but the .hi files aren't.

…askell/ghcide#624) * Use stale information for hover and completions 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. * Remove DelayedActionExtra * hlint * Fix progress * Always block instead of fail on initial computation * Can block for code lens * Update docs Co-authored-by: Zubin Duggal <[email protected]>

* Write a cabal.project file As suggested in haskell/ghcide#617. Taken fron haskell/ghcide#624 * Write a cabal.project.local Otherwise Cabal still errors out * Override default hie dir Otherwise .hi and .hie files end up in different locations, which causes the getDefinition experiment to fail the second time it's run. This is because we assume in ghcide that .hi and .hie files have the same lifetimes, which is not true when the ..hie files are wiped but the .hi files aren't.

…askell/ghcide#624) * Use stale information for hover and completions 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. * Remove DelayedActionExtra * hlint * Fix progress * Always block instead of fail on initial computation * Can block for code lens * Update docs Co-authored-by: Zubin Duggal <[email protected]>

* Write a cabal.project file As suggested in haskell/ghcide#617. Taken fron haskell/ghcide#624 * Write a cabal.project.local Otherwise Cabal still errors out * Override default hie dir Otherwise .hi and .hie files end up in different locations, which causes the getDefinition experiment to fail the second time it's run. This is because we assume in ghcide that .hi and .hie files have the same lifetimes, which is not true when the ..hie files are wiped but the .hi files aren't.

…askell/ghcide#624) * Use stale information for hover and completions 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. * Remove DelayedActionExtra * hlint * Fix progress * Always block instead of fail on initial computation * Can block for code lens * Update docs Co-authored-by: Zubin Duggal <[email protected]>

ndmitchell reviewed Jun 10, 2020

View reviewed changes

pepeiborra reviewed Jun 10, 2020

View reviewed changes

wz1000 force-pushed the wip/shake-queue-2 branch 2 times, most recently from 46be3ff to 3030195 Compare June 11, 2020 12:23

cocreature reviewed Jun 12, 2020

View reviewed changes

pepeiborra added a commit to pepeiborra/ghcide that referenced this pull request Jun 14, 2020

Write a cabal.project file

739a5a0

As suggested in #617. Taken fron haskell#624

pepeiborra mentioned this pull request Jun 14, 2020

Write a cabal.project file in the benchmark example #640

Merged

pepeiborra added a commit to pepeiborra/ghcide that referenced this pull request Jun 14, 2020

Write a cabal.project file

1f8c237

As suggested in #617. Taken fron haskell#624

wz1000 force-pushed the wip/shake-queue-2 branch from f314ba4 to 0835fb6 Compare June 15, 2020 10:23

pepeiborra added a commit to pepeiborra/ghcide that referenced this pull request Jun 15, 2020

Write a cabal.project file

dd6a15e

As suggested in #617. Taken fron haskell#624

wz1000 force-pushed the wip/shake-queue-2 branch from 0835fb6 to 7a498e5 Compare June 19, 2020 11:35

wz1000 mentioned this pull request Jun 23, 2020

ghcide master? haskell/haskell-language-server#173

Closed

wz1000 force-pushed the wip/shake-queue-2 branch 2 times, most recently from 96ece92 to 3a064ee Compare June 23, 2020 09:17

mpickering and others added 2 commits June 29, 2020 17:39

Remove DelayedActionExtra

ab2d73f

wz1000 force-pushed the wip/shake-queue-2 branch from 48349ec to 84aac45 Compare June 29, 2020 12:10

wz1000 added 2 commits June 29, 2020 17:50

hlint

a7f10c1

Fix progress

68d8dfe

wz1000 force-pushed the wip/shake-queue-2 branch from 84aac45 to 68d8dfe Compare June 29, 2020 12:20

wz1000 added 2 commits June 30, 2020 00:17

Always block instead of fail on initial computation

b55dc55

Can block for code lens

816f75b

cocreature approved these changes Jun 30, 2020

View reviewed changes

Update docs

9744f00

cocreature merged commit d999084 into haskell:master Jun 30, 2020

pepeiborra mentioned this pull request Jan 1, 2021

Collect progressEnd timings in benchmarks haskell/haskell-language-server#860

Closed

ndmitchell mentioned this pull request Jan 1, 2021

Upstream things from hls-2 haskell/haskell-language-server#872

Closed

8 tasks

		@@ -218,14 +218,16 @@ runBenchmarks benchmarks = do
		putStrLn $ intercalate ", " $ map (showDuration . snd) results

Use stale information if it's available to answer requests quickly #624

Use stale information if it's available to answer requests quickly #624

Conversation

mpickering commented Jun 10, 2020

digitalasset-cla commented Jun 10, 2020 • edited Loading

ndmitchell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepeiborra commented Jun 10, 2020

mpickering commented Jun 10, 2020

pepeiborra left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpickering commented Jun 11, 2020

cocreature left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wz1000 commented Jun 12, 2020

wz1000 commented Jun 12, 2020

pepeiborra commented Jun 12, 2020

wz1000 commented Jun 12, 2020

pepeiborra commented Jun 12, 2020

wz1000 commented Jun 12, 2020

wz1000 commented Jun 19, 2020

wz1000 commented Jun 23, 2020

pepeiborra commented Jun 23, 2020 • edited Loading

pepeiborra commented Jun 23, 2020

alanz commented Jun 23, 2020

wz1000 commented Jun 23, 2020

wz1000 commented Jun 29, 2020

wz1000 commented Jun 29, 2020

wz1000 commented Jun 29, 2020

wz1000 commented Jun 29, 2020 • edited Loading

pepeiborra commented Jun 29, 2020

wz1000 commented Jun 29, 2020

wz1000 commented Jun 29, 2020

pepeiborra commented Jun 29, 2020 • edited Loading

wz1000 commented Jun 29, 2020

cocreature left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wz1000 commented Jun 30, 2020

digitalasset-cla commented Jun 10, 2020 •

edited

Loading

pepeiborra left a comment •

edited

Loading

pepeiborra commented Jun 23, 2020 •

edited

Loading

wz1000 commented Jun 29, 2020 •

edited

Loading

pepeiborra commented Jun 29, 2020 •

edited

Loading