Make LFS http_client parallel within a batch.

[rremer]

No description provided.

[rremer]

This is somewhat related to #32307 , as without that mirroring upstream lfs files is limited to 20 concurrent threads. For my own internal mirroring, setting [lfs_client].BATCH_SIZE=400 added ~100MB of memory usage and shaved 3 days off of initial mirroring for our admittedly quite large repo :) .

[rremer]

I wanted to point out that this actually reduces the usefulness of this logline. I moved it out of modules/repository/repo.go, but there it had access to which repository it was operating on, so you know where to investigate if you want to track down the lfs pointer/blob. Here in modules/lfs/http_client.go, I don't have access to the repository name. I was thinking I could add a value to the Context which is passed down from repo to here?

[wxiaoguang]

https://go.dev/blog/loopvar-preview

No need such trick?

[rremer]

Please advise, are you suggesting that I can just do this?:

for _, object := range result.Objects {
  go performSingleOperation(groupCtx, object, dc, uc, transferAdapter)
}

[wxiaoguang]

Yup, I think so if it is right.

Maybe just for { errGroup.Go(...) } is enough?

[rremer]

So actually, this closure seems necessary. Admittedly I used this pattern because I'd seen it in practice elsewhere, not because I fully understood why it was in use, but errGroup.Go only takes a function without arguments; you cannot pass arguments to functions inside errGroup. An anonymous function enclosure like this is the only way I see to do this.

[wxiaoguang]

Admittedly I used this pattern because I'd seen it in practice elsewhere,

In history, it is due to https://go.dev/blog/loopvar-preview
(https://github.com/go-gitea/gitea/pull/32369/files#r1821738812)

not because I fully understood why it was in use

Well, that's why I would like to clarify the problem. My philosophy is that "do not write the code which you do not understand"

but errGroup.Go only takes a function without arguments; you cannot pass arguments to functions inside errGroup.

I do not see why it is a problem.

(the demo code in https://github.com/go-gitea/gitea/pull/32369/files#r1823705885)

for _, object := range result.Objects {
	errGroup.Go(func() error {
		return performSingleOperation(groupCtx, object, dc, uc, transferAdapter)
	})
}

[rremer]

had one more force-push after make tidy to handle a compliance check (moving x/sync from indirect to a direct dependency in go.mod). I believe I've addressed all comments except for this one which I'd like clarification/direction.

[wxiaoguang]

Still have the question here (a new thread):

Now the code is:

if uc != nil {
    if err := uc(); err != nil {
        return err
    }
    // did you miss a return here?
}
// It seems that when uc != nil and uc() == nil, then dc is called by mistake?
err := dc()
...

[rremer]

Good question, but no I believe this is correct. What I think may be confusing is that the existing code on main assumes that the uploadCallback will return an error if it is called with a non-nil object.Error. This whole closure is predicated by that, we are guaranteed to have a non-nil object.Error and thus the existing logic (which perhaps not very defensive) is not flawed, a call to uc or dc will always also return an error. So the flow of this code is like:

1. object.Error is non-nil, meaning this specific oid within a batch has some kind of error, maybe it was an upload request, maybe it was a download request, we don't know yet
2. if the upload callback was non-nill, it means this was an upload request, execute the upload callback with the object.Error and get a guaranteed error returned
3. if it was not an upload callback, we must have a download callback, as mentioned in the previous thread there are currently only two kinds of operations allowed by the lfs batch api. Previously this was in an 'else' block, I removed that by advisement of the linter, as the else block would always get executed and it outdents this code even further.

Put another way, when object.Error != nil this closure will always return either error from uploadCallback or downloadCallback, OR nil from downloadCallback if the error should be ignored (404). No other value can be returned from this, it's just a switch for executing the correct upload/dowload callbacks.

[wxiaoguang]

2. if the upload callback was non-nil, it means this was an upload request, execute the upload callback with the object.Error and get a guaranteed error returned

That's the problem. Which test or document guarantees that "upload callback returns an error" in this case?

The changed logic is fragile:

• It does change the original if-flow -- if "upload callback" returns nil. The old code just "continues" to next without calling "download callback", but new code calls "download callback".
• It is counter-intuitive, code readers must think a lot more about how "upload callback" work to understand whether there is a bug.
• If the upload callback behaviors would be changed in the future (for example: ignores some errors), the new logic is difficult to maintain.

To be intuitive, defensive programming, and easier to understand, I think we should choose one explicit behavior:

if uc != nil {
    if err := uc(); err != nil {
        return err
    }
    // choose one explicit behavior:
    // * "return nil" means the upload callback could decide to ignore some kinds of errors, not stop the uploading
    // * "return err" means whatever upload callback does, any error should stop the uploading
    //
    // Actually "return nil" is the old behavior, I think it was well-designed, 
    // and it makes it possible to ignore some errors.
    return nil
}

[wxiaoguang]

One more thing, I do not think "batch size" means "concurrency limit" in this case. If I understand correctly:

• "batch size" means how many objects to be processed in one time.
• "concurrency limit" means how many concurrent connections to the LFS server.

Maybe "batch size" could be 100 or 1000 without causing problem, but "concurrency limit" should be much smaller, eg: 5 or 10. A lot of connections to a LFS server might trigger their rate-limit protection, or be considered as somewhat DoS?

[rremer]

I'm always in favor of making things more configurable, however as an administrator I think most of the time you would want [lfs_client].BATCH_SIZE = (the number of concurrent threads for upload/download within a batch). I'm happy to add a new config though, what should we call it and what should its default be? [lfs_client].BATCH_MAX_THREADS ?

[wxiaoguang]

Maybe [lfs_client].MAX_OPERATION_CONCURRENCY ? (since there is no "thread" concept in Golang)

[rremer]

Fair, how about [lfs_client].BATCH_OPERATION_CONCURRENCY, as I feel there may be other single-threaded operations still in this code that we might want configs for then, and this is specific to lfs client batch operations? I updated this branch with that config, and defaulted it to what [lfs_client].BATCH_SIZE if unset or <1

[wxiaoguang]

We need to match the default git-lfs's behavior:

https://github.com/git-lfs/git-lfs/blob/4cca3f8e7d77c5b86cdfe34c2e6bba2126e3d5cd/docs/man/git-lfs-config.5.ronn#L25-L33

lfs.concurrenttransfers

The number of concurrent uploads/downloads. Default 3.

[rremer]

last force-push was just a rebase from main, this should be ready to review again. I believe I've addressed all comments although suspect there may be opinions on my choice of the new config key under [lfs_client]

[wxiaoguang]

Made 2 commits:

• dfe8978 mainly makes 3 as the default value for concurrent operations Make LFS http_client parallel within a batch. #32369 (comment)
• 3a5889c uses a more controllable error handling logic Make LFS http_client parallel within a batch. #32369 (comment)

[rremer]

the default in lfs client is actually 8, not 3: https://github.com/git-lfs/git-lfs/blob/main/lfshttp/client.go#L89 , but I've documented what you set here: https://gitea.com/gitea/docs/pulls/88

[wxiaoguang]

Good catch .... actually there are different documents (I read an out-dated one) .... the default value also ever changed. 😅

The new document: https://github.com/git-lfs/git-lfs/blob/main/docs/man/git-lfs-config.adoc also says 8

[wxiaoguang]

I will update it to 8 and update the documents.

-> Use 8 as default value for git lfs concurrency #32421

Thank you very much.