#Performance

For @tedbow's question on the MR.

I'm not sure why it thought the MR no longer applies. It was fine. Restoring RTBC.

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Requested a few changes in the MR.

#17 that's pretty accurate. If we remove it outright, we'd need to do the following:

1. Copy the CSS to somewhere in stable9 that's always loaded. stable9 doesn't have a catch-all file, only overrides, so either need to pick an existing file that's not a perfect match or add a legacy.css or something.

2. Add the CSS to Claro's tabs CSS somewhere.

https://github.com/Otamay/potracio is a PHP port of potrace that's GPL licensed.

Some good discussion of the various placeholder generation approaches in https://github.com/axe312ger/sqip/issues/116

New title as we want to proceed with this.

Here's the explain for selecting all entities using e.g. a particular component. We use the same buildDependencyQuery code path for the uninstall validator so the query is the same with only the params changed.

+--+-----------+------------+----------+----+--------------------------------+----------+-------+-----------+----+--------+-----------+
|id|select_type|table       |partitions|type|possible_keys                   |key       |key_len|ref        |rows|filtered|Extra      |
+--+-----------+------------+----------+----+--------------------------------+----------+-------+-----------+----+--------+-----------+
|1 |SIMPLE     |dependencies|null      |ref |PRIMARY,source_entity,dependency|dependency|267    |const,const|1   |100     |Using index|
+--+-----------+------------+----------+----+--------------------------------+----------+-------+-----------+----+--------+-----------+

This is ready for review now

Having written it up, the most important thing here is the 'inline' - if we can make that work, then SVG hopefully lets us make better placeholders but tiny webp would work too.

For the image style, we don't actually need a queue, when rendering the HTML, if the placeholder derivative file exists on disk we can load it and inline it, if it doesn't exist, we can render the URL, set a max-age of 0 (or 30s), and disable the placeholder. When the URL is visited, it'll create the file on disk, and the next time it's render it'll get inlined. Should only happen once in the lifecycle of an individual image, and often immediately after the content is created.

There hasn't been a response to indicate that this is needed or not. Therefor, closing.

If there is work to do here, then either re-open the issue or open a new issue and reference this one. If the choice is to use this issue then add a comment change make sure to change the issue status to 'Active'.

I think this is safe to close, there is no rollback method that is mentioned in the issue summary, a different solution was committed.

Automatically closed - issue fixed for 2 weeks with no activity.

working on the fix. I think for now we should allow e2e test to opt of aggregation

You have the whole configuration set for a given block, which might very well include internal information, maybe you have a block with an API key in the config or something like that.

The lazy builder could take the entity ID of the config or content entity, load it, and then find the config for the block in there. It would need enough information to locate it though, not sure what all of that would be (field name at least on entities etc.) but should be finite.

I think this is at NW based on latest comments.

I don't know too much about XB yet, but I tried a long time ago to do this with page manager, which turned out to be hard to impossible.

A lazy builder must be able to pass all necessary information as a serialized string.

block.module blocks are 100% standalone and isolated, identified just by their config entity ID, so that's easy. The problem for page manager was context (the block/plugin context system). For example, you could have a view with a term argument, and on the page manager page, you have multiple terms as context that you pass in to the various views blocks.

I think XB does not currently have anything like that and not sure if it ever will. But even then, you don't just have a string/ID, You have the whole configuration set for a given block, which might very well include internal information, maybe you have a block with an API key in the config or something like that. So that can't just be serialized out into the HTML placeholder, might also be quite long. Maybe some kind of key-value lookup, a bit like the configuration for entity autocomplete form elements.

Two remarks, but I don't see why this MR can't be RTBC once they are discussed/resolved.

Leaving at NR as there are no actionable items, yet. Depends on where the discussion leads us.

Also, autoplacholdering doesn't disable caching like max-age 0 does, it would still be cached on its own.

Which is why I added:

Then all we'd have to do is adjust RenderCache a bit to also check for that cache tag

