commands: teach 'delay' capability to `git-lfs-filter-process(1)` by ttaylorr · Pull Request #2511 · git-lfs/git-lfs

ttaylorr · 2017-08-15T23:34:21Z

This pull requests teaches the git-lfs-filter-process(1) command the 'delay' capability that was proposed by @larsxschneider in #1632 (comment) and added to Git in git/git@51b8aec.

This implementation allows the process filter to respond with a status=delayed when asked to smudge the contents of a file that it does not already have in the local .git/lfs/objects cache.

If any items were delayed, Git will later ask for blobs that were available with the list_available_blobs command, which Git LFS will either respond with the immediately available blobs, wait until at least one becomes available, or respond with an empty set indicating that the transfer is complete.

To implement this, I added a new delayedSmudge helper function, which behaves the same way as smudge(), but instead adds the pointer to *tq.TransferQueue given as an argument.

Here are some benchmarks when running git clone on GitHub.com and a local copy of GitHub:

	`lfs.concurrenttransfers=1`	`...=3`	`...=8`	`...=16`	`...=32`
`v2.13.2`
github.com	29.2297	-	-	-	-
github.dev	18.4657	-	-	-	-

`v2.14.1.next`
github.com	45.6183	28.6867	14.9877	13.8893	13.7767
github.dev	9.02767	7.41867	7.70833	7.33933	7.71067

For a delta of:

	`lfs.concurrenttransfers=1`	`...=3`	`...=8`	`...=16`	`...=32`
github.com	-43.3866	0.543	14.242	15.3404	15.453
github.dev	9.43803	11.04703	10.75737	11.12637	10.75503

Excluding outliers:

	v2.13.2	`v2.14.1.next`	delta	delta %
github.com	29.2297	17.8351	11.4s faster	164% faster
github.dev	18.4657	7.840934	10.6s faster	236% faster

There were three big performance changes that impact this improvement:

tq: make Add() accept new items during batch processing #2483: Infinitely buffering the tq.(*TransferQueue).Add() function, which allows the TransferQueue to accept new items sent by Git, instead of waiting until the first batch of items were transferred.
tq: increase default lfs.concurrenttransfers to 8 #2506: Increasing the default number of concurrently running transfers from 3 to 8 allows LFS to take advantage of more network bandwidth and process items faster.
Not blocking on list_available_blobs until tq.Wait() returns. Instead, read from tq.Watch() until a read becomes blocking (via select { case <-tq.Wait(): ... default: }. This allows Git to perform the checkout while the tq.(*TransferQueue) is still transferring items.

Following this, I'd like to deprecate the git lfs clone command, since git clone is now as fast or faster than git lfs clone as of the latest tip of git.git.

EDIT: the each number is the average of 5 runs of a given command, with the outliers removed. The benchmark command is written as: benchmark.sh. The average-timings command is written as: average-timings.sh.

Closes: #2466.

/cc @git-lfs/core
/cc @larsxschneider

technoweenie

Looking great. I pointed out a few things below:

technoweenie · 2017-08-16T15:42:28Z

commands/command_filter_process.go

+				status = delayedStausFromErr(err)
+			} else {
+				status = statusFromErr(err)
+			}


IMO, this nested if is complicated. Would something like this work? It looks like calling Flush() on nil is a no-op any way.

if delayed { s.WriteStatus(delayedStausFromErr(err)) } else if ferr := w.Flush(); ferr != nil { s.WriteStatus(statusFromErr(ferr)) } else { s.WriteStatus(statusFromErr(err)) }

Continuing to declare status outside of that scope is fine too, but it didn't look like it was needed anywhere else.

BTW delayedStausFromErr has a typo :)

Way better -- thanks for this suggestion. Changed things up in 1001e1c (and I got that typo!)

technoweenie · 2017-08-16T16:00:57Z

commands/command_smudge.go

+//
+// delayedSmudge returns the number of bytes written, whether the checkout was
+// delayed, the *lfs.Pointer that was smudged, and an error, if one occurred.
+func delayedSmudge(s *git.FilterProcessScanner, to io.Writer, from io.Reader, tq *tq.TransferQueue, filename string, skip bool, filter *filepathfilter.Filter) (int64, bool, *lfs.Pointer, error) {


The tq parameter clobbers the tq package name. Not critical of course, but may be worth changing.

Good call -- I think we should be fine for now, since we don't use any other exported members from the tq package, but this will be a good way to prevent an ambiguous diff later on if that changes. Covered this one in: a9942cd.

technoweenie · 2017-08-16T16:25:19Z

commands/command_filter_process.go

+				}
+			}
+		case "list_available_blobs":
+			closeOnce.Do(func() { go q.Wait() })


