View Issue Details

IDProjectCategoryView StatusLast Update
0021604mantisbtperformancepublic2020-03-19 12:28
Reporteratrol Assigned To 
PrioritynormalSeverityfeatureReproducibilityhave not tried
Status newResolutionopen 
Summary0021604: Discuss recursive configuration options
Description

The discussion started as a side note to PR https://github.com/mantisbt/mantisbt/pull/840

atrol
A bit off topic.
I don't like to introduce one more of this '%var%' stuff.
I would like to discuss removal of recursion in defining configuration variables.
I don't see that much value in it and would expect a better performance if config_eval would be removed and
config_get_global and config_get would be simplified.

dregad

I don't see that much value in it

It makes configuration easier for admins, by avoiding the need to duplicate declaration of several related options. Consider the case of paths, or $g_cookie_prefix for example. Without the '%%' feature, the admin would have to manually set each of the cookies names in their config_inc.php.

You will probably and rightly argue that we do not need to make the cookies names configurable. Of course by refactoring the code (in the above case, getting rid of these config options), we can implement alternative that works without the recursion.

The questions are, whether this is worth the effort and what kind of performance improvement we can obtain by removing this feature.

expect a better performance if config_eval would be removed and config_get_global and config_get would be simplified.

If this is important to you, I would suggest to open an issue in the tracker, and evaluate how much we'd gain with your proposed approach.

evaluate how much we'd gain with your proposed approach.

atrol
Quick and dirty removal of config_eval
https://github.com/atrol/mantisbt/tree/config_eval_remove

There is a bit less memory usage (removed the caching of the evaled options) and there is about 7% better performance in 2nd benchmark, see results below.
Not outstanding, but not that bad for such a small part of the code.

Do you think it's worth to think more about it (obsolete cookie seetings, ...) and target it for a version > 2.0?

My View 2nd call

Original master-1.3.x
Page execution time: 0.3540 seconds
Memory usage: 10,411 KiB

Removed config_eval
Page execution time: 0.3499 seconds
Memory usage: 10,388 KiB

View Issues 2nd call

Original master-1.3.x
Page execution time: 0.2916 seconds
Memory usage: 10,514 KiB

Removed config_eval
Page execution time: 0.2705 seconds
Memory usage: 10,441 KiB

dregad

about 7% better performance in 2nd benchmark

7% is not bad indeed, I did not expect that much actually. But keep in mind that in absolute terms, it's still less than 0.02 s gain.

I'm still concerned about the loss of "just in time" evaluation of configs however, especially for paths that are quite likely to have been changed by admins - we need to be very careful not to cause regressions. Consider this example:

_config_defaultsinc.php

$g_path = (dynamically built from $_SERVER) // e.g. http://internal-hostname.example.com/mantisbt/
$g_icon_path = $g_path . 'images/';

_configinc.php

$g_path = 'http://example.com/mantisbt/'; # required e.g. due to use of reverse proxy

In this case, $g_icon_path == 'http://internal-hostname.example.com/mantisbt/images/, which is invalid in the reverse-proxy scenario; this would force the admin to manually define all variables referencing $g_path (formerly %path% in their config_inc.php (and knowing that they have to do so). Not very user-friendly, and likely to cause support issues when upgrading.

Do you think it's worth to think more about it (obsolete cookie seetings, ...)

Yes definitely. And get others' opinions too.

target it for a version > 2.0?

For sure a 2.x target, if we decide to move forward.

TagsNo tags attached.

Relationships

related to 0026798 closeddregad PHP warning in config_get_global 

Activities

atrol

atrol

2016-08-14 09:06

developer   ~0053790

7% is not bad indeed, I did not expect that much actually. But keep in mind that in absolute terms, it's still less than 0.02 s gain.

0.02 s is not that much, but In best case it means 7% less payment when using CPU based cloud services, or being able to serve 7% more users on the same hardware, or about 2% less power consumption, ...

cproensa

cproensa

2016-08-14 11:06

developer   ~0053793

I have never needed to use recursive configurations, didn't meet the use cases, so i cant provide a precise opinion.

However, it has been talked about changing the configuration system in some ways: json based, in-database, hierarchical properties, etc.
Maybe it's worth to consider all these proposals as one common milestone.

atrol

atrol

2016-08-14 16:00

developer   ~0053797

Maybe it's worth to consider all these proposals as one common milestone.

It is, but we are not that good in dealing with bigger conceptual changes.
It's better to complete small changes instead of having bigger changes nearly completed.

cproensa

cproensa

2016-08-14 17:57

developer   ~0053798

It's better to complete small changes instead of having bigger changes nearly completed.

true!
i'm only refreshing related ideas.

anyway, i would not hurry this proposal, if the only benefit is very small
Let's be wary with that measured 7% gain. Is it O(1), or O(n) related to bug count?
Simplfying code is harder to measure, but, there also exists already enough ugly code, ahem, filter api, or convoluted: Reflection

If going by the numbers, for my view page: see 0021044 for my measure of 75% gain, or 0020138 for 68% gain in view all page (in my specific instance)