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.
Comment | File | Size | Author |
---|---|---|---|
#2 | 2838263-2--speed_up_content_type_call.patch | 1.77 KB | drunken monkey |
|
Comments
Comment #2
drunken monkeyPatch attached, seems to work fine on our site.
Comment #3
rivimeydm, 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.
Comment #4
rivimeyComment #5
darrenwh CreditAttribution: darrenwh as a volunteer and at Investis Digital commentedJust a single DCS check observation:
Only string literals should be passed to t() where possible.
Comment #6
rivimey@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
Comment #7
drunken monkeySecond 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.)
Comment #8
rivimeydrunken 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...
Comment #9
rivimeyMoving back to Needs work solely because of the wish for tests - as far as I can tell the code is ok.