#1779026-155: Convert Text Formats to Configuration System

The code is copied literally from DatabaseStorageController.

This issue will also need to prevent the config entities from being cached in the config factory.

Files: 
CommentFileSizeAuthor
#127 1885830-126.patch29.86 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,058 pass(es), 58 fail(s), and 3 exception(s). View
#126 1885830-126-interdiff.txt2.14 KBBerdir
#123 1885830-123-interdiff.txt839 bytesBerdir
#123 1885830-123.patch28.52 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,012 pass(es), 59 fail(s), and 4 exception(s). View
#118 1885830-118.patch27.7 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,031 pass(es), 690 fail(s), and 17 exception(s). View
#109 1885830-109-interdiff.txt615 bytesBerdir
#109 1885830-109.patch29.71 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,365 pass(es), 684 fail(s), and 32 exception(s). View
#104 interdiff-1885830-104.txt28.5 KBdamiankloip
#104 1885830-104.patch4.16 KBdamiankloip
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1885830-104.patch. Unable to apply patch. See the log in the details link for more information. View
#103 interdiff1885830-103.txt28.09 KBdamiankloip
#103 1885830-103.patch33.4 KBdamiankloip
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,569 pass(es). View
#90 1885830-90.patch34.31 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,831 pass(es). View
#88 interdiff-1885830-88.txt4.23 KBdamiankloip
#88 1885830-88.patch33.35 KBdamiankloip
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,690 pass(es), 3 fail(s), and 0 exception(s). View
#44 tests-interdiff.txt17.93 KBvladan.me
#44 1885830-44.patch30.07 KBvladan.me
FAILED: [[SimpleTest]]: [MySQL] 60,308 pass(es), 16 fail(s), and 1 exception(s). View
#42 1885830-42-interdiff.txt7.81 KBBerdir
#42 1885830-42.patch12.15 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 60,277 pass(es), 59 fail(s), and 1 exception(s). View
#38 1885830-38.patch4.33 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 59,465 pass(es), 105 fail(s), and 4 exception(s). View
#29 1885830-29.patch4.42 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 56,767 pass(es), 107 fail(s), and 4 exception(s). View
#29 interdiff-1885830-29.txt1.92 KBdamiankloip
#25 1885830-25.patch2.79 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,082 pass(es), 103 fail(s), and 3 exception(s). View
#14 cache-1885830-14.patch6.06 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,812 pass(es), 107 fail(s), and 19 exception(s). View
#7 drupal8.config-entity-cache.7.patch5.86 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,756 pass(es), 58 fail(s), and 54 exception(s). View
#5 drupal8.config-entity-cache.5.patch5.85 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,676 pass(es), 55 fail(s), and 265 exception(s). View
drupal8.config-entity-cache.0.patch5.84 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,774 pass(es), 47 fail(s), and 157 exception(s). View

Comments

tim.plunkett’s picture

Issue tags: +Configurables

Tagging

sun’s picture

