Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork412
Optimise module to filename#4600
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ab06e51 toa11ff5bComparea11ff5b to2d8386cCompareguibou commentedMay 28, 2025
I'm running this code in production on our large codebase for the 3rd day. a) My users are super happy, a few quotes:
(it was more than 5 minutes before ;)
Ok, that's all for the shameless plug ;) I think I observed a few reload / performance issues which are not related and would definitely do some followup issues. I'm investigating (this is difficult to describe right now. Looks like HLS is not able to give diagnostic back if it is typechecking a file which depends on the file I just modified and if this typechecking takes time). I also observed a surprising behavior, I need to investigate if that's linked to my MR or to something else:
It looks like a multi-home issue, see.#3237, which would make sense considering that I had done nothing to fix multiple home. But this is still rather surprising because HLS was supposed to restart and hence should have completely ignored the old selda version. Maybe it picked some information from the cache. |
soulomoon commentedMay 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We handle FOI(File of interest) and none-FOI differently. |
guibou commentedMay 29, 2025
@soulomoon Thank you. This is indeed interesting. I will confirm that the bug I'm observing is not linked to my change and open an issue. This is not a dramatic issue (e.g. it is not everyday that you restart the editor with breaking changes in the packages from the package database), but it is not convenient and can be confusing. |
guibou commentedMay 31, 2025
I've found a bug, when looking for module in the import path, an exception is raised if the import path is empty. |
2d8386c tofdf3085Compareguibou commentedSep 21, 2025
Fixed in latest commit. |
fdf3085 toc856c6aCompareguibou commentedOct 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Improved the support for currently edit files but not yet saved to the disk. The implementation will be improved for performance but that's a good first draft. This fixs a lot of unit test, so it is good. I do have a few (around 20) tests faliling in ghcide-tests, but otherwise, most of the tests are ok. Still todo:
|
guibou commentedNov 1, 2025
Multiple-home-unit seems to be fixed now (maybe I've not pushed the MR), but the tests are fine. I have 3 failing tests related to I'll work on removing the |
c856c6a tob20c634Compareguibou commentedNov 1, 2025
Latest push should fix hlint and actually remove the I have 3 (2 are flaky) remaining issues with |
98fc906 to2270264Compareguibou commentedNov 2, 2025
I will need a bit of help here. I'm blocked with the following errors locally: Note that out of the 3 tests related to In the context of this test, My understanding here is limited and especially, it is difficult for me to understand why my changes may have any impact on these tests. In such context, it is tempting to think that there is another problem such as a race condition / timing. During my investigation, I found the code in pPrint ("sessionOpts", cfp, v)caseHM.lookup (toNormalizedFilePath' cfp) vofJust (opts, old_di)->do deps_ok<- checkDependencyInfo old_di pPrint ("deps_ok", deps_ok, old_di)ifnot deps_okthendo-- If the dependencies are out of date then clear both caches and start-- again. modifyVar_ fileToFlags (const (returnMap.empty)) modifyVar_ filesMap (const (returnHM.empty))-- Keep the same name cache modifyVar_ hscEnvs (return.Map.adjust (const[]) hieYaml ) pPrint ("consultCradle not deps_ok", cfp) consultCradle hieYaml cfpelsereturn (opts,Map.keys old_di)Nothing->do pPrint ("consultCradle Nothing", cfp) consultCradle hieYaml cfp (the I observed that the However, It seems that The result of this is that when calling And For each targetfile, we will create a I'll continue this investigation later. |
2270264 tocce8f01Comparecce8f01 tof817d89Comparef817d89 to878bfdbCompareguibou commentedNov 26, 2025
I've rebased and rerun the CI. Latest run was showing only a segfault with GHC 9.10 on window and one failure in the call hierarchy plugin on window with ghc 9.6, which may hence be a flakyness. The latest commit is a tentative fix for the issue described in#4600 (comment) and that I tried to describe and extract in#4754 (And really, I should answer the poor@soulomoon who tried to help me). In order to be in a mergable state, I need to:
|
| -- and also not find 'TargetModule Foo'. | ||
| fs<- filterM (IO.doesFileExist. fromNormalizedFilePath) targetLocations | ||
| pure$map (\fp-> (TargetFile fp,Set.singleton fp)) (nubOrd (f:fs)) | ||
| pure$do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@fendor, this is the offending change.
WhenextendKnownTargets is called with files which are not yet available on disk, theTargetFile f contains a file (either.hs or.hs-boot) andtargetLocations contains a list of files and it seems that it always contains the.hs and.hs-boot.
However, these file do not exist on the disk, so thefilterM always filter them out.
This "fix" allows the file to be added.
The bug is certainly NOT here, see#4754 for more details.
My current understanding is that there is a caching process which, when a.hs or.hs-boot file is registered, it callsextendKnownTargets with both.hs and.hs-boot files listed intargetLocations, but only the.hs or.hs-boot inTargetFile f.
However, it seems that there is some sort of caching which once a.hs-boot file or.hs file is registered, it blocks registration for the other file.
It means that depending on the registration order, we may end up with only one or the other file in theknownTarget.
The result is that during module name to filename association (the target of this MR), we won't be able to find either the.hs or the.hs-boot file, because we look into theknownTarget map for them.
Note that I don't understand why it was not failing before, because the previous implementation was actually looking in theknownTarget too, and on the filesystem. But for files not on the filesystem (as it is the case for tests and newly created files), it seems that the previous implementation was behaving the same.
My guess is that, because it seems that it is a race, my change had an impact on the wayextendKnownTargets is called concurrently and as a result the problem appears more often. But I had not been able to confirm that the issue was there before my MR.
soulomoonNov 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we want to extendKnownTargets, we should do so in therestart thread, see Note [Serializing runs in separate thread] . Otherwise we risk of destablizing the build system.
For the .hs or .hs-boot mess, one of the possible solution, we can update the sessionLoader to invalidate related cache and consult the cradle again if the missing .hs or .hs-boot is added.
soulomoon commentedNov 26, 2025
take a look at |
This is related to#4598.This changes the file to module associating logic done during dependencygraph building.Before, each time a module `Foo.Bar` is found, HLS is testing inside allthe import path for the existence of a relevant fiel.. It means that for`i` import paths and `m` modules to locate, `m * n` filesystemoperations are done. Note also that this involves a lot of complexstring concatenation primitive to build the `FilePath`.A module is tested for each `import` for each of the file of theproject. We also test for `boot` files, doubling the number of test.In#4598 we have a project with `1100` modules, in more than 250 importpaths and we count more than `17000` `import` statments, resulting onover 6 millions test for file existences. This project was blocking formore than 3 minutes during HLS startup.This commit changes the way this is computed:- At startup, a `Map ModuleName FilePath` (the real type is a bit more involved for performance, multiples unit and boot files handling) is built by scanning all the import paths for files representing the different modules.- Directory scanning is efficient and if import path only contains haskell module, this will never do more job that listing the files of the project.- The lookup is now simplify a `Map` lookup.The performance improvement is as follows:- The number of IO operation is dramatically reduced, from multiples millions to a few recursive directories listing.- A lot of the boilerplate of converting path had be removed.- TODO: add an RTS stats before / after with number of allocations- On my project, the graph building time is reduced from a few minutes to 3s.Limitations:- How to rebuild the `Map` if the content of one directory change?- If one directory is filled with millions of files which are not of interested, performance can be damaged. TODO: add a diagnostic during this phase so the user can learn about this issue.Code status:- The `lookup` is not fully restored, especially it does not include the handling of home unit as well as reexport.- The initialisation phase is cached inside a `TVar` stored as a top level identifier using `unsafePerformIO`. This is to be improved.A note about performanceMost users won't see the benefits of these change, but I think theyapply to everbody:- We are still doing 1 lookup per `import` per module. But the lookup result is not multiples IO, so it should be faster by a large amount.- Most project only have 1 (or a few) import paths so won't benefit as dramatically as me from this.TODO for allocations
It does not seem to be used and apparently had never been used (after aquick look in the repository diff).Ask m.pickering about this.
2 tests failing
The cache is now stored inside the `ShakeExtra`. I'm unsure that's theright location for it.This should fix hlint. Also, not having a cache stored as top levelentry may makes it not survive session reinit, which seams sane (forexample, during tests).Still 3 `-boot` related tests are failing. 2 are however flaky.
add all alternate file to knownFilesThis need to be investigated.The different `-boot` tests are flaky / failing in this MR because ofthis.WIP: remove debug statments
878bfdb to1c7b787Compare
(only latests commit are relevant, the other are from other MR which should be merged before this work is merged. Or we can rebase)
This is related to#4598.
This changes the file to module associating logic done during dependency
graph building.
Before, each time a module
Foo.Baris found, HLS is testing inside allthe import path for the existence of a relevant fiel.. It means that for
iimport paths andmmodules to locate,m * nfilesystemoperations are done. Note also that this involves a lot of complex
string concatenation primitive to build the
FilePath.A module is tested for each
importfor each of the file of theproject. We also test for
bootfiles, doubling the number of test.In#4598 we have a project with
1100modules, in more than 250 importpaths and we count more than
17000importstatments, resulting onover 6 millions test for file existences. This project was blocking for
more than 3 minutes during HLS startup.
This commit changes the way this is computed:
Map ModuleName FilePath(the real type is a bit moreinvolved for performance, multiples unit and boot files handling) is
built by scanning all the import paths for files representing the
different modules.
haskell module, this will never do more job that listing the files of
the project.
Maplookup.The performance improvement is as follows:
millions to a few recursive directories listing.
to 3s.
Limitations:
Mapif the content of one directory change?interested, performance can be damaged. TODO: add a diagnostic during
this phase so the user can learn about this issue.
Code status:
lookupis not fully restored, especially it does not include thehandling of home unit as well as reexport.
TVarstored as a toplevel identifier using
unsafePerformIO. This is to be improved.