Problem/Motivation

Looking through the queries of my performance tests, I noticed that we seem to be querying the file_managed table twice, once with a wildcard for all fields, once with all fields listed explicitly.

For example: In \Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache for example.

'SELECT "base"."fid" AS "fid", "base"."uuid" AS "uuid", "base"."langcode" AS "langcode", "base"."uid" AS "uid", "base"."filename" AS "filename", "base"."uri" AS "uri", "base"."filemime" AS "filemime", "base"."filesize" AS "filesize", "base"."status" AS "status", "base"."created" AS "created", "base"."changed" AS "changed" FROM "file_managed" "base" WHERE "base"."fid" IN (1)',
      'SELECT "file_managed".*, "file_managed"."langcode" AS "file_managed__langcode" FROM "file_managed" "file_managed" WHERE "file_managed"."fid" IN (1)',

The first call happens directly in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::getFromStorage() through buildQuery().

The second is in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromDedicatedTables().

I think this is a side effect of the merged query for the configurable, but if there are none of those, then this query doesn't seem to do anything useful.

What I notice is that $chunks is a nested empty array, [0 => []], so it does the loop with nothing. But we can't just skip that as it might contain revision/data. Maybe we can check for those skip if we don't have any of that?

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3608374

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

berdir created an issue. See original summary.

berdir opened merge request !16215

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review

Lets see what the tests think about this.

This saves 12/14 queries for my performance test on cold/cool caches (265 -> 251, 198 -> 186)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Saves 6-7 queries on the umami performance tests too, no other test fails except the performance tests.

catch’s picture

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

This looks great. Approaching 10% query reduction on the lighter pages.

godotislate closed merge request !16215

  • godotislate committed 98c10fdf on main 
    fix: #3608374 Data for non-revisionable/non-translatable/no-dedicated-...
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 98c10fd and pushed to main.

There's a merge conflict in 11.x, presumably in the performance test numbers, so we'll need an 11.x MR.

Also, I think this is good for backport to 11.4.x as well, but will confirm.

godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
catch’s picture

catch
he/him
Primary language English
commented

I think this will be fine for an 11.4.x commit, no logic changes at all.

berdir opened merge request !16233

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Patch (to be ported) » Needs review

Rebased, query counts on the performance tests are the same, it just conflicted on the differences in context (the cache get/sets)

godotislate’s picture

godotislate
he/him
commented

There was a test failure, but I think it's intermittent because I've seen it fail a few times the last couple days, so re-running. Will add to the known list of flaky tests if so.

---- Drupal\Tests\config\Functional\ConfigInstallProfileOverrideTest ----
Status      Duration Info                                                                               
--------------------------------------------------------------------------------------------------------
Error        13.628s testInstallProfileConfigOverwrite                                               
Failure              *** Process execution output ***                                                
    PHPUnit 11.5.55 by Sebastian Bergmann and contributors.
    
    Runtime:       PHP 8.5.8
    Configuration: /builds/project/drupal/core/phpunit.xml.dist
    
    E                                                                   1 / 1 (100%)
    
    Time: 00:14.294, Memory: 10.00 MB
    
    Config Install Profile Override (Drupal\Tests\config\Functional\ConfigInstallProfileOverride)
     ✘ Install profile config overwrite
       ┐
       ├ Error: Call to a member function label() on null
       │
       │ /builds/project/drupal/core/modules/config/tests/src/Functional/ConfigInstallProfileOverrideTest.php:141
       ┴
    
    ERRORS!
    Tests: 1, Assertions: 23, Errors: 1.
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Green now, yes I think I've seen that a few times.

godotislate closed merge request !16233

  • godotislate committed 4d79b69a on 11.4.x 
    fix: #3608374 Data for non-revisionable/non-translatable/no-dedicated-...

  • godotislate committed 978748dd on 11.x 
    fix: #3608374 Data for non-revisionable/non-translatable/no-dedicated-...
godotislate’s picture

godotislate
he/him
commented
Version: 11.x-dev » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 978748d to 11.x and 4d79b69 to 11.4.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.

godotislate’s picture

godotislate
he/him
commented

Created #3608805: [random test failure] Drupal\Tests\config\Functional\ConfigInstallProfileOverrideTest::testInstallProfileConfigOverwrite and added to #2829040: [meta] Known intermittent, random, and environment-specific test failures.

Status: Fixed » Closed (fixed)

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