I started this as a comment on http://groups.drupal.org/node/155559, but chx suggested it'd be better as an issue.

If I was writing it as an issue I'd do it differently, but since this is already written, posting as is. This is more about some of the assumptions of the groups post rather than the overall architecture, in general I like the architecture, but by itself the fact that there are config objects and they're lazy loaded is not necessarily going to solve the current performance issues compared to the variable cache - we may need to add a layer in between to make that slimmer on both counts.

The advantage of storing configuration this way is that rather than the current system where the entire contents of tha variable table is loaded into memory on every page request, thus limiting the data that can be stored there, we can instead target loading of only specific parts of configuration we need for the current request.

The main reason the full contents of the variables table is loaded on every request is because we can't distinguish between data that is set to the default, and data that is not set at all with the variable API. There is not really that much stopping it from individually reading variables from the db except for performance - but the fact it's all stored in one table is not the reason it is such a bad fit for stuffing lots of data into, there are multiple reasons that it's bad as it is currently. With files there is a good reason to split them up (no row-level locking on files), but if they are only read from when being synced with the active data store or diffed, then this really applies to the active store rather than the file store no?

That follows on to this:

inally, go to admin/configuration/system/config or run drush config-update (note: final location/command likely different; making these up right now). This will outline the differences between on-disk and the active store. If the changes seem good, go ahead and confirm to overwrite the content of the active store with the on-disk configuration. The admin tool will offer to regenerate the file signatures if necessary.

chx said in irc that this will be a list of config object keys, and then individual values will be diffed, if this is the case then that sounds fine to me, I would be concerned about a rebuild process that requires loading the full contents of the file store + active store into memory and diffing them.

While at first glance the structure looks like the "variables" table in Drupal 7 and below the fundamental difference is this table stores configuration objects (say, every site information setting: name, mission etc) the variable stored single values (like the site name). Also, as said above, we are not loading the whole table in memory.
All site configuration data gets read out of this wrapper.

Not loading the entire table into memory goes without saying, but there are other concerns here:

Unless I'm misreading, every 'default' value for variables will be stored here, not just those overriden from defaults. So if you have a large site config object, just to get the site name, you're going to need to load the full site_information object into memory each request - which on many sites has null values for mission statement, 404 page etc. as well as values like site e-mail address which are not usually needed each request.

A few different large-ish config objects with all their default properties could end up outweighing the memory savings of not loading the variables table into memory each request.

During a standard D7 page request with no contrib modules installed, there are something like 60 variables requested, these would be from multiple different configuration objects. Let's say 10 configuration objects with 6 variables each (although the actual object may encapsulate 10, 15, 20 different properties which are also going to be loaded).
Core currently writes around 100-200 variables to the db if you install and click around a bit.

So that's an exchange of one big cache item with an array of between 100-200 variables, to maybe 10 database queries which will be objects with 10-20 properties each (some NULL or default strings), there is not necessarily going to be a memory trade off there in practice and there is definitely going to be a cost in terms of database i/o.

With a lot of modules enabled on an older site, D6/7 can easily get to 1-2,000 variables stored in the database - let's say 50 installed modules have one variable they check on every request from their own config object, reading an object with 10-20 variables for each of those 50, and it's in the same range even with the lazy loading.

I'm hopeful that the CacheArrayObject stuff I'm working on would provide a way to reduce the memory footprint of both the current variables table in D7, and it may apply to what goes on here too (for both memory and reducing overall i/o for reads). It's very much going to depend on what get stores where as to how much of an improvement there might be (or not) with the new system, but I don't think it can be taken as a given that it's going to be better from a memory and i/o standpoint if the default behaviour is to load a full config object with all declared variables when one is requested, there may need to be an extra layer to smooth things out.

