Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects)

n * namelen * size / 1024 / 1024 ~= size (MB)

Let's say: 2,000 variables, name len avg ~= 20, size avg ~= 50 (a lot, for the sake of the sample), this is less than 2MB total, not that much (2,000 is really a lot, 1,000 on the sites I usually work with is almost the maximum I get).

I'd say, doing 10 SQL queries to load the lot partially, but end up with 90% of variables loaded in the end will be a performance killer instead of being a real performance gain.

Storing 10 variables: 10 * 20 * 50 = 10,000 bytes is probably less than the core and PHP stack will need to only create and run the single SQL query in order to fetch the data. Storing smaller caches why not, but storing incredibly small caches is a huge error.

I think the development of a new variable system should start with a single cache entry, for the sake of simplicity, and may be cut later after some benchmarking. I'd tend to think that, over all stuff we could legitimately remove from memory, variables is probably the last one we would want to remove for performance reasons.

You are true, but you can still use a partial configuration before actually loading the cache entry. Meaning that, exactly like now, continue to handle to a incomplete $conf array, then load the cache when the necessay systems are up.

#1605324: Configuration system cleanup and rearchitecture will resolve some of this.

But we can probably do more. Thus, moving into core and bumping priority to critical.

I agree with beejeebus, in fact the blobs should be storage implementation detail and not part of the API for the end user, this would make much more sense to have one and only organized tree of configuration variables.

We should create some test scenarios, and process them with multiple variations of the code:
- DrupalCacheArray like caching, one global, entries being filename.key
- DrupalCacheArray like caching, one per path root (as suggested catch, this one is really interesting), entries being filename.key still
- Cache layer per file on top of the active store
- No cache at all

Some interesting measures would be:
- Raw time (and CPU consumption if possible) (min, max, average)
- Memory consumtion (min, max, average)
- Server capacity (requests/second, min, max, average)

Using those 4 possible variations and concrete measures compared we'd have a real idea of the performance at stake here.

Feeding this issue with some data as I noticed config() popping up when profiling the t()/kernel/dic stuff.

With only 18 calls to config so far on the front page as an anon user, config() already uses 9% inclusive call time. Most of that goes to Config::load() with 55% relative call time, followed by Drupal\Core\Config\ConfigFactory::get (init () -> bundle/notification stuff) following up with 25% and then 15% time spent on DIC.

AFAIK most of what's happening inside config() is still a moving target and while we might be able to get rid of the load stuff mostly by relying on a fast cache backend, the notify() stuff takes up almost 40% with load and init combined, which is a lot.

Just a side note, there likely are performance improvements that can be made in EventDispatcher itself, including a few DamZ already highlighted in other issues, and fabpot has said he's very open to those. So when discussing performance improvements, let's keep upstream in mind, too.

Please, let's try a plain static cache in ConfigFactory before we do any major rework...

#1605124: Configuration system performance put a static cache in front of config and it resulted in a big performance increase on reading the same config object again and again.

Perhaps we should wait till #1651206: Added a cache chain implementation and #1702080: Introduce canonical FileStorage + (regular) cache land and then implement a cache chain as part of the new CacheFileStorage class using the MemoryBackend and the DatabaseBackend and then we can get static caching.

#14 sounds reasonable for me

The new config system by using separated "files" (which actually are config namespaces) will by design be slow as long as it loads each file separatly (even if cached, it's still 1 cache request per namespace). The idea of having each file entirely separated instead of having a nice config wrapper (such as config()->get('foo.bar.baz'), where "baz" is the actual file) makes thinks really hard to code a smart proxy implementation that would provide intelligent caching.

The namespaces (e.g. "files") carry in general too few entries, it's way too much exploded, and the result being that the config system will do way too much I/O on files or cache entries: the more we explode the namespaces ("files") the more requests we will do each page build time.

I think it will continue to be slow by design, and only higly unstunainable and complex solutions can improve that. It's by design against performance.

IMO the files backend is OK, but the public API should never have been exposed them to API users, and a more generic API where config is one single huge tree would have been simpler to both manipulate and proxify behing a smart cached implementation.

Re #15 - @catch, Where are the 21 calls to config coming from? Were you requesting the default front page?

If so, that seems like quite a lot. And the number of config files will only go up with some different entities, fields, languages and images on the front page. Wow!

with some different entities, fields, languages and images on the front page

Which will happen on most sites!

Running a fresh 8.x install from git, installed using the standard profile, using xdebug for measures, I got 21 Config::load() calls on the home page, no content, 2nd hit (cache warmed up). The difference is I get 16% of time spent on config() calls, children included. I may say I tested on a WinNT box with a really slow file system which makes the various autoloaders going in front because of file inclusion.

EDIT: Loaded config files are:
system.performance (x5)
system.site (x10)
system.maintenance (x2)
search.settings (x1)
menu.settings (x2)
system.cron (x1)

Storage being used per default seems to be the CachedStorage class. Each load() calls actually triggers a read() on the storage and triggers a cache query: which means that per default each config() call will trigger a real SQL query. On my environment, it's 21 extra SQL queries done just for the config layer, on the default site home page with no content.

This could be improved with static caching, but this probably will make the memory consumption worse: but we cannot guess, so it should be tested and measured.

On a site with a bit of content, set up like this:

drush si
drush en -y devel_generate
drush genu 100
drush genc 100 5

#Fixing D8 devel generate bugs, make sure that something shows up on the front page.
drush sqlq "update node set status = 1, promote = 1"
drush sqlq "update node_revision set status = 1, promote = 1"

config() is at ~10% (14ms) of the page request with 34 calls. 14 calls from image_style_load() make up 40% of those 10%, so yes, we need static caching there. On my system, almost 80% of the time is spent loading the config from the cache backend, not sure how much faster we can make that using a different cache backend. A large part of the other time is event dispatcher.

What worries me is that there are still ~350 calls to variable_get(), 220 of those in t() (custom string override, no idea to what that's going to be converted). So there's a ton more config()'s coming.

Agree with #23, a chained storage sounds more natural to me. The event based approach denotes a highly dynamic system, while configuration should be fixed on a per environement/site basis. The chain seems less brutal and probably more efficient.

Not sure if that can cover the crazy special cases like multilingual domain-access sites that need to switch the language during the request, but it's worth a try.

Also, I did some tests using a hackish port of an Apc Cache backend (I don't think there's anything that can be faster) and I was able to get config() down to 4,5%/5ms, quite evenly split between DIC/EventDispatcher/Load as far as I can see. Also not sure how much of that is xhprof overhead.

Adding a very simple static cache in ConfigFactory::get() as suggested in the comments there gets that down to 3%/3ms and also shows that we're effectively just loading 7 different configs for the 34 config() calls.

Note to investigate: There are hundreds of calls to magic methods on the Attribute classes (__toString() and offsetGet() mostly) that seem to make up 10%+ of the page request together... :(

As Berdir noted (and I suggested 1 month ago):

Adding a very simple static cache in ConfigFactory::get() as suggested in the comments there gets that down to 3%/3ms and also shows that we're effectively just loading 7 different configs for the 34 config() calls.

Really this would be a non existent issue if we had just a plain static cache. How much more complex do we want to make it?

In Drupal 7 we are basically statically caching in different places all the loaded configuration (variables, content types, etc..). Not doing so in D8 is just a huge regression.

Loading 7 config entries is a regression, D7 was loading only one.

Yes, a static cache for the config object creation is a start, but it doesn't help that much, only ~1%. And I'm not sure if there are complications due to things like the DCI, subrequests, testing and what not. Might not even save us that much time if we need to consider these things. Note: This is about instantiating config objects, not *loading* config. There the chained cache is supposed to help.

@pounard: That statement is wrong. Yes, Drupal 7 loads all *variables* at once but it loads a lot more configuration: system table, node types, and so on separately and each has different mechanisms for caching and loading. And yes, most of that needs yet to be converted to config. So there will be many more config entries to be loaded, actually.

It is quite hard to say how this will evolve as more and more stuff is converted to config and how far we can optimize the loading.

Right now, my statement is partly wrong, but I'm afraid that we will continue to do more queries than D7, we already are. The state API and the K/V store will make it even worse if it's being used widely. Plus, as soon as you'll have one config object per module, on a classic site you can easily have 100+ modules (fact I'm really fighting against on all sites I work on) it's not to risky to tell that we could end up with at least 50% of them having a config file, and maybe 20 of those 50 loaded on every page. Considering what is contrib today, and what are the developers habit, I'm really afraid that this way of handling config is an error.

@Berdir,
I think I was a bit confused by your numbers.... Do you have any numbers comparing with/without static cache not using that APC cache backend? (My guess is that will be a major gain).

Really I don't understand what problems we are trying to anticipate for not fixing the real one. Atm it is just like running Drupal without any caching for variables table.

@catch,
If the event dispatcher is the issue when running without listeners (well there may be one, GlobalConfigOverride), then maybe we should really clean up CoreBundle... I don't mean event listeners are the best option, but do you really think that takes longer than a db load + unserialize of each config object? (I really don't know, maybe symphony is that badly broken...)

And I insist in D7 configuration was mostly cached statically everywhere and dropping that static cache without providing an alternate one is just introducing a performance bug.

EventDispatcher is known to be slower than hooks. However, I was talking to Fabien here at Symfony Live and I think there is potential for compiling EventDispatcher, much the same way that we will be compiling the Container. That would make it faster than hooks.

That isn't a deal-breaker here, I just mention it as a side note that events have room to improve in performance and "we're working on it". :-) That shouldn't detract from the other good profiling going on here.

Trying to clarify my statements from above:

There are two different code path's within config() that cost time. Object creation and calling even dispatcher is the first, actually
loading the config and calling event dispatcher again is the second.

The static cache that I added to test the effect was just for the first part, object creation. So the performance gains there have nothing to do with which cache backend is used, the time savings are always the same.

To make loading faster, there are two ways to do it. a) Add a hardcoded "static" cache (could either be drupal_static() or just using properties within a statically cached config class if we have that anyway. b) Rely on the cache chain and whatever backend is used. I'm not sure how much slower the MemoryBackend is compared to drupal_static(). And how much faster than ApcBackend, if you keep your config cache in there.

