Divam Narula (b627d869) at 22 Sep 22:06
HACK: disable pthread_setname_np, as iOS build is broken (cant fine...
... and 1291 more commits
Divam Narula (b29b6662) at 31 Aug 06:22
Merge commit '1f02b743' into dn-ghc...
... and 1291 more commits
@trac-sheaf Hi, I am not actively working on ghc at the moment, and neither have a clear idea about how this MR could be fixed (ie how will IC_solved idea will be used) So please move ahead and get the issue resolved without worry about this MR.. cheers :)
yes, this abstraction works fine for this scenario, because the actions produce HPT, and the contents of HPT more or less remain the same as the current global HscEnv approach. (there is a difference when retypecheckLoop happens)
Nevertheless it is limitation of the abstraction itself and would be hard to workaround without some sort of a "hack" (like using raw MVars). On the other hand I could argue that this itself is no worse than the current use of raw MVars, so at least this patch achieves a reduction of their use.
(I dont think the solution I have in mind is like FRP, but there could be some more complicated/abstract version of this which is like FRP.)
In my case the action has "two outputs" (A,B) where A is good to cache, but B should be used once and freed up
What I was thinking was to get rid of ActionMap
and the cachedInterpret logic.
Since the data dependencies of the actions are known in advance (when making NodeBuildInfo),
So the result of an action could be written to [MVar a]
ActionResult a { actionResult :: [MVar (Maybe a)]
where the list is known when creating the ActionResult
To get this list, in addition to the "build_deps", the "build_rev_deps" is created when building the NodeBuildInfo
, build_rev_deps :: [MVar (Maybe (A,B))]
, build_deps :: WrappedMakePipeline [HomeModInfo]
But this in itself is not sufficient as the readMVar
in the process_deps
might happen quite late in the compilation and it would end up keep holding the value B. (Edit: readMVar
is not good, we need to do takeMVar
)
We need to do (a, _) <- takeMVar
as soon as possible, so it would require a "Concurrent" process_deps
/ takeMVar
.
All this does sound like a lot of work to achieve something which could simply done by using (A, MVar B)
A fix for #19627
Dont print the entire WantedConstraints This was arguably of no (direct) help to the user The entire WantedConstraints can be obtained from -ddump-tc-trace
Now only the first few (4) "unsolved" constraints are printed
Reword the message to be a bit more explanatory than before
Limit the error message to one
Here is how it looks now
src/Linear/Logic/Internal.hs:1:1: error:
Solver exceeded the max iteration limit (= 4) for the following constraints (non-exhaustive)
Unsolved: [[WD] $dIProp'_a4Wz {1}:: IProp'
(INot f) (CDictCan(psc)),
[WD] $dIProp'_a4WA {1}:: IProp' (INot g) (CDictCan(psc))]
Suggested fix:
Set limit with -fconstraint-solver-iterations=n; n=0 for no limit
|
1 | {-# language BlockArguments #-}
| ^
src/Linear/Logic/Internal.hs:419:10: error:
• Could not deduce (IProp' (INot g))
arising from the superclasses of an instance declaration
from the context: IProp' g
bound by the instance declaration
at src/Linear/Logic/Internal.hs:419:10-37
• In the instance declaration for ‘Prop (DWith f g)’
|
419 | instance IProp' g => Prop (DWith f g) where
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Right, I think filtering the error would be a better solution, still it is not clear what the contents of the message should be
I personally dont hit this kind of error in my work, and have no idea how important the output of WantedConstraints
would be, or if it could be improved.
Given that it is not clear what the contents of the message should be, and making the user rely on -ddump-tc-trace is the wrong decision, I would consider this a wrong solution and close the MR.
A fix for #19627
Dont print the entire WantedConstraints This was arguably of no (direct) help to the user The entire WantedConstraints can be obtained from -ddump-tc-trace
Now only the first few (4) "unsolved" constraints are printed
Reword the message to be a bit more explanatory than before
Limit the error message to one
Here is how it looks now
src/Linear/Logic/Internal.hs:1:1: error:
Solver exceeded the max iteration limit (= 4) for the following constraints (non-exhaustive)
Unsolved: [[WD] $dIProp'_a4Wz {1}:: IProp'
(INot f) (CDictCan(psc)),
[WD] $dIProp'_a4WA {1}:: IProp' (INot g) (CDictCan(psc))]
Suggested fix:
Set limit with -fconstraint-solver-iterations=n; n=0 for no limit
|
1 | {-# language BlockArguments #-}
| ^
src/Linear/Logic/Internal.hs:419:10: error:
• Could not deduce (IProp' (INot g))
arising from the superclasses of an instance declaration
from the context: IProp' g
bound by the instance declaration
at src/Linear/Logic/Internal.hs:419:10-37
• In the instance declaration for ‘Prop (DWith f g)’
|
419 | instance IProp' g => Prop (DWith f g) where
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Divam Narula (b767a838) at 31 Jul 09:33
Improvements to the Solver's "too many iterations" error (#19627)
... and 3980 more commits
I thought of (and tried) a number of solutions and found the 1. to be the best approach.
Reduce size of error message and show only one message
As I mentioned in the above comment, just reducing the size does not result in a good user experience, as there could be multiple of these messages. When only a single message is shown I think this achieves a very good result with not too much code changes.
Integrate this scenario properly with the solver's error reporting
There is a good amount of logic around reporting of the solver's errors, including sequencing of messages (out-of-scope first), suppressing errors, etc. Perhaps the iterations exceed message is not the most useful for the user to see first. And having knowledge of hitting the limit of iteration may be used avoid some work.
But to enable this kind of intelligence in the code would require non-trivial amount of modifications.
Suppress the error via a flag
Make a (default) fatal warning
This might have been the best option in this case, but the warnings are completely dropped if there are typecheck errors. There is a good reason why this is the case, at there could be other fatal warnings which would end up as errors and would become annoying for the user.
I stared at this error message for a little while today, unable to parse out what was wrong.
persistent > /home/matt/Projects/persistent/persistent/Database/Persist/ImplicitIdDef.hs:1:8: error:
persistent > File name does not match module name:
persistent > Saw: ‘Databse.Persist.ImplicitIdDef’
persistent > Expected: ‘Database.Persist.ImplicitIdDef’
persistent > |
persistent > 1 | module Databse.Persist.ImplicitIdDef
persistent > | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
persistent >
I typoed Databse
, and it took me an embarrassingly long time to find that out.
Add some spaces after Saw:
so it lines up with Expected:
.
persistent > /home/matt/Projects/persistent/persistent/Database/Persist/ImplicitIdDef.hs:1:8: error:
persistent > File name does not match module name:
persistent > Saw: ‘Databse.Persist.ImplicitIdDef’
persistent > Expected: ‘Database.Persist.ImplicitIdDef’
persistent > |
persistent > 1 | module Databse.Persist.ImplicitIdDef
persistent > | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
persistent >
This makes it much more obvious where the problem is.
This has been fixed by !5648 (closed)
I gave a shot at improving this, and noticed that because there are multiple of these messages, even after reducing the size of the messages it ends up occupying too much screen real estate. I guess there should be ideally only one message about "too many iterations", to indicate to the user that the solver gave up, and for more details use a flag like -ddump-tc-trace
. (or some special flag, if just looking at the WantedConstraints
is helpful).
src/Linear/Logic/Internal.hs:1:1: error:
solveWanteds: too many iterations (limit = 4)
Summary of unsolved constraints: [[WD] $dIProp'_abAP {1}:: IProp'
fsk0 (CDictCan(psc)),
[WD] $dIProp'_abAQ {1}:: IProp' fsk0 (CDictCan(psc))]
See all the details with -ddump-tc-trace
Set limit with -fconstraint-solver-iterations=n; n=0 for no limit
|
1 | {-# language BlockArguments #-}
| ^
src/Linear/Logic/Internal.hs:1:1: error:
solveWanteds: too many iterations (limit = 4)
Summary of unsolved constraints: [[WD] $dIProp'_abBT {1}:: IProp'
fsk0 (CDictCan(psc)),
[WD] $dIProp'_abBU {1}:: IProp' fsk0 (CDictCan(psc))]
See all the details with -ddump-tc-trace
Set limit with -fconstraint-solver-iterations=n; n=0 for no limit
|
1 | {-# language BlockArguments #-}
| ^
src/Linear/Logic/Internal.hs:1:1: error:
solveWanteds: too many iterations (limit = 4)
Summary of unsolved constraints: [[WD] $dIProp'_abEs {1}:: IProp'
fsk0 (CDictCan(psc)),
[WD] $dIProp'_abEt {1}:: IProp' fsk0 (CDictCan(psc))]
See all the details with -ddump-tc-trace
Set limit with -fconstraint-solver-iterations=n; n=0 for no limit
|
1 | {-# language BlockArguments #-}
| ^
src/Linear/Logic/Internal.hs:1:1: error:
solveWanteds: too many iterations (limit = 4)
Summary of unsolved constraints: [[WD] $dIProp'_abFz {1}:: IProp'
fsk0 (CDictCan(psc)),
[WD] $dIProp'_abFA {1}:: IProp' fsk0 (CDictCan(psc))]
See all the details with -ddump-tc-trace
Set limit with -fconstraint-solver-iterations=n; n=0 for no limit
|
1 | {-# language BlockArguments #-}
| ^
Of course this is just a workaround, and the problem is in the abstraction itself. Ideally there should not be any caching of this sort, and the results of an action should be "given" to the upstream actions (via writing to an MVar for each of them. )
So the action should have either [MVar]
or IORef [MVar]
, and it should "notify" the dependent actions of the result by writing to each of this. (this is sort of getting into the FRP territory)
@mpickering The issue about caching is related to the result of Tc action containing additional data (which includes TcGlbEnv
).
Make_CompileModuleTillTc :: ModSummary -> MakeAction (HomeModInfo, Maybe TcData)
In our case only one action needs to read this, the upstream modules actions only need the HomeModInfo
. So I thought putting the TcData in MVar would solve the GC issue.
Make_CompileModuleTillTc :: ModSummary -> MakeAction (HomeModInfo, MVar TcData)
In addition there will be changes required like build_self
will contain possible two actions, lets say
type CompTc = WrappedMakePipeline (Maybe (HomeModInfo, TcData))
type CompBak = WrappedMakePipeline (Maybe HomeModInfo)
And each NodeKey
will have (Maybe CompTc, CompBak)
The build_deps
might need to be [Either CompTc CompBak]
Make_CompTc
mod would wait for the build_deps
, and the Make_CompBak
would wait on the Make_CompTc
The buildSingleModule
would do the necessary checks (like TH, plugin use, etc) to determine whether the deps need to compile till Tc or codegen.
Here is a problematic scenario with this approach..
The following cyclic_mod_graph will end up considering all modules in a single CyclicSCC
graph TB;
subgraph one
L1.hs --> L1_1.hs;
L1_1.hs --> L1.hs;
end
subgraph three
M.hs
end
subgraph two
L2.hs --> L2_1.hs;
L2_1.hs --> L2.hs;
end
L1.hs --> M.hs;
L2_1.hs --> L1.hs;
M.hs --> L2.hs;
These could be (and currently is) compile like below. Note that retypecheckloop will happen after the L1 and M would see the unfoldings of L1. But if you do a single retypecheckloop with all the modules considered together, then M would miss the unfoldings.
graph TB;
subgraph one
L1.hs-boot --> L1_1.hs;
L1_1.hs --> L1.hs;
end
subgraph three
M.hs
end
subgraph two
L2.hs-boot --> L2_1.hs;
L2_1.hs --> L2.hs;
end
L1.hs --> M.hs;
L2_1.hs --> L1.hs;
M.hs --> L2.hs;
@mpickering I did a minor fix to this, now not using MVars in the single threaded upsweep. This brings down the metrics to just marginally higher than ref.
Divam Narula (feffe7e2) at 15 Jul 09:28
Driver: Fix some parUpsweep bugs, remove the old single threaded up...
Divam Narula (6bd8f609) at 15 Jul 09:21
Implement a simpler single thread upsweep which mostly removes the ...
... and 3855 more commits