Use object code for Template Haskell #836
Conversation
|
OK, I figured out the issue ( |
|
ghc-lib :( I have no idea how to locally build this configuration, and worse still, no tests run so I have no way to know if my changes work. |
|
It would be cool to see the performance improvements of this change, but for that we need a benchmark example that uses TH (and doesn't have too many dependencies). EDIT: see #838, now you only need to find a suitable example (the hard part) |
src/Development/IDE/Core/Compile.hs
Outdated
|
|
||
| generateObjectCode :: HscEnv -> TcModuleResult -> IO (IdeResult Linkable) | ||
| generateObjectCode hscEnv tmr = do | ||
| (compile_diags, Just (_, guts, _)) <- compileModule (RunSimplifier True) hscEnv tmr |
There was a problem hiding this comment.
Can we avoid compiling the module here? I think the answer is not, because GenerateCore is not used anymore
src/Development/IDE/Core/Compile.hs
Outdated
| fmap (either (, Nothing) (second Just)) $ | ||
| evalGhcEnv packageState $ | ||
| catchSrcErrors "compile" $ do | ||
| setupEnv (deps ++ [(tmrModSummary tmr, tmrModInfo tmr)]) | ||
|
|
||
| setupEnv [(tmrModSummary tmr, tmrModInfo tmr)] |
There was a problem hiding this comment.
surprised that we don't need to setup the env with the dependencies
There was a problem hiding this comment.
I've looked for every usage of this and made sure it is only called with GhcSessionDeps. In that case I don't think we need to call setupEnv
src/Development/IDE/Core/Rules.hs
Outdated
| setPriority priorityGenerateCore | ||
| packageState <- hscEnv <$> use_ GhcSession file | ||
| liftIO $ compileModule runSimplifier packageState [(tmrModSummary x, tmrModInfo x) | x <- tms] tm | ||
| liftIO $ compileModule runSimplifier packageState tm |
There was a problem hiding this comment.
surprised that this can work with GhcSession and doesn't need GhcSessionDeps
Possibly dumb question, but: we need object code for a module's dependencies if the module uses TH. This PR achieves that by checking a module's dependants to see if they'll eventually use TH. But couldn't we just set up rules for computing the object code and only request it if we need it? i.e.
Isn't this the joy of building our IDE on a build system? I haven't understood the code in detail, but it looks like you're sort of doing that already. So what stops this becoming entirely demand-driven? |
Good question. This is indeed what we were doing before, with the To compile a module, we need the actual typechecked AST that ghc computes after typechecking. However, to avoid excessive memory usage, we only keep typechecked ASTs for files that the user has open in their editor. For all other modules, we compute a This is no good for when we need to compile object/byte code for a module. Previously, we were calling the Now, we figure out if we are going to need the object code before we throw away the typechecked AST, restoring the invariant that we only keep typechecked ASTs in memory for files the user has opened. Furthermore, we avoid duplicating work caused by needing to typecheck the same file twice. |
|
There are a bunch of options here we can expose to the user -
For 1) and 2), I imagine we want to set a default based on some benchmarks. |
|
Interesting, this is potentially a usecase for Core Interface Sections, something we've been working on at IOHK. The idea being that you dump the post-desugaring Core into the interface file, and ideally you should then be able to resume compilation from that step. So you'd be able to do the Typecheck->ObjectCode step later without redoing work. (Speculative, obviously) |
My views:
|
|
I thought bytecode was serialised for the external interpreter? |
wz1000
force-pushed
the
obj-code
branch
2 times, most recently
from
1b7ea9f
to
4d57c12
Compare
|
I think I have found the root cause of the bug that #741 fixed. We call If we want to generate code, we need to use For now, I've changed Also, @mpickering says we always need to run the simplifier, as discussed in #410. We do set |
|
The way to run it at a lower priority is to do: That says start with desugaring, but once you have type checked, put yourself to the back of the queue behind all other type checks. |
|
@ndmitchell what happens if a rule with high priority |
|
The rule with high priority will wait for the low priority one to finish - you get priority inversion - so its generally not a good thing to do (although its not that harmful, unless you have a lot of normal priority stuff waiting around) |
|
I'm a bit confused about the meaning of priorities. We have the following in newtype Priority = Priority Double
setPriority :: Priority -> Action ()
setPriority (Priority p) = reschedule p
priorityTypeCheck :: Priority
priorityTypeCheck = Priority 0
priorityGenerateCore :: Priority
priorityGenerateCore = Priority (-1)
priorityFilesOfInterest :: Priority
priorityFilesOfInterest = Priority (-2)Which one of these will be given preference by the scheduler? Reading the reschedule x = do
(wait, _) <- actionAlwaysRequeuePriority (PoolDeprioritize $ negate x) $ pure ()Too many negations for me to follow 😕 |
|
Getting some strange failures when I add desugaring to kick ¯\_(ツ)_/¯ First one is particularly spooky. Otherwise, I think this is ready after a re-review. |
|
This looks fantastic, great job @wz1000, hopefully fixing all the TH bugs in the process. I will find the time to re-review tomorrow evening. In the meantime:
|
|
The changes to the shake graph are as follows:
Benchmarks and fixing the remaining tests are next on my agenda. I'm still conflicted about whether we need to keep the result of |
|
I'm using the following configuration for benchmarking: example:
name: haskell-lsp-types
version: 0.22.0.0
# # path: path/to/example
module: src/Language/Haskell/LSP/Types/Lens.hsHere are the results: User time and total time increase, max residency is halved. |
|
Here are the results if we don't desugar and just typecheck diff --git a/src/Development/IDE/Core/OfInterest.hs b/src/Development/IDE/Core/OfInterest.hs
index 1caacfc3..dda25030 100644
--- a/src/Development/IDE/Core/OfInterest.hs
+++ b/src/Development/IDE/Core/OfInterest.hs
@@ -94,10 +94,10 @@ kick = mkDelayedAction "kick" Debug $ do
liftIO $ progressUpdate KickStarted
-- Update the exports map for the project
- results <- uses GenerateCore $ HashMap.keys files
+ results <- uses TypeCheck $ HashMap.keys files
ShakeExtras{exportsMap} <- getShakeExtras
let mguts = catMaybes results
- !exportsMap' = createExportsMapMg mguts
+ !exportsMap' = createExportsMapTc $ map tmrTypechecked mguts
liftIO $ modifyVar_ exportsMap $ evaluate . (exportsMap' <>) |
|
If we use bytecode instead of object code, and don't desugar in kick, here is what we get: Something is clearly going wrong somewhere, or we would expect similar performance to upstream |
|
Thank you so much! This is great! |
…l/ghcide#836) * Use object code for TH * Set target location for TargetFiles * Fix tests * hlint * fix build on 8.10 * fix ghc-lib * address review comments * hlint * better error handling if module headers don't parse * Always desugar, don't call interactive API functions * deprioritize desugar when not TH, fix iface handling * write hie file on save * more tweaks * fix tests * disable desugarer warnings * use ModGuts for exports map * don't desugar * use bytecode * make HiFileStable early-cutoff * restore object code * re-enable desugar * review comments * Don't use ModIface for DocMap * fix docs for the current module * mark test as broken on windows
…l/ghcide#836) * Use object code for TH * Set target location for TargetFiles * Fix tests * hlint * fix build on 8.10 * fix ghc-lib * address review comments * hlint * better error handling if module headers don't parse * Always desugar, don't call interactive API functions * deprioritize desugar when not TH, fix iface handling * write hie file on save * more tweaks * fix tests * disable desugarer warnings * use ModGuts for exports map * don't desugar * use bytecode * make HiFileStable early-cutoff * restore object code * re-enable desugar * review comments * Don't use ModIface for DocMap * fix docs for the current module * mark test as broken on windows
…l/ghcide#836) * Use object code for TH * Set target location for TargetFiles * Fix tests * hlint * fix build on 8.10 * fix ghc-lib * address review comments * hlint * better error handling if module headers don't parse * Always desugar, don't call interactive API functions * deprioritize desugar when not TH, fix iface handling * write hie file on save * more tweaks * fix tests * disable desugarer warnings * use ModGuts for exports map * don't desugar * use bytecode * make HiFileStable early-cutoff * restore object code * re-enable desugar * review comments * Don't use ModIface for DocMap * fix docs for the current module * mark test as broken on windows
Rework of mpickering#43
Should improve performance and also functions as a workaround for #614
We now generate object code along with the interface file. Object code is only generated for a module if
For this to work, we require the complete module graph. Earlier, tests were failing because
TargetFile's were not included in the set of known files. I fixed this, but now tests are failing because the modules don't define amainfunction. Any idea why this is happening @pepeiborra? See my last commit.FIxes #614
Fixes #107