Files: 
CommentFileSizeAuthor
#138 1187726_138.patch3.5 KBchx
PASSED: [[SimpleTest]]: [MySQL] 49,947 pass(es). View
#138 interdiff.txt1.04 KBchx
#135 1187726-135.patch3.52 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 49,828 pass(es). View
#132 1187726-132.patch534 bytesbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 49,339 pass(es). View
#131 field_test_module_implements_alter-comment.patch822 bytesyched
PASSED: [[SimpleTest]]: [MySQL] 49,355 pass(es). View
#124 configcache_total.png35.67 KBmsonnabaum
#124 configcache_config_load.png75.06 KBmsonnabaum
#118 drupal8.config-cache.118.patch16.91 KBsun
PASSED: [[SimpleTest]]: [MySQL] 49,370 pass(es). View
#118 interdiff.txt7.91 KBsun
#116 1187726_116.patch16.96 KBchx
PASSED: [[SimpleTest]]: [MySQL] 49,266 pass(es). View
#116 interdiff.txt2.5 KBchx
#113 1187726_113.patch16.64 KBchx
PASSED: [[SimpleTest]]: [MySQL] 49,329 pass(es). View
#113 interdiff.txt2.19 KBchx
#104 1187726_104.patch16.59 KBchx
PASSED: [[SimpleTest]]: [MySQL] 49,234 pass(es). View
#104 interdiff.txt2.22 KBchx
#87 1187726_87.patch34.22 KBchx
PASSED: [[SimpleTest]]: [MySQL] 48,894 pass(es). View
#87 interdiff.txt1.95 KBchx
#83 core--Memory-usage-from-CMI--1187726--83.diff32.78 KBFabianx
FAILED: [[SimpleTest]]: [MySQL] 48,858 pass(es), 39 fail(s), and 15 exception(s). View
#83 core--Memory-usage-from-CMI--1187726--interdiff--81-83.txt2.51 KBFabianx
#81 1187726_80.patch32.61 KBchx
FAILED: [[SimpleTest]]: [MySQL] 48,467 pass(es), 50 fail(s), and 15 exception(s). View
#81 interdiff.txt1.7 KBchx
#77 1187726_76.patch31.72 KBchx
FAILED: [[SimpleTest]]: [MySQL] 48,272 pass(es), 46 fail(s), and 86 exception(s). View
#77 interdiff.txt1.13 KBchx
#72 1187726_72.patch32.48 KBchx
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found. View
#72 interdiff.txt2.65 KBchx
#67 interdiff.txt19.16 KBchx
#66 1187726_66.patch31 KBchx
PASSED: [[SimpleTest]]: [MySQL] 48,916 pass(es). View
#66 interdiff.txt5.05 KBchx
#64 config-cache-test-fixes-1187726-64.patch11.54 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,256 pass(es), 27 fail(s), and 75 exception(s). View
#62 1187726_62.patch6.4 KBchx
FAILED: [[SimpleTest]]: [MySQL] 48,260 pass(es), 780 fail(s), and 143 exception(s). View
#62 interdiff.txt1.75 KBchx
#60 1187726_60.patch4.98 KBchx
FAILED: [[SimpleTest]]: [MySQL] 46,574 pass(es), 1,550 fail(s), and 322 exception(s). View
#60 interdiff.txt438 byteschx
#59 1187726_59.patch5.21 KBchx
FAILED: [[SimpleTest]]: [MySQL] 46,740 pass(es), 1,567 fail(s), and 332 exception(s). View
#59 interdiff.txt767 byteschx
#55 config-cache-lazy-1187726-55.patch4.7 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#52 config-cache-chain-1187726-52.patch3.32 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 48,039 pass(es), 742 fail(s), and 65 exception(s). View
#37 config-cache-chain-1187726-36.patch1.17 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 48,057 pass(es), 730 fail(s), and 66 exception(s). View
#16 Workspace 1_092.png136.19 KBcatch

Comments

pounard’s picture

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.

catch’s picture

The site I usually check for this example has over 2,200 variables stored, this is after a couple of culls of stale variables. In xhprof this comes in at 1.6mb.

Most sites tend to get to more like 1,000-1,500 plus once they've been running for a long time - so closer to 1mb or less.

However, the idea of this system is to take over other configuration - fields, instances, system table, possibly things like node types - all sorts of stuff could be moved to this (views?) - in that case, you absolutely can't be loading everything in one go, but there is still a trade-off (and even stuff that used to be in variables is going to be split up so the argument applies for a straight conversion as well as when you bring other things into it).

