Fix performance issue in 'allDependencies' #668
Conversation
|
Tested on this package run in the API: |
|
The major difference is that Spago's implementation is all folds, but I was using |
| ST.while (map (_ > 0) (Array.ST.length pending)) do | ||
| Array.ST.shift pending >>= case _ of | ||
| Nothing -> pure unit | ||
| Just (ModuleName mod) -> Object.ST.peek mod visited >>= case _ of | ||
| Nothing -> do | ||
| void $ Object.ST.poke mod unit visited | ||
| case Map.lookup (ModuleName mod) graph of | ||
| Nothing -> pure unit | ||
| Just { depends } -> void $ Array.ST.pushAll depends pending | ||
| Just _ -> pure unit |
There was a problem hiding this comment.
This is equivalent to writing a recursive function with an explicit List stack and accumulator, FWIW.
There was a problem hiding this comment.
There was a problem hiding this comment.
Happy to write it that way if you have an example somewhere. I'm curious what the generated code will look like vs. the ST version.
There was a problem hiding this comment.
I don't have an opinion. One is obviously using mutation, so I would expect this to be significantly faster. I just don't use ST in core backend-optimizer because it's not very portable.
There was a problem hiding this comment.
In this case since we don't need portability in the registry and speed does matter (this is part of the pipeline) I would prefer to stay with ST. The generated code looks quite good.
|
@f-f could you take another look at this? it's still blocking publishing. |
|
Ah yes sorry I had the tab open and got distracted with other things - my preference is that we keep the lib portable if we can. We explicitely depend on some node libs though, so I guess this is alright as well? |
|
Yeah, we already explicitly depend on some NPM libraries (ssh2), and Node builtins (the crypto module), so if we were to make the library portable we'd need to move that stuff out altogether and at that time could also replace the ST code. But we'd only need to do any of that if someone came along wanting to use the registry-lib with an alternate backend. |
|
I assume we'll want to use the Scheme backend with Spago (which depends on the registry-lib) once that takes off, but we'll sorry about it then |
In #667 I added an implementation to prune unused dependencies from legacy packages. Unfortunately, while the implementation was tested in the small, it was never tested on a package with a sizable graph. On larger packages the inefficiencies in the implementation would cause the process to run out of memory.
This implementation fixes the issue. We switch to ST so we can keep a mutable map around to track modules we've already visited and push all unvisited dependencies to an array for later processing. Once we go through the full pending array without refilling it we're done and can exit.