Problem/Motivation

Most Yaml files are cached by FileCache so we don't have to reread them on a cache clear. See \Drupal\Component\Discovery\YamlDiscovery

This means that library YAML files have to be re-read on a cache clear even when the files do not change. This is different from info.yml, service.yml and any of the plugin yaml files.

Proposed resolution

Use the FileCache in \Drupal\Core\Asset\LibraryDiscoveryParser

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3414807

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:

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
wim leers’s picture

Title: \Drupal\Core\Asset\LibraryDiscoveryParser should use FileCache to cache the parsed yaml » \Drupal\Core\Asset\LibraryDiscoveryParser should use FileCache to cache the parsed YAML
Issue tags: +YAML

I can't find anything to complain about 🤓

smustgrave’s picture

Question, is this a feature we should add test coverage for?

alexpott’s picture

@smustgrave I've added a test - it's not hard to add something small to a unit test for this.

wim leers’s picture

Issue tags: +needs profiling

One nit on the MR.

Should we do some profiling or benchmarking here? 🤔 I think it's not really necessary because this only happens in the cold cache scenario, per:

This means that library YAML files have to be re-read on a cache clear even when the files do not change. This is different from info.yml, service.yml and any of the plugin yaml files.

… but it sure wouldn't hurt. We don't want to make the cold cache scenario more expensive/slower. But if anything, this should make it faster/cheaper.

alexpott’s picture

Issue tags: -needs profiling

For the given code in test_script.php, Standard profile installed, on PHP 8.2 and APCu installed and enabled on CLI:


use Drupal\Component\Utility\Timer;
$modules = array_keys(\Drupal::getContainer()->getParameter('container.modules'));

foreach($modules as $module) {
  \Drupal::service('library.discovery.parser')->buildByExtension($module);
}


for ($i = 0; $i < 10; $i++) {
  drupal_flush_all_caches();
  Timer::start('library.discovery.parser');
  foreach($modules as $module) {
    \Drupal::service('library.discovery.parser')->buildByExtension($module);
  }
  Timer::stop('library.discovery.parser');
}

$time = Timer::read('library.discovery.parser');
print "Library parsing completed in {$time}ms\n";

With MR using Symfony YAML

$ vendor/bin/drush scr test_script.php
Library parsing completed in 63.54ms

Without patch using Symfony YAML

$ vendor/bin/drush scr test_script.php
Library parsing completed in 117.06ms

Without patch and with PECL YAML installed

$ vendor/bin/drush scr test_script.php
Library parsing completed in 75.47ms
wim leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +scalability

Random failure in https://git.drupalcode.org/project/drupal/-/jobs/662217 because the MySQL DB went away, which made 4 unrelated kernel tests fail. I cannot retest this job 🤷‍♂️

#8 provides the numbers that remove any worries about cold cache scenarios 👍

  • catch committed 92670519 on 11.x
    Issue #3414807 by alexpott, Wim Leers, smustgrave: \Drupal\Core\Asset\...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

Status: Fixed » Closed (fixed)

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