The time to add a new field, and the time to delete a field, grows with the number of existing fields.
I've been doing some benchmarking of the entity field api to explore whether it could serve as the foundation for webform module in D8 (see Issue #2075941). This benchmarking uncovered the following behaviour in 8.0.0-beta9:
This behaviour could pose performance problems for sites with a large number of fields, webform or no.
I used the attached script of custom drush commands to benchmark. (Change filename to 'entityform.drush.inc' and place in .drush folder).
// Add 100 content types.
drush ebct node 100
// Add 10 fields to each content type.
drush ebaf node all 10
Then add/delete a field via the UI.
I'll preface this by saying I am relatively new to D8 core, but I believe the cause has to do with certain methods defined in EntityManagerInterface that fetch field config/field storage config definitions for all bundles of a single entity type, or for all bundles of all entity types. For example, EntityManager::getFieldMap() builds a map of all field definitions across all entities. This method is executed when a field is deleted, and xhprof tells me it's very costly (~9 seconds in the above benchmarking).
I'm not sure if this is related, but from poking around in the DB I also noticed that for each entity type, there exists a serialized array of all field_storage_config entities belonging to it. In the above benchmarking (1,000 total node fields), the size of the serialized array for the node entity type was approaching 1MB in size.
A solution would involve making these methods accept entity type and bundle arguments so that field configuration is only fetched in smaller pieces at a time. Code that uses these methods would need to be refactored. I suspect this would require some refactoring of the config architecture for fields as well, but I'm not familiar enough with those internals to say for sure.
1. Validate above results.
2. If validated, isolate causes and decide whether fixes are necessary.
3. If a fix is deemed necessary, create patch(es).
None that I can see. This is all back-end stuff.
Changes to the EntityManager as discussed above.
Possibly changes to the config/caching system for fields.
| Comment | File | Size | Author |
|---|---|---|---|
| #30 | entity_field_bench.tar_.gz | 11.98 KB | mesch |
| #27 | field-map-state-2473983-27-interdiff.txt | 27.56 KB | berdir |
| #27 | field-map-state-2473983-27.patch | 34.21 KB | berdir |
| #18 | field_map.php_.txt | 1.04 KB | berdir |
| #14 | field-map-state-2473983-14-interdiff.txt | 9.5 KB | berdir |
Comments
Comment #1
mesch commentedComment #2
hass commentedPer plach, https://www.drupal.org/node/2075941#comment-9843115
Comment #3
berdirWe actually discussed this yesterday at a dev days dinner with @fago and @amateescu.
And yes, we're aware of the getFieldMap() bottleneck, and we had two ideas on how to resolve that yesterday:
1) Kill it. But that might not be possible.
2) Make it a persistent storage, not a cache. Introduce new methods on entity manager to notify him about new fields + he listens on on some events, then maintain the field map in state/key value.
Note that I don't think that using bundles configurable fields like this is a scalable solution for webform. Even if we fix this, the configuration system is just not mean to store tens of thousands of configuration files. As I explained in my D8 contrib overview presentation yesterday at DDD, that's a limitation that the contact.module has, otherwise, it's a very interesting alternative if you just need a few contact forms on your site with a few fields.
Maybe you can use the widgets/formatters/field item classes anyway and create runtime FieldDefinition(Storage)Interface objects, that aren't exposed through entity manager....
Comment #4
berdirMight experiment with this a bit during my train ride...
Comment #5
berdirhttps://www.drupal.org/node/2473983
Comment #7
berdirFixing some obvious bugs.
Comment #9
dawehnerSo is the plan to remove the cache there?
Comment #10
berdirProbably not, because we still have a the loop over the field definitions, which might still result in quite a lot of separate cache calls and memory usage.
My idea was to have the mix, with maintaining the per-bundle fields in state, and the base fields by looping over them, otherwise we'd have to keep a separate list of them and act on entity type creation and changes. And I don't think those are a scaling problem, we have much, much less entity types than entity types * their bundles.
Comment #11
dawehnerAh I see, it makes sense to have kinda both. Just tagging the issue as we need to update and describe the implemented proposal
Comment #12
berdirThe remaining test failures should be fixable by implementing onFieldDefinitionDelete() and remove the field again (remember to remove the field completely if no bundles are left) and call that from postDelete() in FieldConfig. They're all about field storage configs getting deleted automatically.
I was also wondering if we should update that and do a config entity query there instead for the check, but that's not really related.
I don't have time before tonight, so if anyone wants to work on this issue, this should be relatively easy to push forward (that method, documentation, unit tests will be harder...)
Comment #13
catchSplitting out seems OK, but why use state? Using the cache API but ensuring it's write-through (and not cleared when it shouldn't be) should be enough?
Comment #14
berdirA write through cache is still a cache. It would get deleted during cache clears and we would have to spend 9s and tons of memory to rebuild the list.
Comment #15
catchWhich cache clears though? We're pretty close to not invalidating caches when we don't need to.
Comment #16
berdirdrupal_flush_all_caches(). We still do that when people clear the cache manually, on update.php and so on.
I'll do some profiling/benchmarking with field creating and cache clears soon.
Comment #17
catchRight I think that's preferable to putting caches into state to work around this. We could also remove the full cache clear from update.php if we want to - vast majority of updates don't need one, and rebuild.php or drush cc all can always be called separately.
The distinction between cache/state/key value is quite nuanced so core should really try to keep the separation clear. I've seen a (high-ish usage) Drupal 7 contrib module use variable_set()/variable_get() as a static cache/global replacement before.
Also drupal_flush_all_caches() or at least rebuild.php I'd expect to clear something like this and force a rebuild. If it got corrupted somehow (which is possible given no transaction support anywhere here) the only way to fix that would be to go in and manually delete the state entry.
Comment #18
berdirStrange.. did some profiling using the attached script, but I didn't quite get the results that I expected. the new approach was faster until I got to bundle + 20 fields #250, then it exploded from taking 3 seconds to 10+. HEAD has a similar effect and it is equally slow but it's slower before.
Looks like there are some other bottlenecks, profiling now....
HEAD:
Patch:
Comment #19
berdirInteresting, looks like I've been running into two problems..
a) Config save getting slow if you have a lot of config. 76% of whole time is spent when creating 10 bundles with 10 fields, when I already have 300++ bundles with 20 fields is in Drupal\Core\Config\Entity\ConfigEntityBase::preSave(), which ensures that the UUID is unique. See See #2423591: Optimize config entity import and specifically #2430219: Implement a key value store to optimise config entity lookups for that problem.
b) Some sort of overhead because I did that before all in the same script. When I started again to just create the mentioned 10 bundles, then it was considerably faster (2.2s for a bundle with 10 fields).
Also, I'm much stupid, because creating fields doesn't actually call getFieldMap(). So the good part is that getFieldMap() is not responsible for the first part of this issue (creating fields), that's really only the UUID problem.
The bad part? getFieldMap() is actually way slower than I thought. In my current site, with 340 bundles, getFieldMap() with xhprof enabled took 80 seconds on HEAD. With the patch, 67 milliseconds. And with xhprof disabled, that's still around 60s on HEAD.
@catch: Sure state was just a first implementation because I was lazy. We can use a key value with a separate collection for example, or something like that. and variable_get()set() is very different from either of those two, because we don't have a global cache for them all.
Comment #20
yched commentedOn a vacation, didn't read all atm.
Just: being able to use field definitions not stored in configuration would be cool, but the problem is that the entity system is completely not injectable atm when it comes to field definitions - you cannot inject arbitrary FDs to use in an entity, the FDs are always pulled from the "official registry" in the EM.
Would be very valuable to fix that IMO, but that might be not completely trivial :-/
Comment #21
mesch commented@Berdir, thanks for looking at this. I will try to help where I can, but I'm still getting familiar w/ D8 core.
@yched re: #20, that would be fantastic. I wonder whether entities could control their own field and bundle configurations, perhaps by way of the entity storage handler. I'd imagine this is probably more of a nice to have for D9 though...
Comment #22
yched commentedCould get a look on the patch at last, +1 on the approach.
Looks like we could unpack the code a bit, shorten some var names, but other than this this looks awesome.
Also, not sure why k/v would be more appropriate than state ?
Comment #23
berdirstate *is* key value. The difference is that state has certain semantics, we already have static caching and I can totally see someone adding an implementation that provides persistent caching, e.g. using a cache collector. state is more for small pieces of data, althoug we mis-use it elsewhere already.
We could for example use a separate key value collection and use a key per entity type, then the update logic would be easier as we'd have fewer nestings, and getFeldMap() could load all values of the collection.
Comment #24
mesch commentedIf a way to conduct performant queries on config entity properties emerges from #2430219: Implement a key value store to optimise config entity lookups, could we do away with getFieldMap() entirely? Grepping about I find three uses of getFieldMap() in core:
\Drupal\forum\Controller\ForumController::create()
- needs to find bundles associated with a single node field.
\Drupal\field\Entity\FieldStorageConfig::getBundles()
- needs to find bundles associated with a single field for a single entity type.
\Drupal\Core\Entity\EntityManager::getFieldMapByFieldType()
- creates a map of fields for a specific field type.
In each case, a tailor-made query would be preferable to incurring the overhead of creating a map of all fields on the site, but obviously it depends on how queryable the config schema is.
Comment #25
berdirThat's not an option.
The field map also contains base fields and other fields registered through hooks. For example, you could define an entity type that has a comment field as a base field, that would not work unless the field map contains those fields as well.
Comment #26
fagoGreat findings! The k/v approach sounds good to me, I agree that state does not fit.
Yep, that would be huge as it would enable the door for contrib to mess with per entity fields also. But yeah, that's for sure non trivial and might be too destructive at this stage :/
Comment #27
berdirSo, switched to key value collection. I changed the constructor, using a collection as a services is a bit strange and it's an overhead as we need to instantiate the database factory service and the collection object even though we will very likely not use it.
Also, lots and lots of unit tests.
Comment #28
berdirI've created #2482295: Rebuilding field map with many bundles/fields is very slow for the field map problem.
We have #2430219: Implement a key value store to optimise config entity lookups to solve the problem with the slow config creation, we also have #2423591: Optimize config entity import to look for further improvements there.
@alexpott also found #2482231: Deleting configuration entities is super slow once you have a few., which is the reason for why deleting a field is slow. We expect that the field map improvements will solve that as well, but we are keeping the issue open to do more profiling once that is fixed.
As far as I see, all known problems have separate issues, and those that must be addressed are critical on their own. It makes sense to keep this open to do more profiling with similar and different scenarios, but I think we can change this to major again. In case we find more critical problems, we can always create new issues or change it again.
Comment #29
wim leersAFAICT #2482231: Deleting configuration entities is super slow once you have a few. is now unblocked, so I unpostponed it.
Comment #30
mesch commentedI did some further benchmarking against HEAD (9f4d5e8) with the attached script via
drush scr entity_field_bench.php | tee [output_file]. Results also attached.The results seem to suggest the number of field storage configs have a much greater bearing on performance than the number of field instances.
Comment #31
mgiffordPatch no longer applies.
Comment #33
berdirI think we can safely close this issue now.
There are certainly limits, the most obvious one seems to simply be the amount of configuration files, which slows down config sync.
We had a site with around 100 contact forms, each with a good amount of fields, a lot of them shared. Plus form and view displays. That was around 2k config files if I remember correctly. We've cleaned up a bit since then, but we had no obvious problem with that amount of forms and that should be enough for the vast majority of the sites.
I wouldn't use this to build something like webform.com, where you might have thousands of complex, completely custom forms. But we never designed for that and I don't think that's a use case that we need to support with entity bundles/fields.