Problem/Motivation

As noticed in #2289201: [Meta] Make drupal install and run within reasonable php memory limits so we can reset the memory requirements to lower levels, using the fast chained cache backend during installation, when there are a lot of invalidations and cache writes results in a huge amount of merge queries due to marking the cache as invalid.

Proposed resolution

The easiest fix would be to add a check for the maintenance mode constant in the factory.

Another thought would be to do it in the factory, and possibly even default to the memory backend, as we invalidate most caches anyway with every installed module?

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#11 skip-fast-chained-during-install-2422657-11.patch835 bytesBerdir
#3 skip-fast-chained-in-maintenance-mode-2422657-3.patch877 bytesBerdir
#1 skip-fast-chained-in-maintenance-mode-2422657-1.patch4.18 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
skip-fast-chained-in-maintenance-mode-2422657-1.patch4.18 KB

Here's a patch.

$ time drush si -y standard

HEAD:

real	0m51.489s
user	0m48.185s
sys	0m0.515s

Patch:

real	0m46.191s
user	0m43.159s
sys	0m0.328s

Not a huge difference, but I imagine the difference gets bigger with non-optimized/slower systems and bigger installation profiles.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

The patch would be perfect if it wouldn't include #1768526: NodeFormController::validate() calls buildEntity() twice :P

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
skip-fast-chained-in-maintenance-mode-2422657-3.patch877 bytes

Perfect is overrated anyway ;)

amateescu’s picture

amateescu CreditAttribution: amateescu commented

If we target only the installation process here, shouldn't we use drupal_installation_attempted() instead?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Just thinking through.

-> site goes into maintenance mode, fast backend gets ignored, this is fine.

-> site goes out of maintenance mode, does that need to explicitly invalidate the fast backend at that moment?

Hadn't seen #4 when writing this, but similar question.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Interesting, I'm not sure how to solve that. Wouldn't the same happen if you e.g. do a drush cache clear and you don't have apcu enabled there? I've never seen any issues so far with that, but I might have apcu enabled.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Hmm that's interesting and seems like a possible bug yes.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Any idea on how to prevent that? I can't think of something right now, the only thing I can think of right now is to do an immmediate invalidation if we get to that point once per request. This might work here, but I'm not sure about drush.

Also, if we limit this to just the installer, do we still need to worry about this or can we assume that the fast cache is empty at this point? I guess it could be a problem if you do multiple repeated installs?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

In general this looks okay and Drupal traditionally has many caches off during Maintenance mode.

But it feels very very wrong to reference a global constant encapsulated in a class ...

I am unsure if it possible to do this differently though.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Well, the installer flag is a constant, it's not in the scope of this issue to change that.

The only alternative is using the drupal_installation_attempted() function, which isn't better in that regard.

What if we limit this to just the installer and not all maintenance modes? Do we still have to worry about stale caches in the fast backend? I guess if you re-install multiple times then it could happen. We're not seeing that in the tests right now, but I suspect that is because every test has a different configuration path, and therefore a different sitePrefix.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
skip-fast-chained-during-install-2422657-11.patch835 bytes

So this was not even used in the in-test installations, lets see if this breaks anything. this might also speed up web tests now?

$ time php core/scripts/run-tests.sh --url http://d8/ --class "Drupal\user\Tests\UserSaveTest"

This seems to be slightly faster, 7.05-7.10s with HEAD vs. ~6.875s with the patch. Note that enabling more modules won't change anything, as those aren't added during the installer.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community

The testbot seems happy with the last patch so I think this is good to go.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue is a major task that will improve performance significantly and it introduces no disruption. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed a53a61f and pushed to 8.0.x. Thanks!

  • alexpott committed a53a61f on 8.0.x 
    Issue #2422657 by Berdir: Skip fast chained cache backend in maintenance...
Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

I created #2427823: ChainedFastBackendFactory depends on drupal_installation_attempted() after speaking with catch to clean up the dependency of the OO code to non-OO code introduced here that makes e.g. unit testing impossible or difficult ...

Not sure what we should do, but I think a container variable should be good or something like that ...

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Title: Skip fast chained cache backend in maintenance mode » Skip fast chained cache backend during installer

Opened #2427861: Fast chained backend is incompatible with cli that has apc.enable_cli = 0 (major) after a discussion with catch on this patch. Though this bug was present before.

With the original scope of this issue any MAINTENANCE MODE cache clears would have not propagated to when the site was put back active.

Status: Fixed » Closed (fixed)

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