I could use some clarification here. It's my understanding that a transfer queue can accept new transfers until Wait() is called. If I'm interpreting the protocol docs correctly, then the process should look like this:

Git sends all pointers through smudge with can-delay=1

Git sends list_available_blobs. The transfer queue is closed.

Git sends more smudge commands for each available blob, never calling list_available_blobs again.

Profit!!

🤔 not quite -- I think the confusion has to do with the readAvailable() func. readAvailable does as many reads as possible before <-tq.Watch() becomes blocking, responding to a list_available_blobs command with any blobs that it currently has ready, or blocking until at least one blob is ready.

The process looks like this:

Git sends all pointers through command=smudge with can-delay=1.

Git issues a list_available_blobs command, indicating that no more new blobs will be sent as above. The transfer queue is accordingly closed.

We read as many currently available blobs as we can before blocking, or block until a) at least one blob becomes available, or b) the transfer queue has no more blobs to send, in which case we send an empty list, and the process is complete.

Git send a command=smudge with can-delay=0 for each blob in the list above.

Git issues another list_available_blobs command.

See step 3.

I did my best to clarify this in 8816463.

technoweenie · 2017-08-16T18:08:31Z

commands/command_filter_process.go

+			if !ok {
+				return ts
+			}
+			return append(ts, t)


Does this part need to exist? If ts is empty, the for should start another iteration that'll attempt to read from the channel. Very possible there's some detail about select that I'm forgetting...

I think so, though the duplication is unfortunate. Specifically, this return append, could be turned into an append only, but the blocking <-ch read causes the enclosing for loop to not spin.

Jeez, the return makes all the difference in the world. Thanks.

technoweenie · 2017-08-17T20:20:28Z

test/test-track.sh


+  if [ ! -d .git/info ]; then
+    mkdir .git/info
+  fi


FWIW I think you could replace this with mkdir -p .git/info.

Good call -- I got this one in: bc376b8.

technoweenie

Really excited to see this land!

larsxschneider · 2017-08-20T16:00:37Z

As suggested by @ttaylorr, I added a progress indicator for filtering content in Git and posted the patch to the Git mailing list.

If you clone a repo it would look like this:

$ git clone https://server/some/repo.git
Cloning into 'repo'...
remote: Counting objects: 6077, done.
remote: Total 6077 (delta 0), reused 0 (delta 0), pack-reused 6077
Receiving objects: 100% (6077/6077), 6.00 MiB | 1.24 MiB/s, done.
Resolving deltas: 100% (4177/4177), done.
Filtering content: 100% (62/62), 7.63 MiB | 304.00 KiB/s, done.

If you checkout a commit that references many LFS files that you don't have locally on your machine yet, then you would see something like this:

$ git checkout mybranch
Filtering content: 100% (62/62), 7.63 MiB | 312.00 KiB/s, done.
Switched to branch 'mybranch'

A nice side effect is that users can easily see the bandwidth performance of their LFS servers 🙂

larsxschneider · 2017-08-20T16:06:41Z

There might be a bug in the current (7115b57) delay implementation. Try these steps:

(1) Checkout a branch of a repo with files in LFS
(2) Delete all files in that repo except for the .git folder
(3) Run git checkout -f HEAD

The checkout operation should be blazingly fast as all objects should be available locally already. However, it looks like as if Git LFS downloads all objects, again.

ttaylorr · 2017-08-20T16:25:13Z

The checkout operation should be blazingly fast as all objects should be available locally already. However, it looks like as if Git LFS downloads all objects, again.

@larsxschneider great catch, this was definitely a bug. With c04745c:

~/D/git-lfs-test (master) $ find .git/lfs/objects -type f | wc -l
     187

~/D/git-lfs-test (master) $ rm -rf ./000 ./001 ./002 ./003 ./004 ./005 ./006 ./007 ./008 ./009 ./010 ./script

~/D/git-lfs-test (master!) $ time git checkout -f HEAD
Your branch is up-to-date with 'origin/master'.
git checkout -f HEAD  0.05s user 0.07s system 84% cpu 0.146 total

EDIT: the above commit had a bug where it would not smudge pointers for which the local object was cached. I fixed that in: 50ecd67.

…inters

larsxschneider · 2017-08-20T17:01:13Z

commands/command_smudge.go

 		}