Issue tags: -Configurables
   public function resetCache(array $ids = NULL) {
...
+    if (isset($ids)) {
+      foreach ($ids as $id) {
...
+    }

@@ -334,6 +410,8 @@ public function save(EntityInterface $entity) {
       $config->save();
+      $this->resetCache(array());

erm, WTF is DatabaseStorageController doing for the insert operation there?

That's a no-op?

sun’s picture

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
5.85 KB
FAILED: [[SimpleTest]]: [MySQL] 50,676 pass(es), 55 fail(s), and 265 exception(s). View

Fixed the variable name mismatch.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
5.86 KB
FAILED: [[SimpleTest]]: [MySQL] 50,756 pass(es), 58 fail(s), and 54 exception(s). View

doh

Status: Needs review » Needs work

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

sun’s picture

Alright - the remaining failures are all caused by stale caches, it seems :)

For the existing config entities in core, the additional caching isn't actually that impactful. However, the more more frequently used things are converted - e.g., like menus - or filter formats, or user roles - or #111715: Convert node/content types into configuration, the additional cache layer will be more measurable.

That said, a persistent entity-level cache would actually have a much bigger impact than just the in-memory static caching. It would instantly allow us to get rid of a lot of custom caches like filter_formats() and node_get_types() and user_roles(), etc. which are all just simply wrappers around entity_load_multiple().

catch’s picture

#1596472: Replace hard coded static cache of entities with cache backends would make it easier to swap out different entity cache controllers.

plach’s picture

Here and in #1919322: entity_load_unchanged() should be part of the storage controller we have repated code between database and config storage controllers. Would it make sense to have an abstract BaseStorageController as a common ancestry?

tim.plunkett’s picture

Yes please. EntityStorageControllerBase would be a more correct name.

plach’s picture

Yes please. EntityStorageControllerBase would be a more correct name.

@sun suggested to address this in #1893772: Move entity-type specific storage logic into entity classes.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.06 KB
FAILED: [[SimpleTest]]: [MySQL] 54,812 pass(es), 107 fail(s), and 19 exception(s). View

Reroll.

Status: Needs review » Needs work

The last submitted patch, cache-1885830-14.patch, failed testing.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Fun fun fun. A lot of that is views, and I have some ideas as to why.

catch’s picture

tim.plunkett’s picture

I have some more Views fixes to post, but I found a big part of the problem with the tests.

During SimpleTest runs (but not page loads), EntityManager is recreated almost every time the plugin.manager.entity service is retrieved.

That means that the property containing all the controllers is reset, and each time a new storage controller will be used.
Which means that the $entityCache for each storage controller is different, and will lead to different results.

Who knows why retrieving a service gives you a new object only during tests?

effulgentsia’s picture

Because modules are constantly being enabled/disabled, and the container rebuilt?

tim.plunkett’s picture

This is within a single test method.

Looking at:
http://api.drupal.org/api/drupal/core%21modules%21shortcut%21lib%21Drupa...

Between the $set = $this->set; and the #this->drupalPost(), EntityManager is recreated three times.

Unless I'm just losing it, which may be possible.

tim.plunkett’s picture

tim.plunkett’s picture

Issue tags: +Stalking Crell

Tagging.

damiankloip’s picture

Status: Needs work » Postponed

I think we should postpone this on #21 for now. As we can reuse cache related methods from there.

damiankloip’s picture

Assigned: tim.plunkett » Unassigned
Status: Postponed » Active
damiankloip’s picture

Status: Active » Needs review
FileSize
2.79 KB
FAILED: [[SimpleTest]]: [MySQL] 58,082 pass(es), 103 fail(s), and 3 exception(s). View

Here's a reroll now we've got that base class. I will also write some tests for this.

Berdir’s picture

We discussed in Portland that static caching on this level is problematic because the config might return different information based on the config context, e.g. language.

So we either can't have a static cache or we need to care about the config context as part of the cache key. We need to check how big the overhead is of creating config entities.

damiankloip’s picture

Hmm, good point. Why aren't things ever easy.. :)

Status: Needs review » Needs work

The last submitted patch, 1885830-25.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
4.42 KB
FAILED: [[SimpleTest]]: [MySQL] 56,767 pass(es), 107 fail(s), and 4 exception(s). View

So maybe we would need to do something like this? Not sure if it will reap many benefits from a performance point of view tbh.

Berdir’s picture

In most scenarios does the UUID not change I guess (same language, domain, ...), so it should bring something.

Will do some profiling after #1971158: Follow-up: Add loadMultiple() and listAll() caching to (cached) config storage is in.

damiankloip’s picture

Issue tags: -Performance, -Stalking Crell

Yeah, I guess even it is is a bit better, it could be worth it, who knows. Let's wait for that issue. We don't want too many layers of static caching either I guess.

damiankloip’s picture

Issue tags: +Performance

Status: Needs review » Needs work

The last submitted patch, 1885830-29.patch, failed testing.

Berdir’s picture

I guess most of those test fails now just need some static cache clears.

damiankloip’s picture

Reminder for the next patch: It would be good if getCurrentContextUuid() actually returned something.

yched’s picture

yched’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture

Ok, so here's another reason why we need this: #2095283: Remove non-storage logic from the storage controllers. That's how often we load roles, up to 4k times in a single test.

Berdir’s picture

Issue summary: View changes
FileSize
4.33 KB
FAILED: [[SimpleTest]]: [MySQL] 59,465 pass(es), 105 fail(s), and 4 exception(s). View

Re-roll.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 38: 1885830-38.patch, failed testing.

damiankloip’s picture

When I was working on this patch before there were issues with forms and clearing of the static cache. Hopefully this has been helped by the work to avoid serialising objects in the form cache etc... That was basically blocking it before iirc.

Berdir’s picture

Status: Needs work » Needs review
FileSize
12.15 KB
FAILED: [[SimpleTest]]: [MySQL] 60,277 pass(es), 59 fail(s), and 1 exception(s). View
7.81 KB

@damiankloip: There is no issue here :)

Those fails just prove that this works. That's what you get when you combine static caches and change the data in them through form submits. There is only one solution and that's manually clearing them (well, the other alternative would be to clear all entity caches on every drupalPost()). This patch starts doing that, still quite a lot to go, but this should be something that someone with less experience can do, I'll try to get @vladan.me on it, he started to work on a lot of entity issues last week.

Status: Needs review » Needs work

The last submitted patch, 42: 1885830-42.patch, failed testing.

vladan.me’s picture

Status: Needs work » Needs review
FileSize
30.07 KB
FAILED: [[SimpleTest]]: [MySQL] 60,308 pass(es), 16 fail(s), and 1 exception(s). View
17.93 KB

Hopefully, this should fix all fails except ViewPageControllerTest, I am not quite sure right now what problem is, test is giving this as description "The test did not complete due to a fatal error."

Status: Needs review » Needs work

The last submitted patch, 44: 1885830-44.patch, failed testing.

vladan.me’s picture

Status: Needs work » Needs review

44: 1885830-44.patch queued for re-testing.
Just to make sure that test results are correct.

Status: Needs review » Needs work

The last submitted patch, 44: 1885830-44.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
30.68 KB
FAILED: [[SimpleTest]]: [MySQL] 60,434 pass(es), 1 fail(s), and 0 exception(s). View
2.52 KB

Fixing the path and views handler test.

No idea about the other one, that one is interesting...

vladan.me’s picture

I couldn't find also cause of last one so I didn't upload those two fixes (had them though). What could cause such error, doesn't seem related at all...

Status: Needs review » Needs work

The last submitted patch, 48: 1885830-48.patch, failed testing.

Berdir’s picture

Here's the fix that @dawehner found: http://paste.ubuntu.com/6418465/

Care to roll a patch with that included?

damiankloip’s picture

I think we just want the second change and not the first.

vladan.me’s picture

Can you be more specific damiankloip? Just to use this?

-      $executable = $view->getExecutable();
+      $executable = static::executableFactory()->get($view);
damiankloip’s picture

Yes, exactly that :)

