improve concurrency of move/copy --from --to
authorJoey Hess <joeyh@joeyh.name>
Tue, 24 Jan 2023 17:45:01 +0000 (13:45 -0400)
committerJoey Hess <joeyh@joeyh.name>
Tue, 24 Jan 2023 17:59:39 +0000 (13:59 -0400)
Use separate stages for download and upload. In the common case where
it downloads the file from one remote and then uploads to the other,
those are by far the most expensive operations, and there's a decent
chance the two remotes bottleneck on different resources.

Suppose it's being run with -J2 and a bunch of 10 mb files. Two threads
will be started both downloading from the src remote. They will probably
finish at the same time. Then two threads will be started uploading to
the dst remote. They will probably take the same time as well. Before
this change, it would alternate back and forth, bottlenecking on src and dst.
With this change, as soon as the two threads start uploading to dst, two
more threads are able to start, downloading from src. So bandwidth to
both remotes is saturated more often.

Other commands that use transferStages only send in one direction at a
time. So the worker threads for the other direction will sit idle, and
there will be no change in their behavior.

Sponsored-by: Dartmouth College's DANDI project
12 files changed:
Annex/Transfer.hs
Command/Copy.hs
Command/Get.hs
Command/Mirror.hs
Command/Move.hs
Command/Sync.hs
Types/Direction.hs [new file with mode: 0644]
Types/Transfer.hs
Types/WorkerPool.hs
doc/git-annex-copy.mdwn
doc/git-annex-move.mdwn
git-annex.cabal

index e33c309fc85765aa9d1ebea019cebfb1b0dd548a..5ead81b65555591550375ae7a940ac45278c0c65 100644 (file)
@@ -120,7 +120,7 @@ alwaysRunTransfer = runTransfer' True
 
 runTransfer' :: Observable v => Bool -> Transfer -> AssociatedFile -> Maybe StallDetection -> RetryDecider -> (MeterUpdate -> Annex v) -> Annex v
 runTransfer' ignorelock t afile stalldetection retrydecider transferaction =
-       enteringStage TransferStage $
+       enteringStage (TransferStage (transferDirection t)) $
                debugLocks $
                        preCheckSecureHashes (transferKey t) go
   where
@@ -244,7 +244,7 @@ runTransferrer
        -> NotifyWitness
        -> Annex Bool
 runTransferrer sd r k afile retrydecider direction _witness =
-       enteringStage TransferStage $ preCheckSecureHashes k $ do
+       enteringStage (TransferStage direction) $ preCheckSecureHashes k $ do
                info <- liftIO $ startTransferInfo afile
                go 0 info
   where
index 11ef1ddd08778ab2e2fc8d4ca1b245247a475f1f..e53e5cdbe4ff0dd461ce9d0f62381f3ef26fc83f 100644 (file)
@@ -50,7 +50,7 @@ seek o = case fromToOptions o of
        Nothing -> giveup "Specify --from or --to"
 
 seek' :: CopyOptions -> FromToHereOptions -> CommandSeek
-seek' o fto = startConcurrency commandStages $ do
+seek' o fto = startConcurrend stages $ do
        case batchOption o of
                NoBatch -> withKeyOptions
                        (keyOptions o) (autoMode o) seeker
@@ -73,6 +73,12 @@ seek' o fto = startConcurrency commandStages $ do
                }
        keyaction = Command.Move.startKey fto Command.Move.RemoveNever
 
+       stages = case fto of
+               FromOrToRemote (FromRemote _) -> commandStages
+               FromOrToRemote (ToRemote _) -> commandStages
+               ToHere -> commandStages
+               FromRemoteToRemote _ _ -> transferStages
+
 {- A copy is just a move that does not delete the source file.
  - However, auto mode avoids unnecessary copies, and avoids getting or
  - sending non-preferred content. -}
index a31b6470a49e01825223bf3522b1d1a478969fd0..be1e9ab41be97b2e5b02c35b171afb94bd6f1aad 100644 (file)
@@ -38,7 +38,7 @@ optParser desc = GetOptions
        <*> parseBatchOption True
 
 seek :: GetOptions -> CommandSeek
-seek o = startConcurrency downloadStages $ do
+seek o = startConcurrency transferStages $ do
        from <- maybe (pure Nothing) (Just <$$> getParsed) (getFrom o)
        let seeker = AnnexedFileSeeker
                { startAction = start o from
index 0699c5e0334c0fee10ae124e814fb1c7115ac5e2..f169aae9283eddd0479b2691452f33d1fa7dfed9 100644 (file)
@@ -48,7 +48,7 @@ seek o = startConcurrency stages $
                =<< workTreeItems ww (mirrorFiles o)
   where
        stages = case fromToOptions o of
-               FromRemote _ -> downloadStages
+               FromRemote _ -> transferStages
                ToRemote _ -> commandStages
        ww = WarnUnmatchLsFiles
        seeker = AnnexedFileSeeker
index 0f66948379f03bfa66407bf58e1417674d6a9e1f..23a319645d058d898dfe5f6509014be6269d2cb7 100644 (file)
@@ -84,10 +84,10 @@ seek' o fto = startConcurrency stages $ do
                , usesLocationLog = True
                }
        stages = case fto of
-               FromOrToRemote (FromRemote _) -> downloadStages
+               FromOrToRemote (FromRemote _) -> transferStages
                FromOrToRemote (ToRemote _) -> commandStages
-               ToHere -> downloadStages
-               FromRemoteToRemote _ _ -> commandStages
+               ToHere -> transferStages
+               FromRemoteToRemote _ _ -> transferStages
        keyaction = startKey fto (removeWhen o)
        ww = WarnUnmatchLsFiles
 
index 781b4e612544d197d58f7e72a0d4201660456fdc..0334c29b84df1bd92c86baf90f167296cbfbef42 100644 (file)
@@ -212,7 +212,7 @@ instance DeferredParseClass SyncOptions where
 seek :: SyncOptions -> CommandSeek
 seek o = do
        prepMerge