But aside from that detail, I agree that making this rely on config might not be such a great idea. Your further clarifications I also tend to agree with. The suggested implementation here is one of "do not cache", but it doesn't mean that other implementations have to go down that same path.

Okay in that spirit I'll go over the MR again, but I would cautiously agree with the approach. We need to be very aware of #108, though. Ideally those issues get fixed sooner rather than later so the changes introduced here don't run havoc there, even if it's technically not "our fault" if it does.

The autoplaceholder config is config. We can change the defaults, but it wouldn't really apply to all existing sites that have customized this. IMHO that makes this very hard to control and those sites will then have negative effects from not having that.

I also think that cache tag bubbling is confusing with that special cache tag. I'm not exactly sure about exact behavior, but it seems unpredictable. As we learned recently, before ✨ Optimize placeholder retrieval from cache Active , placeholdered elements were still included if their element was already render cached, there might be other unexpected side effects.

Also, autoplacholdering doesn't disable caching like max-age 0 does, it would still be cached on its own.

It might not have been clear in the issue where we added it, but I already had this issue in the back of my head to use it here as well, that's why I moved it to the cache namespace and made it more generic. We should extend the docs to mention block plugins as well. In my mind, it's less about supporting CacheOptionalInterface in render arrays (we don't), it's supported specifically for block plugins and makes them not cacheable in the initial definition, but that's an implementation detail of BlockViewBuilder. Other places that support block plugins, such as layout builder, twig or block_field do not currently support per-block caching or placeholdering at all. Which, to reply to #107 as well, is IMHO OK, because it says "cache optional", not "must not be cached". In other words, "not worth caching on its own", which I think fits the use case in access policies.

Just adding this: I like how clean the current MR looks and I can see the reasoning behind it. But flagging the LanguageBlock as "cache optional" does not cover the load here. Because we want it to not be cached, ever. So it's hardly optional. Furthermore, we also want to placeholder it. And I can see that (placeholder & no cache) being the case for more pieces of content in the future.

Hence my suggestion for something that would both indicate we do not want to cache it and we want to placeholder it.

Quoting option B:

B) Use the new \Drupal\Core\Block\BlockPluginInterface::createPlaceholder() to force this to be a placeholder and cache it separately. In our projects, we're seeing 10k+ variants of this block, very rarely reused. That's MR !9798.

However, that MR seems to also involve CacheOptionalInterface? And I'm not sure we want to be using that here.

My reasoning is this: CacheOptionalInterface was introduced with access policies, where multiple smaller pieces will be cached together as a whole. In that context the interface meant: "If all of the smaller pieces indicate they are insignificant, we need not cache the whole."

However, render arrays with cache keys (including the language switcher block) aren't necessarily like that. They get cached individually and as part of a whole (DPC). So using the interface there seems ambiguous as it's not clear in which caching layer they would be considered optional.

Now, with that in mind, LanguageBlock should already be auto-placeholdered by virtue of its max age being 0 and BlockViewBuilder making it use a lazy builder. So setting createPlaceholder() should effectively change nothing about the current situation. The block will be placeholdered and DPC will not cache it.

So it seems the goal is to rid LanguageBlock if its max age of 0 and then make sure it doesn't poison the page. That part is not fully clear from the issue title and summary.

In that case, option B seems like the most straightforward fix, but we would indeed not want to individually cache all variations of LanguageBlock because it varies too much. So removing it's max-age 0 is a good thing, as it would otherwise kill all page caching when combined with 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed .

Then again, if we merely want to placeholder it and we don't want the placeholdered HTML to be cached, we have more options than to resort to max age 0. I'm wondering if this was ever explored. Let's look at renderer.config:

renderer.config:
required_cache_contexts: ['languages:language_interface', 'theme', 'user.permissions']
auto_placeholder_conditions:
max-age: 0
contexts: ['session', 'user']
tags: []
debug: false

