Make hackageIndexLayout faster - #343
Conversation
`hackageIndexLayout` runs `fromPath` in a hot loop, for every file in the Hackage 01-index.tar. However, `fromPath` is rather wasteful when parsing `PackageIdentifier`: it concatenates together package name and version and asks `Cabal-syntax` to parse. This is very backwards, because `PackageIdentifier` is exactly a record with two fields: package name and its version. We already have the name at hand, there is no need to parse it again, and the version can be parsed more efficiently without resorting to `parsec`. This performance optimization was earlier attested in the `hackage-revdeps` package.
`hackageIndexLayout` runs `fromPath` in a hot loop, for every file in the Hackage 01-index.tar. However, `fromPath` is rather wasteful: it uses `splitFragments` (and underlying `System.FilePath.splitDirectories`) to explode `base/4.22.0.0/base.cabal` into `["base", "4.22.0.0", "base.cabal"]`. One could have expected that `splitDirectories` simply splits filepath by '/', but this is not the case. It's a general function, which does a lot of transformations such as checking for absolute paths, splitting drive letter, first retaining '/', then stripping them, etc. The patch replaces `splitDirectories` with a rather specialized operations, aiming to avoid traversing the same characters twice. It was previously attested in the `hackage-revdeps` package that this microptimization brings measurable performance benefits.
|
Tick the box to add this pull request to the merge queue (same as
|
Mikolaj
left a comment
Mikolaj
left a comment
There was a problem hiding this comment.
LGTM
If the speedup is confirmed in practice, this is a great improvement!
|
Do we want to a) release this asap and b) make sure cabal 3.18.1 0 will allow the latest release? |
|
This is not a breaking change, so presumably it can be released as |
| case reverse basename of | ||
| -- ".cabal" reversed | ||
| 'l' : 'a' : 'b' : 'a' : 'c' : '.' : _ -> | ||
| return $ Some $ IndexPkgCabal pkgId | ||
| -- ".json" reversed | ||
| 'n' : 'o' : 's' : 'j' : '.' : _ -> | ||
| return $ Some $ IndexPkgMetadata pkgId | ||
| _ -> Nothing |
There was a problem hiding this comment.
Yes, at least twice faster because you need to check for two extensions. Here is a benchmark:
#!/usr/bin/env cabal
{- cabal:
build-depends: base, deepseq, tasty-bench
default-language: GHC2021
-}
import Control.DeepSeq
import Data.List
import GHC.Generics
import Test.Tasty.Bench
data Extension = Cabal | Json | Other
deriving (Eq, Show, Generic)
instance NFData Extension
classifyExtensionUsingIsSuffix :: FilePath -> Extension
classifyExtensionUsingIsSuffix xs
| ".cabal" `isSuffixOf` xs = Cabal
| ".json" `isSuffixOf` xs = Json
| otherwise = Other
classifyExtensionUsingReverse :: FilePath -> Extension
classifyExtensionUsingReverse xs = case reverse xs of
'l' : 'a' : 'b' : 'a' : 'c' : '.' : _ -> Cabal
'n' : 'o' : 's' : 'j' : '.' : _ -> Json
_ -> Other
filepaths :: [FilePath]
filepaths = ["cabal-install.cabal", "cabal-install.json"]
main :: IO ()
main = do
print $ map classifyExtensionUsingIsSuffix filepaths == map classifyExtensionUsingReverse filepaths
defaultMain
[ bench "isSuffix-based" $ nf (map classifyExtensionUsingIsSuffix) filepaths
, bench "reverse-based" $ nf (map classifyExtensionUsingReverse) filepaths
]Results:
True
All
isSuffix-based: OK
294 ns ± 26 ns
reverse-based: OK
135 ns ± 13 ns
|
I"m going to merge RSN. Any last minute reviews? |
|
Thank you! |
|
@Mikolaj could we have this released please? |
|
We've just released 0.6.3.3, for cabal 3.18, so that would mean bumping the minor version to 0.6.3.4, right? Not major, because no API break. |
|
Bumping the minor version (the third component) would make it 0.6.4.0. Bumping just the patch version (the fourth component) is normally reserved for packaging purposes only, but in this case there is a nontrivial code change. |
|
Oh, well spotted, 0.6.4.0 looks good. @andreasabel: would you like to release or shall I? |
|
Please go ahead @Mikolaj ! |
|
Hackage candidate: https://hackage.haskell.org/package/hackage-security-0.6.4.0/candidate |
|
I checked building Cabal against the candidate, looks good to me. |
|
Shipped. Thank you again! |
5 participants
Before these patches
hackageIndexLayouttakes up to 50% ofcabal updatetime. With the patches applied this reduces to ~10% ofcabal update, making it almost 2x faster.I originally developed this code for
hackage-revdeps, so the approach has been exercised for some time. Besides,hackageIndexLayoutis covered by tests forhackage-security.