I just tested this out locally, and the test doesn't actually pass with the first change added too. So I guess it was the right hunch.

vladan.me’s picture

Status: Needs work » Needs review
FileSize
30.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1885830-55.patch. Unable to apply patch. See the log in the details link for more information. View

Uploading new patch, patch is rerolled because of #2134951, also, made change noted in comment above, no interdiff included.

dawehner’s picture

I just tested this out locally, and the test doesn't actually pass with the first change added too. So I guess it was the right hunch.

If we don't clone the object, we end up in manipulating the same view object somehow.

@berdir here comes a question. Let's assume you have the following code in two different blocks:

class BLock1 {
  public function build() {
    $article = $this->entityStorage->load('article');
    $article->label .= ' Elephant';
    return array('#markup' => $article->label());
    // NO SAVE
  }
}

class BLock2 {
  public function build() {
    $article = $this->entityStorage->load('article');
    $article->label .= ' Dolphin';
    return array('#markup' => $article->label());
    // NO SAVE
  }
}

Both of these pieces of code ran, ... should we not try to clone the cached entity, so we don't have conflicting code at all?

Berdir’s picture

55: 1885830-55.patch queued for re-testing.

Berdir’s picture

I don't know, but that's how it's been for nodes/content entities since the static cache for nodes was added in D6...

Status: Needs review » Needs work

The last submitted patch, 55: 1885830-55.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
30.26 KB
FAILED: [[SimpleTest]]: [MySQL] 59,396 pass(es), 4 fail(s), and 0 exception(s). View

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 60: 1885830-60.patch, failed testing.

moshe weitzman’s picture

damiankloip’s picture

Status: Needs work » Needs review
FileSize
30.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1885830-63.patch. Unable to apply patch. See the log in the details link for more information. View

Just another reroll.

Status: Needs review » Needs work

The last submitted patch, 63: 1885830-63.patch, failed testing.

The last submitted patch, 63: 1885830-63.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

63: 1885830-63.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 63: 1885830-63.patch, failed testing.

damiankloip’s picture

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

Another reroll.

Let's look at what is still failing.

damiankloip’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 68: 1885830-68.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
29.8 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1885830-71.patch. Unable to apply patch. See the log in the details link for more information. View
2.15 KB

And with the config context stuff now removed.

The last submitted patch, 71: 1885830-71.patch, failed testing.

The last submitted patch, 71: 1885830-71.patch, failed testing.

damiankloip’s picture

FileSize
29.85 KB
FAILED: [[SimpleTest]]: [MySQL] 59,820 pass(es), 11 fail(s), and 1 exception(s). View
1.21 KB

The last submitted patch, 74: 1885830-74.patch, failed testing.

damiankloip’s picture

FileSize
32.87 KB
FAILED: [[SimpleTest]]: [MySQL] 59,874 pass(es), 5 fail(s), and 0 exception(s). View
3.01 KB

Let's see how far this gets us.

The last submitted patch, 76: 1885830-76.patch, failed testing.

damiankloip’s picture

FileSize
31.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1885830-78.patch. Unable to apply patch. See the log in the details link for more information. View
1.2 KB

We don't need to override those methods anymore at all, we can just use what's provided by EntityStorageControllerBase for that.

Berdir’s picture

And what if code like user_mail() would load config entities and not raw config?

We will need a test for that somewhere...

I haven't looked at the changes at all yet, but according to @alexpott, there's a way to get the cache key from the config factory that could work as a cache separator.

The last submitted patch, 78: 1885830-78.patch, failed testing.

Berdir’s picture

The method for that would be getCacheKey(), but that is actually flawed, as described in #2177739: Fix inefficient config factory caching, because it can not possibly consider module overrides.

And we do have an example for this in core, just not enough test coverage apparently, see Drupal\Core\Datetime\Date::formatDate()

jibran’s picture

78: 1885830-78.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 78: 1885830-78.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
32.41 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Re-roll, bunch of context conflicts.

Status: Needs review » Needs work

The last submitted patch, 84: 1885830-84.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
32.43 KB
FAILED: [[SimpleTest]]: [MySQL] 64,513 pass(es), 14 fail(s), and 14 exception(s). View
1.4 KB

Status: Needs review » Needs work

The last submitted patch, 86: 1885830-86.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
33.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,690 pass(es), 3 fail(s), and 0 exception(s). View
4.23 KB

back to the same 5 failures then...

Status: Needs review » Needs work

The last submitted patch, 88: 1885830-88.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
34.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,831 pass(es). View
985 bytes

Fixes the textfieldtest

