#1535868: Convert all blocks into plugins slowed down head by 15%.

a chunk of that was config() calls. let's see how preloading cache objects with cache->getMultiple() goes. initial, simple as possible patch first, get data, then adjust.
Postpone on #2228261: Add a local, PhpStorage-based cache backend.

Files: 
CommentFileSizeAuthor
#121 1880766-preload-config-121.patch3.13 KBBerdir
#111 1880766-preload-config-111.patch9.06 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,429 pass(es).
[ View ]
#98 interdiff.txt879 byteskim.pepper
#98 1880766-preload-config-97.patch6.62 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,884 pass(es).
[ View ]
#93 1880766-93.patch6.73 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 64,260 pass(es).
[ View ]
#91 preload_config_objects.patch6.71 KBneetu morwani
PASSED: [[SimpleTest]]: [MySQL] 64,228 pass(es).
[ View ]
#88 1880766-88.patch6.69 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 63,392 pass(es).
[ View ]
#86 1880766-86.patch6.16 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 63,330 pass(es).
[ View ]
#74 1880766-interdiff.txt2.08 KBkim.pepper
#74 1880766-preload-config-74.patch7.97 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 59,869 pass(es).
[ View ]
#72 1880766-interdiff.txt3.74 KBkim.pepper
#72 1880766-preload-config-69.patch6.41 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 59,893 pass(es).
[ View ]
#69 1880766-69.patch6.11 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 59,886 pass(es).
[ View ]
#68 1880766-68-config-preload.patch0 bytesbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 59,752 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#61 1880766-61.patch14.71 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#30 1880766-cmi-preload-30.patch6.51 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1880766-cmi-preload-30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 1880766-cmi-preload-28.patch6.51 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#24 1880766-24-cmi-preload.patch6.5 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 50,372 pass(es).
[ View ]
#13 1880766-13-cmi-preload.patch6.5 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 50,214 pass(es).
[ View ]
#11 1880766-11-cmi-preload.patch6.27 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 50,205 pass(es).
[ View ]
#6 1.preloadcmi.xhprof.gz36.43 KBbeejeebus
#6 2.preloadcmi.xhprof.gz35.55 KBbeejeebus
#6 runs-diff-2.png149.14 KBbeejeebus
#5 1880766-5-cmi-preload.patch4.65 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 48,664 pass(es), 1,235 fail(s), and 548 exception(s).
[ View ]
#3 runs-diff.png138.64 KBbeejeebus
#3 1.preloadcmi.xhprof.gz35.77 KBbeejeebus
#3 2.preloadcmi.xhprof.gz35.85 KBbeejeebus
#2 cmi-preload.patch3.84 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 48,726 pass(es), 1,588 fail(s), and 5,266 exception(s).
[ View ]
config.preload.patch3.07 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/CoreBundle.php.
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, config.preload.patch, failed testing.

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new3.84 KB
FAILED: [[SimpleTest]]: [MySQL] 48,726 pass(es), 1,588 fail(s), and 5,266 exception(s).
[ View ]

ahem, with correct patch.

beejeebus’s picture

StatusFileSize
new35.85 KB
new35.77 KB
new138.64 KB

here's the some xhprof data.

aggregate of 40 runs, 1 is head, 2 is the patch.

Status:Needs review» Needs work

The last submitted patch, cmi-preload.patch, failed testing.

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new4.65 KB
FAILED: [[SimpleTest]]: [MySQL] 48,664 pass(es), 1,235 fail(s), and 548 exception(s).
[ View ]

- fixed preloadCids being altered by getMultiple()
- fixed loadedCids, switch to a count on preloadedCids

new xhprof aggregates coming soon.

beejeebus’s picture

StatusFileSize
new149.14 KB
new35.55 KB
new36.43 KB

ok, this is looking better.

faster yo

hitting the front page as anonymous 6 standard core blocks enabled, seeing a 7% decrease in wall time and an 8% decrease in function calls.

summary png and zipped xhprof files attached. would love it if someone could confirm these numbers...

Status:Needs review» Needs work

The last submitted patch, 1880766-5-cmi-preload.patch, failed testing.

sun’s picture

Can we leverage the existing CacheArray for this?

Also, I'd think that the config names to preload depend on the page/path/route? (Not sure what to do about sub-requests in the long run, but that problem will apply to every route-based CacheArray anyway then.)

Berdir’s picture

Not CacheArray, but maybe #1858616: Extract generic CacheCollector implementation and interface from CacheArray :). CacheArray is slow as it needs to go through ArrayAccess, which was necessary in 7.x but we can define real API's now in 8.x.

beejeebus’s picture

#8-path: i've heard the 'load by path' idea a few times now, seems to be popular.

no, i don't want to do that yet. lets do the simplest thing, get some data, then adjust.

on a more generic cache solution - i'd like to use cids and multi-get, rather than cache a list of data. if we can do that with a generic solution, awesome.

lists of data are a recipe for races and cache inconsistency (hello D7 variable cache). storing the cids means we simply don't have to solve the problem of cache invalidation, and introduce no new races writing cache entries.

beejeebus’s picture

StatusFileSize
new6.27 KB
PASSED: [[SimpleTest]]: [MySQL] 50,205 pass(es).
[ View ]

found this gem in ConfigStorageController::buildQuery():

<?php
        $config
= config($prefix . $id);
        if (!
$config->isNew()) {
         
$result[$id] = new $config_class($config->get(), $this->entityType);
        }
?>

are you kidding me? which part of config()'s contract says "i may return, on occasion, objects that are not, in fact, stored yet. wait, wat?" this reeks of config storage controller knowing (or thinking it knows) something about the innards of config(). this comment is full of stupid, please ignore.

new patch:

- add a hacky Config::setIsNew(), so that ConfigStorageController trusts config(). wait, wat?

