Opened 12 months ago

Last modified 4 weeks ago

#13930 infoneeded bug

Cabal configure regresses in space/time

Reported by: bgamari Owned by: tdammers
Priority: highest Milestone: 8.4.2
Component: Compiler Version: 8.3
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case:
Blocked By: Blocking:
Related Tickets: #13982, #5129 Differential Rev(s):
Wiki Page:

Description

cabal configure apparently has a space leak when compiled with GHC HEAD but not 8.0. It's not clear whether this is really a GHC regression yet, but here it is certainly GHC version-specific.

See https://github.com/haskell/cabal/issues/4589

Change History (20)

comment:1 Changed 12 months ago by bgamari

Thankfully it seems like 8.2 isn't affected by this.

comment:2 Changed 12 months ago by bgamari

Unfortunately I can't easily bisect this due to Cabal issue #4509.

Last edited 12 months ago by bgamari (previous) (diff)

comment:3 Changed 12 months ago by bgamari

Milestone: 8.4.1
Priority: normalhigh
Type of failure: None/UnknownRuntime performance bug

The first bad commit here is,

commit 751996e90a964026a3f86853338f10c82db6b610
Author: Simon Peyton Jones <simonpj@microsoft.com>
Date:   Fri Apr 7 17:10:07 2017 +0100                                                                           
                                                              
    Kill off complications in CoreFVs

    When doing type-in-type, Richard introduce some substantial
    complications in CoreFVs, gathering types and free variables
    of type.  In Trac #13160 we decided that this complication was
    unnecessary, so this patch removes it.

    ...

comment:4 Changed 12 months ago by bgamari

Summary: Cabal configureCabal configure regresses in runtime

comment:5 Changed 12 months ago by bgamari

Summary: Cabal configure regresses in runtimeCabal configure regresses in space/time

comment:6 Changed 11 months ago by RyanGlScott

comment:7 Changed 11 months ago by bgamari

My apologies, Simon: I believe I was mistaken when I said this required profiling to reproduce.

This can be reproduced with current GHC master with,

$ git clone git://github.com/haskell/cabal
$ cd cabal
$ ghc-pkg unregister Cabal
$ cabal install Cabal/ --allow-newer
$ cabal install cabal-install/ --allow-newer --only-dependencies
$ cd cabal-install
$ cabal build
$ dist/build/cabal/cabal

The unregister Cabal bit is necessary for some GHC versions due to Cabal bug #4509.

For the record, a quick profile reveals that most of the allocations seem to be coming from Distribution.ParseUtils and Distribution.Compat.ReadP (both in Cabal).

Last edited 11 months ago by bgamari (previous) (diff)

comment:8 Changed 5 months ago by osa1

I can't reproduce this with ghc 8.5.20180115 and cabal HEAD. configure uses about the same (and very little) amount of memory. please ignore, I think I failed install cabal-install with wire-in Cabal instead of latest Cabal, and now can't delete my comment.

Last edited 5 months ago by osa1 (previous) (diff)

comment:9 Changed 5 months ago by bgamari

Milestone: 8.4.18.4.2

What is the status of this, osa1? Were you able to reproduce this?

Regardless, it's unlikely this will be fixed for 8.4.1..

comment:10 Changed 4 months ago by bgamari

Resolution: invalid
Status: newclosed

It seems that Omer was really unable to reproduce this; odd. Closing.

comment:11 Changed 3 months ago by hvr

Resolution: invalid
Status: closednew

This unfortunately resurfaced in https://github.com/haskell/cabal/issues/5201 and is unfortunately quite easy to reproduce to the point of blocking cabal on GHC 8.4.1

The heap profile in the duplicate ticket #13982 points to readPackageDescription as the main allocation contribution. However, the code involved contains an accumulating loop, where readPackageDescription is supposed to be inside an unsafeInterleavedIO-scope:

    accum srcpkgs btrs prefs (CachePackageId pkgid blockno _ : entries) = do
      -- Given the cache entry, make a package index entry.
      -- The magic here is that we use lazy IO to read the .cabal file
      -- from the index tarball if it turns out that we need it.
      -- Most of the time we only need the package id.
      ~(pkg, pkgtxt) <- unsafeInterleaveIO $ do
        pkgtxt <- getEntryContent blockno
        pkg    <- readPackageDescription pkgid pkgtxt
        return (pkg, pkgtxt)
      case mode of
        ReadPackageIndexLazyIO -> pure () -- THIS BRANCH IS TAKEN
        ReadPackageIndexStrict -> evaluate pkg *> evaluate pkgtxt *> pure ()
      let srcpkg = mkPkg (NormalPackage pkgid pkg pkgtxt blockno)
      accum (Map.insert pkgid srcpkg srcpkgs) btrs prefs entries

commenting out the ReadPackageIndexStrict -> evaluate pkg *> evaluate pkgtxt *> pure () case alternative, fixes the over-allocation; so this seems to imply an optimiser/code generation bug.

Last edited 3 months ago by hvr (previous) (diff)

comment:12 Changed 3 months ago by hvr

Ben asked me to provide the STG dumps of the module containing the accum function; original code as well the variant w/ the ReadPackageIndexStrict branch commented out respectively; I hadn't had time to look at those myself yet, so here they're unabridged:

comment:13 Changed 3 months ago by bgamari

Owner: set to tdammers
Priority: highhighest

comment:14 Changed 3 months ago by hvr

Alex Biehl just pointed out this may be related to the rather old #5129 ticket

comment:15 Changed 3 months ago by osa1

This is indeed #5129. Adding {-# NOINLINE evaluate #-} fixes both this and #5129.

comment:16 Changed 3 months ago by tdammers

Sounds plausible. I managed to reproduce on GHC HEAD, and running on a machine with a HDD clearly shows excessive disk I/O. So my hypothesis so far has been that overly enthusiastic inlining might be the culprit, and if {-# NOINLINE evaluate #-} fixes it, then that would be consistent with that.

comment:17 Changed 3 months ago by tdammers

I suggest parking this for now, let's first see where we end up with #5129.

comment:18 Changed 3 months ago by tdammers

Check if spj's patch on #5129 does indeed fix this one too.

comment:19 Changed 3 months ago by bgamari

Status: newinfoneeded

comment:20 Changed 4 weeks ago by tdammers

Can't seem to be able to reproduce this on GHC HEAD, so let's close this.

Note: See TracTickets for help on using tickets.