The field_update_instance calls in field_ui_field_overview_form_submit do not seem to be scalable as you increase the number of fields and content types (or other entity types) on a site. This issue relates not only to field_ui, but the field system component. Also need to move to 8.x.
Core should not be updating N+1 caches during a page request. Preferably it should only be doing a static number like 1 per cache.
Possible solutions
- Add an optional cache/reset parameter to field_update_instance, add documentation about when to use it, and then change field_ui_field_overview_form_submit to use the parameter and clear cache afterward. There may be other places where this is useful in core. This is an API change. :*(
- Don't use field_update_instance, but read and write to field_config_instance's respective columns. This would need to be run through hook_field_update_instance() as well. I don't think this is very Drupal-y though.
I think #1 is the best approach, and it would be ideal in 7.x as we're only just beginning to explore entities (entity, commerce, workbench, etc...). I haven't looked at 8.x branch much yet to see if it would still apply.
Some db stats
Here's a test with the following scenario with 17 fields on a content type and several content types while adding a new text field. Each of those fields does a field_update_instance, which refreshes not only the field cache, but the entity info cache as well. I don't have any more accurate benchmarks than this. This isn't my site architecture, but something I was brought on to diagnose.
Database server I/O writes: avg: 2mb/s max: 4.3mb/s (wtf)
Normal writes: 1-5kb/s
Comments
Comment #1
mradcliffeAlternatively, user olarin brought up that a field_update_instances would work and possibly save code. Also I can get around this temporarily by rewriting the submit function and form altering my way out of this.
Still an issue for d8. Here are a couple of untested patches. I'm changing to needs review just for the bot, and then going back to active or needs work.
Comment #2
xjmCan we update the inline comment to explain that we clear caches unless it's specified otherwise by the flag?
For these, I'd add matching inline comments:
Then, for the first case, we'd add your first sentence about the instance being created as a second sentence. For the second case, add something like:
21 days to next Drupal core point release.
Comment #3
xjmAlso, this is an API addition only (optional param with the current behavior as the default), so this is backportable.
Comment #4
mradcliffebeejeebus suggested just changing field_ui to check if the weight has changed. This would not solve the n + 1 problem, but would relegate it to such a small use case that less people would run into it in field_ui.
The worry I think we all have is that some module will depend on hook_field_update_instance() depending on that cache clear to do something with the updated field instance (even if it's only a weight that has changed). In this case, documentation++ on field_update_instance() usage.
On the other hand, it is valid to want to update multiple field instances per page request and not expect any performance issues because of it.
Comment #5
catchI've done multiple field_update_instance() before in an update handler. It's possible someone might want to do that and also read back the previous instances, however if doing field crud operations you should always use field_read_instance() anyway.
Comment #6
mradcliffeRe-rolling both patches and update patch comments. I haven't looked at this in a while, but this is still necessary.
Comment #7
mradcliffeRe-rolling d8 patch.
Edit: every single time I get back into the swing of things I screw it up. :(
Comment #8
mradcliffeReal patch without git pull'd cruft attached.
Comment #9
Barry_Fisher CreditAttribution: Barry_Fisher commentedLooking at the change in the function signature, the $reset parameter defaults to TRUE. However, I'm thinking that maybe this ought to default to FALSE. As this is an API change, other modules may be calling field_update_instance() and for some reason are expecting to have the caches flushed on every field- not per request.
My reasoning here is that I believe the flush caches action should happen outside of the field_update_instance() anyway- then the calling function should take the responsibility of flushing the field cache. It could still be reasonable to have a parameter to optionally flush the field cache, but would need to be specified intentionally. To back this up- entity_load() (and friends) have a similar approach to explicitly flushing the cache- although this is flushing a static rather than a firmer db cache, but I believe the princliple is the same.
The proposed patch in #8 is more "safe" from an API standpoint- and a backport to D7 should probably default to TRUE approach to prevent breaking existing calls. However out of curiosity, I'm going to resubmit the patch and see if it passes tests when the $reset defaults to FALSE. I guess any failures could be fixed in other areas to bring everything into line with this change- depending on the amount of work involved.
Any thoughts on this? Patch to follow....
Comment #10
Barry_Fisher CreditAttribution: Barry_Fisher commentedPatch attached with tests and calls to field_update_instance() updated
Comment #12
Barry_Fisher CreditAttribution: Barry_Fisher commentedMissed one of the tests. Trying again.
Comment #13
Barry_Fisher CreditAttribution: Barry_Fisher commentedSpeaking to @tim.plunkett, this patch is an irrelavent point for D8 as field configuration is being replaced by CMI goodness, but would still be relevant for a backport to D7- so I'm marking back to D7.
Patch in #12 although (hopefully) passes is a D8 solution only because of the API change so best to ignore that.
I would suggest a better title too based on this, but perhaps someone else can think of something? Thanks.
Comment #14
mgifford#12: drupal8.x-api_change_to_make_field_update_instance_not_flush_field_cache_by_default-1359494-6386714.patch queued for re-testing.
Comment #16
star-szr#6: 0001-Patch-1359494-by-mradcliffe-Prevent-field_update_ins-d7.patch queued for re-testing.
Comment #17
star-szrRe-testing @mradcliffe's patch from #6 against D7.
Comment #18
star-szrRegarding #9, $reset = FALSE definitely seems to be the pattern elsewhere in core for functions that use a cache, so backporting #12 to D7 might be easier than updating the D7 patch in #6.
A couple examples from the D7 API:
entity_load()
drupal_static()
Comment #19
mradcliffeYes, but it's an API change that would require anyone expecting cache to be refreshed to pass TRUE instead. It would be a lot easier to get this into D7 if it were the other way around despite the $reset = FALSE pattern.
Comment #20
star-szr@mradcliffe - Good point, didn't think of it that way. So the D7 patch in #6 looks pretty good, do we need tests?
Comment #21
jdillick CreditAttribution: jdillick commentedRefreshing patch against latest 7.x
Comment #22
mradcliffeThis is actually still relevant in Drupal 8. The admin ui still exists.
Thank you for updating for Drupal 7, @jdillick. This is a re-roll for Drupal 8. Let's see how it likes it.
Comment #24
YesCT CreditAttribution: YesCT commentedI'm confused. I thought this was not needed in D8 because of cmi.
Comment #25
mradcliffeThat's what I thought as well from #12, but I haven't seen anything related to field ui in CMI lately and the code still exist as it does in D7 in current D8 (see \Drupal\field_ui\OverviewBase::submitForm()). So I re-rolled it a couple of months ago just in case.
I've taken another look at CMI issues, but I don't see anything else that is gonig to change \Drupal\field_ui\OverviewBase::submitForm(). I may be missing something though.
Comment #26
xjmComment #27
swentel CreditAttribution: swentel commentedThe submitForm() methods don't save the instance objects anymore now, so that's at least tackled.
Not sure whether it still makes sense to add this param in D8 even.
Comment #28
mradcliffeAgreed, now that it is done I can move this back to 7.x only.
Comment #29
Olarin CreditAttribution: Olarin commentedRe-rolling against 7.24.
Comment #31
Olarin CreditAttribution: Olarin commentedOops, made a stupid mistake in that re-roll. Let me try that again.
Comment #32
mgifford31: drupal-field_update_instance_no_clear_cache-1359494-31.patch queued for re-testing.