Redmine

?? :-)

how were you even able to update this ticket?

Chiliproject looks to add a lot of code complexity and we should not need to change tests, Jan's patch, on the other side, is pretty clean.

Should settings be cached separately or could the full Hash of Settings be cached instead ?

Jean-Baptiste Barth wrote:

There's at list one possible gotcha with this: Rails.cache defaults to a file backend which leads to potential problems if permissions are not set correctly, or, say, the production server disk is full (which never happens hey?).

In those cases you'll have this behaviour: [...]
... which will break the application completely, whereas the current code would work.

Also it seems even Rails will issue a "read" operation on the file each time we call Rails.cache.read/fetch. Maybe not a good idea on instances with lots of users, this will lead to unecessary IOs.

Oh, I was under the impression that the default store would be MemoryStore, but I can confirm it's FileStore in a vanilla Redmine - you're absolutely right. However, Redmine already uses Rails Caching since r10844. So, I would say, it wouldn't hurt to make more use of it.

Otoh I admit it would be nice to rely on Rails.cache if we configure a memory store or a smart database (e.g. memcached or redis).

Ideally, I'd like to make this optional. What do you think ?

Hmmmm, personally I would not make this optional. I think the current caching mechanism of the Setting model might be a relic from times where Rails Caching wasn't available, and that should be replaced. To make it more obvious that caching is used there and less confusing for cases where in-memory caching is not the best option, I would really like to move this to Rails.cache.

I agree that a default to MemoryStore for Redmine in general may be a good idea. But then again, it may also be nice just to follow Rails' defaults. I think users who deploy Redmine in production will have their own opinion on Caching anyways and will want to define their favorite cache store. (And if they do, I think they would be confused by the fact that the current Setting cache does not follow their configuration for Rails.cache but uses its own in-memory mechanism...)

The Rails default :file_store seems to be OK for most use cases but I agree with Jean-Baptiste and would definitely not use it to cache settings that are heavily used (some requests generate > 1000 setting reads).

And the problem with the proposed patch is that it doesn't work if the cache is not shared between Redmine processes (eg. if using :memory_store):
redmine-settings-cleared-on is not initialized and Setting.check_cache never clears the cache (redmine-settings-cleared-on would only be set in Setting.clear_cache).

From a performance point of view:

With patch applied:

irb(main):004:0> Benchmark.ms {1000.times {Setting.app_title}}
=> 218.401

With patch applied and Rails cache store set to :memory_store

irb(main):004:0> Benchmark.ms {1000.times {Setting.app_title}}
=> 31.2

With current implementation:

irb(main):004:0> Benchmark.ms {1000.times {Setting.app_title}}
=> 0.0

Jean-Philippe Lang wrote:

From a performance point of view: ...

Cannot argue with that. 0.0ms is a very convincing argument ;-) In that case, using Rails.cache would maybe only be beneficial if it's in fact being shared between several processes or app servers. In that case, maybe coming back to Jean-Baptiste's proposal to make this optional could be a way forward.

Of course, if you feel that this doesn't really benefit Redmine, we can also just close this issue...