pounard’s picture

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.

catch’s picture

I would agree with one cache entry during any one request, but I would want to seriously look at caching based on content type, request method, and possibly very low level context like path root ('node', 'admin' etc.).

There's also the issue of whether the config system has to be initialized to serve cached pages or not, in core at the moment you can bypass variable_init() and hook_boot() - so the only required hit to cache is the actual cache_get() for the page itself and the database may not be initialized at all. It would be a shame to lose that.

pounard’s picture

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.

sun’s picture

Project: Configuration management initiative » Drupal core
Version: » 8.x-dev
Component: Code » configuration system
Priority: Normal » Critical
Issue tags: +Configuration system

#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.

beejeebus’s picture

i'd like to see us store each value against a filename-name compound key by default in the active store, and get rid of per-file blobs.

i wrote up some reasoning, mostly around races on writes, where per-file blobs just suck in general: https://drupal.org/node/1560060#comment-6038254

having all values stored by key will make it easier to cache 'used on every request' keys and front load them. this cache layer will be most efficient if it stores values across different files, so there'll be a complexity cost working around the 'files are always in your face API', but i think it would be worth it.

the end goal being we front load all variables that are used on all pages (or some variation), so we have a small number of cache queries and a small amount of data loaded into memory.

pounard’s picture

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.

Berdir’s picture

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.

catch’s picture

I assume the notify stuff is for the listener overrides? If it's that bad we might want to switch it to chained backends (so $conf -> locale -> database) maybe?

beejeebus’s picture

@catch: i'm planning to start profiling / benchmarking this code for real once #1702080: Introduce canonical FileStorage + (regular) cache lands.

at that point we'll kind of have the final big architectural pieces in place, so it will be worth looking at perf in more detail.

Crell’s picture

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.

Jose Reyero’s picture

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

alexpott’s picture

#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.

podarok’s picture

#14 sounds reasonable for me

catch’s picture

FileSize
136.19 KB

With the new file + cache implementation, config is still 20% of wall time (over 23ms on my machine) and nearly 1mb of memory on a default install. Attaching xhprof screenshot.

pounard’s picture

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.

Lars Toomre’s picture

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!

pounard’s picture

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

Which will happen on most sites!

pounard’s picture

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.

catch’s picture

We can do static caching via #1651206: Added a cache chain implementation, bumping that issue to major.

Berdir’s picture

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.

catch’s picture

I'm quite concerned that we added event dispatcher to config for overrides. We'd never have added an alter hook to variable_get(), and overriding variables could be done by a chained storage which should have less overhead.

pounard’s picture

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.

Berdir’s picture

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... :(

Jose Reyero’s picture

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.

pounard’s picture

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

Berdir’s picture

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.

pounard’s picture

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.

Jose Reyero’s picture

@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.

Crell’s picture

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.

beejeebus’s picture

re #31 - can we please not consider that as a useful idea until we have no other choice? i know we've already accepted that D8 will suck at performance without generating php cache files, but the more bits that rely on that the scarier it is. say i have a big site that runs on N web heads - with D7, i never, ever have to worry about the characteristics of my shared file system for cache behaviour. with D8, it seems that i have to worry about it. a lot. because if this cache gets stale, my site could break.

re. the GlobalConfigOverride - we can probably move that out of listeners and into a parameter that we pass to config objects __construct.

re. cache chain - can someone explain how that will help? not saying it won't, i'd just like to see how that will play out here.

re. my post in #7 - i don't know if that is going to be worth it.

catch’s picture

Yeah I'm not overly keen on compiling to disk for everything either, we have the PHP writer in core but it's very, very new. However it'd be worth keeping that in mind as well, also let's please not say things like 'faster than hooks' until it's been written and benchmarked...

On the cache chain the idea is this:

- the default configuration for the config cache bin would be ArrayBackend + DatabaseBackend (note this requires a way to set per-bin defaults, currently we don't).

With that configuration it'll load from database, set into Array (because it'll always be a miss the first hit to the ArrayBackend), then the storage load for that config object next time during the request reads from ArrayBackend. So you get 'static' caching of storage requests without a real static cache. That would at least get us to the point where we have one query per config object, right now we have multiple queries per config object which is just mad.

@Jose I'm sure the database queries are worse than event dispatcher but they're still showing up as a noticeable performance issue, and they may well end up more expensive than a request from an APC cache implementation for example, once we're down to query-per-config-object we can see how much is left.

I'd still like to look at an ArrayCache-ish implementation for config similar to #7, but there's going to be so much in there, like field/instance definitions that it might not be at all feasible at this point.

@pounard: just the number of database hits isn't everything, the current variables system is responsible for all kinds of nasty performance issues - both the loading of 5-year-old stale variables on every request, and the stampedes when any variable is set. However yes it's definitely a regression at the moment with the config system which is why this issue is critical.

Berdir’s picture

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.

pounard’s picture

@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.

catch’s picture

Category: task » bug

Random test failures due to this slowing things down so much that the test bot was timing out after innocently converting things to config() makes this a bug now.

Berdir’s picture

Status: Active » Needs review
FileSize
1.17 KB
FAILED: [[SimpleTest]]: [MySQL] 48,057 pass(es), 730 fail(s), and 66 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, config-cache-chain-1187726-36.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

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 ;)

