Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Folder jumping #2818

Closed
wants to merge 2 commits into from
Closed

Folder jumping #2818

wants to merge 2 commits into from

Conversation

sapk
Copy link
Contributor

@sapk sapk commented Mar 13, 2016

Simple implemation with the adding in git-module (gogs/git-module#7) for implementing #1149

Certainly not the best implementation in term of efficiency.

capture d ecran_2016-03-13_03-09-40

@sapk
Copy link
Contributor Author

sapk commented Mar 13, 2016

Maybe a better choice could be to edit the TreeEntry struct and a JumpedFolder attribute relink the treeentry to the pointing file. In this case the treeEntry would reflect the real pointed file and not the folder. It could also limite the calculation to one time. But doing so maybe break other things ...

@unknwon unknwon added the status: waits for review It is waiting to be reviewed by maintainers label Mar 13, 2016
@saboya
Copy link
Contributor

saboya commented Mar 13, 2016

I tested this in my local install, latest master + this merge. It worked fine, but the performance impact was huge, loading times went up A LOT.

Gogs version: 0.9.8.0312 (+ merge)
Go version: 1.6
OS: FreeBSD 10.1 amd64

@unknwon unknwon added status: review later The pull request is not confident to be review at the moment and removed status: waits for review It is waiting to be reviewed by maintainers labels Mar 15, 2016
@sapk
Copy link
Contributor Author

sapk commented Mar 16, 2016

I rebase code since it was base on old version and i did some measures. (done with sqlite for db and gogs as repo with some folders to jump, Go version: 1.6)

Before Rebase :
Gogs Version: 0.9.1.0306 Page: 740ms Template: 10ms
Gogs Version (with PR): 0.9.1.0306 Page: 1119ms Template: 14ms

After rebase :
Gogs Version: 0.9.12.0316 Page: 725ms Template: 4ms
Gogs Version (with PR): 0.9.12.0316 Page: 1011ms Template: 4ms

@sapk sapk changed the title [WIP] Folder jumping Folder jumping Mar 17, 2016
@xor-gate
Copy link

Very nice 👍

@mhartkorn
Copy link
Contributor

I find the performance impact of the current solution very high. On a small Java project (3 MB code) the response times went from 270ms to 4200ms. Java projects often have a lot of empty folders because of the package names and this sums up to a lot of loops in the code.

Edit: "Empty" as in "single-entry".

@xor-gate
Copy link

@mhartkorn you are right on this, gogs would benefit from aggresive caching at a few places. On our KVM hosted gogs even loading the mainpage takes ~1 second for rendering.

@mhartkorn
Copy link
Contributor

@xor-gate In theory gogs supports redis and memcached but they don't seem to be used at all. But even if gogs was able to use redis for folder structures, a 4 second page load for every single folder in a repository for people that don't use redis is too long to be considered useful in my opinion. Maybe this feature can be an option.

@xor-gate
Copy link

People could benefit if in-memory cache works for this long operations. IMHO 4 seconds is crazy for a repo of 3 megabyte. Seems a little performance regression here probably. Even when the disk cache is not warmed up it should not take 4 seconds.

@unknwon
Copy link
Member

unknwon commented Mar 19, 2016

Just a note Gogs has unified cache interface, it does not matter you use in-memory, redis or memcache.

ethantkoenig pushed a commit to ethantkoenig/gogs that referenced this pull request Nov 10, 2017
* fix wrong translations

* fix tab on yml
@unknwon unknwon closed this Oct 25, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: review later The pull request is not confident to be review at the moment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants