Share HTML template renderers and create a watcher framework #20218

zeripath · 2022-07-03T17:45:20Z

The recovery, API, Web and package frameworks all create their own HTML
Renderers. This increases the memory requirements of Gitea
unnecessarily with duplicate templates being kept in memory.

Further the reloading framework in dev mode for these involves locking
and recompiling all of the templates on each load. This will potentially
hide concurrency issues and it is inefficient.

This PR stores the templates renderer in the context and stores this
context in the NormalRoutes, it then creates a fsnotify.Watcher
framework to watch files.

The watching framework is then extended to the mailer templates which
were previously not being reloaded in dev.

Then the locales are simplified to a similar structure.

Fix #20210
Fix #20211
Fix #20217

Signed-off-by: Andrew Thornton art27@cantab.net

The recovery, API, Web and package frameworks all create their own HTML Renderers. This increases the memory requirements of Gitea unnecessarily with duplicate templates being kept in memory. Further the reloading framework in dev mode for these involves locking and recompiling all of the templates on each load. This will potentially hide concurrency issues and it is inefficient. This PR stores the templates renderer in the context and stores this context in the NormalRoutes, it then creates a fsnotify.Watcher framework to watch files. The watching framework is then extended to the mailer templates which were previously not being reloaded in dev. Then the locales are simplified to a similar structure. Fix go-gitea#20210, go-gitea#20211, go-gitea#20217 Replace go-gitea#20159 Signed-off-by: Andrew Thornton <art27@cantab.net>

Signed-off-by: Andrew Thornton <art27@cantab.net>

6543 · 2022-07-04T10:17:48Z

lease resolve conflict ;)

Signed-off-by: Andrew Thornton <art27@cantab.net>

modules/templates/htmlrenderer.go

lunny · 2022-07-04T14:43:54Z

I would like to split locale related code to another PR.

zeripath · 2022-07-04T17:31:55Z

I would like to split locale related code to another PR.

It's kinda intimately related.

We're simply creating a new watcher from the same framework and then completely simplifying the localestore to remove the complicated locking that was previously necessary.

I mean if you really want it separated that's fine.

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2022-07-09T19:06:48Z

Testing this locally, it reduces the baseline memory use of Gitea by 17Mb from 80Mb to 63Mb in dev mode.

It also speeds up considerably the python package endpoint.

integrations/api_activitypub_person_test.go

modules/options/base.go

Signed-off-by: Andrew Thornton <art27@cantab.net>

Gusted

Otherwise L-GTm

go.mod

Gusted

LGTM.

Signed-off-by: Andrew Thornton <art27@cantab.net>

…nd-better-reload

This reverts commit 959595b. syncthing/notify opens goroutines etc even on prod mode. This is unacceptable and therefore we should stick with fsnotify which is already a dependency because of unrolled.

go.mod

modules/options/dynamic.go

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2022-08-24T14:46:36Z

I am not quite sure about the locale part and if it belongs here… Isn't that pretty similar to #19916?

As I said above the locale work is completely related. The watcher work makes it possible to completely simplify this code.

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: delvh <dev.lh@web.de>

zeripath · 2022-08-27T15:55:03Z

@delvh is there anything else that needs to be done?

modules/util/path.go

delvh · 2022-08-27T16:12:49Z

modules/translation/translation.go

+				lock.Lock()
+				defer lock.Unlock()


Can we always be certain that lock == nil <==> setting.IsProd?
Or is there even a remote possibility that this might be changed at some point?
Because if yes, we will have a NPE here…

Look at the code.

I don't mean now, I meant in the future.
I've seen the current code, and here it's a valid assumption.
However, I can't predict the future:
Some code hasn't been changed in a few years.
And other parts get changed on a seemingly monthly basis.
That's why I asked, because once someone changes this assumption, there's a possiblity he won't notice that he created an NPE with it as well…

The assumption is made within the same function.