Berdir’s picture

Status: Needs review » Needs work
sun’s picture

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?)

pounard’s picture

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.

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
pounard’s picture

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.

Status: Needs review » Needs work

The last submitted patch, config-cache-chain-1187726-36.patch, failed testing.

catch’s picture

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?)

It should be able to remove references to objects, then garbage collection would clean them up if they're not referenced anywhere else. I also had it in mind to write a cache backend using weak references since there's a PECL module now, although that's more suited to entities and long running processes.

@Berdir: why would we only want to clear the memory backend? I can see the use for testing but not sure it applies elsewhere?

With the test failures it looks like the in-memory cache is persisting in the parent process - same as we have to clear static caches after drupalPost() sometimes.

Jose Reyero’s picture

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.

Berdir’s picture

- 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.

Jose Reyero’s picture

@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.

catch’s picture

I just don't know how bad the performance loss from event dispatching is- maybe we could save the 'init'

Last time I looked it was quite bad, worse than a module_invoke_all() or drupal_alter(), although Crell (I think) reckoned there was some potential optimization to be had in EventDispatcher - but I don't think anyone's working on that yet.

Since the event dispatching is potentially a serious performance issue - we won't know for sure until we see a complex-ish site 100% converted to CMI, then I don't see a problem changing the API from an event to a set of plugins (or stacked storage engines or however else we chose to do it). This isn't an API that anyone else is using yet (at least not the event bit) so it's up for grabs until code freeze if we find a better way to do it for me.

I'm agnostic on just caching the storage here, vs. trying to cache the event as well.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.32 KB
FAILED: [[SimpleTest]]: [MySQL] 48,039 pass(es), 742 fail(s), and 65 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, config-cache-chain-1187726-52.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review

@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.

Berdir’s picture

FileSize
4.7 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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.

Status: Needs review » Needs work

The last submitted patch, config-cache-lazy-1187726-55.patch, failed testing.

Jose Reyero’s picture

@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.

chx’s picture

Assigned: Unassigned » chx

Let me see what I can do.

chx’s picture

Status: Needs work » Needs review
FileSize
767 bytes
5.21 KB
FAILED: [[SimpleTest]]: [MySQL] 46,740 pass(es), 1,567 fail(s), and 332 exception(s). View

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

chx’s picture

FileSize
438 bytes
4.98 KB
FAILED: [[SimpleTest]]: [MySQL] 46,574 pass(es), 1,550 fail(s), and 322 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 1187726_60.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
6.4 KB
FAILED: [[SimpleTest]]: [MySQL] 48,260 pass(es), 780 fail(s), and 143 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 1187726_62.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
11.54 KB
FAILED: [[SimpleTest]]: [MySQL] 49,256 pass(es), 27 fail(s), and 75 exception(s). View

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...)

Status: Needs review » Needs work

