Problem/Motivation

LibraryDependencyResolver::getMinimalRepresentativeSubset is used as part of the new asset pipeline which is used to generate urls and dependencies. This can be a pretty big deal for high performance websites as the consequence can be that the generated hash when generating the js / css file and when delivering said file wont match, causing increase load as the website now need to generate the file on every request.

Steps to reproduce

I will provide a patch with a test case, as that is the easiest way to reproduce the error.

Proposed resolution

Correct the code in getMinimalRepresentativeSubset

CommentFileSizeAuthor
#2 3400485-test-fail.patch1.84 KBgoogletorp

Issue fork drupal-3400485

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

googletorp created an issue. See original summary.

googletorp’s picture

googletorp commented
StatusFileSize
new 3400485-test-fail.patch1.84 KB

I have created a test case which shows the failing issue and created a fix for the problem in the issue fork.

In general the problem is that before we did this:

foreach ($libraries as $library) {
  $with_deps = $this->getLibrariesWithDependencies([$library]);
  // We don't need library itself listed in the dependencies.
  $all_dependencies += array_diff($with_deps, [$library]);
}

The key problem is here:

$all_dependencies += array_diff($with_deps, [$library]);

I guess whoever wrote this assumes that this gives you the sum of the two arrays, but that is not the case, this only add the dependencies that exceeds the amount of dependencies we have of the left hand side. The test case illustrates this.

googletorp’s picture

googletorp commented
Status: Active » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

nod_ opened merge request !5366

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review

Created a MR from your branch so that the issue can be reviewed

smustgrave’s picture

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

Change I believe makes sense to avoid duplicates

xjm’s picture

xjm
she/her
Primary language English
commented
Title: LibraryDependencyResolver::getMinimalRepresentativeSubset calculates dependencies incorrectly » LibraryDependencyResolver::getMinimalRepresentativeSubset() calculates dependencies incorrectly

Saving credits.

xjm’s picture

xjm
she/her
Primary language English
commented

Finally got the test-only job to run and confirmed the results still match the previous test-only patch:

1) Drupal\Tests\Core\Asset\LibraryDependencyResolverTest::testGetMinimalRepresentativeSubset with data set #14 (array('test/deps_a', 'test/deps_d', 'test/no_deps_d'), array('test/deps_a', 'test/deps_d'))
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'test/deps_a'
     1 => 'test/deps_d'
+    2 => 'test/no_deps_d'
 )
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/project/drupal/core/tests/Drupal/Tests/Core/Asset/LibraryDependencyResolverTest.php:174
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/project/drupal/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/project/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/project/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:97
FAILURES!

  • xjm committed c0895d57 on 11.x 
    Issue #3400485 by googletorp, nod_: LibraryDependencyResolver::...

  • xjm committed 58fe39f6 on 10.2.x 
    Issue #3400485 by googletorp, nod_: LibraryDependencyResolver::...

  • xjm committed 3307cfb8 on 10.1.x 
    Issue #3400485 by googletorp, nod_: LibraryDependencyResolver::...
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 10.2.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 11.x, 10.2.x, and 10.1.x as a minimally disruptive data integrity bugfix. Thanks!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2407187: Optimize LibraryDependencyResolver::getMinimalRepresentativeSubset() and win >=4%, +#2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests

Wow! Looks like this was a regression introduced by #2407187: Optimize LibraryDependencyResolver::getMinimalRepresentativeSubset() and win >=4%, since that changed the logic originally introduced by #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests.

This likely reduced the ~4% performance improvement that #2407187 brought, but obviously correctness is more important.

Status: Fixed » Closed (fixed)

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

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Related issues: +#3397713: [Performance regression] Starting with Drupal 10.1, some sites hit PHP for every page view due to aggregated asset URL hash mismatch from different order of asset items

So glad someone else was able to narrow down the problem here. I spent a long time on this in #3397713: [Performance regression] Starting with Drupal 10.1, some sites hit PHP for every page view due to aggregated asset URL hash mismatch from different order of asset items but wasn't able to pin it down. Thank you!!