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.I've got a bunch of forms using panels and ctools_entity_form_field_content_type_content_types()
was making lots of cache calls through field_info_instances()
This can be avoided by not making direct calls to each entities' bundles' field instances in a loop but instead just ask for them all upfront.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 2850260-3.patch | 1002 bytes | joelpittet |
Comments
Comment #2
joelpittetHere's the patch for review.
Comment #3
joelpittetForgot relative flag on the last patch.
Comment #4
rivimey@joelpittet, this all looks great. My only concern is whether
field_info_instances() can be considered equivalent to field_info_instances(e,b) in all relevant cases.
Looking at the code for both I think it's probably true, but I'm not sure... can you elaborate?
Apart from that, good to go!
Comment #5
joelpittetThanks for the review @rivimey! Yes, the first grabs all field instances' info for all entities and their bundles where the other grabs a specific entity and bundle pair's field instances' info.
The way it worked before it grabbed all the entities and bundles then looped through the entities and bundles to grab the instances. The new one grabs all the fields upfront (which may be cached statically from previous calls) and I added the condition in there because one bundles may not have fields as it loops through.
It's the same function call but there is a pattern in Drupal to return all things without arguments and a specific item if there is an argument passed. (see most commerce API functions). The big difference is for performance is the possibility of caching all the fields already is greater than the individual field instance being cached AND multiple function calls in a loop can slow things down some depending on how many things it's looping through (in my case a lot of entities and 250ish field instances).
Sorry if I went a bit long on my explanation hope it helps
Comment #6
rivimeyThanks Joel, that helps. I get the thing about the pattern - that is fine and I completely accept the approach you are using - but if you examine the field_info_instances() function itself the implementation of the two cases (no args, with args) is quite different. I guess though that one could call a fundamental change here a bug in Core and so not a reason to reject this.
Ok, I am happy with the change; however it would be great if you could include some simpletests for this code, even something fairly trivial. I know the change you're making is minor, but we need to get the test coverage up above < trivial > percent :-) I think the decision is that tests for plugins live close to the plugin, rather than in the ctools-root tests folder, but anything is good!
Comment #7
joelpittetOne thing that eased my mind about the patch even more is if you look in the xhprof results the function call change on all the other child functions didn't change. Meaning it did the same volume of build-up. But also I was stepping through with xdebug too in PHPStorm to ensure the results were the same after I was done.
Comment #8
rivimeyWould have liked tests, but... :(
Comment #9
rivimeyComment #10
joseph.olstadWe've applied this change, thanks again @joelpittet for the excellent xhprof illustration of performance results and patch!
Comment #11
joseph.olstadThis patch is golden for us, would think that it'd be a big win for the #2828925: Plan for CTools 7.x-1.13 release
thanks again joelpittet , really looking forward to this being in a release
Comment #14
japerryNice gain! Committed.