sun’s picture

  1. +++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorAdminTest.php
    @@ -139,6 +139,7 @@ function testAdmin() {
         $this->drupalPostForm(NULL, $edit, t('Save configuration'));
         $expected_settings['plugins']['stylescombo']['styles'] = "h1.title|Title\np.callout|Callout\n\n";
    +    \Drupal::entityManager()->getStorageController('editor')->resetCache();
         $editor = entity_load('editor', 'filtered_html');
         $this->assertTrue($editor instanceof Editor, 'An Editor config entity exists.');
    

    Hm. The amount of manual calls to reset caches in web tests worries me a bit. Degrades the DX of writing web tests for everyone... :-/

    Technically, we already have a solution for updating the process in the test runner with changes from the child site:

    Every WebTestBase request calls into refreshVariables(), in order to reset static caches on e.g. config factory, etc.

    Until we have a more generic solution (→ StaticCacheInterface + tagged services), I wonder whether it wouldn't make more sense to replace all of these manual resetCache calls with a single call in refreshVariables()?

    I see that the storage controller is individually selected throughout this patch — is there a way to iterate over all available entity managers, get their storage controllers, and call resetCache() on them?

  2. +++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.php
    @@ -270,14 +270,14 @@ public function testRenameDeleteBundle() {
         $type->save();
    +    \Drupal::entityManager()->getStorageController('entity_view_display')->resetCache();
         $old_display = entity_load('entity_view_display', 'node.article.default');
         $this->assertFalse($old_display);
    +    \Drupal::entityManager()->getStorageController('entity_form_display')->resetCache();
         $old_form_display = entity_load('entity_form_display', 'node.article.default');
         $this->assertFalse($old_form_display);
    

    These two (and because of these two most likely some more) cache resets do not appear to be needed, because the code runs in a single process.

Berdir’s picture

1. Well, it just makes it as bad as it always was for content entities. Sure, we could consider to loop over all defined entity types and clear their static caches, but what we can not do is just do it for those that were actually used, so we'd have to initially a lot of storage controllers just to empty the obviously empty static cache :) Unless we introduce a new method on the entity manager...

We would also have to only do it for config entities IMHO, because it would be a bit unfortunate for content entities as soon as #597236: Add entity caching to core lands, as that would have to cache tag clears all the time.

See also my latest comment in that issue (https://drupal.org/comment/8541147#comment-85411479), clearing the caches all the time would hide that pretty serious bug, so I'd prefer to live with the worse DX. I think a lot of people that have written tests in 7.x are already pretty used to entity and drupal_static() caches interfering with tests...

2. Hm, that does look strange, but I think we just went through test fails and fixed them? Sounds like that can either be removed or would be a cache clear bug ?

Berdir’s picture

FileSize
34.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,276 pass(es), 2 fail(s), and 0 exception(s). View

Re-roll.

#91.2 was in fact an actual bug, we didn't clear the cache for renamed entities. This should address that.

Did some micro-benchmarks:

use Drupal\Component\Utility\Timer;

Timer::start('cache');

for ($i = 0; $i < 10000; $i++) {
  entity_load('node_type', 'article');
}

print (Timer::stop('cache')['time']) . "ms\n";

HEAD: ~650ms
Patch: ~180ms

=> ~3.5 times faster.

Question is, how much does it affect and actual page load, that's harder to answer. I did some basic tests, but on a frontpage with two nodes and a custom block, we apparently didn't have a single static cache hit. #2211723: FieldInstance::__construct() loads all field_config entities for example is a bigger problem right now. Not sure if that would change if we'd no longer kept cached entities in FieldInfo.

Based on that, this is probably not worth the effort at the moment, unless someone is able to provide different numbers?

Berdir’s picture

FileSize
1.68 KB

Forgot the interdiff.

Status: Needs review » Needs work

The last submitted patch, 93: 1885830-93.patch, failed testing.

moshe weitzman’s picture

Latest profiling by @msonnabaum suggests that this would really help. He noticed us loading up the list of roles many many times for access checks. Anyone up for revisiting this?

Berdir’s picture

Interesting, I did expect that to show up as well, but wasn't able to see that, care to share what/how you profiled?

For the specific case of permissions/roles, @alexpott also opened an issue to cache the list of permissions in User:.hasPermission(), we used to have built-in caches there which were all dropped.

msonnabaum’s picture

It saw it when profiling a view that listed a large number of nodes/users. I forget where they were coming from, but the calls were to UserSession::hasPermission, which fetches roles each time.

Berdir’s picture

Ah that makes sense, access checks for operations mostly I guess...

Berdir’s picture

One thing that I discussed with @fago in Szeged was that was could provide the functionality but not enable it by default for config entities, we can do that very easily now with @ConfigEntityType.

Then config entity types that are loaded a lot can explicitly enable it and benefit, without having to do so for the dozen or more types that are not used that frequently.

Still for permission checks, even just looping over the roles and their permissions might be more overhead than we want, so we might want to do #2202185: Statically cache Role entities to speed up (User|UserSession)::hasPermission() anyway, and then I'm not sure if we want this to be enabled for roles?

msonnabaum’s picture

It doesn't hurt for it to be enabled for roles, but I agree it's worth doing 2202185 if for no other reason than it's a code improvement.

I'd much prefer that static caching we're opt-out. Historically, opt-in results in people not knowing about it and then performance issues popping up that are only found when profiling (this happened with ctools plugins in contrib).

Berdir’s picture

The problem with opt-out can be seen in the current patch, if we enable it for all types on core, then we run into all those test issues that we need to fix, and manually disabling it for them will probably result in people copying that without thinking about it :)

Not enabling in this case also only means we just fall through to the next static cache, it's not like we need to execute database queries or something, so it's not *that* bad as long as we don't call it as often as roles can be.

Might also be useful to compare the memory usage when enabling it.

Note that I'm fine with moving forward with this, just want to make sure that we are sure that it's worth it and do it in a way that doesn't result in worse DX than it needs to be (static caches in web tests are the pain, it took us forever to get this green).

damiankloip’s picture

Status: Needs work » Needs review
FileSize
33.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,569 pass(es). View
28.09 KB

Needs a bit of a reroll with the getStorageController > getStorage change.

I think I would definitely like to see this opt-out too. I basically agree totally with #101.

I also agree that this current patch is pretty cumbersome. The amount of ad-hoc cache clears needed is obviously not ideal. If we were to use a real storage backend (memory backend), we could always just use a null backend in tests or something. This is more difficult all the time we have baked in logic like this. However, we discussed this a while ago (in Prague) and using a 'proper' backend/backend chain right would be very difficult to implement, and possibly controversial.

damiankloip’s picture

FileSize
4.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1885830-104.patch. Unable to apply patch. See the log in the details link for more information. View
28.5 KB

we could just try something like this?

reset the container in post/postForm. Otherwise, we could just reset controllers on the entity manager. That seems a little special snowflake like though.

Status: Needs review » Needs work

The last submitted patch, 104: 1885830-104.patch, failed testing.

Berdir’s picture

Note that #2225955: Improve the DX of writing entity storage classes will do #100 because static cache will then be supported for all entity storage implementations that use the default methods.

jibran’s picture

104: 1885830-104.patch queued for re-testing.

The last submitted patch, 104: 1885830-104.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
29.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,365 pass(es), 684 fail(s), and 32 exception(s). View
615 bytes

Re-roll.

- Almost no changes are now necessary because most of the static cache handling is now in the base class, also moved the one for deletion.
- Removed the by default disabled static cache flag for config entities but I still think this makes sense to do, just want to see what the testbot has to say. There might be a few that can use it, but most don't because we don't load them that much and then it's just unnecessary memory being used.
- I also still think that we need to correctly check the cache key for a given object or we will have problems with overriden config.

Status: Needs review » Needs work

The last submitted patch, 109: 1885830-109.patch, failed testing.

effulgentsia’s picture

The method for that would be getCacheKey(), but that is actually flawed, as described in #2177739: Fix inefficient config factory caching, because it can not possibly consider module overrides.

Looks like that issue is now fixed, but from what I can tell, subsequent patches here haven't incorporated the call to getCacheKey(). So, I did so in #2303881: Config entity static cache doesn't get reset and isn't override aware.

effulgentsia’s picture

In #2303881-6: Config entity static cache doesn't get reset and isn't override aware, Berdir asked me:

Would also appreciate your thoughts in #1885830 in regards to enabling it by default vs. only doing it for config entities where we know it's worth it, like roles.

So here's my thoughts:

  • Caching is about spending memory to purchase speed.
  • Since we already have static caching on the raw config arrays, the speed gains in also static caching the config entities on top of that is minimal: (constructor, postLoad(), hook_ENTITY_TYPE_load(), and hook_entity_load()) so long as there aren't expensive implementation of hook_entity_load().
  • However, we don't know what contrib will add. If contrib adds an expensive hook_entity_load() implementation, then static caching the entities is worthwhile.
  • We could answer the above by saying that core shouldn't statically cache config entities by default, and let contrib alter that decision when it implements an expensive hook_entity_load(). But, I have some concerns that if we don't cache by default, then when contrib tries to turn caching on, it will find errors that we didn't catch in our integration tests due to not having that be the default.
  • So, maybe it would be best to cache by default? But if we're concerned about redundant memory usage, would it make sense to somehow tell ConfigFactory to not statically cache the raw config arrays for the config entities that we cache?
  • Or, is the memory usage of config arrays so tiny that it's not worth the bother to optimize away the redundancy?
Crell’s picture

effulgentsia: The double-caching seems unnecessary to me. If we cache the object but NOT the array, wouldn't that get us the CPU savings we're talking about without a significant increase in memory usage? (I'm assuming that the array takes up the bulk of the memory of the object, which seems a safe assumption.)

effulgentsia’s picture

Yes, I think for config entities, caching the entities and not the underlying config array makes the most sense. But ConfigFactory will still need to cache the non-entity config arrays, so this would add some complexity in terms of ConfigFactory knowing (or being told) which config arrays are non-entities and which are entities (or alternatively, which individual config IDs or ID patterns to cache/not-cache). I'm sure that's a solveable problem: just in general, while it's ok for the ConfigEntity system to depend on the Config system, we'd rather not have the Config system knowing too much about the ConfigEntity system.

Crell’s picture

Agreed entirely on the last point. Is there some way for ConfigEntity to tell Config, when loading data, "don't cache this one, I got it"? (In some generic way, of course.)

Berdir’s picture

No, at the moment, there is not, and I also don't think that would be a good idea, config entity queries work on the raw data AFAIK, we want those to rely on the static cache too.

The actual bugs related to config entity caching have been fixed in the related issue, it is being enabled for roles in #2202185: Statically cache Role entities to speed up (User|UserSession)::hasPermission().

In the test fails that we had here, we AFAIK had no real bugs (other than those that were fixed now), only the usual test static cache that had to be cleared after web requests.

The question to ask is IMHO what config entities we expect to load multiple times. And enable static caching for those. Some are only expected to be loaded once or a few times on normal pages, so it does't matter too much if a hook_entity_load() is slow or not.

yched’s picture

Just a note that if we start statically caching EntityViewDisplays, then EntityViewBuilder::viewField() will need to be modified to clone the EVD it loads and modifies...

Berdir’s picture

Status: Needs work » Needs review
FileSize
27.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,031 pass(es), 690 fail(s), and 17 exception(s). View

Not sure yet, but profiling showed some some config entity types being loaded a lot, although entity displays actually are the worst and based on what @yched said, we might need to clone them anyway. Or maybe we can improve how they're loaded, seeing a lot of calls originating in views in my case.

Status: Needs review » Needs work

The last submitted patch, 118: 1885830-118.patch, failed testing.

yched’s picture

profiling showed some some config entity types being loaded a lot, although entity displays actually are the worst

Like, we load the same entity display over and over in the same request ? Weird, I wouldn't have expected that. I wrote #117 "just in case", but I didn't think those would be the primary config entity that needs optim.

Berdir’s picture

Yeah, I think it's if you have views that render fields, then it creates it for every row. Not sure, will need to check again.

yched’s picture

@Berdir: oh right. Can't find it atm but we have an issue open with a plan to make "by field" views less horribly inefficient. Been opened since Portland I think, but we didn't get to it yet :-/

Berdir’s picture

Status: Needs work » Needs review
FileSize
28.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,012 pass(es), 59 fail(s), and 4 exception(s). View
839 bytes

Yeah, and that is actually called by EntityViewController, so we do the collecting twice per request for a normal entity view :( And I hope that was breaking most of those tests, was just testing one of them.

Anyway, cloned directly in collect, because we have that alter there, so we need to do it anyway I guess?

Status: Needs review » Needs work

The last submitted patch, 123: 1885830-123.patch, failed testing.

yched’s picture

Yeah, that $entity->title->view('full') in EnityViewController is weird anyway, we should be passing explicit hardcoded formatter settings there, and not rely on the configuration of the 'full' Display. @jhodgdon opened an issue for that, was stuck on in-place-edit tests breaking.

Anyway, yeah, cloning there makes sense I guess.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.14 KB

EntityStorageBase::delete() assumes that entities are keyed by ID, but we never enforce it. Automatically deleted field storage configs are not, for example.

Berdir’s picture

FileSize
29.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,058 pass(es), 58 fail(s), and 3 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 127: 1885830-126.patch, failed testing.

alexpott’s picture

Issue summary: View changes

Adding a note due to discussion on #2406543: Remove ConfigFactory::setOverrideState and ConfigFactory::getOverrideState(). This issue will also need to prevent the config entities from being cached in the config factory. I think the best implementation will be a parameter on the get() and loadMultiple() methods on the ConfigFactory. However this is not simple as @Berdir points out:

But agreed that for those that we do cache statically, we should not cache them twice. Which is more complicated than it sounds I think, maybe the entity storage class could tell the factory to not cache what it is loading, but there is also the entity query implementation that afaik works on raw config values, so that would make entity queries possibly slower.

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.

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.

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.