HLS 1.9 seems to do a huge amount of work when multiple units are open and one file is changed

[ocharles]

Your environment

NixOS, GHC 9.2.5, Haskell.nix, Helix. hie.yaml is:

cradle:
  cabal:

Steps to reproduce

Open two files in the same cabal.project that belong to different units, I think where the files are somewhat dependent (e.g., A.hs is a dependency of B.hs). Then edit something in A.hs that causes a type error (syntax errors don't seem to be affected) and wait a long time until any error feedback arrives.

Expected behaviour

HLS reports the typo promptly.

Actual behaviour

I've tried to record this happening with asciinema, but it's maybe not immediately obvious. See https://asciinema.org/a/hymieQM8CZY4KkIRnfJERrnwX.

In the first 20s or so I open one file and make some edits to current errors. When I make changes, the error goes away (while type checking happens) and then comes back, pretty swiftly. Next, I open a new file, and then return to the original file. Now making the same edits takes a lot longer. Also notice the status line at the bottom - it starts showing a lot more activity, but I don't really know what it means.

As my session grows, this time gets much much longer, to the point where I find myself waiting upwards of 10/20s just to get any feedback.

[michaelpj]

Does Helix support changed files notifications? We observed this also for neovim users, and we tracked it down to that. HLS has much worse dependency tracking if we don't get accurate file changed notifications from the client. It does however seem that this has regressed somewhere, but we don't know where yet.

cc @guibou

[guibou]

@michaelpj I cannot confirm that Helix doest support file notifications, but when I observed the problem with neovim, I also observed something similar with Helix.

Note that the problem was, as far as I know, already there before, but maybe made worse because of

#3452

@ocharles I don't know about helix, but here what I've done to fix the problem with neovim:

• Use the changes from Two recompilation avoidance related bug fixes #3452
• FORCE HLS to use the codepath it uses when the editor support file changed notifications. It leads to weird behavior when files changes from outside of the editor (e.g. git checkout), but it dramatically improves the performances for me until neovim (maybe helix) support file change notification. When the problem happen, you just need to locally open/edit/save the changed files in the editor.

If it may help you, here is my workflow.

ghcide-avoid-recomp.diff:

diff --git a/ghcide/src/Development/IDE/Core/Compile.hs b/ghcide/src/Development/IDE/Core/Compile.hs
index 82addbdf..3d023d0d 100644
--- ghcide/src/Development/IDE/Core/Compile.hs
+++ ghcide/src/Development/IDE/Core/Compile.hs
@@ -102,6 +102,7 @@ import qualified Language.LSP.Types                as LSP
 import           System.Directory
 import           System.FilePath
 import           System.IO.Extra                   (fixIO, newTempFileWithin)
+import           System.IO.Unsafe
 import           Unsafe.Coerce

 #if MIN_VERSION_ghc(9,0,1)
@@ -435,6 +436,43 @@ tcRnModule hsc_env tc_helpers pmod = do
 -- anywhere. So we zero it out.
 -- The field is not serialized or deserialised from disk, so we don't need to remove it
 -- while reading an iface from disk, only if we just generated an iface in memory
+--
+
+
+
+-- | See https://github.com/haskell/haskell-language-server/issues/3450
+-- GHC's recompilation avoidance in the presense of TH is less precise than
+-- HLS. To avoid GHC from pessimising HLS, we filter out certain dependency information
+-- that we track ourselves. See also Note [Recompilation avoidance in the presence of TH]
+filterUsages :: [Usage] -> [Usage]
+#if MIN_VERSION_ghc(9,3,0)
+filterUsages = filter $ \case UsageHomeModuleInterface{} -> False
+                              _ -> True
+#else
+filterUsages = id
+#endif
+
+-- | Mitigation for https://gitlab.haskell.org/ghc/ghc/-/issues/22744
+shareUsages :: ModIface -> ModIface
+shareUsages iface = iface {mi_usages = usages}
+  where usages = map go (mi_usages iface)
+        go usg@UsageFile{} = usg {usg_file_path = fp}
+          where !fp = shareFilePath (usg_file_path usg)
+        go usg = usg
+
+filePathMap :: IORef (HashMap.HashMap FilePath FilePath)
+filePathMap = unsafePerformIO $ newIORef HashMap.empty
+{-# NOINLINE filePathMap #-}
+
+shareFilePath :: FilePath -> FilePath
+shareFilePath k = unsafePerformIO $ do
+  atomicModifyIORef' filePathMap $ \km ->
+    let new_key = HashMap.lookup k km
+    in case new_key of
+          Just v -> (km, v)
+          Nothing -> (HashMap.insert k k km, k)
+{-# NOINLINE shareFilePath  #-}
+

 mkHiFileResultNoCompile :: HscEnv -> TcModuleResult -> IO HiFileResult
 mkHiFileResultNoCompile session tcm = do
@@ -444,7 +482,7 @@ mkHiFileResultNoCompile session tcm = do
   details <- makeSimpleDetails hsc_env_tmp tcGblEnv
   sf <- finalSafeMode (ms_hspp_opts ms) tcGblEnv
   iface' <- mkIfaceTc hsc_env_tmp sf details ms tcGblEnv
-  let iface = iface' { mi_globals = Nothing } -- See Note [Clearing mi_globals after generating an iface]
+  let iface = iface' { mi_globals = Nothing, mi_usages = filterUsages (mi_usages iface') } -- See Note [Clearing mi_globals after generating an iface]
   pure $! mkHiFileResult ms iface details (tmrRuntimeModules tcm) Nothing

 mkHiFileResultCompile
@@ -486,7 +524,7 @@ mkHiFileResultCompile se session' tcm simplified_guts = catchErrs $ do
   let !partial_iface = force (mkPartialIface session details simplified_guts)
   final_iface' <- mkFullIface session partial_iface
 #endif
-  let final_iface = final_iface' {mi_globals = Nothing} -- See Note [Clearing mi_globals after generating an iface]
+  let final_iface = final_iface' {mi_globals = Nothing, mi_usages = filterUsages (mi_usages final_iface')} -- See Note [Clearing mi_globals after generating an iface]

   -- Write the core file now
   core_file <- case mguts of
