load modules concurrently #4545 - #4549
Conversation
Sorry, something went wrong.
|
@f-f anything blocking merge here? this change looks lovely |
Sorry, something went wrong.
|
We'll need another review from the core team - could anyone take a look? |
Sorry, something went wrong.
rhendric
left a comment
There was a problem hiding this comment.
Nits aside, I'm surprised that this is reported to decrease memory use because my understanding is that mapConcurrently will spawn every one of these tasks at once. Has that been confirmed on very large projects? What about consumption of other resources—open file handles or something?
I'd be more comfortable with this if we used primitives like pooledMapConcurrently instead. OTOH, I don't do much maintenance of the Ide modules so if the team members who do are happy with this I won't block it.
Sorry, something went wrong.
| import Language.PureScript.Ide.Usage (findUsages) | ||
| import System.Directory (getCurrentDirectory, getDirectoryContents, doesDirectoryExist, doesFileExist) | ||
| import System.FilePath ((</>), normalise) | ||
| import Control.Concurrent.Async.Lifted (mapConcurrently, mapConcurrently_) |
There was a problem hiding this comment.
Nit: insert into the imports in alphabetical order, please.
Sorry, something went wrong.
| import Control.Concurrent.Async.Lifted (mapConcurrently) | ||
| import Control.Monad.Trans.Control (MonadBaseControl) |
There was a problem hiding this comment.
Same nit.
Sorry, something went wrong.
| import Language.PureScript qualified as P | ||
| import Language.PureScript.Errors.JSON qualified as P | ||
| import Language.PureScript.Ide.Filter.Declaration (DeclarationType(..)) | ||
| import Control.Monad.Trans.Control (MonadBaseControl) |
There was a problem hiding this comment.
Same nit.
Sorry, something went wrong.
|
Also, I think this does merit its own changelog entry. |
Sorry, something went wrong.
Description of the change
On large projects loading the modules concurrently lowers memory usage substantially. Should help address #4545
Checklist:
Relevant checklist points addressed in #4546