We have a huge site, with a huge list of fields where we are using Commerce for a tiny part.

Trying to decrease the memory usage, I noticed that the entity properties are loaded from the cache and use 3MB memory for that. I tracked it down to a single call in commerce_product_reference_entity_view(). We are not using product references at all here, so this will never do anything except loading all these properties on pretty much every request.

While there are places that use more memory, this one should be easy to fix by conditionally creating the wrapper if not already set inside the foreach.

Will try to come up with a patch later but wanted to write this down so that I'm not forgetting it.

CommentFileSizeAuthor
#2 load-wrapper-on-demand.patch1.51 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

Hmm, I was thinking that we could just store the return value of commerce_info_fields() in a variable prior to the foreach() and only wrap the entity if that isn't empty, but my hunch is that even that wouldn't be sufficient. If you have any node type at all that uses a product reference field, even if the field doesn't specify field injection, it will still execute the loop.

It seems like perhaps we should get the return value of commerce_info_fields() in a variable first and perform the second half of this if statement before the current foreach() that loads the product and looks for fields to render:

      // If we found a product and the reference field enables field injection...
      if (!empty($product) && $instances[$field_name]['settings']['field_injection']) {

This means we wouldn't enter into the current master foreach() in that function unless we both found product reference fields on the current entity type and ensured that they turned on product field injection.

Berdir’s picture

Status: Active » Needs review
FileSize
1.51 KB

I'm not sure if I fully understand what you are saying. But the wrapper is already used above that check to get the product.

There might be more advanced ways of doing this (like using array_intersect_key() to get the fields we're interested in out of the instances) but I think this is the easiest change and solves the biggest problem.

Berdir’s picture

#2: load-wrapper-on-demand.patch queued for re-testing.

rszrama’s picture

Status: Needs review » Fixed

Not sure how I missed your patch, but reviewing it it's obvious that I misunderstood you when I posted comment #1. It appears I thought the problem was with loading field data not with loading all the entity property metadata when the entity is wrapped. Delaying the load of this wrapper is an obvious performance win, so I've committed as is.

Status: Fixed » Closed (fixed)

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