@@ -1462,7 +1500,8 @@ loadInterface session ms linkableNeeded RecompilationInfo{..} = do
           regenerate linkableNeeded

     case (mb_checked_iface, recomp_iface_reqd) of
-      (Just iface, UpToDate) -> do
+      (Just iface', UpToDate) -> do
+             let iface = shareUsages iface'
              details <- liftIO $ mkDetailsFromIface sessionWithMsDynFlags iface
              -- parse the runtime dependencies from the annotations
              let runtime_deps
diff --git a/ghcide/src/Development/IDE/GHC/Compat.hs b/ghcide/src/Development/IDE/GHC/Compat.hs
index 83195acd..46da9223 100644
--- ghcide/src/Development/IDE/GHC/Compat.hs
+++ ghcide/src/Development/IDE/GHC/Compat.hs
@@ -43,6 +43,8 @@ module Development.IDE.GHC.Compat(
     myCoreToStgExpr,
 #endif

+    Usage(..),
+
     FastStringCompat,
     bytesFS,
     mkFastStringByteString,
@@ -167,7 +169,7 @@ import           GHC.Runtime.Context                   (icInteractiveModule)
 import           GHC.Unit.Home.ModInfo                 (HomePackageTable,
                                                         lookupHpt)
 #if MIN_VERSION_ghc(9,3,0)
-import GHC.Unit.Module.Deps (Dependencies(dep_direct_mods))
+import GHC.Unit.Module.Deps (Dependencies(dep_direct_mods), Usage(..))
 #else
 import GHC.Unit.Module.Deps (Dependencies(dep_mods))
 #endif

ghcide-force-runsubset.diff:

diff --git a/ghcide/src/Development/IDE/Main.hs b/ghcide/src/Development/IDE/Main.hs
index 081c78ac..720fa4c9 100644
--- ghcide/src/Development/IDE/Main.hs
+++ ghcide/src/Development/IDE/Main.hs
@@ -332,7 +332,7 @@ defaultMain recorder Arguments{..} = withHeapStats (cmapWithPrio LogHeapStats re
                   let def_options = argsIdeOptions config sessionLoader

                   -- disable runSubset if the client doesn't support watched files
-                  runSubset <- (optRunSubset def_options &&) <$> LSP.runLspT env isWatchSupported
+                  runSubset <- pure True
                   log Debug $ LogShouldRunSubset runSubset

                   let options = def_options

I do have that in my nix overlay (considering that you are using nixos and haskell.nix, I suppose you'll know what to do):

      ghcide = hprev.ghcide.overrideAttrs (old: {
        #
        patches = [
          # hls 1.9 enables for an unknown reason some optimization only
          # if the client supports filewatching, which is not the case of
          # neovim, which makes the LSP slow
          # this unconditionnally enables the optimization
          ../patches/ghcide-force-runsubset.diff

          # Avoid too much rebuilds
          # https://github.com/haskell/haskell-language-server/pull/3452
          ../patches/ghcide-avoid-recomp.diff
        ];
      });

[guibou]

@ocharles I just tested with helix in my developement shell (integrating the above patchs) and it is way faster.

[guibou]

Latest point (not related to this ticket, but may be linked for future users), I've observed regression with GHC 9.4.4 as described here: https://gitlab.haskell.org/ghc/ghc/-/issues/22779 where hs boot files are always reloaded. I don't know how it impacts HLS (here the boot files are reloading really quickly), but maybe also something to have a look for if you are using GHC 9.4.4

[ocharles]

@michaelpj

Does Helix support changed files notifications? We observed this also for neovim users, and we tracked it down to that.

Interesting! Do you know where you tracked it down? I.e., is there a Github issue where I can read more?

[michaelpj]

No, it was discussion on chat, thanks to @guibou for recording his findings here.

[dariooddenino]

I'm having the same issue.
GHC 9.2.5, hls 1.9.0.0 compiled from master, ubuntu and helix.

The time to get error feedbacks gets progressively worse and it can quickly get in the ~25 seconds range for just a small change.

[ocharles]

Helix doesn't support file change notifications (yet): helix-editor/helix#5467 (comment)

[ocharles]

Unfortunately 1.9.1.0 still seems to have this problem. I wasn't having this problem on 1.9 when I used a combination of #3452 and the extra patch in #3458 (comment) (ghcide-force-runsubset.diff)

Edit: actually I guess this isn't surprising, given that patch pretends that the client supports watching files, which Helix doesn't.

[dariooddenino]

Unfortunately 1.9.1.0 still seems to have this problem. I wasn't having this problem on 1.9 when I used a combination of #3452 and the extra patch in #3458 (comment) (ghcide-force-runsubset.diff)

Edit: actually I guess this isn't surprising, given that patch pretends that the client supports watching files, which Helix doesn't.

Do you have a fork with those patches applied? I've tried updating to 1.9.1.0 and the situation got even worse for me.

[ocharles]

@dariooddenino I don't have a fork, I run 1.9.1.0 from Hackage and apply just ghcide-force-runsubset.diff from above.

[dariooddenino]

Mm I've tried both ghcup compile hls --git-ref master --ghc 9.2.5 -o "1.9.1.0fix" -p "path/to/ghcide-force-runsubset.diff" and with --version 1.9.1.0 instead of --git-ref master and the result is

No file to patch. Skipping patch.
1 out of 1 hunk ignored

[ocharles]

Here is the patch I use:

diff --git a/ghcide/src/Development/IDE/Main.hs b/ghcide/src/Development/IDE/Main.hs
index 081c78ac..720fa4c9 100644
--- ghcide/src/Development/IDE/Main.hs
+++ ghcide/src/Development/IDE/Main.hs
@@ -332,7 +332,7 @@ defaultMain recorder Arguments{..} = withHeapStats (cmapWithPrio LogHeapStats re
                   let def_options = argsIdeOptions config sessionLoader

                   -- disable runSubset if the client doesn't support watched files
-                  runSubset <- (optRunSubset def_options &&) <$> LSP.runLspT env isWatchSupported
+                  runSubset <- pure True
                   log Debug $ LogShouldRunSubset runSubset

                   let options = def_options

but I don't think this is any different from above. I would look at why that patch isn't applying, anyway. Maybe fork HLS and make the edits there so you can be 100% confident

[michaelpj]

Okay, so the situation as I understand it remains:

• HLS has a safe but slow mode for clients that don't support file change notifications
• Using the fast mode is unsafe in that situation but maybe worth it for some people

So the default behaviour is probably right (we should be safe by default), but maybe we need a configurable option to do the unsafe thing?