-       startConcurrency downloadStages (seek' o)
+       startConcurrency transferStages (seek' o)
        
 seek' :: SyncOptions -> CommandSeek
 seek' o = do
diff --git a/Types/Direction.hs b/Types/Direction.hs
new file mode 100644 (file)
index 0000000..a18b836
--- /dev/null
@@ -0,0 +1,25 @@
+{- git-annex transfer direction types
+ -
+ - Copyright 2012 Joey Hess <id@joeyh.name>
+ -
+ - Licensed under the GNU AGPL version 3 or higher.
+ -}
+
+{-# LANGUAGE OverloadedStrings #-}
+
+module Types.Direction where
+
+import qualified Data.ByteString as B
+
+data Direction = Upload | Download
+       deriving (Eq, Ord, Show, Read)
+
+formatDirection :: Direction -> B.ByteString
+formatDirection Upload = "upload"
+formatDirection Download = "download"
+
+parseDirection :: String -> Maybe Direction
+parseDirection "upload" = Just Upload
+parseDirection "download" = Just Download
+parseDirection _ = Nothing
+
index 8f13af702ebb36df92c0d4650cdf879e4bb7f348..55b3de5921cd27f28f65330081582f0dfc7f1c71 100644 (file)
@@ -5,20 +5,22 @@
  - Licensed under the GNU AGPL version 3 or higher.
  -}
 
-{-# LANGUAGE OverloadedStrings #-}
 {-# LANGUAGE FlexibleInstances #-}
 
-module Types.Transfer where
+module Types.Transfer (
+       module Types.Transfer,
+       module Types.Direction
+) where
 
 import Types
 import Types.Remote (Verification(..))
 import Types.Key
+import Types.Direction
 import Utility.PID
 import Utility.QuickCheck
 import Utility.Url
 import Utility.FileSystemEncoding
 
-import qualified Data.ByteString as B
 import Data.Time.Clock.POSIX
 import Control.Concurrent
 import Control.Applicative
@@ -55,18 +57,6 @@ data TransferInfo = TransferInfo
 stubTransferInfo :: TransferInfo
 stubTransferInfo = TransferInfo Nothing Nothing Nothing Nothing Nothing (AssociatedFile Nothing) False
 
-data Direction = Upload | Download
-       deriving (Eq, Ord, Show, Read)
-
-formatDirection :: Direction -> B.ByteString
-formatDirection Upload = "upload"
-formatDirection Download = "download"
-
-parseDirection :: String -> Maybe Direction
-parseDirection "upload" = Just Upload
-parseDirection "download" = Just Download
-parseDirection _ = Nothing
-
 instance Arbitrary TransferInfo where
        arbitrary = TransferInfo
                <$> arbitrary
index 9792f5e98d5290b39c6e1d7a5c731b671d8a3e88..0df87992031ef1e397b15b31c2b028a8a759dbd0 100644 (file)
@@ -1,12 +1,14 @@
 {- Worker thread pool.
  -
- - Copyright 2019 Joey Hess <id@joeyh.name>
+ - Copyright 2019-2023 Joey Hess <id@joeyh.name>
  -
  - Licensed under the GNU AGPL version 3 or higher.
  -}
 
 module Types.WorkerPool where
 
+import Types.Direction
+
 import Control.Concurrent
 import Control.Concurrent.Async
 import qualified Data.Set as S
@@ -49,7 +51,7 @@ data WorkerStage
        -- ^ Running a CommandPerform action.
        | CleanupStage
        -- ^ Running a CommandCleanup action.
-       | TransferStage
+       | TransferStage Direction
        -- ^ Transferring content to or from a remote.
        | VerifyStage
        -- ^ Verifying content, eg by calculating a checksum.
@@ -82,15 +84,24 @@ commandStages = UsedStages
        , stageSet = S.fromList [PerformStage, CleanupStage]
        }
 
--- | When a command is downloading content, it can use this instead.
--- Downloads are often bottlenecked on the network or another disk
--- than the one containing the repository, while verification bottlenecks
--- on the disk containing the repository or on the CPU. So, run the
--- transfer and verify stage separately.
-downloadStages :: UsedStages
-downloadStages = UsedStages
-       { initialStage = TransferStage
-       , stageSet = S.fromList [TransferStage, VerifyStage]
+-- | This is mostly useful for downloads, not for uploads. A download
+-- is often bottlenecked on the network or another disk than the one
+-- containing the repository. When verification is not done incrementally,
+-- it bottlenecks on the disk containing the repository or on the CPU.
+-- So it makes sense to run the download and verify stages separately.
+-- 
+-- For uploads, there is no separate verify step to this is less likely
+-- to be useful than commandStages. However, a separate stage is provided
+-- for Uploads. That can be useful when a command downloads from one remote
+-- (eg using the network) and uploads to another remote (eg using a disk).
+transferStages :: UsedStages
+transferStages = UsedStages
+       { initialStage = TransferStage Download
+       , stageSet = S.fromList
+               [ TransferStage Download
+               , TransferStage Upload
+               , VerifyStage
+               ]
        }
 
 workerStage :: Worker t -> WorkerStage
index acf3b2a32d3629c3c7af10c510af232761d3a545..0ef70a4503dedd99a0f8d5bc011a7761f49d9262 100644 (file)
@@ -48,6 +48,9 @@ Paths of files or directories to operate on can be specified.
 
   Setting this to "cpus" will run one job per CPU core.
 
+  Note that when using --from with --to, twice this many jobs will
+  run at once, evenly split between the two remotes.
+
 * `--auto`
 
   Rather than copying all specified files, only copy those that don't yet have
index 162595fb9720a3203e4779646896ccb3e710c165..b451412eec464a3d9b2abc9b1454cd7df5772009 100644 (file)
@@ -55,6 +55,9 @@ Paths of files or directories to operate on can be specified.
 
   Setting this to "cpus" will run one job per CPU core.
 
+  Note that when using --from with --to, twice this many jobs will
+  run at once, evenly split between the two remotes.
+
 * `--all` `-A`
 
   Rather than specifying a filename or path to move, this option can be
index f578808ad78bceece217e3e33f5fa54c5132cf20..46f1bc0a00f96ef617ec8fe673a3490f47d74d92 100644 (file)
@@ -1009,6 +1009,7 @@ Executable git-annex
     Types.DeferredParse
     Types.DesktopNotify
     Types.Difference
+    Types.Direction
     Types.Distribution
     Types.Export
     Types.FileMatcher