Use microlens-platform instead of lens#3400
Conversation
`lens` is pretty big of a dependency with many transitive dependencies. Build times with `lens` are kind of long. We use a small subset of `lens`, almost everything we use also exists in `microlens`. The few things we don't use are available in `microlens-ghc`, `microlens-mtl`, and `microlens-th`. And `microlens-platform` bundles all of that stuff together. `microlens-platform` is a much smaller dependency. We _could_ drop down to `microlens` (which purports a 3.5s build time). but that would take some bigger changes. This change was small and quick, and that seems ideal to see if it's a worthwhile change.
joneshf
left a comment
joneshf
left a comment
There was a problem hiding this comment.
Added some questions about things I ran into.
| } deriving (Show, Eq, Ord, Generic, NFData) | ||
|
|
||
| makePrisms ''IdeDeclaration | ||
| _IdeDeclValue :: Traversal' IdeDeclaration IdeValue |
There was a problem hiding this comment.
Looks like we can write these as traversals rather than prisms without losing anything in our logic. Is that right?
There was a problem hiding this comment.
I don't think I'm using the re part anywhere, so it's fine.
| _IdeDeclKind f (IdeDeclKind x) = map IdeDeclKind (f x) | ||
| _IdeDeclKind _ x = pure x | ||
|
|
||
| anyOf :: Getting Any s a -> (a -> Bool) -> s -> Bool |
There was a problem hiding this comment.
This doesn't really belong here, but was used in four modules so I stuffed it here. If we want these changes, I'll find a better home for it (or remove it maybe?).
There was a problem hiding this comment.
¯\(ツ)/¯ I don't mind it, you could put it into Ide.Utils unless it's used in the Types module.
|
|
||
| properNameT :: Iso' (P.ProperName a) Text | ||
| properNameT = iso P.runProperName P.ProperName | ||
| properNameT :: Getting r (P.ProperName a) Text |
There was a problem hiding this comment.
We ended up not needing the full power of an iso here. Is that right?
There was a problem hiding this comment.
If all we need is a SimpleGetter then we might as well just use to P.runProperName in the usage.
|
cc @kritzcreek since this is almost all IDE code and I figure you've probably got the most context on whether this is an acceptable move or not. |
kritzcreek
left a comment
kritzcreek
left a comment
There was a problem hiding this comment.
I think the primary problem on Travis is that it trashes our caches. This change looks perfectly fine to me though, and if it reduces compile times I'm all for it.
|
Honestly I don't "need" the lens stuff in the IDE part of the codebase. If it's too much of a hassle, or it breaks with the remaining style too much just let me know and I'll rip it out. |
|
Thanks! |
`lens` is pretty big of a dependency with many transitive dependencies. Build times with `lens` are kind of long. We use a small subset of `lens`, almost everything we use also exists in `microlens`. The few things we don't use are available in `microlens-ghc`, `microlens-mtl`, and `microlens-th`. And `microlens-platform` bundles all of that stuff together. `microlens-platform` is a much smaller dependency. We _could_ drop down to `microlens` (which purports a 3.5s build time). but that would take some bigger changes. This change was small and quick, and that seems ideal to see if it's a worthwhile change.
3 participants
#3389 inspired me to look at the build and see what was causing it to take as much time as it does.
One thing is that we've got a dependency on
lens, which is a pretty big dependency. Turns out we don't use too much oflensand we could drop it in favor ofmicrolens-platform–which is a much smaller dependency. Chosemicrolens-platforminstead ofmicrolens(which is even smaller) because we do someixstuff and we're already paying the cost of things likeunordered-containers. We can totally go with the smaller footprint ones if we want to rewrite some code. Anyway, the change was small and quick, so I made a PR.The result of this change on my laptop is about 10% decrease in build time for dependencies:
lensmicrolens-platformIt moves the travis builds closer to being within the time limit. It doesn't make the travis builds 100% reliable since those computers have fewer resources and we still have a good deal of dependencies, but they're quicker than currently so more reliable.
In any case, if we're interested in this change, I can clean up some stuff in the PR. I didn't want to spend too much time on an experiment 😃.