The reason both approaches would help a lot is that the same configs are loaded many times on the same request, see the list in #20 for an example.

@Berdir Memory cache backend won't be a huge difference compared to drupal_static() it won't be noticeable since it's a matter of only maybe one or two extra function call in a set() then get() scenario. Compared to the gain it will provide compared to do a complete SQL query. In the end drupal_static() does not fit in here because it's global state.

Here is a proof of concept patch that adds hardcoded* cache chain configuration for config to test how this affects the performance.

Seems to get test runtime locally down from 7s for a webtest to 3s. But seeing some failures when running tests locally. Most pass, though.

Haven't yet checked what's causing it. Might be something easy or it might not.

* Yeah, this is ugly. The cache DIC issue which will add a BackendChainFactory which should allow us to make this nicer but still not sure where we'd define the backend configuration. We'll see.

Test run duration: 47 min 37 sec.

And that was before heyrocker's patch landed.

Going to trigger another re-test once the testbot situation has stabilized... now sleep ;)

The test failures are pretty obvious IMHO ;)

This patch adds a cache layer, but does not adjust the Config class to invalidate the cache on every call to save() and delete() :)

I'd think we want to inject the chained cache backend definition into the config factory service, no?

Hm. And another thought: Can't we use a simple in-memory per-request "property" cache on the config factory for this? This is actually where I expect the most gains...?

Bonus: Is the LRU cache backend able to work on in-memory class property data, too? (i.e., allowing us to discard older cached items that weren't recently requested within the same process/request?)

I'd think we want to inject the chained cache backend definition into the config factory service, no?

I think that both cache chain usage (hardcoded or injected) sounds fine. The only real backend we require to be swappable remains the remote one (database, mongo, etc...). Cache chain here is just a normalization of the static cache so it doesn't really mater to have it swappable too.

Hm. And another thought: Can't we use a simple in-memory per-request "property" cache on the config factory for this? This is actually where I expect the most gains...?

Please explain, I'm not sure to understand that one. If you mean a classical static cache as we are used too, I'm not expecting replacing the cache chain by this to be significantly faster.

The test failures are pretty obvious IMHO ;)

This patch adds a cache layer, but does not adjust the Config class to invalidate the cache on every call to save() and delete() :)

I'm not so sure. I'm not adding a new cache layer, I'm changing how the existing one behaves. For example, this works right now in a test:

config('bla');
$this->drupalPost() // changes config
config('bla');

But now it doesn't anymore.

I'd think we want to inject the chained cache backend definition into the config factory service, no?

We do. It's just transparent to it. It's a Reference to cache.config, which calls CacheFactory. I think a goal of the cache chain was to be transparent to the caller, but I'm not sure if that works. For example, I think we really want to be able to say that we want to clear just the memory backend. So maybe add another interface and the modules can optionally rely on that.

I think that both cache chain usage (hardcoded or injected) sounds fine. The only real backend we require to be swappable remains the remote one (database, mongo, etc...). Cache chain here is just a normalization of the static cache so it doesn't really mater to have it swappable too.