- also, clear the config.factory cache from ConfigStorageController::save()

beejeebus’s picture

Status:Needs work» Needs review
beejeebus’s picture

StatusFileSize
new6.5 KB
PASSED: [[SimpleTest]]: [MySQL] 50,214 pass(es).
[ View ]

- get rid of setIsNew(), and use a new function, preload(), which sets isNew to false

sun’s picture

Status:Needs review» Needs work

1) The language/attitude in this issue generally discourages others from commenting.

2) The fact that a setIsNew() method is required on Config only clarifies even further that we've lost a sane separation between ConfigFactory and Config. Config was the sole and only responsible service that handled the loading and state of config objects previously. ConfigFactory suddenly took over ~50% of that, without a clear definition of which service is responsible for what. This consequence was raised as critical concern for the change proposal that added caching to ConfigFactory, but was brushed away. This patch here tries to advance on that caching, so it's only logical and natural that it is running into the exact architectural problems that were known upfront.

3) The caching layer in the config system has to be transparent to all other systems and services. If any external code needs to manually reset or flush the cache of the config factory, then there is a bug in config's caching mechanism itself. There's pre-existing, similar code that manually accounts for renames in the config factory, but that only exists because the issue that introduced the config factory caching did not want to account for the already existing #1739900: Add a rename operation to config storage controllers. Again, known issue, also brushed away. There's a follow-up issue somewhere to fix that. In any case, manual calls into the config factory's cache reset methods are wrong. Renames are the only exception currently.

beejeebus’s picture

#14.1 - yeah, sorry about that. i'll tone it down a notch, i'm not being helpful

#14.2 - i took setIsNew() out in #13. i'm trying to avoid re-architecting things here. so far, this patch is small, and i'm trying to keep it that way.

#14.3 - i added

+ drupal_container()->get('config.factory')->reset($prefix . $id);

without knowing if it's really necessary. will take it out once i get this green and see what happens.

sun - i'm not sure what you're proposing. scrap this patch, step back and change Config and ConfigFactory and the way they relate to each other? to me, that feels like a bigger, separate issue.

sun’s picture

This patch unintentionally hits some big nails on the broken separation of concerns between Config and ConfigFactory. Ignoring that problem space further and working around it by slapping new, public API methods and behavior onto the system doesn't feel right to me and is guaranteed to make a later, proper refactoring considerably harder.

The architectural problem stems from the fact that this change adds a preload facility to the system, which attempts to inject preloaded data into the system.

One potential way to work around the architectural problems is to cache the actual Config object instead of its data within. In essence, allow a Config object instance to be serialized (in one environment) and deserialized (in another environment).

http://php.net/manual/en/class.serializable.php might help us to achieve that. (I've seen it being actively used in Symfony only recently, which looks really neat.) That way, we can skip the workarounds for isNew entirely and serialize the required properties instead.

beejeebus’s picture

pulling a list of serialized config objects out of cache in ConfigFactory, as opposed to just the data, would simplify this patch a little, but not a lot.

but it won't change the basic idea here at all - front load a bunch of config data with a cache multi-get in the ConfigFactory. unless i'm misunderstanding #16? sun - does the basic idea i'm suggesting work with the refactoring you want to do in the future?

sun’s picture

Yeah, the general idea of preloading is fine I think. We had it for variables already, and due to the bundling of multiple values into larger config objects, it was pretty clear from the beginning that we'll have to (re-)introduce preloading.

That said, I'm not sure whether I didn't expect more performance gains from this change. AFAICS, the patch cuts away ~10-20 minutes from the total testsuite time... although that's comparing the time of two different testbots, so it doesn't really mean anything, and alas, it's also fairly easy to make HEAD faster these days, since HEAD really can't get any slower.

In other words, I'm a little concerned about the net effect of this change. This optimization was (at least in my mind) planned to be one of the last that would happen for the D8 release - considerably improving the overall system performance after fixing all other performance regressions - or in hypothetical numbers: sitting in front of a beta release with a total testsuite time of 25 minutes and cutting it down to 20 with this final optimization. Focus on "final", because after this, there's not much left that can be tweaked. :-/

beejeebus’s picture

i straw-polled #drupal-contribute, and the consensus from xjm, jthorson, EclipseGc etc was that it's worth getting these speedups in sooner rather than later, so i'm going to continue with this patch until it's green.

Wim Leers’s picture

#18: I'm intrigued by your third paragraph. Why would we only want to do this optimization very late in the development cycle? Because we'd be less inclined to fix other performance problems, since they'd be less impactful, relatively speaking? But then you say there's not much that can be tweaked after this… which I don't really understand.

catch’s picture

There's no value to leaving performance optimizations until the last minute.

Currently the incredibly poor performance of config() is making it very difficult to detect other performance regressions introduced into core. A fixed 5ms slow down might be 1% of the request instead of 5% given a slower system overall.

This means that patches that introduce lots of config() calls alongside other performance regressions are being let in with unknown status on just how bad they are, because it's hard to separate where exactly the regression comes from if it's from multiple places.

Caching per-route sounds worth looking at but not necessarily in this issue.

Where the caching is going to be most likely to be polluted is config entities - i.e. we don't want to load every view on every page, or every field config or instance (although those aren't configuration entities yet anyway but when they are). Also admin pages, since setting/viewing a configuration object doesn't necessarily mean it's needed on runtime.

beejeebus’s picture

i'll try caching by route in the next iteration.

beejeebus’s picture

hmm. so i had a look at trying to cache by path, and at this point i think i'll just use request_path() calls.

current_path() just looks like a hot mess. if you call it early, you get the raw request_path(), if you call it late, you get the system path.

so it's completely useless for a system that wants to get a path early for a read, then get a path late for a write.

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new6.5 KB
PASSED: [[SimpleTest]]: [MySQL] 50,372 pass(es).
[ View ]

