Optimize get() function in Gdn_Configuration class

[alex-mtl]

I have around 4K calls to this function.
I doubt that we have to call it that many time and/or we can optimize the performance.
array_key_exists calls inside that function is clearly detected as one of weak points by blackfire

[bleistivt]

A general observation:
The Configuration class should hold a flattened version of the configuration array.
For every call something like Garden.Embed.Allow gets parsed into parts and then iterates through the configuration similar to valr().

But the large majority of configuration calls do not use the array-functionality (e.g. fetching the c('Garden.Embed') subset)

In fact, I would advocate for making the sub-array functionality more explicit. If you are expecting an array of config values, it should be something like c('Garden.Embed.*').

Gdn_Configuration::get() could then just use default array access if no asterisk is found.

[bleistivt]

I've done some tests. Around 70% of Gdn_Configuration::get() calls return the default value.
So caching which keys do not exists in the configuration is probably the easiest save.

I also see a lot of repeated calls for 'Garden.Deployed' in assetVersion() and 'Garden.Format.WarnLeaving' in Gdn_Format::links().

Edit: The overall number of calls on a standard install is more around 500 - 1000 for me. Memory caching nonexistent values as mentioned above didn't really make a measurable difference.

[alex-mtl]

@bleistivt do you have access to our branches other than Master?
Oh, I just checked they are open even to public, so you should be able to checkout that optimization branch: feature/optimization-7618-4
I am not sure when those changes will be merged into Master, but you can get an idea of what kind of performance you can get. Any input and/or tests are very welcome.

So far I have 2x speed up on my test home page.

[bleistivt]

Looks like my local setup is mostly IO bound. I'll have to try it on a better server to check the speed.

Call to undefined method DropdownModule::tAll()

Probably introduced here:
e52d21

Apart from that no problems, everything works the same.

[alex-mtl]

@bleistivt Sorry for that. I just pushed few updates - should not fail with tAll() anymore

[bleistivt]

What's the intended upgrade path? Importing the trait and replacing all t() with self::t()?

Everything up to this point looks pretty safe and didn't break anything for me, so good job. But what about permissions and config values? Plugins sometimes change these mid-request to trigger a certain behaviour. Will this be reflected in the cached version?

The data flow to the cache traits is currently unidirectional. How are changes in the source (config, permissions) handled?

[bleistivt]

vanilla/library/Garden/Translation.php

Line 30 in e52d21f

return Gdn::translate($key);

The translation default isn't used very often (and probably not encouraged anymore?) but shouldn't it be passed along anyway?

[alex-mtl]

The data flow to the cache traits is currently unidirectional. How are changes in the source (config, permissions) handled?

This trick with static cache is to apply where we don't expect data to be changed or generated during php execution. It expects data to exist at the moment php gets request and not changed during the execution. That is one of the reasons why trait seem to be convenient solution. So if we have some config values or translation exist in our environment and we expect them to be "static" during the php code execution - we can apply these traits.

I did expect that Permissions are "static". But realized that some permissions are "dynamic/mutable". And when I did try to cache them on very first calls - later some plugins failed because of wrong cached values. I did revert that approach since found some dynamic changes in the code and at the moment I have no solution for those.

I hope Configuration has no dynamic/mutable part, but probably I am wrong. That is why I do not apply all this static cache globally but try to apply it locally on some classes and some values I can see and prove are "static"

[charrondev]

Configuration is mutable. Passing true as the 3rd parameter to get() will update the configuration value in memory. This type of optimization can also make testing quite difficult as it requires resetting the environment entirely to clear out old configuration values.

I'm closing this for now because StaticCacheConfigTrait seems to be enough to optimize hot spots in static methods.

If you are in an instance method I would recommend dependency injecting your configuration values with ContainerUtils::config()

https://docs.vanillaforums.com/developer/framework/dependency-injection/