That's the question. Do you still want to use memory if you're using the APC cache (not sure if it's a good idea to do that, there's going to be a lot in config. But even if not, you can use the chain with just a single backend, so I guess yes.

#37: config-cache-chain-1187726-36.patch queued for re-testing.

That's the question. Do you still want to use memory if you're using the APC cache (not sure if it's a good idea to do that, there's going to be a lot in config. But even if not, you can use the chain with just a single backend, so I guess yes.

Yes you might be right here, needs some bench and testing thought.

While I've been begging for a while for *any kind of caching* here, I don't think an LRU is the best option because, guess what, it may introduce inconsistencies in how the API behaves:

First case:
1. Function A gets config object.
2. Function A calls function B, which gets the same config object, *same* as it's cached and changes it.
3. Function A, on return from B uses again the config object (which has been updated by function B).

Second case:
1. Function A gets config object.
2. Function A calls function B, that, after some long processing and retrieving a lot of other config objects, gets the "same" config object (but a different object as the original one has been cleaned by LRU algorithm) and updates it.
3. Function A, on return from B uses again the config object (but it's not up to date because it's a different object the one B has updated).

Summary: LRU caching, given it's unpredictable behavior should only be used when really needed, or objects are not updatable (And so far there's no proof of memory getting out of hand here.)

Other options:
- Add the caching in the Storage layer (and make sure we reuse always the same storage object for the same backend)
- Only clean objects from cache when de-referenced (and garbage collected), so objects need to know the factory and inform it on __destroy().
- (My preferred one) Use a plain predictable cache, for which I would simply add an static property to the ConfigFactory.

Please keep it as simple as possible.

- Add the caching in the Storage layer (and make sure we reuse always the same storage object for the same backend)

That's more or less what my patch in #37 does by adding a memory cache to the cache_get() call.

- (My preferred one) Use a plain predictable cache, for which I would simply add an static property to the ConfigFactory.

As said before, config factory and config loading are two separate things right now. First we get a config object from the factory (which invokes an event, which makes it slow) then we call load() (which does load from storage and then invokes an event).

So we can add a simple static cache to ConfigFactory, which is what I did for testing purposes in #9. Without caching load(), that part only makes up a relatively small amount of the time spent in config(). Once you add caching for load(), as I'm doing in #37 (only the storage part, event is still invoked), the factory actually starts to use more time of config(), relatively. So it makes sense to cache that as well.

So..
- in #9, I added static caching to factory, which avoided creating new objects but still loaded them with new data on each config() call.
- in #37, I added caching for the config storage, so we still create new config objects but feed them with the memory cached data on load(), still invoking the init and load events.

It should be possible to combine it, what I'm not sure about is those events. Do we still need to invoke e.g. the load event when loading config storage from memory? If not, then we might want to handle load() completely different and e.g. make load conditional and only call it if we don't have data yet. Which would also mean that there's no need for a cache chain here.

@Berdir,

That's more or less what my patch in #37 does by adding a memory cache to the cache_get() call.

Sorry, I missed that part, by looking at the patch I cannot tell where exactly in config it applies (is it complete?)

I don't think it makes sense to duplicate the caching at different levels, so the idea of caching on storage (only) looks good to me. However if our main bottleneck are events, then maybe we should go for the other option (caching config objects only).

... what I'm not sure about is those events. Do we still need to invoke e.g. the load event when loading config storage from memory? If not, then we might want to handle load() completely different and e.g. make load conditional and only call it if we don't have data yet. Which would also mean that there's no need for a cache chain here.

As the events are atm, yes, we need to call them because they act on the config object itself, not on the loaded data.
Anyway, events could be simplified a little bit -I just don't know how bad the performance loss from event dispatching is- maybe we could save the 'init'

Or maybe, as we finally have plugins, it would be time to drop events in favor of config plugins. Though I don't know how we are about that and the feature freeze timespan...

There's also the issue of the config api going around the factory and using storage directly (which is something to fix IMHO but it looks like other people thinks this is ok) and this would make caching on storage our only option.

About time spent in Config::load() vs. ConfigFactory::get() and possible caching approaches. Tested on a page with 300 comments and 2100 calls to config(), but mostly the same configs, not sure how real-world this scenario is. Cache saves are obviously the more different config files we load on a page. This is probably a very extreme (as in few different configs) scenario :)

Looks like that is relatively stable at 75% in Config::load( 90% of that is config storage load, < 10% the event) and 22-3% in ConfigFactory::get() right now.

With the patch from #37 applied, the numbers shift to 60% spent in ConfigFactory::get() and 30% in Config::load(). (~30% of the previous total time)

When additionally keeping the created config objects in ConfigFactory(), the number again shifts back to 75% spent in Config::load(), pretty much nothing in ConfigFactory::get() (another 50% faster, config() is now at 7% of the total wall time, 33% with 8.x). So this is caching the init event but not the load one, which might be something that is possible.

So, another proof of concept patch, haven't looked at the test failures yet.

@Berdir,
Just some background info:
- While 'init' is done by ConfigFactory, 'load' is done in config(), which ends up in repeating the same operation multiple times for no reason even when statically caching objects.
- Worse, load doesn't take into account whether there is data already loaded or not. And does it over again.

This is maybe why caching on storage is some important saving, but static caching on the container adds very little.

So I think to properly take advantage of object caching, we should fix this before. I'd rather have the Config object doing deferred loading (and only once) when any of the get/set methods are used.

Yes, I understand the reasons, I actually think that the savings in the factory are quite high considering that it does nothing more than instantiating an object and invoking an event.

I do like the idea of lazy loading and keeping stuff in Config object, for a number of reasons.

- It is about as fast as it can get (we can still looking into to caching bigger chunks of data together in storage, CacheArray-like, but that's a separate topic)
- It is easy to understand. The data will be loaded when accessed the first time.
- It is easy to refresh in e.g. tests. Just call load() again yourself. Compare that the previous patch, where the memory cache was deeply hidden and there's no obvious way to clear it (Sure, we could add a clear() to Config, but that would need to go through storage and add knowledge that the storage might do caching to the Config class.
- It makes creating a Config object relatively light-weight so that we can e.g. inject them into services and only load the data when required. We actually do have cases already where we call config() but never use it. ConfigStorageController::buidQuery(), for example.
- In regards to performance, on a frontpage with 10 nodes and 67 calls to config(), config uses 14% of the total wall time. With this patch, config() is 1% and Config::get() is 2%.

What might be a bit more involving here is that there's no flush caches anymore, not sure if we should do something like allowing to empty the config objects in the factory. Running tests works fine here, haven't tried the installer yet. If this works then we should see about the same amount of failures as in the previous patch.

@Berdir,
I think that is really the way to go but one step at a time :-)

