Fix dashboard ignored system setting cache

[lunny]

This is a performance regression from #18058

[silverwind]

#21611 related?

[lunny]

#21611 related?

yes

[silverwind]

Not sure thought, the issue mentiones 1.17 but #18058 is only on 1.18.

[lunny]

Not sure thought, the issue mentiones 1.17 but #18058 is only on 1.18.

Wow, so that's not the same issue.

[lunny]

@zeripath done.

[zeripath]

OK, there are a few other problems here:

1. The length of time these things are cached needs to be considered. These settings should probably be only cached for 5 minutes rather than the 16h that they are being cached for by default. This is particularly an issue with an external cacher as that time will persist across restarts.
2. In the case of the internal caches like the memory or twoqueue variants if a pointer is returned from Get it is the pointer that was stored. This means that things that come out of the cache need to be immutable or treated as immutable, otherwise there will be races and they will need to be copied before being touched. Are we sure that these settings are being used immutably?
3. Could we get away without adding this caching at all because we can just get this data out of the db once per request? I suspect we could restructure things to just make getting the settings ONCE per request very easily.
4. I suspect there is little point in storing these settings in an external cache like redis or memcache - it seems likely that the time taken to request the data from the db would be equal to (or even potentially less than) the time taken to get this data from the cache. So really this stuff should only be being stored in some memory cache like an LRU but with a strict time requirement.

---

As an aside, we really need to rethink our caching infrastructure. Ideally I think what we need is a two-or-three level caching infrastructure.

1. Request-level cache (this could be a smallish LRU) - (this may be unnecessary)
2. Internal server cache (e.g. memory or twoqueue)
3. External cache(s) (if present e.g. leveldb, redis, memcache etc - perhaps even a DB one could be made)

Things cached in the internal memory caches would need be considered immutable on Get - request level cache could be considered mutable because they're unlikely to affected by concurrency and the external caches would create new structs by default anyway.

---

So, I am now concerned that this may not be the correct solution at all and I think the issue that this data is being requested over and over again unnecessary. With a little thought we could prevent this problem entirely.

[lunny]

OK, there are a few other problems here:

1. The length of time these things are cached needs to be considered. These settings should probably be only cached for 5 minutes rather than the 16h that they are being cached for by default. This is particularly an issue with an external cacher as that time will persist across restarts.

Why the time is important need to be changed? I think once the value changed, the cache has been updated.

2. In the case of the internal caches like the memory or twoqueue variants if a pointer is returned from Get it is the pointer that was stored. This means that things that come out of the cache need to be immutable or treated as immutable, otherwise there will be races and they will need to be copied before being touched. Are we sure that these settings are being used immutably?

I think yes. We should always return the values as string or int64 so that they cannot be changed.

3. Could we get away without adding this caching at all because we can just get this data out of the db once per request? I suspect we could restructure things to just make getting the settings ONCE per request very easily.

I think no, we can't remove the caching, it has produced actual affect in the main branch especially in dashboard from some users' reports. A memory cache or a redis cache operation will be nanosecond level but a database operation is microsecond level, memory/redis is fast about 10~1000 than database operations. And even in one request, the same setting maybe reused serveral times.

4. I suspect there is little point in storing these settings in an external cache like redis or memcache - it seems likely that the time taken to request the data from the db would be equal to (or even potentially less than) the time taken to get this data from the cache. So really this stuff should only be being stored in some memory cache like an LRU but with a strict time requirement.

As above.

As an aside, we really need to rethink our caching infrastructure. Ideally I think what we need is a two-or-three level caching infrastructure.

1. Request-level cache (this could be a smallish LRU) - (this may be unnecessary)
2. Internal server cache (e.g. memory or twoqueue)
3. External cache(s) (if present e.g. leveldb, redis, memcache etc - perhaps even a DB one could be made)

Things cached in the internal memory caches would need be considered immutable on Get - request level cache could be considered mutable because they're unlikely to affected by concurrency and the external caches would create new structs by default anyway.

So, I am now concerned that this may not be the correct solution at all and I think the issue that this data is being requested over and over again unnecessary. With a little thought we could prevent this problem entirely.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (main@fd89c06). Click here to learn what that means.
The diff coverage is 46.66%.

@@           Coverage Diff           @@
##             main   #21621   +/-   ##
=======================================
  Coverage        ?   48.19%           
=======================================
  Files           ?     1030           
  Lines           ?   140403           
  Branches        ?        0           
=======================================
  Hits            ?    67664           
  Misses          ?    64629           
  Partials        ?     8110           

Impacted Files	Coverage Δ
routers/web/admin/config.go	23.17% <0.00%> (ø)
modules/cache/cache.go	36.97% <25.39%> (ø)
models/user/setting.go	56.09% <52.94%> (ø)
models/avatars/avatar.go	33.62% <85.71%> (ø)
models/system/setting.go	54.59% <86.36%> (ø)
models/system/setting_key.go	100.00% <100.00%> (ø)
models/user/avatar.go	58.20% <100.00%> (ø)
modules/activitypub/user_settings.go	55.55% <100.00%> (ø)
modules/templates/helper.go	46.51% <100.00%> (ø)

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

[zeripath]

1. The length of time these things are cached needs to be considered. These settings should probably be only cached for 5 minutes rather than the 16h that they are being cached for by default. This is particularly an issue with an external cacher as that time will persist across restarts.

Why the time is important need to be changed? I think once the value changed, the cache has been updated.

This is not definitely the case. Imagine a clustered system with a non-clustered cache. Imagine a change to the caching configuration.

...

3. Could we get away without adding this caching at all because we can just get this data out of the db once per request? I suspect we could restructure things to just make getting the settings ONCE per request very easily.

I think no, we can't remove the caching, it has produced actual affect in the main branch especially in dashboard from some users' reports. A memory cache or a redis cache operation will be nanosecond level but a database operation is microsecond level, memory/redis is fast about 10~1000 than database operations. And even in one request, the same setting maybe reused serveral times.

I meant could we restructure this code to only request this only once per HTTP request (eg. per GET/POST level)? There's no good reason to repeatedly get the setting from the db over and over again. In fact it could/would even lead to inconsistent results within the handling of a request if the value were to change mid-way through something.

This is a good example where context passing could be used to ensure that data is cached at a HTTP request level.

[zeripath]

I guess we should approve this PR as it stands as a bug-fix but we really need to address the issues I've noted above.

It's silly that we risk having to keep hit the db like this. There should be a better way of doing this.

[lunny]

I guess we should approve this PR as it stands as a bug-fix but we really need to address the issues I've noted above.

It's silly that we risk having to keep hit the db like this. There should be a better way of doing this.

It's a good idea to cache per request based on context.Context but that could be another PR since it will have a bigger changes.

[lunny]

make L-G-T-M work