As you can see, we could achieve the very same result of having LanguageBlock auto-placeholdered by providing a cache tag that indicates the desired behavior. Much like the special "CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:" cache tag we have. This tag would also bubble up to FinishResponseSubscriber, but it would have zero effect on the max-age of the page.

So rather than use CacheOptionalInterface in a way that might confuse people, why don't we introduce a "SHOULD_AUTO_PLACEHOLDER_BUT_NOT_CACHE" cache tag, add it to LanguageBlock and renderer.config.auto_placeholder_conditions.tags?

Then all we'd have to do is adjust RenderCache a bit to also check for that cache tag and we'd have a system that works for ALL render arrays (with a lazy builder) rather than having to figure out ways to apply an interface to them.

Legitimate failure:

Tried to find [href*="components/my-hero/my-hero.css"] in <head> 

Due to this test assuming non-aggregation:

    // Confirm that the iframe loads the SDC CSS.
    cy.getIframe(
      '[data-xb-preview="lg"][data-test-xb-content-initialized="true"][data-xb-swap-active="true"]',
    )
      .its('head')
      .should('not.be.undefined')
      .then((head) => {
        expect(
          head.querySelector(
            'link[rel="stylesheet"][href*="components/my-hero/my-hero.css"]',
          ),
          `Tried to find [href*="components/my-hero/my-hero.css"] in <head> ${head.innerHTML}`,
        ).to.exist;
      });

Trivial conflict resolution.

🐛 Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active is fixed

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

re #13.2 that seems to be known side effect of 📌 Compile Tailwind CSS globally for code components Active . It will be fixed by 🐛 Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active

Postponing on #3508937 because it will be easier to test after that

1. re #12

   Instead of enabling aggregation for XbPageVariantTest I enabled aggregation in \Drupal\Tests\experience_builder\Functional\FunctionalTestBase::setUp which is the base test that XbPageVariantTest extends. This way all our functional tests will run with CSS aggregation on not just XbPageVariantTest.

   I since found the only test that fails in this situation is \Drupal\Tests\experience_builder\Functional\AssetLibraryAttachmentTest because we are checking for CSS and JS file paths in the HTML, so I disabled aggregation just for this test

2. During manual testing I was finding weird behavior where the code component do not look like do in the editor preview. This affects user 1 also with aggregation off. I have tried testing on 0.x and still see the problem. so I am investigating that.

#10: can you rephrase? I don't understand. 😅

re #8

also enable CSS + JS aggregation for \Drupal\Tests\experience_builder\Functional\XbPageVariantTest

Since XbPageVariantTest extends FunctionalTestBase enabled it in FunctionalTestBase so we don't write functional tests that only work aggregation off. But if there is fail I can switch to XbPageVariantTest is needed.

Doing the manual testing now

For #8.

larowlan → credited fabianx → .

Committed to 11.x - thanks!
Published the change record

larowlan → closed merge request !11263

Since these are all minor, going to self-RTBC for the changes.

Added to $pre_bubbling_elements - it won't make any difference in practice due to CachedPlaceholderStrategy but Renderer itself doesn't know this, so we should do it. Also applied the other suggestions and added the extra comment to the test pre_render.

larowlan → closed merge request !11838

Committed to 11.x
I think we could make a case that this is a bug and backport it - thoughts?

Left one question on the MR about adding the key and value to $pre_bubbling_elements

@tedbow: Please:

1. also enable CSS + JS aggregation for \Drupal\Tests\experience_builder\Functional\XbPageVariantTest to ensure we have pure BE test coverage (because I doubt the end-to-end tests test the live site, they AFAIK only test the XB editing UX)
2. do a final round of manual testing with a code component that has an auto-save entry, and verify that this also works as the anonymous user.

Looking good!

Looks great!

