Skip to content

Various strictness improvements #3413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 21, 2022
Merged

Various strictness improvements #3413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb99e7d
Various strictness improvements
wz1000 Dec 20, 2022
d3542de
Merge branch 'master' into wip/mem-strictness
wz1000 Dec 21, 2022
66152fe
Merge branch 'master' into wip/mem-strictness
mergify[bot] Dec 21, 2022
d49e24c
Merge branch 'master' into wip/mem-strictness
wz1000 Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 2 additions & 14 deletions ghcide/src/Development/IDE/Core/Compile.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,7 +986,7 @@ loadModulesHome
-> HscEnv
loadModulesHome mod_infos e =
#if MIN_VERSION_ghc(9,3,0)
hscUpdateHUG (\hug -> foldr addHomeModInfoToHug hug mod_infos) (e { hsc_type_env_vars = emptyKnotVars })
hscUpdateHUG (\hug -> foldl' (flip addHomeModInfoToHug) hug mod_infos) (e { hsc_type_env_vars = emptyKnotVars })
#else
let !new_modules = addListToHpt (hsc_HPT e) [(mod_name x, x) | x <- mod_infos]
in e { hsc_HPT = new_modules
Expand Down Expand Up @@ -1454,18 +1454,6 @@ loadInterface session ms linkableNeeded RecompilationInfo{..} = do

case (mb_checked_iface, recomp_iface_reqd) of
(Just iface, UpToDate) -> do
-- If we have an old value, just return it
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that reusing the same ModDetails as we had last time can keep the previous HscEnv and corresponding HomePackageTable data structures alive. Solution is to delete this code and always regenerate the ModDetails from the interface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!! Very important to do this (and cheap)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a cautionary note? should we somehow get rid of old_value entirely? or is it okay to use it so long as we don't retain anything from it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need it for the old ModIface (which is safe to use).

case old_value of
Just (old_hir, _)
| isNothing linkableNeeded || isJust (hirCoreFp old_hir)
-> do
-- Perform the fine grained recompilation check for TH
maybe_recomp <- checkLinkableDependencies get_linkable_hashes (hsc_mod_graph sessionWithMsDynFlags) (hirRuntimeModules old_hir)
case maybe_recomp of
Just msg -> do_regenerate msg
Nothing -> return ([], Just old_hir)
-- Otherwise use the value from disk, provided the core file is up to date if required
_ -> do
details <- liftIO $ mkDetailsFromIface sessionWithMsDynFlags iface
-- parse the runtime dependencies from the annotations
let runtime_deps
Expand Down Expand Up @@ -1552,7 +1540,7 @@ showReason (RecompBecause s) = s
mkDetailsFromIface :: HscEnv -> ModIface -> IO ModDetails
mkDetailsFromIface session iface = do
fixIO $ \details -> do
let hsc' = hscUpdateHPT (\hpt -> addToHpt hpt (moduleName $ mi_module iface) (HomeModInfo iface details Nothing)) session
let !hsc' = hscUpdateHPT (\hpt -> addToHpt hpt (moduleName $ mi_module iface) (HomeModInfo iface details Nothing)) session
initIfaceLoad hsc' (typecheckIface iface)

coreFileToCgGuts :: HscEnv -> ModIface -> ModDetails -> CoreFile -> IO CgGuts
Expand Down
2 changes: 1 addition & 1 deletion ghcide/src/Development/IDE/GHC/Compat.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,7 @@ getModuleHash = mi_mod_hash . mi_final_exts

disableWarningsAsErrors :: DynFlags -> DynFlags
disableWarningsAsErrors df =
flip gopt_unset Opt_WarnIsError $ foldl' wopt_unset_fatal df [toEnum 0 ..]
flip gopt_unset Opt_WarnIsError $! foldl' wopt_unset_fatal df [toEnum 0 ..]

isQualifiedImport :: ImportDecl a -> Bool
isQualifiedImport ImportDecl{ideclQualified = NotQualified} = False
Expand Down
4 changes: 2 additions & 2 deletions ghcide/src/Development/IDE/Types/Action.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,13 @@ abortQueue :: DelayedActionInternal -> ActionQueue -> STM ()
abortQueue x ActionQueue {..} = do
qq <- flushTQueue newActions
mapM_ (writeTQueue newActions) (filter (/= x) qq)
modifyTVar inProgress (Set.delete x)
modifyTVar' inProgress (Set.delete x)

-- | Mark an action as complete when called after 'popQueue'.
-- Has no effect otherwise
doneQueue :: DelayedActionInternal -> ActionQueue -> STM ()
doneQueue x ActionQueue {..} = do
modifyTVar inProgress (Set.delete x)
modifyTVar' inProgress (Set.delete x)

countQueue :: ActionQueue -> STM Natural
countQueue ActionQueue{..} = do
Expand Down
3 changes: 2 additions & 1 deletion plugins/hls-class-plugin/src/Ide/Plugin/Class/Types.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
{-# LANGUAGE LambdaCase #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE ViewPatterns #-}
{-# LANGUAGE BangPatterns #-}

module Ide.Plugin.Class.Types where

Expand Down Expand Up @@ -45,7 +46,7 @@ data GetInstanceBindTypeSigs = GetInstanceBindTypeSigs

data InstanceBindTypeSig = InstanceBindTypeSig
{ bindName :: Name
, bindRendered :: T.Text
, bindRendered :: !T.Text
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this needs to be strict while the other values arent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It showed up in a profile, the others are likely to already be evaluated I think.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then a comment would be nice.
Also, would a guideline be wise that generally says everything in records needs a bang, and only in certain exceptional cases you don't need them?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it seems clearer to bang everything in situations like this? Unsure.

, bindDefSpan :: Maybe SrcSpan
-- ^SrcSpan for the bind definition
}
Expand Down