Field_collection currently is using many DB queries in the uncached case.

By using field_collection_field_formatter_prepare_view and recursing into the entities the performance can be increased by 50%.

With three field collection fields and one of them containing another field_collection I get:

Before: 304 calls to entity_load
After: 4 calls to entity_load

Before for field_collection_item_load_multiple: 8,308,574
After for field_collection_item_load_multiple: 2,525,895

Patch and xhprof shots attached.

Enjoy!

Best Wishes,

Fabian

CommentFileSizeAuthor
#17 1427158_Use-field_collection_field_formatter_prepare_view-to-increase-performance_17.patch2.7 KBjoseph.olstad
#12 1427158_Use-field_collection_field_formatter_prepare_view-to-increase-performance_12.patch2.7 KBmrfelton
#3 1427158_Use-field_collection_field_formatter_prepare_view-to-increase-performance_3.patch3.17 KBFabianx
xhprof_detail_after.png77.4 KBFabianx
xhprof_detail_before.png69.96 KBFabianx
xhprof_after_patch.png97 KBFabianx
xhprof_before_patch.png101.34 KBFabianx
field_collection_0001-Bug-9357-Massively-increase-performance-of-faculty-d.patch3.74 KBFabianx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, field_collection_0001-Bug-9357-Massively-increase-performance-of-faculty-d.patch, failed testing.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Version: 7.x-1.x-dev » 7.x-1.0-beta3

Before:

before

After:

after

field_collection_item_load_multiple before:

detail before

field_collection_item_load_multiple after:

detail after

The above is after a fresh cache clear with just homepage loaded afterwards and then this page with XHPROF.

And some live data for the record on DevCloud with the record already cached with entity_cache, field_cache, etc.:

Without Patch:

Avg | Max
1,376.0 | 1,374.0

With Patch:

Avg | Max
964.0 | 949.5

So a good 400ms saved on average.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Version: 7.x-1.0-beta3 » 7.x-1.x-dev
Status: Needs work » Needs review
FileSize
1427158_Use-field_collection_field_formatter_prepare_view-to-increase-performance_3.patch3.17 KB

Attached new patch against 7.x-1.x-dev

dealancer’s picture

dealancer CreditAttribution: dealancer commented

An awesome job!

dealancer’s picture

dealancer CreditAttribution: dealancer commented 
+++ b/field_collection.moduleundefined
@@ -804,6 +804,75 @@ function field_collection_field_formatter_settings_summary($field, $instance, $v
+	  foreach ($fields as $field_name => $field) {

There is a tab here.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Version: 7.x-1.0-beta3 » 7.x-1.x-dev

Out of curiosity, why the use of field_read_fields and not field_info_fields?
I need to give this a real review soon.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

Hi,

field_read_fields was used, because below in the code it said that field_info_fields had been replaced with field_read_fields.

So I used the same function for consistency reasons.

However probably there is no problem to change it to field_info_fields.

Best Regards,

Fabian

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

Bumpy bump!

Hey, tim.plunkett, any chance you can review the patch / approach?

grasmash’s picture

grasmash CreditAttribution: grasmash commented

I'd love to see this patch committed. According to my performance tracking tools, field collection is responsible for nearly 90% of my db hits for a single node view. That's with just one field collection per node, and no field collection nesting.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Needs review » Needs work

oh, yes we should definitely do a multiple load in pre-view here.

The patch needs work though:
* $item['entity'] is already the place for the field-collection
* No static cache needed, the entity controller has that already.
* Comments (hook implementations, ..) are missing and need work.
* Variable names should be descriptive and match with what is used in the code else.

mrfelton’s picture

mrfelton CreditAttribution: mrfelton commented

This brings massive improvements to our setup, which has 4 multiple value field collections, each with 6 fields attached to a custom entity type. When loading the edit form for an entity with about 100 rows of field collection data, this slashes the number of queries from 2431 down to 1117, cuts about 250ms off the page execution time, and reduces the memory footprint from 217MB down to 180MB. A massive improvement, although it's still a little on the slow side.

mrfelton’s picture

mrfelton CreditAttribution: mrfelton commented
FileSize
1427158_Use-field_collection_field_formatter_prepare_view-to-increase-performance_12.patch2.7 KB

Oh, and the patch didn't apply to the latest dev, as the change to field_collection_field_get_entity seems to already be in the latest codebase. Updated patch attached.

k.skarlatos’s picture

k.skarlatos CreditAttribution: k.skarlatos commented

This looks very interesting. Does the updated patch address the issues of #10?

klokie’s picture

klokie CreditAttribution: klokie commented
Status: Needs work » Needs review
jerdavis’s picture

jerdavis
Location Minneapolis, MN
CreditAttribution: jerdavis commented

12: 1427158_Use-field_collection_field_formatter_prepare_view-to-increase-performance_12.patch queued for re-testing.

jenlampton’s picture

jenlampton
she / her
CreditAttribution: jenlampton at Jeneration Web Development for Metropolitan Transportation Commission commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I'm seeing the same kinds of improvements on my site as well. Thanks for the patch @Fabianx!

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
FileSize
1427158_Use-field_collection_field_formatter_prepare_view-to-increase-performance_17.patch2.7 KB

great patch, rerolling just for testbot.

credit to FabianX and mrfelton

wildfeed’s picture

wildfeed CreditAttribution: wildfeed as a volunteer commented

Wondering about status of patch in #17.
Completed testing?
Ready to be merged into dev branch?
Thoughts?

  • ram4nd committed 9c5842b on 7.x-1.x authored by Fabianx 
    Issue #1427158 by Fabianx, mrfelton, joseph.olstad: Use...
ram4nd’s picture

ram4nd CreditAttribution: ram4nd as a volunteer commented
Status: Reviewed & tested by the community » Fixed
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

Thanks @ram4nd !

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented

The change in this issue has changed what views_get_view_result() does and broken our site; please see #3109657: field_collection_field_formatter_prepare_view() changes return value of views_get_view_result(). I would appreciate any insight into the problem.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented

This problem appears to have been fixed in #3091188: Field Collection stop working. It would be great to get a new release with this fix.