But \Drupal\Tests\experience_builder\Kernel\Plugin\ExperienceBuilder\ComponentSource\BlockComponentTest::testRenderComponentLive() (unlike what @longwave indicated in #11? 🤔) still has no changes and hence:

'cacheability' => $default_cacheability,

for all of its tested blocks. Let's make \Drupal\xb_test_block\Plugin\Block\XbTestBlockInputNone vary by the user cache context and have it do something like:

diff --git a/tests/modules/xb_test_block/src/Plugin/Block/XbTestBlockInputNone.php b/tests/modules/xb_test_block/src/Plugin/Block/XbTestBlockInputNone.php
index c7a152a19..52a413e00 100644
--- a/tests/modules/xb_test_block/src/Plugin/Block/XbTestBlockInputNone.php
+++ b/tests/modules/xb_test_block/src/Plugin/Block/XbTestBlockInputNone.php
@@ -19,7 +19,7 @@ final class XbTestBlockInputNone extends BlockBase {
    */
   public function build() {
     return [
-      '#markup' => '<div>Hello, XB!</div>',
+      '#markup' => '<div>Hello %user-name, from XB!</div>',
     ];
   }
 

🐛 Auto-saved Javascript Components CSS changes do not work with CSS aggregation Active is in!

Moving back to "Needs work" as after rebasing some unit tests are failing.

Committed 8c4591e and pushed to 11.x. Thanks!

longwave → closed merge request !11837

🥳🤓

POC in draft MR
Need to move ComponentAuditTest to a Kernel test, mocking database calls is the trigger to move it from a Unit test

FieldTypeUninstallValidatorTest and ComponentAuditControllerTest are passing with this stored for querying.

I've tried passing the display's cache tags into the set() call so, in theory, it should be invalidated automatically when the display is saved but that doesn't seem to be the case. Must be missing something?

Much nicer solution, thanks for the links. I didn't see any decisions on 🌱 [policy] Standardize how we implement in-memory caches Needs work with how these memory cache services should be setup wrt. service id names or how specific/generic they should be but I've loosely tried to follow what other things are doing in core already with cache.asset_memory and system.module_admin_links_memory_cache

Although there aren't any specific tests I don't see why this would cause any problems, let's ship it and hopefully improve performance.

Committed b323820 and pushed to 11.x. Thanks!

longwave → closed merge request !11537

longwave → credited kristiaanvandeneynde → .

TokuDB for MySQL looks to be abandoned since about 2015 and https://www.tokutek.com/ doesn't even resolve any more so there seems no point in keeping this open.

📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active is in.

@larowlan: I'd love to take you up on your offer! 😄😇

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

RTBC++ Nice to see it solved via new interface!

Agreed with @kimpepper's feedback on the MR. Didn't do an in-depth review of everything.

The order change makes sense, back to RTBC.

Examples for #59. Let's assume we have one cache item, and one cache invalidation for node:1, and our one cache item is tagged with node:1

If we clear cache bins before tags, then the following can happen:

1. Cache bins are cleared.

2. Cache miss happens, cache item is written with node:1 having 1 invalidation - because cache tag invalidations haven't been purged yet.

3. Cache invalidations are purged.

4. If node:1 is invalidated before our cache item is requested, then we're back to 1 invalidation again, and the item could be considered valid.

But if we purge cache tags before bins:

1. cache tags are purged

2. Any cache tagged cache items immediately become invalid if they assume any invalidations, because tag invalidations were reset.

3. Any new cache items get written as if there are no invalidations. Or with one invalidation if one happens during this (short) window.

4. But then, all the cache bins are emptied anyway, any new items, regardless of how many cache tag invalidations there are, will be valid.

Given that, purging tags first feels like it should be 100% correct as soon as the full cache clear is complete.

Seems already been reviewed and feedback has been addressed. Didn't see anything additional.

Added change record.

Also in converation with @catch on Slack, moved the `cachetags` table truncation before clearing the cache bins, because having entries in the cachetags table doesn't hurt anything, but there could be an issue if the cache bin tables do without corresponding checksums in cachetags. Pushing back to NR for that.