Drop elabGPkgDescription from the elaborated plan - #12464

This MR attempts to regain sharing after-the-fact during serialisation. I think that's papering over the issue in an incomplete and non-robust way.

As far as I understand, the regression was due to the addition of elabGPkgDescription to ElaboratedConfiguredPackage. So the fix should be to remove that field and find another way to thread that information through which avoids the duplication.

A similar win in this area is to stop elabPkgDescription being duplicated in every single unit of a package, which could be similarly achieved by removing it from the unit-specific data and shared, but that change is not strictly needed to address the regression.

done: dropped elabGPkgDescription, the in-library configure reads the .cabal file like Setup does. n=80 repro: improved-plan 27.3 → 14.8 MB. happy to do the same for elabPkgDescription in a follow-up.

Yes, much better approach, thanks. We will need to think through what it means for error messages for users, e.g. ensure that it doesn't cause duplicate error messages.

Thank you for working on this: the regression does look nasty.

Re-parsing cabal file feels like a stopgap solution though, and since we’re not releasing cabal any time soon, we still have time to think about a proper solution.

My advice would be to factor out PD-related structures into a new data type (say, ElaboratedPackageSource) and add a map PackageId -> (this new type) to ElaboratedSharedConfig. At first, the new type could only hold GPD, and that alone would allow to solve the regression. Then we can move more stuff there (PD and friends) to kill more duplication. ElaboratedSharedConfig seems to travel to all the right places for the idea to be viable.

Thank you for working on this: the regression does look nasty.

Re-parsing cabal file feels like a stopgap solution though, and since we’re not releasing cabal any time soon, we still have time to think about a proper solution.

My advice would be to factor out PD-related structures into a new data type (say, ElaboratedPackageSource) and add a map PackageId -> (this new type) to ElaboratedSharedConfig. At first, the new type could only hold GPD, that would allow to solve the regression. Then we can move more stuff there (PD and friends) to kill more duplication. ElaboratedSharedConfig seems to travel to all the right places for the plan to be viable.

Yes, I completely agree with this kind of approach.

Thank you for working on this: the regression does look nasty.

Re-parsing cabal file feels like a stopgap solution though, and since we’re not releasing cabal any time soon, we still have time to think about a proper solution.

My advice would be to factor out PD-related structures into a new data type (say, ElaboratedPackageSource) and add a map PackageId -> (this new type) to ElaboratedSharedConfig. At first, the new type could only hold GPD, and that alone would allow to solve the regression. Then we can move more stuff there (PD and friends) to kill more duplication. ElaboratedSharedConfig seems to travel to all the right places for the idea to be viable.

Agreed, this sounds like a better direction. Would it be useful if I tried the GPD-only version and reported back with results from my larger Haskell projects?

@PaleRoses absolutely. Thank you.

@sheaf @ulysses4ever
I pushed a GPD-only version in 9083dfc. On a synthetic project with 80 sublibraries, the plan went from 118 MB to 11 MB and the median no-op time from 2.0s to 0.5s. I haven’t tried it on my larger projects yet.

sounds promising. In the 118 MB → 11 MB case, what's the number for cabal 3.16?

@ulysses4ever
On the same synthetic project with 80 sublibraries, 3.16.1.0 gives an 8.30 MB plan and a 0.263s median no-op time. The PR base gives 118.42 MB / 1.915s, and 9083dfc gives 10.90 MB / 0.430s. So this recovers most of the regression, though there’s still a gap to 3.16. All three were built with GHC 9.12.4 and -O1, with assertions enabled.

Looks right to me: the config cache key stays the normalised elab, the hash is computed once per package (components are record updates of elab0), and the hash is needed since configure consumes the GPD.

One thing before calling #12463 fixed: the map holds a GPD for every Configured solver package, i.e. every Hackage dependency, including the ones already in the store. 3.16 had one of those. the 80-sublib benchmark is a single package, so it doesn't exercise this.

haskell-language-server would be a good real-world check: 31 plugin sublibraries in one package plus a large hackage closure. You won't even need to build it, running cabal build all --dry-run twice is enough. Then compare the size of dist-newstyle/cache/improved-plan and the time of the second run across 3.16.1.0, the PR base and this PR.

The first commit is the superseded re-parse approach, so this wants squash+merge (IMHO).

@ulysses4ever thanks. Both inline points are done in 00d86fe, and the description now describes the shared map and the hash, with the final numbers and 3.16 for reference.

@andreabedini I ran the HLS comparison; the table is in the description. Against the base, improved-plan goes 59.92 → 32.28 MB and the second dry-run 1.273 → 0.959 s, with 3.16 at 22.56 MB / 0.582 s. On the store question: every source in the map was still needed in these plans, so the retention cost is unmeasured here. Squash+merge is fine by me.

@sheaf the duplicate-messages question was about re-reading the .cabal file, which is gone. The shared map hands configure the same description the field did, so nothing is reported more often than before. Could you take another look?

Yes, this looks great. To me the new approach is nice, principled and elegant. Something we can build on for future improvements. Thank you for persevering!

Was this by any chance prepared with some help from AI? We need to put a label for that in that case. Otherwise it’s good to go. Link the follow-ups from here if any

Yes, AI helped prepare this. The follow-up is #12500.

✅ Merged into master as 3c15ba7 at 02:10 UTC

2 h 9 min in the queue, 1 h 58 min of it CI.

So far

• ✅ Entered the queue Wed 11 Nov, 00:01 · rule squash-merge · triggered by rule Put pull requests in the squash+merge queue
• ✅ Passed · draft merge queue: merged #12464: Drop elabGPkgDescription from the elaborated plan #12569
• ✅ Merged 02:10 · 3c15ba7 · squash

_{Times in UTC · dashboard · how the merge queue works}