diff options
author | Simon Peyton Jones <simonpj@microsoft.com> | 2021-04-14 16:45:17 +0100 |
---|---|---|
committer | Marge Bot <ben+marge-bot@smart-cactus.org> | 2021-04-15 12:28:18 -0400 |
commit | 0a8c14bd5a5438b1d042ad279b8ffff1bc867e7e (patch) | |
tree | e4b2afa56c4010704a0da1d4041081c4bfc42881 | |
parent | da92e7288fe9c0e83768b7dd0898bca30b9ff2ce (diff) | |
download | haskell-0a8c14bd5a5438b1d042ad279b8ffff1bc867e7e.tar.gz |
Fix handling ze_meta_tv_env in GHC.Tc.Utils.Zonk
As #19668 showed, there was an /asymptotic/ slow-down in zonking in
GHC 9.0, exposed in test T9198. The bug was actually present in earlier
compilers, but by a fluke didn't actually show up in any of our tests;
but adding Quick Look exposed it.
The bug was that in zonkTyVarOcc we
1. read the meta-tyvar-env variable
2. looked up the variable in the env
3. found a 'miss'
4. looked in the variable, found `Indirect ty`
5. zonked `ty`
6. update the env *gotten from step 1* to map the variable
to its zonked type.
The bug is that we thereby threw away all teh work done in step 4.
In T9198 that made an enormous, indeed asymptotic difference.
The fix is easy: use updTcRef.
I commented in `Note [Sharing when zonking to Type]`
-------------------------
Metric Decrease:
T9198
-------------------------
-rw-r--r-- | compiler/GHC/Tc/Utils/Zonk.hs | 73 |
1 files changed, 42 insertions, 31 deletions
diff --git a/compiler/GHC/Tc/Utils/Zonk.hs b/compiler/GHC/Tc/Utils/Zonk.hs index 0e34d97c46..96118af3b3 100644 --- a/compiler/GHC/Tc/Utils/Zonk.hs +++ b/compiler/GHC/Tc/Utils/Zonk.hs @@ -1825,29 +1825,41 @@ Problem: the same as zonkTcTypeToType. (If we distinguished TcType from Type, this issue would have been a type error!) -Solution: (see #15552 for other variants) - - One possible solution is simply not to do the short-circuiting. - That has less sharing, but maybe sharing is rare. And indeed, - that turns out to be viable from a perf point of view - - But the code implements something a bit better - - * ZonkEnv contains ze_meta_tv_env, which maps - from a MetaTyVar (unification variable) - to a Type (not a TcType) - - * In zonkTyVarOcc, we check this map to see if we have zonked - this variable before. If so, use the previous answer; if not - zonk it, and extend the map. - - * The map is of course stateful, held in a TcRef. (That is unlike - the treatment of lexically-scoped variables in ze_tv_env and - ze_id_env.) - - Is the extra work worth it? Some non-systematic perf measurements - suggest that compiler allocation is reduced overall (by 0.5% or so) - but compile time really doesn't change. +Solutions: (see #15552 for other variants) + +One possible solution is simply not to do the short-circuiting. +That has less sharing, but maybe sharing is rare. And indeed, +that usually turns out to be viable from a perf point of view + +But zonkTyVarOcc implements something a bit better + +* ZonkEnv contains ze_meta_tv_env, which maps + from a MetaTyVar (unification variable) + to a Type (not a TcType) + +* In zonkTyVarOcc, we check this map to see if we have zonked + this variable before. If so, use the previous answer; if not + zonk it, and extend the map. + +* The map is of course stateful, held in a TcRef. (That is unlike + the treatment of lexically-scoped variables in ze_tv_env and + ze_id_env.) + +* In zonkTyVarOcc we read the TcRef to look up the unification + variable: + - if we get a hit we use the zonked result; + - if not, in zonk_meta we see if the variable is `Indirect ty`, + zonk that, and update the map (in finish_meta) + But Nota Bene that the "update map" step must re-read the TcRef + (or, more precisely, use updTcRef) because the zonking of the + `Indirect ty` may have added lots of stuff to the map. See + #19668 for an example where this made an asymptotic difference! + +Is it worth the extra work of carrying ze_meta_tv_env? Some +non-systematic perf measurements suggest that compiler allocation is +reduced overall (by 0.5% or so) but compile time really doesn't +change. But in some cases it makes a HUGE difference: see test +T9198 and #19668. So yes, it seems worth it. -} zonkTyVarOcc :: ZonkEnv -> TyVar -> TcM TcType @@ -1864,7 +1876,7 @@ zonkTyVarOcc env@(ZonkEnv { ze_flexi = flexi ; case lookupVarEnv mtv_env tv of Just ty -> return ty Nothing -> do { mtv_details <- readTcRef ref - ; zonk_meta mtv_env ref mtv_details } } + ; zonk_meta ref mtv_details } } | otherwise = lookup_in_tv_env @@ -1874,19 +1886,18 @@ zonkTyVarOcc env@(ZonkEnv { ze_flexi = flexi Nothing -> mkTyVarTy <$> updateTyVarKindM (zonkTcTypeToTypeX env) tv Just tv' -> return (mkTyVarTy tv') - zonk_meta mtv_env ref Flexi + zonk_meta ref Flexi = do { kind <- zonkTcTypeToTypeX env (tyVarKind tv) ; ty <- commitFlexi flexi tv kind ; writeMetaTyVarRef tv ref ty -- Belt and braces - ; finish_meta mtv_env ty } + ; finish_meta ty } - zonk_meta mtv_env _ (Indirect ty) + zonk_meta _ (Indirect ty) = do { zty <- zonkTcTypeToTypeX env ty - ; finish_meta mtv_env zty } + ; finish_meta zty } - finish_meta mtv_env ty - = do { let mtv_env' = extendVarEnv mtv_env tv ty - ; writeTcRef mtv_env_ref mtv_env' + finish_meta ty + = do { updTcRef mtv_env_ref (\env -> extendVarEnv env tv ty) ; return ty } lookupTyVarOcc :: ZonkEnv -> TcTyVar -> Maybe TyVar |