+
+		// Write 'statusFromErr(nil)', even though 'perr != nil', since
+		// we are about to write non-delayed smudged contents to "to".


The fix seems to work! Thank you 👍! However, I still try to decipher the comment 😄. perr should be nil here, no? TBH I don't really understand the same comment a few lines above either. Can you give me a hint?

Plus, could it be that this was the reason for the degraded performance in the lfs.concurrenttransfers=1 case?

Ah, good question -- I think the comment is out of date but the code is correct here: this case occurs when we have the contents locally and can smudge without delay. It is certainly possible that this could affect cases where lfs.concurrenttransfers=1, but I think this would be negligible. I fixed this up in 4cefa24.

larsxschneider · 2017-08-20T17:35:42Z

Cloning repositories via SSH with a large number of LFS files seems to consistently result in a "batch response: Bad credentials" error:

$ git clone git@github.com:larsxschneider/lfstest-manyfiles.git testrepo
Cloning into 'testrepo'...
warning: templates not found /Users/lars/share/git-core/templates
remote: Counting objects: 15012, done.
remote: Total 15012 (delta 0), reused 0 (delta 0), pack-reused 15012
Receiving objects: 100% (15012/15012), 2.02 MiB | 1.57 MiB/s, done.
Downloading 1/test11791.bin (6 B)00), 17.69 KiB | 0 bytes/s
Error downloading object: 1/test11791.bin (c5a39d8): Smudge error: Error downloading 1/test11791.bin (c5a39d8045fd604bdcf307bbe06e7e41dd59a2399dc2a1807e4a6e3dfef5148b): batch response: Bad credentials

Errors logged to /Users/lars/Temp/lfstest/testrepo/.git/lfs/objects/logs/20170820T190408.392912345.log
Use `git lfs logs last` to view the log.
error: external filter 'git-lfs filter-process' failed
fatal: 1/test11791.bin: smudge filter lfs failed
warning: Clone succeeded, but checkout failed.
You can inspect what was checked out with 'git status'
and retry the checkout with 'git checkout -f HEAD'

Plus, in case of an error Git LFS prints Downloading 1/test11791.bin (6 B) over the Filtering content: 100% (62/62), 7.63 MiB | 312.00 KiB/s progress.
Can/should we disable the Downloading... output in Git LFS?

technoweenie · 2017-08-20T18:04:53Z

Cloning repositories via SSH with a large number of LFS files seems to consistently result in a "batch response: Bad credentials" error:

Hmm, I wouldn't think this PR would affect that. It's probably an unrelated issue. Can you try with an HTTPS remote (just to confirm whether this is a general transfer queue issue, an ssh auth issue, or a delay filter issue)?

As for the SSH thing, my hunch is that the expiration timeout is not being respected. Is this happening at around the 10 minute mark?

$ ssh git@github.com git-lfs-authenticate github/git-lfs-test download
{
  "href": "https://lfs.github.com/github/git-lfs-test",
  "header": {
    "Authorization": "RemoteAuth booya"
  },
  "expires_at": "2017-08-20T18:10:12Z",
  "expires_in": 599
}

Two potential spots that I can think of:

Does the Batch API try to refresh the Authorization header after it expires? I thought it recognizes that and automatically retries. Maybe there's a race condition, and it should refresh the token if expires_in is < 5 or something.
Does the SSH cred cacher respect the expires_in? It should be falling back to a real ssh call after expires_in seconds have passed.

Can/should we disable the Downloading... output in Git LFS?

I think we still need some output for legacy smudges. The filter should disable them (or even implement its own progress output).

larsxschneider

I added a few nits that you can address if you want (most relevant from my point of view is the delayedStatusFromErr suggestion).

I am happy to see that merged 👍

ttaylorr · 2017-08-24T22:28:13Z

🙇 thanks for the review, everybody!

jjgod · 2017-09-14T22:01:51Z

What git version do I need for this?

~$ git lfs version
git-lfs/2.3.0 (GitHub; darwin amd64; go 1.8.3; git 4dd2bf73)
~$ git --version
git version 2.14.1

Yet it doesn't seem to work, no concurrent transfers when I do git clone .

For cloning the same repository:

git lfs clone:

real	0m28.256s
user	0m24.853s
sys	0m9.224s

git clone:

real	2m5.616s
user	0m15.168s
sys	0m10.439s

larsxschneider · 2017-09-15T08:39:44Z

@jjgod You need the upcoming Git version 2.15 😄
See https://twitter.com/kit3bus/status/908579232422486016

DJm00n · 2017-10-31T10:06:14Z

@larsxschneider

