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

diff --git a/Annex/Ingest.hs b/Annex/Ingest.hs
index b8d98f1084692adf1e2204eeaed80d954c1d6105..ed2c6d5ac526a216793ae0f282088bed98b009ad 100644 (file)
--- 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 0363d22b72ad8971a1e729126886d7ea343e62c8..849503469248015a59f18bdefc413c35220e3caf 100644 (file)
--- 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 <id@joeyh.name>  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 aed3e464444e5dc7764797fc2e2ceb045b713324..a0ea3823aed2a5081bac56aa211c6cfc82876c8b 100644 (file)
--- 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 deeeec00b8aa315138bb7d1103219d510106460a..32eb08e11320c6549a0b8072cf383f36aa8c8bec 100644 (file)
--- 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.
 """]]