Slow performance during repository uploadFile. #14668
Comments
|
Thank you for the listing pprof results. Hmm. No clear smoking gun in that dump - it's all just a little a slow. Which I guess is related to you running on Synology. (You are running on a Synology right?) One surprisingly slow thing was: At 110ms but that doesn't really make much sense to me and it's not like it's a clear thing to improve. It would be good to catch one of the really slow uploads because then we'd really be able to compare. Now, some things:
|
|
I have created a small application to benchmark, and attached it.
I am not. I'm running on local filesystem.
I am trying 1.14 and there are still severe performance issues present.
I am automating the process of creating users, repositories and files in those repositories. I'm using the automatically generated SDK from https://github.com/belidzs/Gitea which appears to call the methods defined in Swagger, and nothing more really.
It appears as though there isn't any cut-off point.
I am using a SSD in perfect healthy condition.
I'm using Git version 2.30.0. |
|
@atillabyte Could you try to use other database i.e. mysql/postgres rather than sqlite3? |
|
That data suggests that master is at least twice as fast and up to 10 times faster - possibly even more if you account for GC time. (You previously had logs with 9s delays.) Whilst I appreciate you may want this to be running at sub 100ms speeds - I'm not certain we will ever get to that point. Some acknowledgement of the fact that it is actually at least twice as fast would be appreciated. Now... looking at the pprof SVG, we have 2.11s of work running in the http pathways. About half of that time is spent in json.Decode. So that would imply that there would be some benefit to switching from the default encoding/json library - but the place that is calling the json.Decode here isn't shown in that graph. One thing that could be more useful going forward is to change the values of
but you can change them in the REPL that go tool pprof gives too. Setting these too extreme like this may make the graph more difficult to understand but you can change these values to decrease the noise as necessary. I also note that SVG is clearly not helping you to post the images in here - it may be better to use It may be easier to meet on discord to help work through things a bit quicker. |
|
Is there any plan to improve the perf of file creation / upload to a repo? I've been monitoring this bug for quite some time. We consistently see it take 1.9-2.5 seconds when committing trivial files to a new repo through the gitea API. |
|
@KyKoPho have you checked on main? |
|
(I've spent most of this cycle doing performance fixes elsewhere - so I apologise that this hasn't had been looked at.) |
|
Hi @zeripath , thanks for the fast reply. I've tried the latest release, if that's what you mean. I could also clone and build from there. |
|
1.14.3 to be specific. |
|
I mean main. 1.15-dev. |
|
We are using the public docker containers. Is gitea/gitea:latest built off of main? The only dev tags I see are back around 1.7.0, etc |
|
I just tried gitea/gitea:latest and did see about an ~8% improvement. I was unable to find the gitea/gitea:1.15-dev image anywhere, but I'm guessing the latest image will have some of that baked in already. |
|
Hi @KyKoPho sorry I missed this... Would it be possible to get some update pprofs or good test example for this? It would be good to know where to target improvements. My suspicion is that the interpretation of the json payload is still going to be the largest delay - but changes targeting that would be involved so it would be good to prove this first. |
|
@zeripath Sure! Let me work on that. Never done that before, so may take some time. |
|
Hopefully this helps some. This is the result of
and also |
|
That's memory ... We were looking at time |
|
@zeripath here is top 10 from pprof during the same call / workflow. Note this is all work coming in via the API, not the web interface for gitea: |
|
Looking at the graph, it actually looks like most of the time is being spent decrypting the user password from disk with the pbkdf2 algo. (maybe?) |
|
Not surprised that pbkdf2 is a cause of slow down here - this is the cost of not having session support. If that really is the only slowdown left we've won though. BTW I forgot to ask which version are you running. It's really important to state the version - plus its really only worth doing this on 1.15-RC2 or main (not 1.14.x). |
|
Have you tried to use username and token with the API? |
|
@lunny Once I saw this was in the hash algo, I tried that last night. No dice, I think whether you use a token or basic auth, the same algo still gets used. I'm looking into setting up a reverse proxy for auth as we internally have a solution for that. @zeripath I tried this on 1.14.2, 1.14.3, 1.14.5 and 1.15.0+dev-543-g1b29747f0. They all seem to have the same perf in this regard. |
So unfortunately the tokens would still necessitate a pbkdf2 calculation as currently coded. We'd need to cache successful tokens in an LRU cache, have sessions, or move to a JWT token approach. (JWT tokens is on my list of things to do.) |
|
We can change the login check sequence on API. Firstly index the access_token table by token if token length is matched. If there is not exist then check password. |
We already do that - #16164 (& #16171), #6724. There are several inefficiencies with the current approach: Lines 69 to 82 in 6a33b29 Take a look at the loop starting line 77. The underlying assumption here is that the number of tokens returned is small - preferably one - however on a large install there may be many such tokens returned always in the same order. Every time such a "pathological" token is used it will be hashed against every single token returned. This alone could be serious slowdown and in fact prior to #16164 this would take effect on passwords >8 characters if the last 8 characters were hex. #16164 means that this can only happen on passwords which are 40 characters long and all hex. (Hmm... thinking on I'm not sure how much this would happen) This pathological case could be avoided if the user provides the username by limiting the tokens to those that match the username - I'll pop up a PR for this - but I'm not sure how often this would happen and paradoxically it may slow things down. However, that still leaves us with a pbkdf2 hash for each and every API call. In the case of avoiding the pbkdf2 hash for each and every API call. The options are:
(As an aside, this issue is also why we need to convince administrators to ban BASIC authentication using passwords because these suffer the same problems.) In terms of how to proceed:
EDIT: Thinking on, I think that the pathological case described above is should be somewhat uncommon. |
One of the issues holding back performance of the API is the problem of hashing. Whilst banning BASIC authentication with passwords will help, the API Token scheme still requires a PBKDF2 hash - which means that heavy API use (using Tokens) can still cause enormous numbers of hash computations. A slight solution to this whilst we consider moving to using JWT based tokens and/or a session orientated solution is to simply cache the successful tokens. This has some security issues but this should be balanced by the security issues of load from hashing. Related go-gitea#14668 Signed-off-by: Andrew Thornton <[email protected]>
|
@KyKoPho I suspect #16547 would significantly improve your experience and would be easily backportable 1.14 and 1.15. |
|
@zeripath thanks so much for the heads up. |
One of the issues holding back performance of the API is the problem of hashing. Whilst banning BASIC authentication with passwords will help, the API Token scheme still requires a PBKDF2 hash - which means that heavy API use (using Tokens) can still cause enormous numbers of hash computations. A slight solution to this whilst we consider moving to using JWT based tokens and/or a session orientated solution is to simply cache the successful tokens. This has some security issues but this should be balanced by the security issues of load from hashing. Related #14668 Signed-off-by: Andrew Thornton <[email protected]> Co-authored-by: Lunny Xiao <[email protected]>
[x]):Description
When uploading a file into an existing repository through the API, it can take several seconds to complete, even when on localhost and the file is under a megabyte in size.
...
Screenshots
The text was updated successfully, but these errors were encountered: