Analyzing a slow site recently, I found that for some page requests about 25% of the time were spent in just the ctools_entity_field_extra_content_type_content_type() function.
The problem with it is that, while only a single plugin is needed, the function actually just calls ctools_entity_field_extra_content_type_content_types(), which computes all available plugins for this type and can become incredibly slow for sites with lots of entity types, bundles and/or fields (cf. #2666046: Performance issues with ctools_entity_field_extra_content_type_content_types). It also doesn't even use caching, though the information won't change a lot.

However, while caching might be a nice solution for the other issue, this one can be solved much simpler: just implement ctools_entity_field_extra_content_type_content_type() without calling ctools_entity_field_extra_content_type_content_types(), by just computing the information for a single entity type/field combination. Unfortunately, we still seem to need to loop over all available bundles, but it should still be much faster. Caching could then, of course, still be easily added.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

Title: ctools_entity_field_extra_content_type_content_type() using up lots of request time » ctools_entity_field_extra_content_type_content_type() could be much faster
Status: Active » Needs review
FileSize
1.77 KB

Patch attached, seems to work fine on our site.

rivimey’s picture

Issue tags: +Needs tests

dm, thank you for your contribution.

The patch itself looks fine, although it duplicates some of the code in ctools_entity_field_extra_content_type_content_type, making ctools less easy to maintain. Would it be possible to refactor things to minimise that?

There is also a push within ctools to implement many more simpletest tests, so it would be a great help to include a test set that could be used to demonstrate that your change does not affect functionality, making it much easier to accept.

rivimey’s picture

darrenwh’s picture

Status: Needs review » Needs work

Just a single DCS check observation:

+++ b/plugins/content_types/entity_context/entity_field_extra.inc
@@ -14,10 +14,44 @@
+      $entity_type_label = t(ucfirst($entity_type));

Only string literals should be passed to t() where possible.

rivimey’s picture

@darrenwh I noticed this too, but this construct is old behaviour (not introduced in this patch) and I don't think this issue is the one to change it. If you wish feel free to create a new issue to address it.

Propose -> needs review

drunken monkey’s picture

Status: Needs work » Needs review

Second that, this shouldn't be the place to fix copy/pasted DCS violations. (Thoguh I generally very much agree that this shouldn't be done.)

rivimey’s picture

drunken monkey, is there any chance of you adding in some tests for this? I have started adding more test code but there is so much to do...

rivimey’s picture

Status: Needs review » Needs work

Moving back to Needs work solely because of the wish for tests - as far as I can tell the code is ok.