About lazy loading I had already this patch (committed but that part was dropped) as @sun had some objections against that, so I guess we should ask him whether he thinks it makes sense now, as a performance optimization, https://drupal.org/node/1671198#comment-6197688
(And whatever we do I'd rather check for isset($this->data) better than having a new property, for some reasons, one of them is modules being able to use setData() and then stop the object from actually loading).

So my idea for now would be to just add the static caching, do the loading on Config::init() and remove the load() from config() function.

What might be a bit more involving here is that there's no flush caches anymore, not sure if we should do something like allowing to empty the config objects in the factory.

Right, and we may need something atm just because objects are created/updated/instantiated without going through the config factory, see the install & update code in config.inc

Also, to be truth I wouldn't like config caching committed at this very moment because that would clash with some other thing we have in queue right now (for which I'd appreciate your review btw) #1763640: Introduce config context to make original config and different overrides accessible
-- Though that may look incompatible with config caching, it's not actually, as we wouldn't need to cache for all contexts, maybe just for the global one, or optionally a per-context cache could be also worked out.

Let me see what I can do.

Ah yes, I think I saw this before, indeed. The fix has a certain 'rhyme' to it: $old_config->load(); $new_config->noLoad();

Hrm, no, disregard that, here is a much prettier fix. Interdiff is against berdir's patch.

Note that this is a band-aid type fix. A broader fix is necessary so that when the storage is written, the corresponding config object is nuked. Perhaps pass the dispatcher into the file storage and subscribe the config factory to it.

Updated patch on which @chx and I worked to fix those tests. Most should actually be passing now if we didn't introduce any new bugs.

Changes include:
- Clearing configs after drupalPost(), same behavior as we do with resetVariables()
- fixed rename and isNew handling in ConfigStorageController. Warning: There's ugly stuff in there, that will need to be moved to a better place, but we're just trying to figure out what's all necessary to get this green at this point.
- Also contains some php 5.4 related fixes that are also in #1856546: config import should use array_diff_key.

About rename, there are two things:
- *Somehow* we need to be able to inform the config factory about the rename, because it's giving us back a renamed object if we request for the old name. There is one very simple solution that I can think of for this. Checking the name in ConfigFactory::get(), something like this:

    if (isset($this->configs[$name])) {
      // The config might have been renamed, only return if the name still matches.
      if ($this->configs[$name]->getName() == $name)
         return $this->configs[$name];
      }
      // Rename it in the array if it's not there already.
      elseif (empty($this->configs[$this->configs[$name]->getName()])) {
        $this->configs[$this->configs[$name]->getName()] = $this->configs[$name];
        unset($this->configs[$name]);
      }
    }

- Wondering why ConfigStorageController needs to take care of the rename manually with delete().. shouldn't this be supported by the config API? (there is a rename in the config storage API...)

berdir discovered that we are much better off if we put the 'throw config objects away' into config sync instead of config import cos then default config installs work. We figured that splitting the timezone info allows for the session system to retrieve timezones before language init. Finally, it seems system_list gets a stale config() object from ... somewhere. I am still investigating where it does come from -- the kernel updates drupal_container which should mean there's a new factory so I do not quite understand wazzup but certainly re init'ing here works. I believe we should go ahead and file followups to this one to clean it up.

Wrong interdiff attached. As you can see, most of it is the timezone split.

On content entities we have a static cache on loaded entities.
Not on config entities, though :

$entity = entity_load('node', 1);
$entity->foo = 'bar';
$entity_2 = entity_load('node', 1);
dsm($entity_2->foo); // Displays 'bar'

$entity = entity_load('view', 'my_view');
$entity->foo = 'bar';
$entity_2 = entity_load('view', 'my_view');
dsm($entity_2->foo); // Property doesn't exist

Shouldn't we aim for consistency across entity types ?
Would mean a static cache at the entity level for config entities, and a static cache at the config() level for non-entity config ?

I would think that unless $entity->save() is called, that the second behavior with the view entity type is actually the 'proper' and expected result in both cases.

Yay, green!

@yched: config() doesn't know what it returns, so I'm not sure how that could work. I agree that static caching for config entities might make sense, atleast for some of them (comments currently don't have it either, but that's a problem because we loud them multiple times now when having nested comments). So introducing it as an option for those which are loaded multiple times (text formats, maybe?) might make sense.

@Dave Reid: That's not an option ;) It would either mean disabling static cache completely or cloning all objects before we return them, which would double the memory footprint of the entity system.

So the reason that module_enable needs an init is because the DrupalKernel update creates a new factory which of course throws the config objects away and config objects become unsynced. As there were talk of persisting database connections across kernel rebuilds I added a persist facility.

#72: 1187726_72.patch queued for re-testing.

Re: #71 / entity caching:
So if I get #1596472: Replace hard coded static cache of entities with cache backends, the goal is to let each entity type control which cache backend is used by entity_load() - none, static, some flavor of persistent, persistent + static...

That would mean that config()-level static cache should not be leveraged for config entities that do have static caching at the entity level ?
As Berdir said, I'm not sure how this would work.
Sounds like a flag on the low-level config get() and set() operations, leaving it to the caller to leverage static caching or not, defaulting to 'yes'.
But ConfigStorage controller having to figure out whether the cache backend used by a given config entity type includes static caching or not sounds tricky.

Weird, passed locally.

It seems to me the problem we are facing now is all the seemingly innocuous decisions made in the past about CMI architecture are becoming a burden now for something as "simple" (it should be simple I guess) as adding a cache into the stack.

However I don't think this is the place to fix them all at once (nor to cry about what it would have been had we ever added the cache first, then the complex parts). I think this should be better the place to create a meta-issue about all these "minor fixes" we need to end up with something that is actually cacheable without ugly workarounds.

Some of these issues we have identified so far are:
1. Config api, or config() doing the data loading outside of the 'factory'.
2. Unneeded loading of data we may not eventually use (aka need for lazy loading of config objects for best performance).
3. Bad API encapsulation, as the config objects get to "rename themselves" (or delete themselves)
3. Way too mixed data/object layers when we've got the config install/update functions accessing the configuration through storage controllers directly bypassing the config factory.

... (add yours here)...?

Then this issue may become eventually a plain "add caching to configuration system" instead of some "trojan horse" kind of issue (of which I've seen quite a few already in D8) that gets to rearchitect the whole thing in order to add some caching to it.

Here's a gentler reset and also much faster because even less work is done now.

After discussion with chx:

We need to rename explicitly instead of implicitly via setName() as the ConfigFactory needs to know about the renaming.

The StorageController does:

* Create New
* Delete Old

The reason for the rename() operation is that we need to take 3 things into account:

* The old object should be available (cloned) at the old slot for the StorageController to delete it.
* The old object needs to be renamed so all instances of this config object are renamed.
* The old object needs to be moved to the correct slot, so that new calls to the new name return the same object.

This patch does that.

Lets see if it passes.

I endorse #83 but I would endorse a followup which cleans up ConfigStorageController so that instead of this postSave thing we just do a $config->rename. I will ask timplunkett or someone similarly knowledgeable about Thingies(TM) to chime in why that's not done.

One part of the fails is that persistIds were not working for compiled containers (ie #77 was a hatchet job).

But, the rdf tests fail but I do not think that's my fault because field_test sets its weight to 1 and rdf is 0. So rdf_entity_info_alter fires first and there are no bundles because field_test_entity_info_alter adds those and that causes the breakage. So, I enforce the right order in field_test. How did these pass beforehands??

Yah! Green!

Question remaining from #83 in IRC was, if we want to rather have setName call back the config factory to inform it about the rename before it happens. In that case we would need to pass the ConfigFactory to the Config objects. But that could also be a follow-up ...

So, I enforce the right order in field_test. How did these pass beforehands??

That is a really good question.

Ok, feel free to ignore #80, I won't take it personally :-/

But really there's an issue to fix before we move on, as someone has created a "future bug report if this gets committed", please see #1859110: Circular dependencies and hidden bugs with configuration overrides

The quick and easy solution would be to flush the config cache right after modules are initialized.

@Fabianx,

Question remaining from #83 in IRC was, if we want to rather have setName call back the config factory to inform it about the rename before it happens.

What about renaming config objects in the Factory? (Add a rename method to ConfigFactory, the factory takes care of renaming Config objects and properly "re-caching" them)

In that case we would need to pass the ConfigFactory to the Config objects. But that could also be a follow-up ...

I like this one because that would allow us to move the event dispatching to the factory itself and this way the factory gets notified of any event related to the config objects it contains (or throws future hooks if we replace events with hooks).
Then we'll take advantage of it for this other issue (of which early version did exactly this, passing the factory) #1763640: Introduce config context to make original config and different overrides accessible

But really there's an issue to fix before we move on, as someone has created a "future bug report if this gets committed", please see #1859110: Circular dependencies and hidden bugs with configuration overrides

The direct bug is resolved by this patch (else tests would not pass) and is the timezone related stuff.

But the real symptom can only be fixed as follow-up in #1859110: Circular dependencies and hidden bugs with configuration overrides.

Ok, feel free to ignore #80, I won't take it personally :-/

I personally found #80 too "meta" and not actionable. Yes, there are problems, but this is IMHO not the issue to investigate and fix them, but to add a cache.

That things are necessary like "persist" in Container or renameConfig() call in ConfigFactory means that caching needs this new primitives, but not that the API per se is bad. Iterative Development with continuos integration ...

And to:

It seems to me the problem we are facing now is all the seemingly innocuous decisions made in the past about CMI architecture are becoming a burden now for something as "simple" (it should be simple I guess) as adding a cache into the stack.

Caching is never easy when dealing with live objects (ever worked with the static node cache in D6 and tried to read after write?) and especially cache invalidation is not:

@see:

"There are only two hard problems in Computer Science: cache invalidation and naming things." -- Phil Karlton

@see http://api.drupal.org/api/drupal/modules%21node%21node.module/function/n... (for the problem in D6 with static node cache and that was seemingly _very_ simple.)

What about renaming config objects in the Factory? (Add a rename method to ConfigFactory, the factory takes care of renaming Config objects and properly "re-caching" them)

Uhm, that is exactly what #83 does, but currently (see interdiff 81-83) the StorageController directly calls the Factory, so you can still totally break the config system by calling setName on a config object. Such the question to either incorporate or it being a follow-up ...

In that case we would need to pass the ConfigFactory to the Config objects. But that could also be a follow-up ...

I like this one because that would allow us to move the event dispatching to the factory itself and this way the factory gets notified of any event related to the config objects it contains (or throws future hooks if we replace events with hooks).

Yes, I think it would be nice if a config object would be able to call its factory for invalidation, etc.

But, are we sure that this is not better in a follow-up?

This patch already is complex as is.

Back to CNR, to get more reviews on #87.

@Fabianx,
Ok, thanks, all that makes sense, I feel better now :-)

Still I insist this is one of that "trojan horse" issues which under some innocent name that looks like "performance / memory optimization" is doing a partial rework of the config stack so, nothing really against the changes but please allow me to change the title to give a more accurate idea of what's going on here.

About the patch only one major issue / complaint / "please don't":

--- a/core/modules/config/lib/Drupal/config/Tests/LocaleConfigOverride.php
+++ b/core/modules/config/lib/Drupal/config/Tests/LocaleConfigOverride.php
@@ -37,7 +37,7 @@ function testLocaleConfigOverride() {
// Spoof multilingual.
$GLOBALS['conf']['language_count'] = 2;
drupal_language_initialize();
- $config = config($name);
+ $config->init();
$this->assertIdentical($config->get('foo'), 'en bar');
}

So please, correct me if I'm wrong but: Are we really "fixing" the test so it doesn't catch a bug introduced by this patch?
(And note it was not me who called it a bug, but #1859110: Circular dependencies and hidden bugs with configuration overrides

No, it's not a bug, it's different semantics. Previously calling config('foo') automatically re-initialized the object now it does not. That's not a bug. That's a performance enhancement which has consequences. Are those consequences dire? Not at all. Most of the time during the course of a request the config objects can only change by saving them and the last few patches worked on making sure that every call of config('foo') returns the same object and so the save affects them all the same so that's not a problem any more. That's why I was not content with #66 -- any random code needing to call config->init would've made life a nightmare. But the code what #91 points to is not any random code, it's a test hacking Drupal to change an override. In this case, re-init is necessary and I do not have a problem with it. A more full test -- much slower -- would use a series of drupalPosts and gets to check this thing works. Other tests do that. Instead we hack, so we hack config too.

The actual caching needs to be a followup, this is just part 1. It's hard enough.

Addressing the point made in #85 about ConfigStorageController doing weird things with save()/postSave() and not using Config::rename():

Config::rename() was added after the fact in #1739900: Add a rename operation to config storage controllers and I don't think we were aware of it when writing ConfigStorageController. So yes, that's a valid follow-up.

1) I wonder whether the idea of persisting services doesn't ultimately get us back to a bootstrap container that is merged into the kernel container. The code involved here looks different, but when taking a step back, the high-level concept appears to be identical.

2) The minimal bootstrap/rebuild ContainerBuilder contains hard-coded definitions for a couple of services, among which is the config service that is forced to use the FileStorage. How do we prevent those from being persisted into the kernel DIC?

3) Isn't the 'persist' tag more or less the same as the synthetic flag for services, just from a different angle?

4) There's a new ConfigFactory::renameConfig() introduced here. When and from here is that called? And what happens to the existing Config::rename() method?

5) Can we name the new ::resetConfig() and ::resetConfigs() methods a bit more clearly? The latter could be resetAll(), the former resetName(). However, the term "reset" doesn't really seem to fit and match what is actually being done. Perhaps reload() and reloadAll().

6) Given that there are rename() and resetOverriddenData() methods on Config object already, I wonder whether it is proper to put this logic into ConfigFactory instead of Config. With this patch, both classes each seem to handle some parts/fragments of the config object management and initialization, loading, event/notification, and override logic, but neither is really responsible. The pre-existing addition of the init() method invocation from the ConfigFactory muddied the waters already. The separation of concerns is no longer clear to me.