rerolled #13 for head, no new changes.

Status:Needs review» Needs work

The last submitted patch, 1880766-24-cmi-preload.patch, failed testing.

sun’s picture

Title:preload configuration objects» Preload configuration objects
Issue tags:+Performance, +Configuration system, +Configurables

Tagging.

andyceo’s picture

Status:Needs work» Needs review
heyrocker’s picture

StatusFileSize
new6.51 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Just a reroll, but would love to see this moving forward. Will try and do a proper review soon.

Status:Needs review» Needs work

The last submitted patch, 1880766-cmi-preload-28.patch, failed testing.

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new6.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1880766-cmi-preload-30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

ooooook. got this going again once berdir pointed out for me in IRC that error handling during the installer is A BUNCH OF DAMN LIES.

so the only change change needed to get this to install was:

<?php
diff
--git a/core/lib/Drupal/Core/Config/ConfigFactory.php b/core/lib/Drupal/Core/Config/ConfigFactory.php
index 1f2440a
..3b14ed6 100644
--- a/core/lib/Drupal/Core/Config/ConfigFactory.php
+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -98,7 +98,7 @@ public function __construct(StorageInterface $storage, ContextInterface $context
         $this
->preloadCids = $preloadCidsCache->data;
        
$this->preloadCidsCount = count($this->preloadCids);
         foreach (
$this->storageCache->getMultiple($preloadCidsCache->data) as $name => $cacheItem) {
-         
$this->cache[$name] = new Config($name, $this->storage, $this->eventDispatcher);
+         
$this->cache[$name] = new Config($name, $this->storage, $this->getContext());
          
$this->cache[$name]->init();
          
$this->preloadData[$name] = $cacheItem->data;
         }
?>

patch attached for the bot's consideration.

Status:Needs review» Needs work
Issue tags:-Performance, -Configuration system, -Configurables

The last submitted patch, 1880766-cmi-preload-30.patch, failed testing.

beejeebus’s picture

Status:Needs work» Needs review

#30: 1880766-cmi-preload-30.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Performance, +Configuration system, +Configurables

The last submitted patch, 1880766-cmi-preload-30.patch, failed testing.

beejeebus’s picture

grrr, HEAD changes keep breaking this patch.

yched’s picture

Highly interested in this.

Field API currently maintains its own cache of field and instance definitions (FieldInfo class). These are about to become ConfigEntities, and thus FieldInfo will cache ConfigEntities. Ideally, we'd want to get rid of that extra layer and use the CMI cache instead of caching our own ConfigEntities on top of it.

The thing is, though, that FieldInfo currently goes a long way to implement smart cache strategy - that was #1040790: _field_info_collate_fields() memory usage.
In short : field and instance definitions are cached in separate entries by bundle, and loaded on-demand only for the entity types and bundles that are involved in the current page request.

From a (quick) glance, it seems the current patch would do : "the moment a field definition has been loaded once, it will be preloaded on each subsequent request" ?
Would put us back to "all field and instances definitions for all entity types and bundles are loaded in memory unconditionnally" pretty quick, no ?

catch’s picture

We discussed that issue in irc a few weeks ago in respect to both Views and Field API. The outcome of that was to have two pre-load caches:

a. Preloads everything from CMI until there's a route name.

b. Preloads everything between the route name and the end of the request.

This way, (a) is loaded every request, (b) caches based on the route name and only holds items needed in there (and not already in (a)).

The idea being that the same views / entity types / bundles ought to be found for the same routes more or less, while there's also a finite number of route names (except for stuff under admin/ which makes up > 70% on most sites) so should still get a good hit rate (as opposed to caching per-path or similar).

xjm’s picture

Priority:Normal» Major

Shouldn't this be at least major?

yched’s picture

The plan in #36 sounds appealing - cache config by route. Or more precisely, "cache the list of config items names to multiple-pre-load at the beginning of the request, by route", if I get this right ?

If something like this gets in, I guess it would deprecate the smart-but-complex "by bundle" caching strategy in FieldInfo, since two caching strategies on top of each other become hell to predict and debug.

I wonder whether "by route" is going to be granular enough, though. For the same route, the set of config items needed might vary a lot depending on various considerations ? Permissions of the current user comes to mind first, but I guess there are others...
So for example we'd end up loading much more config data than actually needed to serve an anonymous page request ?

beejeebus’s picture

yes, #38 is spot on in terms of the limitations of per-route caching.

you can add cache-clears to the per-user issue - if an anonymous user on a 'light' route happens to be the request that rebuilds some caches, you can see a large number of items cached that are not normally associated with that route.

we have to make sure whatever we implement as the default is easy to override, so we don't force a one-size-fits-all solution on all sites.

yched’s picture

What about maybe just aiming for the ability to do multiple-load on config items, without trying to be too smart for now ?

beejeebus’s picture

hmm. can you elaborate?

are you suggesting building a 'get a list of CMI objects' functionality that this code can use?

or, simplifying the cache loading to store the CMI data as a list? or ... ?

yched’s picture

I mean just being able to do something like the following would speed Field API quite a bit :

<?php
$config_names
= array('mymodule.foo', 'mymodule.bar');
$config_items = config_load_multiple($config_names); // <- uses $cache->getMultiple()
$item = $config_items['mymodule.foo']->get();
?>
beejeebus’s picture

ah, that is a nice idea :-)

Berdir’s picture

beejeebus’s picture

hmm. i don't know how to do this any more. i tried following the context code paths, and failed.

hopefully someone else can figure it out, because it's beyond me.

beejeebus’s picture

ok, so #45 was a bit of a whinge.

this patch, and also Berdir's nice patch over in [#1971158:15] can handle multi-load from cache easily. Berdir's patch also includes some nice code to make file scanning more efficient.