$ git --version
git version 2.15.0.windows.1

$ git lfs version
git-lfs/2.3.4 (GitHub; windows amd64; go 1.8.3; git d2f6752f)

Clone speed is fine but no progress output and looks like hang:

Speed:
git clone

real 0m34.317s
user 0m0.000s
sys 0m0.000s

git lfs clone

Git LFS: (16 of 16 files) 130.94 MB / 130.94 MB
real    0m38.161s
user    0m0.000s
sys     0m0.015s

DJm00n · 2017-10-31T10:40:42Z

@larsxschneider Strange, seems like git/git@52f1d62e is merged to git 2.15 already. macOS output is without progress meter too.

larsxschneider · 2017-10-31T15:34:56Z

Yeah, that is indeed strange. However, I only noticed it on Windows: https://public-inbox.org/git/7A5EBFA0-3DC4-4AA1-91DB-450DC89BA8E7@gmail.com/ I am AFK for a two weeks but I'll investigate when I am back.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

DJm00n · 2017-10-31T17:16:06Z

@larsxschneider, missed that thread. On your test repo
git clone https://github.com/larsxschneider/lfstest-manyfiles prints "Filtering content" progress too on my Macbook. But not on my vsonline test repo. So it looks like more general git issue not git-for-windows only.

MacBook-Pro:Develop djm00n$ git version
git version 2.15.0
MacBook-Pro:Develop djm00n$ git lfs version
git-lfs/2.3.4 (GitHub; darwin amd64; go 1.9.1)

Git and Git LFS are installed from Homebrew.

DJm00n · 2017-12-04T15:43:54Z

@larsxschneider just updated to git 2.15.1 and still no progress showing on clone. Any updates?
If any info/logs are needed just ask and ill provide them.
my test repo:
repo https://dimitriy-ryazantcev.visualstudio.com/DefaultCollection/_git/Test1
gittest1/gittest999!

larsxschneider · 2017-12-04T16:43:27Z

Thanks @DJm00n ! I can reproduce the issue with your repo! Let's see if I can find the problem...

larsxschneider · 2017-12-04T21:16:52Z

@DJm00n I think I found and fixed the issue. Let's see what the Git experts think: https://public-inbox.org/git/20171204203647.30546-1-lars.schneider@autodesk.com/

ttaylorr · 2017-12-05T01:16:20Z

@DJm00n I think I found and fixed the issue. Let's see what the Git experts think: https://public-inbox.org/git/20171204203647.30546-1-lars.schneider@autodesk.com/

@larsxschneider Excellent find, and thanks for keeping us up-to-date.

awmcclain · 2018-05-09T00:02:12Z

Sorry for the late question, but does this mean that by upgrading lfs & git that the speed will increase for git pull's where a large number of added files are tracked?

ttaylorr · 2018-05-09T00:03:41Z

but does this mean that by upgrading lfs & git that the speed will increase for git pull's where a large number of added files are tracked?

That's correct!

The "git lfs clone" command has been deprecated since Git LFS v2.3.0 and Git v2.15.0 were released, so we update our manual page for the command to reflect this fact and clarify when and why the command should still be used, if at all. As discussed in git-lfs#2466, Git's support for long-running filter processes (introduced in Git v2.11.0 via the filter.<driver>.process setting) was enhanced in v2.15.0 with the "delay" capability whereby the filter can signal that it will operate asynchronously on multiple blobs, thus allowing it to process multiple blobs in parallel. Git LFS was updated to make use of the "delay" capability in a series of PRs culminating in PR git-lfs#2511, all of which were included in the Git LFS v2.3.0 release. We also make some minor edits to the manual page to improve the formatting of literal strings.

The "git lfs clone" command has been deprecated since Git LFS v2.3.0 and Git v2.15.0 were released, so we update our manual page for the command to reflect this fact and clarify when and why the command should still be used, if at all. As discussed in git-lfs#2466, Git's support for long-running filter processes (introduced in Git v2.11.0 via the "filter.<driver>.process" setting) was enhanced in v2.15.0 with the "delay" capability whereby the filter can signal that it will operate asynchronously on multiple blobs, thus allowing it to process multiple blobs in parallel. Git LFS was updated to make use of the "delay" capability in a series of PRs culminating in PR git-lfs#2511, all of which were included in the Git LFS v2.3.0 release.

ttaylorr added 2 commits August 15, 2017 16:51

commands/smudge: teach 'delayedSmudge' func

ef3e6cf

commands/filter_process: implement 'delay' capability

e764429

ttaylorr added the review label Aug 15, 2017

ttaylorr added this to the v2.3.0 milestone Aug 15, 2017

ttaylorr requested review from larsxschneider and technoweenie August 15, 2017 23:34

technoweenie reviewed Aug 16, 2017

View reviewed changes

ttaylorr and others added 5 commits August 16, 2017 11:00

commands: clarify 'list_available_blobs' process

8816463

commands: clarify err to status conversion

1001e1c

commands/smudge: change tq -> q

a9942cd

Merge branch 'master' into delay

5fb19e4

commands: assign 'delayedStatusFromErr' to 'status'

0416db2

technoweenie reviewed Aug 16, 2017

View reviewed changes

ttaylorr added 3 commits August 17, 2017 11:28

commands: avoid shadowing 'err' variable

9f8e099

commands: clarify err to status conversion

396622e

commands: correct incomingOrCached buffering logic

d33e972

ttaylorr force-pushed the delay branch from 5e2587d to d33e972 Compare August 17, 2017 17:43

commands: don't fallback to nil pointers

7115b57

technoweenie reviewed Aug 17, 2017

View reviewed changes

technoweenie approved these changes Aug 17, 2017

View reviewed changes

ttaylorr added 2 commits August 17, 2017 15:46

commands: simplify error and flush result to status conversion

0177183

test: simplify test command with mkdir -p

bc376b8

commands/smudge: teach 'delayedSmudge' only to transfer uncached files

c04745c

commands/smudge: teach 'delayedSmudge' to write smudge non-delayed po…

50ecd67

…inters

larsxschneider reviewed Aug 20, 2017

View reviewed changes

larsxschneider approved these changes Aug 24, 2017

View reviewed changes

ttaylorr added 4 commits August 24, 2017 18:00

Merge branch 'master' into delay

3e334a4

commands: call 's.WriteStatus' explicitly

8c9a180

test: remove outdated assertion skip

8bd7aac

commands: bind 'delayedStatusFromErr' to 'statusFromErr'

700a275

ttaylorr merged commit ef961a1 into master Aug 24, 2017

ttaylorr deleted the delay branch August 24, 2017 22:28

This was referenced Aug 25, 2017

ci: disable integration tests on 'next' branch of git #2524

Merged

git: make filter status a sentinel #2525

Merged

commands: mark 'git lfs clone' as deprecated #2526

Merged

tools: compare expiration to "time.Now" instead of "from" #2530

Merged

larsxschneider mentioned this pull request Jan 31, 2018

Feature request: "lfs worktree add" command that's similar to "lfs clone" #1401

Closed

chrisd8088 mentioned this pull request Oct 16, 2023

Remove git lfs clone command #5544

Open

chrisd8088 mentioned this pull request Sep 11, 2025

git lfs pull seems to not respect 429 retry-after headers #6106

Open

jenkins-infra-bot mentioned this pull request Jan 20, 2022

[JENKINS-59139] don't set GIT_LFS_SKIP_SMUDGE=1 anymore while cloning with git 2.15+ jenkinsci/git-client-plugin#1601

Open

Conversation

ttaylorr commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

technoweenie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

technoweenie left a comment

Choose a reason for hiding this comment

Uh oh!

larsxschneider commented Aug 20, 2017

Uh oh!

larsxschneider commented Aug 20, 2017

Uh oh!

ttaylorr commented Aug 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsxschneider commented Aug 20, 2017

Uh oh!

technoweenie commented Aug 20, 2017

Uh oh!

larsxschneider left a comment

Choose a reason for hiding this comment

Uh oh!

ttaylorr commented Aug 24, 2017

Uh oh!

jjgod commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsxschneider commented Sep 15, 2017

Uh oh!

DJm00n commented Oct 31, 2017

Uh oh!

DJm00n commented Oct 31, 2017

Uh oh!

larsxschneider commented Oct 31, 2017 via email

Uh oh!

DJm00n commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DJm00n commented Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsxschneider commented Dec 4, 2017

Uh oh!

larsxschneider commented Dec 4, 2017

Uh oh!

ttaylorr commented Dec 5, 2017

Uh oh!

awmcclain commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttaylorr commented Aug 15, 2017 •

edited

Loading

ttaylorr commented Aug 20, 2017 •

edited

Loading

jjgod commented Sep 14, 2017 •

edited

Loading

DJm00n commented Oct 31, 2017 •

edited

Loading

DJm00n commented Dec 4, 2017 •

edited

Loading

awmcclain commented May 9, 2018 •

edited

Loading