remove unnecessary FileExists dependency in GetHiFile #589
Conversation
pepeiborra
changed the title
There was a problem hiding this comment.
Awesome!
Minor remark that if there is no .hi file version we still check the modification time of the .hs file, even though we don't need to. I assume something else also checks that .hs file anyway, so it will be in the cache and not cost any meaningful time. Rearranging the code to avoid the second dependency if it's not needed is also a little fiddly. Up to you whether it's worth it.
|
I think it's not a big deal, since the |
|
Benchmark BEFORE: Benchmark AFTER: |
pepeiborra
force-pushed
the
hi-files-simple
branch
from
743792e to
8cc7501
Compare
src/Development/IDE/Core/Rules.hs
Outdated
| | otherwise = False | ||
| let isUpToDate = case (modificationTime <$> mbHieTimestamp, modificationTime srcTimestamp) of | ||
| -- If the src file is in VFS, then we know it must have been typechecked | ||
| -- by now (due to kick) and therefore its .hie file is up to date |
There was a problem hiding this comment.
Is this true, What happens if you just opened the file? I don’t see what guarantees that the .hie file has been generated before we get here.
There was a problem hiding this comment.
Hmm, you are right, we don't have a dependency on the Typecheck rule here. Looking at the rule for GetModIface, there are a couple of different scenarios depending on whether the module is a file of interest or not, etc. I'm not keen on duplicating that logic, but I think we can declare a dependency on GetModIface as a tight overapproximation of what is needed here.
It is subsumed by the GetModificationTime dependency. One less dependency per .hi file, one less redundant file system access, five fewer lines of code.
pepeiborra
force-pushed
the
hi-files-simple
branch
from
8cc7501 to
b5310fc
Compare
…#589) * remove unnecessary FileExists dependency It is subsumed by the GetModificationTime dependency. One less dependency per .hi file, one less redundant file system access, five fewer lines of code. * Clarify modification time comparisons for .hi and .hie filesAddresses haskell/ghcide#591 * Fix staleness checking for .hie files (thanks @cocreature)
…#589) * remove unnecessary FileExists dependency It is subsumed by the GetModificationTime dependency. One less dependency per .hi file, one less redundant file system access, five fewer lines of code. * Clarify modification time comparisons for .hi and .hie filesAddresses haskell/ghcide#591 * Fix staleness checking for .hie files (thanks @cocreature)
…#589) * remove unnecessary FileExists dependency It is subsumed by the GetModificationTime dependency. One less dependency per .hi file, one less redundant file system access, five fewer lines of code. * Clarify modification time comparisons for .hi and .hie filesAddresses haskell/ghcide#591 * Fix staleness checking for .hie files (thanks @cocreature)
It is subsumed by the GetModificationTime dependency.
One less dependency per .hi file,
one less redundant file system access,
five fewer lines of code.
This does not implement all the improvements discussed in #583, it's just a minor efficiency fix.
I have not measured the performance impact either, since there is no easy-to-run standard benchmark, but I expect the impact to be very small.