but having added a bunch of complexity in order to make this vaguely performant, we then have to do stuff like this per object (from Berdir's patch):

<?php
/**
+   * Initializes a configuration object with pre-loaded data.
+   *
+   * @param array $data
+   *   Array of loaded data for this configuration object.
+   *
+   * @return Drupal\Core\Config\Config
+   *   The configuration object.
+   */
+  public function initWithData(array $data) {
+   
$this->isLoaded = TRUE;
+   
$this->overrides = array();
+   
$this->isNew = FALSE;
+   
$this->notify('init');
+   
$this->replaceData($data);
+   
$this->notify('load');
+    return
$this;
+  }
?>

and what do context event listeners to do on 'config.load'? run expensive cache/filesystem code per object.

so, with these optimisations we do expensive stuff (number of 'config.load' listeners * number of config objects) times. without them, we do expensive stuff ((1 + number of 'config.load' listeners) * number of config objects) times.

there are two ways we can go from here.

1. just say "if you use config event listeners to override stuff from your custom .yml files **cough**any multilingual site**cough**, your site will be slow". that sucks, and i bags not having that chat with Gabor.

2. push multi-load down in to the event listener API ('config.load-multiple' something something), so they can add the extra complexity just like we've done here. that sucks.

i'm not sure which i hate the most, so i'll take direction on which way to go.

yched’s picture

[edit: never mind, this comment is irrelevant]

Config listeners are below cache point, right ?
So multiple load would still be a win on warm caches (multiple cache reads) even if they're a wash on cold caches ?

beejeebus’s picture

sorry, i didn't follow #47. can you elaborate?

what i'm trying to communicate is

- listeners are called per-object, no matter how we fetch them from cache or file storage
- the most important core listener is locale, which on load, will try to fetch another config object, at which point we're back to 'do expensive stuff per object'

Gábor Hojtsy’s picture

If we push down multiload to this level then we can also rely on multiload in locale speeding up the loading of those overrides somewhat. I'm not sure how faster that is going to be.

catch’s picture

I'd probably go for pushing the multiple load down to the listeners as well. I never really liked the listeners approach very much and this was the main reason why - having to have a complete layer on top of everything doing stuff twice but we're stuck with that now.

yched’s picture

re @beejeebus #48: right, sorry, I thought listeners kicked in before data was cached. My bad, scratch #47.

Berdir’s picture

Issue tags:+D8 cacheability

Tagging.

jibran’s picture

Issue tags:+Needs reroll

Tagging.

beejeebus’s picture

Issue tags:-Needs reroll

going to revive this patch. should only be 400% bigger with a config.loadMultiple approach.

beejeebus’s picture

i started working on this, and discussed possible implementations with msonnabaum in IRC.

currently, we fire the config events from a single config object, which makes it impossible to get past the original 'expensive operation per object per listener' pattern here.

to fix that, we'll need to:

- move the 'init' and 'load' events from Config to ConfigFactory
- make the ConfigEvent return an array of Config objects
- remove 'new Config()', and replace it with calls to the factory in ConfigImporter
- port ConfigGlobalOverrideSubscriber and LocaleConfigSubscriber to handle a list of Config objects

optionally, we can convert ConfigFactory to ConfigRepository, because if we do the above things, it's really not a factory any more. but, i want to get feedback from committers on the above list first.

catch’s picture

That's fine with me. I wasn't comfortable with config events going in at all and there's only a couple of contrib modules that are likely to use them anyway.

beejeebus’s picture

coolio.

also - i plan to just work around our broken Config storage abstractions in contrib for D8 with Config db and friends...

Gábor Hojtsy’s picture

#1966538: Translation is not updated for configuration, when the config changes may be some stuff related, although it does not make drastic changes to the locale event listener, last time I checked.

twistor’s picture

If we're going to convert ConfigFactory to ConfigSomething, can we get a ConfigSomethingInterface? AFAIR ConfigFactory has been hardcoding Config(). While I have no immediate plans for something better, I would like that our premises align with our reality.

Generalizing to multiple++

alexpott’s picture

I've never been entire comfortable with the context solution and anything we can do to improve performance here will be a big bonus.

Lets explore the suggestions in #55

beejeebus’s picture

Status:Needs work» Needs review
Issue tags:-API change, -Approved API change
StatusFileSize
new14.71 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

ok, so here's a first pass at making read-time Config events use a 'handle multiple config objects' approach.

many yaks were shaved to bring you this patch.

beejeebus’s picture

nom nom nom tags nom nom nom

Status:Needs review» Needs work

The last submitted patch, 1880766-61.patch, failed testing.

beejeebus’s picture

Status:Needs work» Postponed

marking as postponed on #2098119: Replace config context system with baked-in locale support and single-event based overrides - no point trying to do this until after that lands.

catch’s picture

Priority:Major» Critical
kim.pepper’s picture

Issue summary:View changes
Status:Postponed» Needs work

Un-postponed!

beejeebus’s picture

Assigned:Unassigned» beejeebus

i have some old code i'll resurrect for this.

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new0 bytes
FAILED: [[SimpleTest]]: [MySQL] 59,752 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

ok, here's an incomplete first cut.

this patch adds a preload event, and adds a subscriber which looks for names to preload from Settings.

we may want to put this one in, then add more core subscribers in follow up patches.

beejeebus’s picture

StatusFileSize
new6.11 KB
PASSED: [[SimpleTest]]: [MySQL] 59,886 pass(es).
[ View ]

zero byte patch. drupal.org--

The last submitted patch, 68: 1880766-68-config-preload.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -184,6 +194,11 @@ public function get($name) {
    +    if (!$this->preloadEventFired) {
    +      $this->preloadEventFired = TRUE;
    +      $names = array_unique($names + $this->getPreloadNames());
    +    }

    This code is a bit hart to understand. If it is not fired yet, set it to fired. Let's just switch the two lines.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigPreloadEvent.php
    @@ -0,0 +1,65 @@
    +   * @return self
    ...
    +   * @return self

    We use "@return $this" now.

  3. +++ b/core/lib/Drupal/Core/Config/ConfigPreloadSubscriber.php
    @@ -0,0 +1,40 @@
    +    $settings_config_names = Settings::getSingleton()->get('config_preload_names', array());

    It would be cool if we could just inject the settings service instead.

  4. +++ b/core/lib/Drupal/Core/Config/ConfigPreloadSubscriber.php
    @@ -0,0 +1,40 @@
    +  /**
    +   * Returns the names of any configuration objects in Settings.
    +   *
    +   * @param \Drupal\Core\Config\ConfigPreloadEvent $event
    +   *   Configuration preload event to respond to.
    +   */

    The documentation is a bit inaccurate.

kim.pepper’s picture

StatusFileSize
new6.41 KB
PASSED: [[SimpleTest]]: [MySQL] 59,893 pass(es).
[ View ]
new3.74 KB

Fixes issues in #71

jibran’s picture

If we have to reroll again.

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -262,6 +277,18 @@ protected function loadModuleOverrides(array $names) {
+   * Get a list of configuration object names to preload.

Gets

kim.pepper’s picture

StatusFileSize
new7.97 KB
PASSED: [[SimpleTest]]: [MySQL] 59,869 pass(es).
[ View ]
new2.08 KB

Thanks Jibran. I've fixed that and added a unit test for ConfigPreloadSubscriber::onConfigPreloadNamesGetSettingsNames().

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

It is always good to see a unit test!

beejeebus’s picture

yep, i'm happy with this patch to go in as is.

as a follow up, i've opened #2177601: Determine which config objects core should preload to figure out which core modules should subscribe to this event.

yched’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -184,6 +194,11 @@ public function loadMultiple(array $names) {
+    if (!$this->preloadEventFired) {
+      $names = array_unique($names + $this->getPreloadNames());

Does this mean that the first call to loadMultiple() will receive more results than the ones it asked for ?
Looks like it might have surprising effects if the code foreaches on the results, you're not really supposed to receive results for keys you never asked ?
Couldn't we sliently add the items to preload to the list but then ditch the entries before returning actual results ?

kim.pepper’s picture

webchick’s picture

Status:Reviewed & tested by the community» Needs review

#77 looks like a needs review.

pounard’s picture

Another idea maybe a bit too late and maybe a bit more hardcore would have been to dump configuration as generated PHP files containing arrays, such as the container is being dumped. This way it could also serve as a security layer where files could not be changed by a hacker that does not have access to writing to the filesystem, and be protected by paranoid admin with restrive file permissions, and would also go to OPCode cache ensuring both 0 file stat and 0 SQL or cache backend queries. This would vanish any need for preloading anything, would be super simple, altered configuration could be dumped too (considering that alteration is not supposed to be variable) and give a very good performance boost altogether (and maybe tend to a 0-query bootstrap).

beejeebus’s picture

Status:Needs review» Needs work

#77 - wow, i'm so bad at this game. yep, we totes need to fix that.

#80 - sounds like a cool contrib module.

Berdir’s picture

Cross-referencing #2172561: Config overrides may spill over to undesired places, I started looking into this and got distracted by performance issues with the config override stuff in general, posted my findings over there for now as that seems to become a general follow-up issue for that...

beejeebus’s picture

i've created #2177739: Fix inefficient config factory caching to deal with the lulz berdir found.

sun’s picture

This mostly looks good, but I noticed some additional issues:

  1. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -74,6 +76,13 @@ class ConfigFactory implements EventSubscriberInterface {
    +  protected $preloadEventFired;

    @@ -184,6 +194,11 @@ public function get($name) {
       public function loadMultiple(array $names) {
    ...
    +    if (!$this->preloadEventFired) {
    +      $names = array_unique($names + $this->getPreloadNames());
    +      $this->preloadEventFired = TRUE;
    +    }

    @@ -262,6 +277,18 @@ protected function loadModuleOverrides(array $names) {
    +  protected function getPreloadNames() {

    +++ b/core/lib/Drupal/Core/Config/ConfigPreloadEvent.php
    @@ -0,0 +1,64 @@
    +class ConfigPreloadEvent extends Event {

    Hm. I'm concerned about the name "preload".

    Everywhere else in Drupal, "preload" denotes a callback/hook facility for a specific "load" operation.

    → "preload" is even more confusing here, because we are, in fact, in a loadMultiple() method.

    What we're really doing here is to "prime" the config factory cache. It's a cache priming facility.

    Could we rename these classes and variables accordingly to avoid confusion? E.g.:

    $preloadEventFired → $isCachePrimed + isCachePrimed() method
    getPreloadNames() → primeCache()
    ConfigPreloadEvent → ConfigCachePrimeEvent

  2. +++ b/core/lib/Drupal/Core/Config/ConfigPreloadSubscriber.php
    @@ -0,0 +1,54 @@
    +    $events['config.preload.names'][] = array('onConfigPreloadNamesGetSettingsNames', 48);

    48? Why 48? :)

    Shouldn't a base/default event subscriber like this just simply use a priority of 0?

    It's the first and only subscriber for this event, no?

  3. +++ b/sites/default/default.settings.php
    @@ -278,6 +278,14 @@
    +#$settings['config_preload_names'] = array();

    All other commented out default settings in settings.php are using a space after the #.

twistor’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigPreloadEvent.php
@@ -0,0 +1,64 @@
+  public function addNames(array $names) {
+    $this->names += $names;

Shouldn't this be an array_merge()?

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new6.16 KB
PASSED: [[SimpleTest]]: [MySQL] 63,330 pass(es).
[ View ]

rerolled after the config caching fix patch landed.

Wim Leers’s picture

Status:Needs review» Needs work

I've reviewed the code from an understandability POV — I don't know the config system at all. Mostly nitpicks though.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigPreloadSubscriber.php
    @@ -0,0 +1,40 @@
    +  }
    +}

    Should have an empty line in between.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -154,6 +164,11 @@ public function get($name) {
    +    if (!$this->preloadEventFired) {
    +      $this->preloadEventFired = TRUE;
    +      $names = array_unique($names + $this->getPreloadNames());
    +    }

    Why would you set this to TRUE without actually firing it?

    The preload event is fired in getPreloadNames(). This should be set to TRUE after that method is called. Or maybe it should even be set it inside that method?

  3. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -232,6 +247,18 @@ protected function loadModuleOverrides(array $names) {
    +    $this->eventDispatcher->dispatch('config.preload.names', $config_preload_event);
    +    return $config_preload_event->getNames();

    While reading the patch, I thought this event was triggered *after* preloading had occurred, to indicate to other code that config preloading was done.

    But apparently it's quite the opposite: this event is used to *collect* a list of names of config objects to be preloaded!

    I think this could be documented more clearly in this class, and in ConfigPreloadEvent.

  4. +++ b/core/lib/Drupal/Core/Config/ConfigPreloadEvent.php
    @@ -0,0 +1,65 @@
    +

    +++ b/core/lib/Drupal/Core/Config/ConfigPreloadSubscriber.php
    @@ -0,0 +1,40 @@
    +

    Trailing newlines.

  5. +++ b/core/lib/Drupal/Core/Config/ConfigPreloadSubscriber.php
    @@ -0,0 +1,40 @@
    +   * Implements EventSubscriberInterface::getSubscribedEvents().

    Shouldn't this be {@inheritdoc}?

  6. +++ b/core/lib/Drupal/Core/Config/ConfigPreloadSubscriber.php
    @@ -0,0 +1,40 @@
    + * Core configuration preload subscriber.

    s/preload subscriber/preload event subscriber/?

  7. +++ b/core/lib/Drupal/Core/Config/ConfigPreloadSubscriber.php
    @@ -0,0 +1,40 @@
    +   * Returns the names of any configuration objects in Settings.

    What is "Settings"?

    If it's settings.php, then why is it written with a capital letter?

    Maybe an @see to relevant code would be helpful here?

beejeebus’s picture

Status:Needs work» Needs review
StatusFileSize
new6.69 KB
PASSED: [[SimpleTest]]: [MySQL] 63,392 pass(es).
[ View ]

new patch, i've tried to incorporate feedback from #84 and #87.

Wim Leers’s picture

Interdiff?

sun’s picture

Thank you — The naming looks much better now! :)

  1. +++ b/core/lib/Drupal/Core/Config/ConfigCachePrimeEvent.php
    @@ -0,0 +1,67 @@
    +   * Configuration names.
    +   *
    +   * @var array
    +   */
    +  protected $names = array();

    Is this a list of names to prime, or a list of names that have been primed? Can we clarify the phpDoc?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigCachePrimeEvent.php
    @@ -0,0 +1,67 @@
    +   * Gets configuration names.
    +   *
    +   * @return array
    +   *   The list of configuration names that can be overridden.
    +   */
    +  public function getNames() {

    1. s/Gets/Returns/ ?

    2. The @return description appears to be copied from elsewhere?

  3. +++ b/core/lib/Drupal/Core/Config/ConfigCachePrimeEvent.php
    @@ -0,0 +1,67 @@
    +   * @return self
    ...
    +    return $this;
    ...
    +   * @return self
    ...
    +    return $this;

    @return $this

  4. +++ b/core/lib/Drupal/Core/Config/ConfigCachePrimeEvent.php
    @@ -0,0 +1,67 @@
    +  public function addNames(array $names) {
    +    $this->names += $names;
    +    $this->names = array_unique($this->names);

    If $names is an indexed array, then we need to use array_merge() here, because += will not add indexes that exist already.

    array_merge() should also make the array_unique() obsolete.

  5. +++ b/core/lib/Drupal/Core/Config/ConfigCachePrimeSubscriber.php
    @@ -0,0 +1,40 @@
    +class ConfigCachePrimeSubscriber implements EventSubscriberInterface {

    Do we really need a separate subscriber object here?

    What's wrong with making ConfigFactory subscribe directly?

  6. +++ b/core/lib/Drupal/Core/Config/ConfigCachePrimeSubscriber.php
    @@ -0,0 +1,40 @@
    +    $settings_config_names = Settings::getSingleton()->get('config_cache_prime_names', array());

    Fine with this name for now, but we need to review + fix the overall situation of setting names at some point:

    #2160705: settings.php setting names are inconsistent (and undocumented)

  7. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -71,6 +73,13 @@ class ConfigFactory implements ConfigFactoryInterface, EventSubscriberInterface
    +   * Whether or not the cache has been primed yet.

    s/yet/already/ ?

  8. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -232,6 +246,24 @@ protected function loadModuleOverrides(array $names) {
    +    // Yes, this is stupid, but this is Drupal.

    The comment reads fine and is informative, if you'd remove this line. :P

  9. +++ b/sites/default/default.settings.php
    @@ -278,6 +278,14 @@
    + * Configuration cache prime.
    + *
    + * The core configuration system will prime it's cache with any configuration
    + * objects named in this array.

    In summary: "Configuration object names to prime in cache."

    In description:
    - Let's remove "core"
    - s/it's/its/
    - s/any/the/

neetu morwani’s picture

StatusFileSize
new6.71 KB
PASSED: [[SimpleTest]]: [MySQL] 64,228 pass(es).
[ View ]

Last patch re-rolled. Now the patch applies successfully.

neetu morwani’s picture

Last patch re-rolled. Now the patch applies successfully.

beejeebus’s picture

StatusFileSize
new6.73 KB
PASSED: [[SimpleTest]]: [MySQL] 64,260 pass(es).
[ View ]

updated patch to address #90.

didn't remove the new subscriber, don't really care either way.

alexpott’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests

We need some tests of the functionality.

beejeebus’s picture

hmm. started writing some tests.

might need unit tests with mocks for parts of this, because it's hard to assert from outside the factory that the desired code path was exercised.

alexpott’s picture

Issue tags:+beta target
beejeebus’s picture

Assigned:beejeebus» Unassigned
kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new6.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,884 pass(es).
[ View ]
new879 bytes

From my limited understanding, I think this will require some integration tests. Not sure whether this is a new test class or there is an existing one which we can add to?

In the meantime, I've rerolled and cleaned up some imports and removed an unused global $conf.

Berdir’s picture

The list of cache gets that I posted in #1194136-52: Re-organise core cache bins could be interesting for this. Note that this is the list after applying the patch there, which renames a lot of bins, but doesn't touch cache_config.

Berdir’s picture

Status:Needs review» Needs work
+++ b/core/lib/Drupal/Core/Config/ConfigCachePrimeSubscriber.php
@@ -0,0 +1,40 @@
+    $events['config.prime.cache'][] = array('onConfigCachePrimeGetSettingsNames');

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -228,6 +239,23 @@ protected function loadModuleOverrides(array $names) {
+    $this->eventDispatcher->dispatch('config.cache.prime', $config_cache_prime_event);

Let's play a game of "Spot the difference" :)

Yeah, this definitely needs tests :p

alexpott’s picture

Use the ConfigEvents class to make life easier too :)

Berdir’s picture

After fixing that, it does work, but there's a problem right now..

The are a few things that are happening during bootstrap that have a huge influence whether and what you want to preload. The most important is the page cache.

If you look at the list from the referenced issue, this is what happens in early bootstrap:

cache_config: system.module*
cache_config: system.performance
=== > CACHE HIT/MISS.

cache_bootstrap: module_implements
cache_config: system.file
cache_config: system.filter

* in DrupalKernel::initializeContainer(), should apparently *not* happen unless module list changed, but that logics seems broken. different issue, we still have the problem even there would only be system.performance

Obviously, if you're going to have a page cache hit, you don't want to load all the stuff that comes below. But the preload kicks in on the first config file being loaded, which right now is system.module. Which is too early to know that.

One idea I have is to special case system.module and system.performance, only trigger preload() after that.

Another possibly crazy idea is to invert the preload pattern, trigger something somewhere that simply calls $cache_factory->loadMultiple() instead of the implicit magic that we have right now. That would also make it equally easy to have an on request event listener that is capable of loading route specific config. Risk is to end up with various pre-loaders that all load a few config files.

catch’s picture

We have page_cache_without_database, that should skip all config gets on a cache hit when set. At least provides a workaround for sites that know about it.

The preloaded config is similar to the variables table in 7.x in that it's all or nothing when that's disabled though yeah.

If we were going to special case that system.performance, then it should only happen if the page is eligible to be a cache hit - i.e. no session cookie. Looks like an easy change in _drupal_bootstrap_page_cache() we just don't do that currently. Although still needs a way to trigger preloading or not.

Then we'd only load the config separately for requests that have a chance of a cache hit, and not for authenticated users / users with a session where we know there's no chance of a hit, and they don't want the extra i/o.

catch’s picture

Also cross-linking #2177461: Refactor page caching into a service. I haven't reviewed that patch yet but it's obviously relevant to this.

beejeebus’s picture

alexpott suggested creating a test class that extends the factory, and do a phpunit test.

there are examples of this in the cached storage test.

alexpott’s picture

The example I was thinking of was ThemeHandlerTest. The CachedStorageTest gives examples of mocking config storage.

kim.pepper’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new9.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,321 pass(es).
[ View ]
new4.17 KB

OK. Here's a crack at a unit test for loading the primed config names in loadMultiple. It actually picked up a bug in merging the name arrays. How about that??

Also added a constant for the event to ConfigEvents.

beejeebus’s picture

woo, nice work kim!

i can't really RTBC this, because i wrote too much of the code...

larowlan’s picture

Looks good, a few questions and some general nitpicks

  1. +++ b/core/lib/Drupal/Core/Config/ConfigCachePrimeEvent.php
    @@ -0,0 +1,66 @@
    +use Drupal\Core\Language\Language;

    +++ b/core/lib/Drupal/Core/Config/ConfigCachePrimeSubscriber.php
    @@ -0,0 +1,40 @@
    +use Symfony\Component\HttpKernel\KernelEvents;

    Appears unused?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
    @@ -57,4 +57,11 @@
    +   * @see \Drupal\Core\Config\ConfigCachePrimeSubscriber::getSubscribedEvents().

    Should we reference subscribers here? Seems like tail wagging the dog.

  3. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -228,6 +239,23 @@ protected function loadModuleOverrides(array $names) {
    +    $this->isCachePrimed = TRUE;

    could we have an 'isPriming' property for this use case instead? Just to distinguish between the two states.

  4. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigFactoryTest.php
    @@ -0,0 +1,67 @@
    +      'name' => 'Config factoryt test',

    %s/factoryt/factory

  5. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigFactoryTest.php
    @@ -0,0 +1,67 @@
    +   * Tests loading multiple with prime config names.

    suggest: Tests loading multiple config objects with primed config names?

kim.pepper’s picture

StatusFileSize
new8.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,254 pass(es).
[ View ]
new2.23 KB

Thanks for the review @larowlan!

  1. Cleaned up imports
  2. Removed @see reference to subscriber
  3. I don't think the isCachedPrimed is used for anything but ensuring preventing a redirect loop. Maybe we can just rename to isCachingPriming to make more sense?
  4. Typo fixed
  5. Fixed comment
kim.pepper’s picture

StatusFileSize
new9.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,429 pass(es).
[ View ]

Re-roll

moshe weitzman’s picture

Can we come up with an initial list of names that should be preloaded. It seems silly for this to go in without any usage in core. Code looks good.

Wim Leers’s picture

Agreed with everything in #112.

Only the silliest of silly nitpicks to be found:

  1. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigFactoryTest.php
    @@ -0,0 +1,67 @@
    +  public function testLoadMultipleWithCachePrimeNames() {
    +

    Extraneous newline.

  2. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigFactoryTest.php
    @@ -0,0 +1,67 @@
    +  }
    +}

    Missing newline.

catch’s picture

Not sure why this is an event as opposed to either tagged services or just a single service implementing CacheCollector that could be replaced. Earlier patches were along those lines. Trying to think of a case where you'd need to have more than one active strategy for preloading - it seems like that could result in its own overhead if those in turn are caching lists of cids.

beejeebus’s picture

re #118 - i don't care about events or not. i don't really understand 'more than one strategy' though, because there is only one now.

the intent is to enable easy module interaction. an event allows a simple, well known path for modules to take to alter the list of names we'll preload.

if there's another, better way to do that, i'm fine with using it.

catch’s picture

For 'more than one strategy', I mean:

- listener A adds configuration IDs by route

- listener B adds configuration IDs by role

or some other combination that different listeners try to do.

beejeebus’s picture

Status:Needs review» Needs work

discussed this with catch a bit in IRC.

we should stop any work on this patch until we agree on an approach.

catch would like us to go back to earlier approaches of this patch.

catch’s picture

Need to think about this more but beejeebus explained the expectation was each module would provide a static list of configuration objects to pre-load via the event. I'm always very wary of anything opt-in for performance because it's extremely to ignore.

I've been assuming that we'd still try to come up with some kind of strategy like #35-38 - although admittedly that might not work at all, but it doesn't feel very compatible with the events.

Berdir’s picture

Here's another reason why the current approach (preload on first load call) is problematic: It's useless for multilingual sites. All of it would need to be loaded again once the language negotiation happened.

We have a RoutePreloader now which is a request event subscriber, I think this should follow that model. Then it's a separate, optional step and the ConfigFactory doesn't need to be made more complicated due to it?

We will still need a solution for the handful of early bootstrap calls, but that could maybe just be a hardcoded list somewhere?

beejeebus’s picture

ok, lets ditch this approach.

Berdir’s picture

StatusFileSize
new3.13 KB

Attaching a simple proof of concept when doing it as a separate subscriber. I think that's much easier and decoupled, so that the config factory doesn't need to know about this.

We drifted away into a lot of more or less related discussions when looking at this, posting a few notes of things to do separately or improve here:

- Should we have multiple separate subscribers that load on their own or should we have one request subscriber that invokes the existing config event that we have right now in the previous patches? More complexity but it would allow us to group multiple strategies together as a single load.
- This happens after language negotiation, so we still need a solution for the few calls before there:

Without multilingual:
- system.module: This is a (we think flawed) attempt at trying to support multiple webheads and rebuilds on them, but there are many situations that we don't cover with this, like pushing new versions of modules, so we can just as well remove it
- system.performance: This is supposed to be overridable with settings, but it's broken => needs to be fixed.
- Then we have two config calls to system.filter and system.file.

With multilingual, we get 4 additional calls, and that's two to load the languages (I already have a patch to do it in a single call, will open an issue after this comment.) and we load the language types and language negotation settings. Not sure what to do about the remaining 3 after my issue.

catch’s picture

Status:Needs work» Postponed

Discussed this in depth with Alex Pott, Wim Leers, beejeebus and others at devdays.

The most difficult thing about getting preloading right is the following:

- When lots of configuration objects are used, you want to preload as many as possible.
- When there are render cache hits, less configuration objects will be needed.

Hence the better our overall, high-level caching, pre-loading can result in a lot of information loaded that's not actually required for the request.

Assuming #1880766: Preload configuration objects works and gets in, the individual configuration loads will result in more or less zero i/o (at least when using read-only PhpStorage, or compared to the db even with preloading). So if that gets in, we can skip preloading altogether and mark this duplicate. Postponing for now though.

pounard’s picture

There's another way of improving things which would to make bigger config files and having less files (for example put all of the system module in only one or maybe two files). The file number could be dramatically reduced. In most pages you are going to load almost everything of system module anyway ; I'm quite sure this is true for most modules. Having the file structure exposed at the API level was a very bad idea at the very beggining anyway and that's what is causing you the most trouble today. So just reduce the file number. But that probably belongs to another issue(s).

webchick’s picture

Sorry, which issue is this postponed on? #122 contains a self-reference. :)

twistor’s picture

catch’s picture

@pounard while that might help a bit for static configuration, it would make no difference to configuration entities, which in the end will be the largest number of configuration objects on a site. Entity bundles, fields, instances, display modes, views, image styles etc. can't be put into a single file.

Also even one file per module for static configuration could result in over a hundred database queries where there's 100+ modules enabled.

pounard’s picture

Indeed.

jibran’s picture

catch’s picture

Priority:Critical» Major
Status:Postponed» Active

Also #2248767: Use fast, local cache back-end (APCu, if available) for low-write caches (bootstrap, discovery, and config) and #2231595: Add a cache backend that checks an inconsistent cache, then falls back to a consistent cache backend.

Those are pretty far along, so I'm bumping this down to major/active.

Pre-loading could be useful to at least some extent even with those patches in, since with opt-in pre-loading it'd be possible to multiple load on cache misses. But that's much less of an issue with a local cache.

Fabianx’s picture