From 3af4c9a29aef596765a5391cf80ecd601c8969bd Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Tue, 15 Jun 2021 09:32:12 -0400 Subject: [PATCH] fix exponential blowup when adding lots of identical files This was an old problem when the files were being added unlocked, so the changelog mentions that being fixed. However, recently it's also affected locked files. The fix for locked files is kind of stupidly simple. moveAnnex already handles populating unlocked files, and only does it when the object file was not already present. So remove the redundant populateUnlockedFiles call. (That call was added all the way back in cfaac52b88e157dd4e71626fe68af37015b9c9bd, and has always been unncessary.) Sponsored-by: Dartmouth College's Datalad project --- Annex/Ingest.hs | 45 ++++++++++--------- CHANGELOG | 2 + ...erformance_regression_impacting_datal.mdwn | 2 + ..._6e5121e066998a303cf68ebc53e9fc15._comment | 13 ++++++ 4 files changed, 41 insertions(+), 21 deletions(-) diff --git a/Annex/Ingest.hs b/Annex/Ingest.hs index b8d98f1084..ed2c6d5ac5 100644 --- a/Annex/Ingest.hs +++ b/Annex/Ingest.hs @@ -178,9 +178,7 @@ ingest' preferredbackend meterupdate (Just (LockedDown cfg source)) mk restage = golocked key mcache s = tryNonAsync (moveAnnex key naf (contentLocation source)) >>= \case - Right True -> do - populateUnlockedFiles key source restage - success key mcache s + Right True -> success key mcache s Right False -> giveup "failed to add content to annex" Left e -> restoreFile (keyFilename source) key e @@ -198,8 +196,8 @@ ingest' preferredbackend meterupdate (Just (LockedDown cfg source)) mk restage = cleanOldKeys (keyFilename source) key linkToAnnex key (keyFilename source) (Just cache) >>= \case LinkAnnexFailed -> failure "failed to link to annex" - _ -> do - finishIngestUnlocked' key source restage + lar -> do + finishIngestUnlocked' key source restage (Just lar) success key (Just cache) s gounlocked _ _ _ = failure "failed statting file" @@ -215,25 +213,30 @@ ingest' preferredbackend meterupdate (Just (LockedDown cfg source)) mk restage = finishIngestUnlocked :: Key -> KeySource -> Annex () finishIngestUnlocked key source = do cleanCruft source - finishIngestUnlocked' key source (Restage True) + finishIngestUnlocked' key source (Restage True) Nothing -finishIngestUnlocked' :: Key -> KeySource -> Restage -> Annex () -finishIngestUnlocked' key source restage = do +finishIngestUnlocked' :: Key -> KeySource -> Restage -> Maybe LinkAnnexResult -> Annex () +finishIngestUnlocked' key source restage lar = do Database.Keys.addAssociatedFile key =<< inRepo (toTopFilePath (keyFilename source)) - populateUnlockedFiles key source restage - -{- Copy to any unlocked files using the same key. -} -populateUnlockedFiles :: Key -> KeySource -> Restage -> Annex () -populateUnlockedFiles key source restage = - whenM (annexSupportUnlocked <$> Annex.getGitConfig) $ do - obj <- calcRepo (gitAnnexLocation key) - g <- Annex.gitRepo - ingestedf <- flip fromTopFilePath g - <$> inRepo (toTopFilePath (keyFilename source)) - afs <- map (`fromTopFilePath` g) <$> Database.Keys.getAssociatedFiles key - forM_ (filter (/= ingestedf) afs) $ - populatePointerFile restage key obj + populateUnlockedFiles key source restage lar + +{- Copy to any other unlocked files using the same key. + - + - When linkToAnnex did not have to do anything, the object file + - was already present, and so other unlocked files are already populated, + - and nothing needs to be done here. + -} +populateUnlockedFiles :: Key -> KeySource -> Restage -> Maybe LinkAnnexResult -> Annex () +populateUnlockedFiles _ _ _ (Just LinkAnnexNoop) = return () +populateUnlockedFiles key source restage lar = do + obj <- calcRepo (gitAnnexLocation key) + g <- Annex.gitRepo + ingestedf <- flip fromTopFilePath g + <$> inRepo (toTopFilePath (keyFilename source)) + afs <- map (`fromTopFilePath` g) <$> Database.Keys.getAssociatedFiles key + forM_ (filter (/= ingestedf) afs) $ + populatePointerFile restage key obj cleanCruft :: KeySource -> Annex () cleanCruft source = when (contentLocation source /= keyFilename source) $ diff --git a/CHANGELOG b/CHANGELOG index 0363d22b72..8495034692 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -30,6 +30,8 @@ git-annex (8.20210429) UNRELEASED; urgency=medium * Added annex.adviceNoSshCaching config. * Added --size-limit option. * Future proof activity log parsing. + * Fix an exponential slowdown when large numbers of duplicate files are + being added in unlocked form. -- Joey Hess Mon, 03 May 2021 10:33:10 -0400 diff --git a/doc/bugs/significant_performance_regression_impacting_datal.mdwn b/doc/bugs/significant_performance_regression_impacting_datal.mdwn index aed3e46444..a0ea3823ae 100644 --- a/doc/bugs/significant_performance_regression_impacting_datal.mdwn +++ b/doc/bugs/significant_performance_regression_impacting_datal.mdwn @@ -15,3 +15,5 @@ Currently 8.20210428+git282-gd39dfed2a and first got slow with 8.20210428+git228-g13a6bfff4 and was ok with 8.20210428+git202-g9a5981a15 [[!meta title="performance edge case when adding large numbers of identical files"]] + +> [[fixed|done]] --[[Joey]] diff --git a/doc/bugs/significant_performance_regression_impacting_datal/comment_33_6e5121e066998a303cf68ebc53e9fc15._comment b/doc/bugs/significant_performance_regression_impacting_datal/comment_33_6e5121e066998a303cf68ebc53e9fc15._comment index deeeec00b8..32eb08e113 100644 --- a/doc/bugs/significant_performance_regression_impacting_datal/comment_33_6e5121e066998a303cf68ebc53e9fc15._comment +++ b/doc/bugs/significant_performance_regression_impacting_datal/comment_33_6e5121e066998a303cf68ebc53e9fc15._comment @@ -7,4 +7,17 @@ Oh, there's a much better solution: If the annex object file already exists when ingesting a new file, skip populating other associated files. They will have already been populated. moveAnnex has to check if the annex object file already exists anyway, so this will have zero overhead. + +(Maybe that's what yarik was getting at in comment #30) + +Implemented that, and here's the results, re-running my prior benchmark: + +run 1: 0:03.14 +run 2: 0:03.24 +run 3: 0.03.35 +run 4: 0.03.45 +run 9: 0:03.65 + +That also shows the actual overhead of the diffing of the index, +as its size grows, is quite small. """]] -- 2.30.2