The last submitted patch, config-cache-test-fixes-1187726-64.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
5.05 KB
31 KB
PASSED: [[SimpleTest]]: [MySQL] 48,916 pass(es). View

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.

chx’s picture

FileSize
19.16 KB

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

yched’s picture

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 ?

Dave Reid’s picture

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.

Berdir’s picture

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.

catch’s picture

Entity static caching should be replaced by #1596472: Replace hard coded static cache of entities with cache backends, once that's done it could presumably be made to work for config entities too.

chx’s picture

FileSize
2.65 KB
32.48 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found. View

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.

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, 1187726_72.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 1187726_72.patch, failed testing.

yched’s picture

Status: Needs review » Needs work

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.

chx’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
31.72 KB
FAILED: [[SimpleTest]]: [MySQL] 48,272 pass(es), 46 fail(s), and 86 exception(s). View

Weird, passed locally.

catch’s picture

Status: Needs work » Needs review

I think the config cache controller should be locked down the same as storage currently is (if only by convention but there's an issue for that) - i.e. swappable globally like a cache backend or the CMI storage, but not per config entity-type.

Status: Needs review » Needs work

The last submitted patch, 1187726_76.patch, failed testing.

Jose Reyero’s picture

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.

chx’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
32.61 KB
FAILED: [[SimpleTest]]: [MySQL] 48,467 pass(es), 50 fail(s), and 15 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 1187726_80.patch, failed testing.

Fabianx’s picture

FileSize
2.51 KB
32.78 KB
FAILED: [[SimpleTest]]: [MySQL] 48,858 pass(es), 39 fail(s), and 15 exception(s). View

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.

Fabianx’s picture

Status: Needs work » Needs review
chx’s picture

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.

Status: Needs review » Needs work

The last submitted patch, core--Memory-usage-from-CMI--1187726--83.diff, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
1.95 KB
34.22 KB
PASSED: [[SimpleTest]]: [MySQL] 48,894 pass(es). View

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??

Fabianx’s picture

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.

Jose Reyero’s picture

Status: Needs review » Needs work

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

Fabianx’s picture

Status: Needs work » Needs review

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.

Jose Reyero’s picture

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

@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

chx’s picture

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.

chx’s picture

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

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

catch’s picture

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

@Jose this issue is neither Trojan nor innocent. I opened it before any CMI code had even been committed to core and it's been critical for half a year.

tim.plunkett’s picture

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.

sun’s picture

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?

linclark’s picture

chx asked in IRC how the test was passing before. I haven't worked on that test since the contrib Entity API was added to core, so I don't know how it was passing. I did point out that this was an issue with the contrib Entity API module, #1092192: Using hook_entity_info_alter to add the bundles prevents some modules from altering entities.

For what it's worth, I'm pretty sure that RDF module is going to be completely overhauled in a way that will solve this, but that work still needs to be done.

Crell’s picture

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.

Fabianx’s picture

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()).

chx’s picture

> 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.

Jose Reyero’s picture

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.

chx’s picture

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.

Jose Reyero’s picture

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.

chx’s picture

FileSize
2.22 KB
16.59 KB
PASSED: [[SimpleTest]]: [MySQL] 49,234 pass(es). View

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.

Jose Reyero’s picture

@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();
Fabianx’s picture

     // 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’s picture

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.

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. 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.

Jose Reyero’s picture

@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.

Fabianx’s picture

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.

chx’s picture

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.

heyrocker’s picture

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.

Fabianx’s picture

#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

chx’s picture

FileSize
2.19 KB
16.64 KB
PASSED: [[SimpleTest]]: [MySQL] 49,329 pass(es). View

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.

Berdir’s picture

+++ 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).

chx’s picture

chx’s picture

FileSize
2.5 KB
16.96 KB
PASSED: [[SimpleTest]]: [MySQL] 49,266 pass(es). View

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

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

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

=> RTBC

sun’s picture

FileSize
7.91 KB
16.91 KB
PASSED: [[SimpleTest]]: [MySQL] 49,370 pass(es). View

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal8.config-cache.118.patch, failed testing.

catch’s picture

