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

WIP: Defer last commit info #16063

Closed
wants to merge 7 commits into from

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Jun 3, 2021

We use the commitinfo cache because we recognise that generating its data is slow. However, when we have a commit info cache we allow it to cause the page to not be rendered until the commit info has been calculated.

This PR simply changes the code to allow the page to still be rendered at least partially whilst we're waiting.

@zeripath zeripath added the performance/speed performance issues with slow downs label Jun 3, 2021
@zeripath zeripath added this to the 1.15.0 milestone Jun 3, 2021
@zeripath zeripath added the topic/ui Change the appearance of the Gitea UI label Jun 6, 2021
@zeripath zeripath mentioned this pull request Jun 9, 2021
6 tasks
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@codecov-commenter
Copy link

Codecov Report

Merging #16063 (67b817e) into main (fb3ffeb) will decrease coverage by 0.00%.
The diff coverage is 52.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16063      +/-   ##
==========================================
- Coverage   44.19%   44.18%   -0.01%     
==========================================
  Files         694      695       +1     
  Lines       82324    82432     +108     
==========================================
+ Hits        36379    36422      +43     
- Misses      40057    40107      +50     
- Partials     5888     5903      +15     
Impacted Files Coverage Δ
modules/repository/cache.go 35.00% <0.00%> (-1.85%) ⬇️
routers/init.go 60.00% <0.00%> (-1.65%) ⬇️
routers/web/repo/commit.go 28.83% <0.00%> (-0.11%) ⬇️
modules/git/commit_info_nogogit.go 63.47% <7.14%> (-3.66%) ⬇️
routers/web/repo/view.go 41.03% <16.66%> (-0.29%) ⬇️
modules/repository/last_commit_queue_nogogit.go 60.71% <60.71%> (ø)
modules/git/last_commit_cache_nogogit.go 39.65% <100.00%> (+4.46%) ⬆️
modules/git/notes_nogogit.go 70.73% <100.00%> (+0.73%) ⬆️
modules/process/manager.go 75.00% <100.00%> (+2.16%) ⬆️
modules/queue/queue_channel.go 91.66% <0.00%> (-3.34%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb3ffeb...67b817e. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 9, 2021
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiniest of nits. Still going through the other code though.

modules/repository/last_commit_queue_gogit.go Outdated Show resolved Hide resolved
modules/repository/last_commit_queue_nogogit.go Outdated Show resolved Hide resolved
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

Things to be aware of:

The use of a queue with the default workers of 0 and boost of 1 with queue length 20 means that there is only 1 repository getting its commit info generated at a time unless there are 20 waiting. We may want to increase the number of boost workers for this queue by default.

@zeripath
Copy link
Contributor Author

In light of information found in #16035 I think I'm going to have to make this WIP and have another think.

A different solution is required.

@zeripath zeripath removed this from the 1.15.0 milestone Jun 22, 2021
@zeripath zeripath added the pr/wip This PR is not ready for review label Jun 22, 2021
@zeripath zeripath changed the title Defer last commit info WIP: Defer last commit info Jun 22, 2021
@tuaris
Copy link

tuaris commented Jul 6, 2021

@zeripath I am wondering what prompted you to rethink this? Unless I am misunderstanding the proposed solution, #16035 is irrelevant?

The way I understand the approach is:

  1. Remove the 'on the fly' extraction of the commit message via git log during page rendering.
  2. Instead look for the commit message as a value in a SQL database, Elasticsearch, or this new Ledis thing (I don't believe using Redis or Memcache make sense here).
  3. If there is no value then display nothing (perfectly acceptable behavior).
  4. A background process will fill/update/delete the SQL/Elasticsearch values.

@@ -146,6 +146,16 @@ func NewQueueService() {
_, _ = section.NewKey("CONN_STR", Indexer.IssueQueueConnStr)
}

// Default the last_commit_queue to use the level queue
section = Cfg.Section("queue.issue_indexer")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the queue name is not right.

This was referenced Jul 14, 2021
@delvh
Copy link
Member

delvh commented Oct 8, 2021

Can this PR be closed then as #16467 got merged now?

@zeripath
Copy link
Contributor Author

zeripath commented Oct 9, 2021

@delvh whilst this does do things differently and does some novel things I think you're right and I'm gonna close it.

@zeripath zeripath closed this Oct 9, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
@zeripath zeripath deleted the defer-last-commit-info branch December 29, 2022 19:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. performance/speed performance issues with slow downs pr/wip This PR is not ready for review topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants