Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#17 | interdiff.txt | 2.35 KB | yched |
#17 | config_entity_query-2163919-17.patch | 3.08 KB | yched |
#13 | interdiff.txt | 944 bytes | yched |
#13 | config_entity_query-2163919-13.patch | 2.93 KB | yched |
#12 | interdiff.txt | 1.13 KB | mtift |
Comments
Comment #1
yched CreditAttribution: yched commentedpatch uses conditions on the ID key to narrow a list of config records to load.
Comment #2
yched CreditAttribution: yched commentedGah.
Comment #3
yched CreditAttribution: yched commentedAs 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'")
Comment #4
yched CreditAttribution: yched commentedAfter a bit of self review
Comment #5
mtiftThis probably should be "config ID" to match the other uses in all caps.
Sorry for my ignorance, but when is this true? I have not figured out how to properly debug this patch.
Comment #6
BerdirSomething like this should allow you to test it:
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()).
Comment #7
yched CreditAttribution: yched commentedAttached 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):
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 ?
Comment #8
yched CreditAttribution: yched commentedAlso, 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
, 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.
Comment #9
yched CreditAttribution: yched commentedScratch 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)
Comment #10
mtiftReroll after #2005716: Promote EntityType to a domain object. (I still plan to look at this one more closely.)
Comment #12
mtiftAnd now this should fix the new code, too :)
Comment #13
yched CreditAttribution: yched commentedThanks @mtift !
Just a minor polish I thought I fixed in a previous iteration but actually didn't.
Comment #14
mtiftI would say this patch is a definite improvement. I created a simple "Hello World" module that includes this call to Query::execute():
Here is the XHProf Overall Diff Summary with before (left/52caece0e37bb) and after (right/52caebbc2e590):
Comment #15
yched CreditAttribution: yched commentedBump ? RTBC anyone ? :-)
Comment #16
sunThis 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
Comment #17
yched CreditAttribution: yched commentedSure.
Comment #18
sunThanks!
Not sure why the additional is_string(field) condition is needed, but it certainly can't hurt.
Comment #19
yched CreditAttribution: yched commentedis_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.
Comment #20
alexpottCommitted ddfbbf5 and pushed to 8.x. Thanks!