Problem/Motivation

These are requested multiple times on cold caches, the class already has a static cache for the entity definitions.

While this is in chained fast, on a fully cold cache request, chained fast doesn't kick in yet (any other cache set invalidates the cache, and we have an explicit warm-up time after #3526080: Reduce write contention to the fast and consistent backend in ChainedFastBackend.

Steps to reproduce

NA

Proposed resolution

Setting a protected variable entityFieldStorageDefinitions to store the entity definition vs doing cache lookup everytime.

Remaining tasks

User interface changes

NA

Introduced terminology

NA

API changes

NA

Data model changes

NA

Release notes snippet

NA

Issue fork drupal-3564969

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

catch opened merge request !14179

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

smustgrave made their first commit to this issue’s fork.

smustgrave’s picture

smustgrave commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Re-ran the tests 3 times but they are green. Some media test just didn't want to play along.

Tried to update the summary best I could but may need a tweak. But performance reduction speaks for itself.

alexpott’s picture

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

Needs a reroll because there are MR conflicts.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Rebased.

alexpott’s picture

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

This MR is causing failures in core/modules/media/tests/src/FunctionalJavascript/MediaSourceFileTest.php and another JS test somehow... reproduced the fails locally.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I think we're missing a static cache reset in \Drupal\Core\Entity\EntityLastInstalledSchemaRepository::deleteLastInstalledDefinition

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Back to green. I switched from = NULL to unset() which doesn't do anything but looks nicer to me, and added an unset where it was missing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 43a14ed5f0c to main and 8b9a7e5d8a6 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed 8b9a7e5d on 11.x 
    perf: #3564969 Static cache field storage definitions

By: catch
By:...

  • alexpott committed 43a14ed5 on main 
    perf: #3564969 Static cache field storage definitions

By: catch
By:...

nicxvan closed merge request !14179

Status: Fixed » Closed (fixed)

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