Improve shellFor selectComponents performance

[TeofilC]

Improves performance of #1145 by only traversing local packages in search of components to add to
selectComponents.

Let me know if you have any suggestions

[hamishmack]

Not sure if I understand correctly, but is there a use case where we might want cabal to build a package from hackage inside the shell (and therefor include it in the selected packages)? For instance trying out project with different versions of a dependency that is in hackage. With this change would you still be able to do that (or would it leave out the dependencies of that package)?

Do you have an example where this increases performance?

[TeofilC]

That's a good point. I didn't include something like this this in the original PR, because I was worried about use cases like that too.
Just to clarify: this PR would just mean that if I say added yesod and conduit to my packages, shellFor wouldn't automatically add yesod-core. You can still put non-local packages in packages, you just won't have intermediate dependencies automatically added.

Testing this against my work proprietary codebase, I'm getting a decrease from 15s to 10s for starting up a shellFor without any packages specified. Oddly I still seem to get that performance boost, if I remove the part that filters out non-local packages. So, maybe the regression has something to do with the implementation of flatLibDepends.

This was originally prompted by this comment #1145 (comment) . I'd be interested to hear if this PR fixes that issue. And if it does whether removing the part that filters out non-local packages causes the issue to return.

If the local packages filtering is crucial to solving this performance regression, I could implement something that filters by local packages when only local packages are selected and don't filter otherwise.

[michaelpj]

I can confirm this makes it much faster. Sorry, I don't have the cycles right now to think about whether it's a good idea 😅

[TeofilC]

No worries!
After looking into this a bit further it seems to be that this performance regression was caused by the getKey function in flatLibDepends. This forces the outPath of each package traversed, which instantiates the derivation. A side-effect of the changes in this PR is that this extra instantiation is avoided.

My thinking now is that we should change the key to something like the name attr. This would avoid instantiating the derivation, and we'd also benefit from this in other uses of this function. This would supersede the current contents of this PR and sidestep the worries about local package filtering.

Something like this maybe:

diff --git a/lib/default.nix b/lib/default.nix
index 359367d..92e9e77 100644
--- a/lib/default.nix
+++ b/lib/default.nix
@@ -132,14 +132,7 @@ in {
   ## flatLibDepends :: Component -> [Package]
   flatLibDepends = component:
     let
-      # this is a minor improvement over the "cannot coerce set to string"
-      # error.  It will now say:
-      #
-      # > The option `packages.Win32.package.identifier.name' is used but not defined.
-      #
-      # which indicates that the package.Win32 is missing and not defined.
-      getKey = x: if x ? "outPath" then "${x}" else (throw x.identifier.name);
-      makePairs = map (p: rec { key=getKey val; val=(p.components.library or p); });
+      makePairs = map (p: rec { key=val.name; val=(p.components.library or p); });
       closure = builtins.genericClosure {
         startSet = makePairs component.depends;
         operator = {val,...}: makePairs val.config.depends;

How does that sound?

[hamishmack]

How does that sound?

That sounds like an awesome idea!

I remember a year or two ago we refactored something (I think it was the code that gathers inputs for the haskell derivations) so that it did not use flatLibDepends and got a big performance gain. This is probably why. The only other place flatLibDepends is used is when building ghcjs.

[michaelpj]

I guess there's a risk that that might not be a unique key? Whereas the out path is going to be unique. Probably the derivation name (which would be a combination of package name and version, I think) ought to do it, though!

[TeofilC]

I've force pushed commits with the idea I mentioned.

I think I'm using the derivation name as the key, so hopefully that will be unique enough

[hamishmack]

bors try

[iohk-bors]

try

Build succeeded:

• ci/hydra:Cardano:haskell-nix:required
• buildkite/haskell-dot-nix