CachedConfigCollection is written to disk on every page load

[kinglozzer]

Just something I spotted while doing some profiling, not a major problem but AFAICT it’s an unnecessary filesystem operation if nothing in the cache has actually changed

https://github.com/silverstripe/silverstripe-config/blob/1.0.10/src/Collections/CachedConfigCollection.php#L150-L162

PRs

• FIX: Prevent unnecessary cache writes (fixes #34) #58
• CachedConfigCollection is written to disk on every page load #89

[brettt89]

For users who have OPCache enabled but don't have APCu, this causes Wasted Memory within OPCache which eventually causes either full memory or opcache resets to free up the wasted memory.

https://github.com/silverstripe/silverstripe-framework/blob/master/src/Core/Cache/DefaultCacheFactory.php#L68

PHPFilesCache class uses PHP code to cache data in files located within the TEMP directory. OPCache will cache these files and their content if able. CachedConfigCollection performing a write to this cache on __destruct (every request) results in OPCache invalidating the previous version, adding to the 'Wasted Memory'.

As OPCache is not able to fragmentate its invalidated cache items, the only way it can clear this is by resetting opcache. As a result, the memory consuption of OPCache constantly increases with every request to the application until is consumes all memory made available.

If OPCache is configured correctly, it will automatically reset to free up this 'Wasted Memory' (which has consumed the majority of memory) and has to reload the cached items again. If this happens during a high-load event it can cause OPCache to not be able to cache many of these items resulting in a stampede.

Recommend either removing this entirely, or only updating the cached information if it has actually changed. (If this happens during a flush, then we can maybe just remove it entirely?).

[sciutand]

just found this as well..... 20ms delay.. with APCu it is also an issue as it cause 100% fragmentation after a while (depending on how many requests you cater for of course). Once you reach 100% your cache can no longer write.

[kinglozzer]

I’ve been playing around with some XHProf benchmarking around this today. The existing write-on-destruct behaviour allows the cache to store computed values (MemoryConfigCollection::$callCache) with/without middlewares applied. I think to remove the __destruct() behaviour without risking a performance hit, we’d need to introduce some hydration logic that can populate those cached values on dev/build.

[lekoala]

maybe we can do something like, the only price to pay is to store a copy of the serialized config. that seems a much lower price than triggering a save, and when we have multiple requests incoming (ajax, etc) it may make quite a difference.
working with xdebug is a real pain at the moment due to the symfony var exporter being called so many times. that config object tends to become a wild beast :-)

    public function __destruct()
    {
        // Ensure back-end cache is updated
        if ($this->collection && $this->cachedCollection) {
            if (serialize($this->collection) != $this->cachedCollection) {
                $this->cache->set(self::CACHE_KEY, $this->collection);
            }

            // Prevent double-destruct
            $this->collection = null;
        }
    }

[sb-relaxt-at]

Seems fine to me, we have a patch doing almost the same in use for several months in several projects. Two small differences:

• We are storing a hash of the serialized config as the config might be quite large. Just a computation versus memory trade-off.
• We weren't storing the cachedCollection/hash on a cache miss in getCollection, but it seems to be the right way to do so, as it may save another write on cold caches.

[lekoala]

a hash seems like a better way. did you just md5 the whole serialized object? i'm thinking we can probably even just json_encode the whole thing since it's only storing "simple" values that can be expressed in yml

[sb-relaxt-at]

Yeah, we just did it this way:

md5(serialize($this->collection));

Edit: Good point, json_encode might be an alternative here. From my limited testing it is about 10^4 times faster. Although both are quite fast, for about 610K stored config on disk, md5(serialize(.)) needs 1-2ms

[lekoala]

i've updated the pr
didn't find a significant difference that justify using json encode in my test project

[kinglozzer]

just found this as well..... 20ms delay.. with APCu it is also an issue as it cause 100% fragmentation after a while (depending on how many requests you cater for of course). Once you reach 100% your cache can no longer write.

We’re running sites on servers with APCu enabled and are experiencing the same - cache fragmentation starts out low, but as requests ramp up it trends toward 100%. Once it reaches 100%, it’s no longer possible to write to the cache: krakjoe/apcu#369 (comment)

It’s probably exacerbated by this issue, but I wonder if we should stop defaulting to using APCu? It seems it doesn’t cater well to a large number of small files, which is what caches in Silverstripe are primarily used for

[lekoala]

:-) good thing the solution is almost there then

[emteknetnz]

Linked PR has been merged, it will be released as part of 2.1.0