7) The tristate $isLoaded flag looks problematic to me, since it is not clear which of the conditions are checking for FALSE and which are checking for NULL, which makes it hard to follow which exact cases are expected to cause an autoload and in which cases the code would actually trigger a recursive load. Can we split that up into two Booleans, $isLoading and $isLoaded?

chx asked me to look over the patch in #87, in particular with an eye toward #97.6.

I don't see much of a problem with the current approach, conceptually. Config objects themselves can get marked "dirty", meaning "needs rebuild". Then they rebuild lazily as needed.

The factory tracks the objects that it has spawned. That's perfectly reasonable, and we do it in a bunch of places. The factory is then able to mark config objects as dirty en-masse.

The only weird bit there is that normally a factory cache would flush the objects entirely rather than marking them dirty. The dirty bit here is just a performance optimization over that, so I think this is conceptually fine.

The only weird bit there is that normally a factory cache would flush the objects entirely rather than marking them dirty. The dirty bit here is just a performance optimization over that, so I think this is conceptually fine.

No, this is not just for performance.

The problem is you can do:

$config1 = config("var");
// some calll changing "var" (like loading a module )
$config2 = config("var");

If we flush the cache, then $config1 != $config2, which led to undefined behavior in the system, while before the config object was always re-loaded.

That led to #81 in the first place and was the reason why #66 passed (#66 did an explicit $config1->init()).

> I wonder whether the idea of persisting services doesn't ultimately get us back to a bootstrap container that is merged into the kernel container. The code involved here looks different, but when taking a step back, the high-level concept appears to be identical.

The fundamental difference is how these are overridden -- you can add a bundle which overrides config.factory and then that config.factory will be persisted across containers.

> The minimal bootstrap/rebuild ContainerBuilder contains hard-coded definitions for a couple of services, among which is the config service that is forced to use the FileStorage. How do we prevent those from being persisted into the kernel DIC?

I have no idea what you are talking about, there's no such thing any more but if you mean the installer container, don't tag them with persist and that's it.

> Isn't the 'persist' tag more or less the same as the synthetic flag for services, just from a different angle?

Not quite, synthetic means you always need to set an object to them otherwise they won't quite work, these are copied from one container to the next but can instantiate on their own just fine.

> There's a new ConfigFactory::renameConfig() introduced here. When and from here is that called? And what happens to the existing Config::rename() method?

It's temporary, see timplunkett's answer (which I presume you havent seen given how close in time the two replies were)

I discussed with Crell and I think the separation of concerns is fine for now.

> isLoaded/isLoading

Also with Crell I will split realData into two functions instead.

And finally, also with Crell I briefly discussed moving the if !loaded load() code into a method and I think we want to punt on that. It can wait even after API freeze cos it's internal and it saves a function call per config call. I know it's microoptimization but there might be a hell lot of these.

I'll try again:

This patch is introducing a bug and changing the test so it doesn't get caught.

See #91, #92 and #1859110: Circular dependencies and hidden bugs with configuration overrides

@chx,
I didn't call it a bug, you did. And no, that is not how configuration translation is supposed to work.
Thanks though, for creating the bug report before the bug actually exists.

Now if I am the only one who finds this really ugly, I won't bother you more and let you go ahead happily with this one. I'll just add less tests in the future for saving the trouble of "fixing" them.

One "dirty" bit more or less won't make a big difference anyway.

There's no new bug introduced here. It's just that most of the time the return value of a config() call from early bootstrap is not persisted and so the bug is not visible. Consider a class that relies on CMI and puts a whole config object (or just values of it) in an object property. If it is used early enough then the translation won't fire on it. The test you are talking of is a quick hack I have written just to make sure the locale override subscriber works. It has little to do with anything else. Perhaps it'd been cleaner to directly call the subscriber than this hack. A more complete test would, as I said, go the long route and issue a number of drupalPosts to add a language and a translation. Then the bug would not have surfaced because drupalPost reinits all the config objects. In fact the test totally accidentally is great because it shows the face of the bug clearly. Writing more tests is always great.

Configuration objects loaded before the language system is initialized cannot be translated, we just need to live with that, and that's not an issue IMO. The same will be true for any other module doing config overrides based on whatever thing before the "thing" is initialized.

Now config objects are meant to be cached, we just need to either translate the preloaded objects or flush the cache and this is what we need to deal with in this very same patch, it's not hard to do and I don't know why we need to make a big deal of it.

Since any other module overriding the global configuration is likely to depend on language too (groups have languages, domains have languages, etc...) I think the right place to flush the configuration cache is right after drupal_language_initialize().

You may or may not like configuration overrides but they were there long before event subscribers were added (they used global $conf before) so this is not an issue with the "new config subscribers", it is a plain caching issue and yes, unfortunately, introduced in this patch.

Call it what you want but this needs to be fixed here and not "fixed" like changing the test to hide it. I mean fixed like getting it working for real. I can do the reroll if you want, I just need to know the fix won't be dropped in the next reroll just because of "semantics".

Then you can open a new one for fixing configuration overrides if you don't like them, sure that's up for discussion. Or you can try just dropping them in this very same patch, that should be ok too.

Dropping features is ok, breaking them is not.

Please let's just get this fixed. Let me know if you want me to reroll the patch and take care of it.

Let me repeat: if you persist a cache object before language is booted it is not translated. Previously, nothing persisted these. Now we do. That's all. Given how the services are available super early it is entirely possible this scenario would've played out with a contrib overriding one of the bootstrap service. Good we discovered the existing circular dependency.

But think of this much simpler example: should an object use t() in its constructor, if it's firing early enough then the data is not translated. What is new here is the long lifespan of objects due the dependency injection container. But the issue is way too complex (circular dependencies usually are) that's why there is a followup.

So, I removed the tristate isLoaded flag and I hope we can commit this and then have a persistent cache as a followup and figure something for the override.

@chx,

Let me repeat: if you persist a cache object before language is booted it is not translated. Previously, nothing persisted these. Now we do. That's all.

Sure, good summary. And that is why you need to fix something that was translated before and now it isn't.

I'm really done with this discussion. This is my final summary abot this patch, to whom it may interest:

This patch gets a big -1 from me because of the following reasons:
- It creates a bug: #1859110: Circular dependencies and hidden bugs with configuration overrides
- Instead of attempting to fix the bug, this patch patches existing tests to hide the bug it creates.
- The above happens not only for the translation tests, it just breaks completely configuration overrides (same trick):

--- a/core/modules/config/lib/Drupal/config/Tests/ConfigOverrideTest.php
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigOverrideTest.php
@@ -64,7 +64,7 @@ function testConfOverride() {
     $this->assertIdentical($config->get('404'), $expected_original_data['404']);
 
     // Reload the configuration object.
-    $config = config('config_test.system');
+    $config->init();

In addition to that, there are many other reasons why I don't like the patch though we could live with these ones:
- It adds an incredible complexity to configuration object loading which was pretty straightforward before.

- It mixes api boundaries, we don't know anymore what belongs to the config object, what to the config factory. It just adds methods here and there so the thing kind of works. Example: ConfigFactory::renameConfig(), Config::rename(), Config::setName()....

- Properties and methods are badly named, which I think is just a symptom of a very poorly engineered feature. Examples Config::isLoaded, Config:isLoading, ConfigFactory::resetConfig(), ConfigFactory::resetConfigs()
Config::setDataReal()... (Did I miss the setDataUnreal() method?)

- Inconsistencies don't get only into the config system, it's even worse, they propagate everywhere. Just one "wtf" example (Now we have two different kinds of containers, cool):

    * Initializes the service container.
    */
   protected function initializeContainer() {
+    $persist = array();
+    if (isset($this->container)) {
+      // This is a non-compiled container.
+      if (method_exists($this->container, 'findTaggedServiceIds')) {
+        $persist_ids = array_keys($this->container->findTaggedServiceIds('persist'));
+      }
+      // This is a compiled container.
+      else {
+        $persist_ids = $this->container->getParameter('persistIds');
+      }
+      foreach ($persist_ids as $id) {
+        $persist[$id] = $this->container->get($id);
+      }
+    }

- Oh, wait, the above was not that bad, it "fixes" WebTestBase itself so tests have a chance to pass:

--- a/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -1227,6 +1227,7 @@ protected function drupalPost($path, $edit, $submit, array $options = array(), a
           $out = $this->curlExec(array(CURLOPT_URL => $action, CURLOPT_POST => TRUE, CURLOPT_POSTFIELDS => $post, CURLOPT_HTTPHEADER => $headers));
           // Ensure that any changes to variables in the other thread are picked up.
           $this->refreshVariables();
+          drupal_container()->get('config.factory')->resetConfigs();

     // Reload the configuration object.
-    $config = config('config_test.system');
+    $config->init();

Well, the comment says it all already. If you want to reload something you need to at least add a reset FLAG like:

     // Reload the configuration object.
    $config = config('config_test.system', TRUE);

or specifically request a reset:

     // Reload the configuration object.
    $config = config('config_test.system')->reset();

or maybe its called reload:

     // Reload the configuration object.
    $config = config('config_test.system')->reload();

or just re-init:

     // Reload the configuration object.
    $config = config('config_test.system')->init();

which is the same as:

     // Reload the configuration object.
    $config->init();

So in this case while the DX might be improved to remove confusion, the change itself is legit.

Inconsistencies don't get only into the config system, it's even worse, they propagate everywhere. Just one "wtf" example (Now we have two different kinds of containers, cool):

The truth is that we already have two different kinds of containers (compiled vs. non-compiled). This is just one of the rare cases where the difference plays a role.

           // Ensure that any changes to variables in the other thread are picked up.
           $this->refreshVariables();
+          drupal_container()->get('config.factory')->resetConfigs();

This is the same story as with the variables. Now you need to only synchronize the vars, but also the config. That again is a legitimate change. A test framework needs to be able to reset static variables. I. e. we also do not remove all drupal_static_reset calls from the Tests, just because they have been added to make tests pass.

In general I agree that the DX of this patch could be improved, which probably would also reduce confusion.

And on the other hand:

This patch needs to get in. There is no way D8 can ship and re-load configuration data every time from disk - one of the most slow subsystems. Such the complexity is needed.

@catch,

Then you need to build the configuration object twice, once before language bootstrap, once after. . I don't see how that can possibly be a good idea when the whole point of this issue is that building those objects is too expensive.

It will still save a lot of object rebuilds (before and after). But anyway, that is not the point. The point is this patch is 1) Breaking existing API and b) Tricking the tests so it gets hidden.

Are you seriously telling me that needing to do $config->init() on any part of the code to get the proper value of a config object is not breaking the existing API?

The difference with $conf is that it's always available before the configuration system is, in fact that's the whole point of it, so there's never been a risk of a circular dependency there.

Then why @chx's patch is "fixing" that tests too?

Really if this patch is a performance improvement, that it is, then I see no hurry on committing it before that imaginary circular dependency is fixed. And I mean FIXED.

We missed the chance of building a simple cache at the very beginning (like I was begging btw) so these issues never happened. Because caching stuff has other consequences and implications.

Since we didn't do it in time now let's deal with that. And tricking the tests so they pass is not doing it.

Are you seriously telling me that needing to do $config->init() on any part of the code to get the proper value of a config object is not breaking the existing API?

Well, we can easily change that to:

$GLOBALS['conf']['language_count'] = 2;
drupal_container()->get('config.factory')->resetConfig($name);
drupal_language_initialize();

or

$GLOBALS['conf']['language_count'] = 2;
drupal_container()->get('config.factory')->resetConfigs();
drupal_language_initialize();

The result for that particular test however is the same as just calling:

$config->init();

So I am not sure to what you are opposed here?

If I mid-request want to change some $conf value I can, but the result is undefined unless I clear the relevant caches.

There is nothing to "trick" tests into working. Drupal never supported changing $conf mid-request in any way and IMHO should not.

For that we still have settings.php and settings should be set correctly from the start.

We are splitting hairs here. We are changing expectations. This is not a bug. I mentioned the problem exists in Drupal 7 just it evidences itself with t(): try creating a t'd string in hook_boot (since t() is now in bootstrap.inc it wont fatal) and then keep it around, say in a static or an object property. Lo and behold , the string won't magically update itself when the language bootstrap happens. So why are expecting config to do the same? Yes this a problem but not one that should keep this patch up. Yes we will suffer from this for long but it doesn't mean it's new.

Right now this issue appears to be doing two things

- Fixing a problem so that we can actually properly add caching
- Adding caching

Can we split the former problem off into a separate issue with its own independent summary, and postpone this one on it? I think that would make reviewing a lot easier and would properly isolate these completely separate issues. I got really confused around #66 when this changed course around what the problem was, and a separate issue with a real summary of its own would help a lot.

#111: That won'r really work.

Spoke with chx.

As a compromise we could do:

* Add a call to drupal_language_initialize() to flush all config objects via drupal_container()->get('config.factory')->resetConfigs();
* Add a @todo to remove that call once #1859110: Circular dependencies and hidden bugs with configuration overrides has landed.

The advantage is:

* Some caching is better than no caching and objects instantiated before drupal_language_initialize() are probably not many.
* The language override test does not need to be changed, which should make Jose more happy
* I think Jose asked several times to flush the config cache in drupal_language_initialize() and be done with that, so adding that should be good

---

Besides that:

We should do:

* Remove isLoading var - with the change to setDataReal this is no longer needed or used.

Additionally we might want to do to make things more clear:

* Add ->reload() method that internally calls ->init(), the code in question would then be

     // Reload the configuration object.
    $config->reload();

which makes much sense to me.

* Maybe change setDataReal to _setData to show that this is a purely internal method. Not sure what the Symfony or Drupal best practice would be.

* We can probably (untested, just thinking) remove the distinction between compiled and non-compiled container by moving

+ $container->setParameter('persistIds', array_keys($container->findTaggedServiceIds('persist')));

directly after the foreach loop that sets the persisted services. Hmm, but maybe that is wrong for the more general case as not all bundles are initialized ... Not sure. It would however probably work fine as long as persist is only set for config.factory.

* Add a follow-up to clean up the new API calls involved (resetConfig, resetConfigs, renameConfig) as part of a larger API re-structuring for Config object
* Add a follow-up to pass the config factory to the config object to be able to call the configFactory->renameConfig method from setName directly

Yes, removing the static makes ~zero difference, there are very few lines that actually add a cache the rest of the complexity just stems from that.

This reroll mostly just removes unnecessary code that accumulated and adds the resetconfigs to language boot.

I would rather not a reload because init and load is not the same -- init is where conf overrides kick in because those are not storage dependent but language fires on load.

I also would not change setDataReal to _setData -- it's protected and that's enough.

I removed the distinction between compiled and non-compiled container, hopefully it works.

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined
@@ -64,26 +66,37 @@ public function __construct(StorageInterface $storage, EventDispatcher $event_di
+  function resetConfigs() {
+    foreach ($this->configs as $config) {
+      $config->init();
+    }
   }
 
+  function resetConfig($name) {
+    if (isset($this->configs[$name])) {
+      $this->configs[$name]->init();
+    }
+  }
+
+  function renameConfig($old_name, $new_name) {

Those methods need a simple docblock.

The pattern used by the entity storage controller is "public function resetCache(array $ids = NULL);". The default NULL argument means clear everything. Do we want to follow that here?

Also, adding all these methods essentially makes this more than a simple factory.. do we need a real interface here so that it could be replaced with a different implementation? Because we are passing this into other services in some cases and that will blow up.... (that is true for all factories that are passed in like that, even if they just have a get method, actually).

Filed followups:
#1865206: Clean up ConfigFactory + ConfigStorageController::save/postsave/rename()
#1865208: Add interface to Config

Consolidated the two resetConfig methods and doxygen'd the new ConfigFactory methods.

Looks good, passes tests, all concerns have been addressed, it has received several rounds of revisions

=> RTBC

Cleaned up a couple of things. With that, this looks indeed ready to go for me.

#118: drupal8.config-cache.118.patch queued for re-testing.

I checked the interdiff and it is fine for me. Bot hick-up was a random fail => Back to RTBC

Only need to profile this now ...

Catch asked for profiling, so this isn't quite ready to go yet...

Just did a quick before and after profiling because chx asked me to. I'm not familiar with the patch so I'm not marking back to RTBC, but the results look quite good to me.

Here's the difference between 100 aggregated xhprof runs with and without the patch. Each run is a view showing 10 nodes.

14% faster should be quite enough to convince anyone. The number of load calls dropping to a fifth speaks volumes. And we are not even finished with CMI conversion.

I finally got to benchmark this as well:

100 nodes on frontpage via /node using my find-min method.

=== core..core--1187726--core compared (50cac2c37c989..50caca1db98a5):

ct  : 189,269|189,286|17|0.0%
wt  : 701,671|701,071|-600|-0.1%
cpu : 700,043|696,044|-3,999|-0.6%
mu  : 13,522,064|13,507,224|-14,840|-0.1%
pmu : 14,537,784|14,522,888|-14,896|-0.1%

=== core..core--1187726--118 compared (50cac2c37c989..50caca4218f53):

ct  : 189,269|157,706|-31,563|-16.7%
wt  : 701,671|551,060|-150,611|-21.5%
cpu : 700,043|552,035|-148,008|-21.1%
mu  : 13,522,064|13,580,456|58,392|0.4%
pmu : 14,537,784|14,582,160|44,376|0.3%

---
fc = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

First run shows comparison with baseline (same branch, core unchanged), second shows vs. #118.

So about 20% improvement, while only needing 0.5% more memory. Thats nice :).

X-POST: This is of course RTBC.

Could we just add a word of explanation about why this was needed ?

+function field_test_module_implements_alter(&$implementations, $hook) {

Also, I guess there should be a "@todo Remove when http://drupal.org/node/1822458 is fixed" ?

@yched see #87. Should we roll a patch?

Yeah, let's add a comment please.
(I'll be bold and put straight to RTBC)

Note : #131 & #132 are two different patches :-)

In #1872522-12: Compiled data in PHP storage is cleared too late in drupal_flush_all_caches(), I'm getting a test failure as part of a larger patch that includes some changes to the $persist logic in DrupalKernel::initializeContainer() that was introduced in this issue. To isolate the problems in that issue from the general cleanup of $persist, here's a patch for just the general cleanup, which I expect to pass tests. I'm posting here, since the interested parties are already on this issue. Let me know if I should open a spin-off instead.

Do not override services explicitly instantiated on the new container prior to the invocation of this function.

I would like to hear more. How on earth can that happen when we call this from initializeContainer?

In the other issue, we also add synthetic services to those that are persisted, which includes, for example, both 'request' (which we do want persisted), and 'service_container', which is set in Container::__construct(), and we do not want that overridden with the prior container. However, rather than special casing 'service_container', I think it's more robust to assume that any instance that already exists in the new container should take precedence over instances from the old container.

Well then why don't you say so :) ? I've rewritten the comments in the new methods, no other changes.

Thanks. Back to active per #134.

Persisting the config factory but not its dependencies introduces an interesting problem. Please see #1878512: Clean up Simpletest's kernel/container preparation/rebuild logic now that the kernel is responsive to module enabling for details.

Another optimization: #1885830: Enable static caching for config entities

This looks like another issue that is made obsolete by #2161591: Change default active config from file storage to DB storage

Just because it's in the database now by default doesn't mean that we no longer need caching if someone uses a different cache backend for example. There are now dozens of queries against the config table.

I don't think we have any integration tests right now that check if that's actually still possible, but that should probably be a separate issue and I know that I will try it, so I will see what will happen then ;)

And topics like pre-loading are still relevant, as we're starting to figure out that we can't switch to APC/phpstorage automatically.

doesn't mean that we no longer need caching

@berdir, Is it still relevant? Right now active storage is wrapped by CachedStorage.

Anyway can we close this issue? It seems rather outdated. The summary and discussion mostly about variable system which was removed from Drupal 8 a long time ago.