Config\Entity\Query\Query::execute() loads all config records for the entity type in memory, and parses them all to check for conditions.

In simple cases where the query contains conditions on the entity 'id', that's a lot of needless computation.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

patch uses conditions on the ID key to narrow a list of config records to load.

yched’s picture

Status: Active » Needs review

Gah.

yched’s picture

As a side note, this would really enhance performance of #2090509: Optimize entity_get_render_display() for the case of "multiple entity view" - which is on the critical path for listing pages (the goal over there is "among this list of entity display ids, load the ones with 'active: true'")

yched’s picture

After a bit of self review

mtift’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/Query/Query.php
@@ -124,4 +117,54 @@ public function execute() {
+    // If there are conditions on the config id, we can narrow the list of

This probably should be "config ID" to match the other uses in all caps.

+++ b/core/lib/Drupal/Core/Config/Entity/Query/Query.php
@@ -124,4 +117,54 @@ public function execute() {
+        if ($condition['field'] == $entity_info['entity_keys']['id']) {

Sorry for my ignorance, but when is this true? I have not figured out how to properly debug this patch.

Berdir’s picture

Something like this should allow you to test it:

$ids = Drupal::entityQuery('field_entity')
  ->condition('id', 'node.body')
  ->execute();

That's of course not a very useful example, but it could for example be combined with an additional condition to only load those with a given value in another property.

What about supporting STARTS_WITH as well? That could be used to e.g. load all fields of a given entity type or instances of a specific bundle in a fairly performant way (cached list query + cached getMultiple()).

yched’s picture

Attached patch fixes remark #5 (also fixes a 80 char overflow)

@mtift: yes, the specific use case I have is this, which happens on entity_view_multiple($entities):

$ids = Drupal::entityQuery('entity_display')
  ->condition('id', array(... N candidate ids [2 per displayed $entity] ...)
  ->condition('active', TRUE)
  ->execute();

With the patch, this will load and parse at most N records in memory, instead of all entity_display records currently.

@Berdir - STARTS_WITH:
Could be cool indeed. listAll() would support this, the only thing that makes me hesitate is that CachedStorage::listAll($prefixes) creates a cache entry for all prefixes that ever get searched. While this is reasonable when the prefix is the $entity_info['config_prefix'], I'm not sure we want to let EFQs silently fill cache entries ?

yched’s picture

Also, our current critical code for "load all fields of an entity type / all field instances of a bundle" in FieldInfo doesn't need an EFQ to get a list of $ids to multiple_load(), that list is build using the fieldMap().

The patch here is only useful when you do an EFQ because you want to filter by another property in addition to knowing a list of ids. Not saying that there wouldn't be a case for optimising queries like

$efq->condition('id', 'foo.bar', 'STARTS_WITH')
  ->condition(.. other stuff...)

, but at least right now I see no critical need within Field API :-)

This being said, block_list() could probably use listAll("block.block.{$theme}_") instead of an entity_load_multiple_by_properties('block', array('theme' => $theme)) currently...
But that's not an EFQ either.

yched’s picture

Scratch that last part about block_list() & listAll("block.block.{$theme}_"), I realized that $theme is only there because it's part of the machine names of blocks shipped by standard.profile.

More thoughts on blocks perf in #2161591-8: Change default active config from file storage to DB storage.

Meanwhile, any thoughts on STARTS_WITH ? (see end of #7)

mtift’s picture

Reroll after #2005716: Promote EntityType to a domain object. (I still plan to look at this one more closely.)

The last submitted patch, 10: config_entity_query-2163919-10.patch, failed testing.

mtift’s picture

And now this should fix the new code, too :)

yched’s picture

Thanks @mtift !

Just a minor polish I thought I fixed in a previous iteration but actually didn't.

mtift’s picture

I would say this patch is a definite improvement. I created a simple "Hello World" module that includes this call to Query::execute():

    $ids = \Drupal::entityQuery('field_entity')
      ->condition('id', array('node.body'))
      ->execute();

Here is the XHProf Overall Diff Summary with before (left/52caece0e37bb) and after (right/52caebbc2e590):

Run #52caece0e37bb Run #52caebbc2e590 Diff Diff%
Number of Function Calls 340,558 45,235 -295,323 -86.7%
Incl. Wall Time (microsec) 6,354,192 223,447 -6,130,745 -96.5%
Incl. CPU (microsecs) 1,400,088 220,014 -1,180,074 -84.3%
Incl. MemUse (bytes) 29,584,816 13,562,288 -16,022,528 -54.2%
Incl. PeakMemUse (bytes) 29,820,232 13,668,336 -16,151,896 -54.2%
yched’s picture

Bump ? RTBC anyone ? :-)

sun’s picture

+          $operator = $condition['operator'] ?: (is_array($condition['value']) ? 'IN' : '=');
+          if ($operator == '=' || $operator == 'IN') {
+            $ids = is_array($condition['value']) ? $condition['value'] : array($condition['value']);
+            // We can stop at the first condition on ID. In the (weird) case
+            // where other conditions additionally restrict IDs, results will
+            // be eliminated when the conditions are checked on the loaded
+            // records.
+            break;
+          }

This code path could have been split into two code paths via if + elseif, since one of them operates on an array and the other one operates on a string. The single code path makes it hard to read the code.

That would also allow us to be a bit more explicit about the IN code path. That is, because I'm not sure whether this automatic assumption of IN is fully compatible with the discussion in #2161943: Throw a helpful exception for empty IN conditions in Database\Query\Condition

yched’s picture

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Not sure why the additional is_string(field) condition is needed, but it certainly can't hurt.

yched’s picture

is_string(field) : that's because some conditions are "AND / OR conditions groups" rather than conditions on a specific field. In those cases, $condition['field'] is a ConditionInterface rather than a string. We only look at non-group conditions.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ddfbbf5 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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