Would be good to profile this to see how much it helps at the moment.

Fabianx’s picture

Status: Needs work » Needs review

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

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

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 ...

webchick’s picture

Status: Reviewed & tested by the community » Needs review

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

msonnabaum’s picture

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.


chx’s picture

Status: Needs review » Reviewed & tested by the community

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.

Fabianx’s picture

Status: Reviewed & tested by the community » Needs review

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 :).

Fabianx’s picture

X-POST: This is of course RTBC.

catch’s picture

Category: bug » task
Status: Needs review » Active

OK I've committed the latest patch to 8.x.

I feel like we still need an issue open to explore this more, but probably not this one. i.e. one looking at where the remaining time in CMI is spent (especially as we get closer to 100% conversion and potentially add context), then another one looking at whether the is it loaded or isn't it logic could be factored out a bit. There might be another spin-off or two more so leaving this at 'critical task' for now.

yched’s picture

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" ?

chx’s picture

Assigned: chx » Unassigned

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

yched’s picture

Status: Active » Reviewed & tested by the community
FileSize
822 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,355 pass(es). View

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

beejeebus’s picture

FileSize
534 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,339 pass(es). View

looks like we left in $conf when we don't need it any more? patch attached.

yched’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Active

Committed/pushed both follow-ups, back to active per #128.

effulgentsia’s picture

Status: Active » Needs review
FileSize
3.52 KB
PASSED: [[SimpleTest]]: [MySQL] 49,828 pass(es). View

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.

chx’s picture

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?

effulgentsia’s picture

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.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.04 KB
3.5 KB
PASSED: [[SimpleTest]]: [MySQL] 49,947 pass(es). View

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

effulgentsia’s picture

Status: Fixed » Active

Thanks. Back to active per #134.

catch’s picture

Whoops, thanks :)

catch’s picture

Title: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects) » Follow-up: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects)
Priority: Critical » Normal
effulgentsia’s picture

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.

beejeebus’s picture

#1880766: Preload configuration objects now that the blocks patch slowed down head by 15%.

sun’s picture

mtift’s picture

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

Berdir’s picture

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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • catch committed 452ee7a on 8.3.x
    Issue #1187726 by yched: add a comment to...
  • catch committed b9be32e on 8.3.x
    Issue #1187726 by beejeebus: remove dead global $conf declaration.
    
  • catch committed bc801aa on 8.3.x
    Issue #1187726 by chx, Berdir, Fabianx, sun: Fixed Add caching for...
  • catch committed 72b297f on 8.3.x
    Issue #1187726 by effulgentsia: (followup) clean up service persist...

  • catch committed 452ee7a on 8.3.x
    Issue #1187726 by yched: add a comment to...
  • catch committed b9be32e on 8.3.x
    Issue #1187726 by beejeebus: remove dead global $conf declaration.
    
  • catch committed bc801aa on 8.3.x
    Issue #1187726 by chx, Berdir, Fabianx, sun: Fixed Add caching for...
  • catch committed 72b297f on 8.3.x
    Issue #1187726 by effulgentsia: (followup) clean up service persist...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • catch committed 452ee7a on 8.4.x
    Issue #1187726 by yched: add a comment to...
  • catch committed b9be32e on 8.4.x
    Issue #1187726 by beejeebus: remove dead global $conf declaration.
    
  • catch committed bc801aa on 8.4.x
    Issue #1187726 by chx, Berdir, Fabianx, sun: Fixed Add caching for...
  • catch committed 72b297f on 8.4.x
    Issue #1187726 by effulgentsia: (followup) clean up service persist...

  • catch committed 452ee7a on 8.4.x
    Issue #1187726 by yched: add a comment to...
  • catch committed b9be32e on 8.4.x
    Issue #1187726 by beejeebus: remove dead global $conf declaration.
    
  • catch committed bc801aa on 8.4.x
    Issue #1187726 by chx, Berdir, Fabianx, sun: Fixed Add caching for...
  • catch committed 72b297f on 8.4.x
    Issue #1187726 by effulgentsia: (followup) clean up service persist...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Chi’s picture

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.