Any situation that messes with the locking at line 52 that doesn't deal with the lock here and continues to allow the Watcher is a concurrency race and it deserves to NPE and bring down the development server until they think more carefully about concurrency.

It really is also worth remembering that this is ONLY used when !setting.IsProd

All of this code is for dev servers.

delvh · 2022-08-27T16:26:42Z

modules/translation/i18n/errors.go

+import "errors"
+
+var (
+	ErrLocaleAlreadyExist = errors.New("lang already exists")


That will be funny in #20891…

One PR at a time.

Signed-off-by: Andrew Thornton <art27@cantab.net>

modules/util/path.go

Signed-off-by: Andrew Thornton <art27@cantab.net>

…nd-better-reload

codecov-commenter · 2022-08-28T09:35:01Z

Codecov Report

Merging #20218 (aec5c2f) into main (9e0c437) will increase coverage by 0.00%.
The diff coverage is 50.51%.

@@           Coverage Diff            @@
##             main   #20218    +/-   ##
========================================
  Coverage   47.03%   47.04%            
========================================
  Files         993     1003    +10     
  Lines      136484   136649   +165     
========================================
+ Hits        64198    64287    +89     
- Misses      64402    64476    +74     
- Partials     7884     7886     +2

Impacted Files	Coverage Δ
cmd/admin.go	`0.00% <0.00%> (ø)`
cmd/web.go	`0.00% <0.00%> (ø)`
models/activities/action_list.go	`74.28% <ø> (ø)`
models/activities/notification.go	`62.22% <0.00%> (ø)`
models/activities/statistic.go	`0.00% <0.00%> (ø)`
models/admin/task.go	`34.00% <ø> (ø)`
models/error.go	`44.11% <ø> (+5.45%)`	⬆️
models/org.go	`45.33% <ø> (-17.66%)`	⬇️
models/repo/repo.go	`61.73% <0.00%> (-0.89%)`	⬇️
models/repo/upload.go	`2.08% <0.00%> (ø)`
... and 163 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

zeripath · 2022-08-28T09:58:49Z

This should substantial reduce the baseline memory requirements of Gitea through proper sharing of templates. (When wrote this it reduced the memory reqs from 80Mb to 63Mb.)

It should also help prevent the hiding of concurrency errors on dev builds and simplifies considerably the locale and localestores.

zeripath added the type/enhancement An improvement of existing functionality label Jul 3, 2022

zeripath added this to the 1.18.0 milestone Jul 3, 2022

zeripath added 2 commits July 3, 2022 21:31

placate lint

07dc4f2

Signed-off-by: Andrew Thornton <art27@cantab.net>

fix windows

f265ce5

Signed-off-by: Andrew Thornton <art27@cantab.net>

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 4, 2022

zeripath added 2 commits July 4, 2022 13:49

Merge branch 'main' into load-templates-once-and-better-reload

726bf7c

Init needs a ctx

aa90128

Signed-off-by: Andrew Thornton <art27@cantab.net>

lunny reviewed Jul 4, 2022

View reviewed changes

modules/templates/htmlrenderer.go Show resolved Hide resolved

zeripath added 4 commits July 4, 2022 21:29

make SSPI also share the templates too

0209fa4

Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge branch 'main' into load-templates-once-and-better-reload

a04e94a

Merge branch 'main' into load-templates-once-and-better-reload

7dd067b

Merge branch 'main' into load-templates-once-and-better-reload

f61f79c

Gusted reviewed Jul 10, 2022

View reviewed changes

integrations/api_activitypub_person_test.go Show resolved Hide resolved

integrations/api_activitypub_person_test.go Outdated Show resolved Hide resolved

modules/options/base.go Show resolved Hide resolved

use todo only

cbdc8bc

Signed-off-by: Andrew Thornton <art27@cantab.net>

Gusted reviewed Jul 10, 2022

View reviewed changes

go.mod Show resolved Hide resolved

zeripath added 2 commits July 12, 2022 13:33

Merge branch 'main' into load-templates-once-and-better-reload

9e63150

Merge branch 'main' into load-templates-once-and-better-reload

1159e0f

Gusted approved these changes Jul 12, 2022

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 12, 2022

zeripath added 3 commits July 17, 2022 00:39

Switch to use syncthing/notify instead of fsnotify/fsnotify

959595b

Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge remote-tracking branch 'origin/main' into load-templates-once-a…

aca2416

…nd-better-reload

Revert "Switch to use syncthing/notify instead of fsnotify/fsnotify"

426eb8f

This reverts commit 959595b. syncthing/notify opens goroutines etc even on prod mode. This is unacceptable and therefore we should stick with fsnotify which is already a dependency because of unrolled.

Gusted reviewed Jul 18, 2022

View reviewed changes

go.mod Show resolved Hide resolved

modules/options/dynamic.go Show resolved Hide resolved

Gusted approved these changes Jul 18, 2022

View reviewed changes

as per review

3841573

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath and others added 4 commits August 24, 2022 16:44

Fix tests

2dbfc50

Signed-off-by: Andrew Thornton <art27@cantab.net>

placate lint

0ff8921

Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge branch 'main' into load-templates-once-and-better-reload

a442905

Update modules/templates/htmlrenderer.go

c5bdfef

Co-authored-by: delvh <dev.lh@web.de>

delvh reviewed Aug 27, 2022

View reviewed changes

delvh approved these changes Aug 27, 2022

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 27, 2022

placate lint

55c4ec6

Signed-off-by: Andrew Thornton <art27@cantab.net>

wxiaoguang reviewed Aug 28, 2022

View reviewed changes

modules/util/path.go Outdated Show resolved Hide resolved

zeripath added 2 commits August 28, 2022 10:05

as per wxiaoguang

f7a5cd7

Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge remote-tracking branch 'origin/main' into load-templates-once-a…

aec5c2f

…nd-better-reload

zeripath merged commit bb0ff77 into go-gitea:main Aug 28, 2022

zeripath deleted the load-templates-once-and-better-reload branch August 28, 2022 09:43

lunny mentioned this pull request Dec 27, 2022

Add more test directory to exclude dir of air, remove watching templates from air include dir because gitea has internal mechanism #22246

Merged

lunny mentioned this pull request Dec 27, 2022

Add more test directory to exclude dir of air, remove watching templates from air include dir because gitea has internal mechanism (#22246) #22247

Merged

go-gitea locked and limited conversation to collaborators May 3, 2023

Uh oh!

Share HTML template renderers and create a watcher framework #20218

Share HTML template renderers and create a watcher framework #20218

Uh oh!

Conversation

zeripath commented Jul 3, 2022 • edited by 6543 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

6543 commented Jul 4, 2022

Uh oh!

Uh oh!

lunny commented Jul 4, 2022

Uh oh!

zeripath commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeripath commented Jul 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gusted left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Gusted left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zeripath commented Aug 24, 2022

Uh oh!

zeripath commented Aug 27, 2022

Uh oh!

Uh oh!

delvh Aug 27, 2022

Choose a reason for hiding this comment

Uh oh!

zeripath Aug 27, 2022

Choose a reason for hiding this comment

Uh oh!

delvh Aug 27, 2022

Choose a reason for hiding this comment

Uh oh!

zeripath Aug 27, 2022

Choose a reason for hiding this comment

Uh oh!

delvh Aug 27, 2022

Choose a reason for hiding this comment

Uh oh!

zeripath Aug 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Aug 28, 2022

Codecov Report

Uh oh!

zeripath commented Aug 28, 2022

Uh oh!

Uh oh!

zeripath commented Jul 3, 2022 •

edited by 6543

Loading

zeripath commented Jul 4, 2022 •

edited

Loading