Use a static cache in commerce_product_reference_field_extra_fields()

Do you know how many times it gets called on your site average/rough numbers on the high end(maybe a listing page)?

The diff looks poorly so I thought I'd show a non-whitespace version.

-  $extra = array();
-
+  $extra = &drupal_static(__FUNCTION__);
+  if (!isset($extra)) {
     // Loop through the product reference fields.
     foreach (commerce_info_fields('commerce_product_reference') as $field_name => $field) {
       foreach ($field['bundles'] as $entity_type => $bundles) {
@@ -105,6 +105,7 @@ function commerce_product_reference_field_extra_fields() {
         }
       }
     }
+  }

My page gets about 250 calls to t from within that function.

Also could we avoid the giant nesting if by returning the cached version at the top of the function or preceeding as normal. It would make this patch easier to read and hopefully make it easier to maintain as well.

Strange that didn't apply. This it just has one extra context line.

Well it seems that maybe some conditions may be call dependent? Or else why would this fail to produce the data for that test?

@torgosPizza That test is probably a saving grace when it comes to adding static variables. The case is probably the extra fields is called, the new product display with reference field is added and then going to that same page in the same request.

You could change the test, but safer bet would be reset that static cache when fields are saved/deleted? What do you think?

I wonder if this function gets called before data is available in some cases, resulting in that $extra array being empty the first go round and populated on subsequent requests. : ?

Same as #8, but uses !empty instead of isset. The variable is always set, and this should catch any wonkiness if it passes through as empty once, or something.

@mglaman weird because the docs use isset() checks. https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/drupal...

Well isset() works if it is NULL, however what if it randomly gets an empty array set (#17)

I think the test patches passes because the test is written wrong, for what it's worth.

Using #19 in production right now, because commerce_product_reference is currently one of the slowest modules according to NewRelic. It's still not great though. Any other ideas on how we can boost perf of this module?

Needs work. #19 doesn't do anything special. #8 does. Attaching initial Blackfire reports pre patch and post. Did simple profiling off of demo CK2 install of /tops/guy-short-sleeve-tee.

Pre

Post

However, disclaimer, my env has xhprof and xdebug enabled, so some things might be skewed. but there is a win. Test just needs to be fixed.

Confirming the test is the problem, see commerce_product_field_extra_fields().

function commerce_product_field_extra_fields() {
  $extra = &drupal_static(__FUNCTION__);

  if (!isset($extra)) {

Here is updated test which actually retrieves the field attach properly. This should do it!

+++ modules/product_reference/commerce_product_reference.module
@@ -36,14 +36,12 @@ function commerce_product_reference_commerce_product_uri($product) {
-  $extra = &drupal_static(__FUNCTION__);
+  $extra = &drupal_static(__FUNCTION__, array());
...
-  $extra = array();
-

I'm stupid here. It's set then by providing default return value.

I don't know what I did to make these patches hate me.

Test should be fixed now to get extra field info properly.

+++ b/modules/product_reference/commerce_product_reference.module
@@ -36,6 +36,12 @@ function commerce_product_reference_commerce_product_uri($product) {
 function commerce_product_reference_field_extra_fields() {

+++ b/modules/product_reference/tests/commerce_product_reference.test
@@ -262,8 +262,9 @@ class CommerceProductReferenceAdminTest extends CommerceBaseTestCase {
-    $extra_fields = commerce_product_reference_field_extra_fields();
-    foreach ($extra_fields['node'][$this->display_type->type]['display'] as $display) {
+    $extra_fields = field_info_extra_fields('node', $this->display_type->type, 'display');
+
+    foreach ($extra_fields as $display) {

This looks to be the only test for that function it could be a bad test or something it's hard to tell but removing the function call is sketch as the solution.

call is sketch as the solution.

How? It's a hook invocation. The test is now properly invoking the hook like the system typically would.

Oh you are both right, sorry didn't realize it was a hook. Sorry.

So yeah that test needs to be getting everything provided by that hook.

:) Yeah I don't get how this test was even working before by calling it direct. I think that's why static cache broke it. I think a call within the method built the field cache class and something goofy happened. But calling it properly has everything primed.

Boom. Committed.