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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mradcliffe’s picture

Alternatively, 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.

xjm’s picture

Status: Needs review » Needs work
+++ b/modules/field/field.crud.incundefined
@@ -535,7 +538,9 @@ function field_update_instance($instance) {
   // Clear caches.
-  field_cache_clear();
+  if ($reset) {
+    field_cache_clear();
+  }

Can we update the inline comment to explain that we clear caches unless it's specified otherwise by the flag?

+++ b/modules/field_ui/field_ui.admin.incundefined
@@ -755,7 +755,10 @@ function field_ui_field_overview_form_submit($form, &$form_state) {
+      // Field cache will be cleared when the field instance is created, and
+      // field cache should not be cleared N+1 times.
+      field_update_instance($instance, FALSE);

@@ -1385,10 +1388,13 @@ function _field_ui_add_default_view_mode_settings($entity_type, $bundle, $view_m
-      field_update_instance($instance);
+      field_update_instance($instance, FALSE);

For these, I'd add matching inline comments:

Prevent the cache from being cleared repeatedly.

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:

Caches will be cleared after all instances have been processed.

21 days to next Drupal core point release.

xjm’s picture

Also, this is an API addition only (optional param with the current behavior as the default), so this is backportable.

mradcliffe’s picture

beejeebus 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.

catch’s picture

I'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.

mradcliffe’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
3.2 KB
3.25 KB

Re-rolling both patches and update patch comments. I haven't looked at this in a while, but this is still necessary.

mradcliffe’s picture

Re-rolling d8 patch.

Edit: every single time I get back into the swing of things I screw it up. :(

mradcliffe’s picture

Real patch without git pull'd cruft attached.

Barry_Fisher’s picture

Looking 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....

Barry_Fisher’s picture

Patch attached with tests and calls to field_update_instance() updated

Status: Needs review » Needs work
Barry_Fisher’s picture

Missed one of the tests. Trying again.

Barry_Fisher’s picture

Version: 8.x-dev » 7.x-dev

Speaking 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.

mgifford’s picture

Status: Needs review » Needs work
star-szr’s picture

Status: Needs work » Needs review
Issue tags: +Performance, +API addition, +Needs backport to D7
star-szr’s picture

Re-testing @mradcliffe's patch from #6 against D7.

star-szr’s picture

Regarding #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()

mradcliffe’s picture

Yes, 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.

star-szr’s picture

@mradcliffe - Good point, didn't think of it that way. So the D7 patch in #6 looks pretty good, do we need tests?

jdillick’s picture

Refreshing patch against latest 7.x

mradcliffe’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Barry_Fisher » Unassigned
FileSize
2.05 KB

This 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.

Status: Needs review » Needs work

The last submitted patch, drupal-8-field_update_instance_no_clear_cache-1359494-22.patch, failed testing.

YesCT’s picture

I'm confused. I thought this was not needed in D8 because of cmi.

mradcliffe’s picture

That'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.

xjm’s picture

Issue tags: +needs profiling
swentel’s picture

The 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.

mradcliffe’s picture

Version: 8.x-dev » 7.x-dev

Agreed, now that it is done I can move this back to 7.x only.

Olarin’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.69 KB

Re-rolling against 7.24.

Status: Needs review » Needs work

The last submitted patch, 29: drupal-field_update_instance_no_clear_cache-1359494-29.patch, failed testing.

Olarin’s picture

Status: Needs work » Needs review
FileSize
2.41 KB

Oops, made a stupid mistake in that re-roll. Let me try that again.

mgifford’s picture