Entity cache integration

Not sure how to test, but if your happy with this feel free to commit.

This is one of the two issues left that I would like to push into the latest 7.x-2.x branch. After the dust settles on these two new features, I would like to push out 7.x-2.0 tag.

I want to push out a release soon, as I just discovered that I was incorrectly using hook_hook_info(). This particular bug could cause significant issues if another module decides to share the same namespace, namely complete loss of functionality in the core country element, thus the continent country widget too.

With no new features really pending, this would be the last foreseen release till the next ISO update, which could be months off. With this in mind, I'll tag it as the first stable 2.x release (7.x-2.0).

So would you be happy if I commit this? Or should I skip this for the time being?

lol, sounds good, it can go with #1458938: Add schema columns properties maybe in a month or three.

There are only 3 users (Me, yourself and pcambra) subscribed to either issue.

Alan, don't forget the silent listeners...
I have this patch in my test system - It doesn't break anything, but I can't see any improvements, either.

Anyway, I just wanted to plug htis issue #1596178: too many calls to DrupalCacheInterface::set() ?, which might be related.

Before adding the additional overhead, I think that we should step back and realise that we are effectively handling static data here, so maybe we should follow the KISS policy.

Without hooks to clear on insert,update or delete, and no drupal static, this is by far the simplest and is faster than using cores correct method of country_get_list():

function countries_get_countries($property = 'all', $filters = array()) {
  static $countries = NULL;

  if (!isset($countries)) {
    global $language;
    $langcode = $language->language;

    //cache_clear_all('countries_get_countries:' . $langcode);
    $cache = cache_get('countries_get_countries:' . $langcode);
    // Make sure cache has data.
    if (isset($cache->data)) {
      $countries = $cache->data;
    }
    else {
      $countries = entity_load_multiple_by_name('country');
      cache_set('countries_get_countries:' . $langcode, $countries);
    }
  }
  ....
}

Times are around 7ms for the cache load, and then drop to 0.01ms.

Using the core list, with alters but NO country alter (this hook was removed for testing):

  $s = microtime_float();
  include_once DRUPAL_ROOT . '/includes/locale.inc';
  $list = country_get_list();
  $e = microtime_float();
  dpm("Executed full load in " . (($e - $s) * 1000) . ' ms');

Times were around 9ms and 0.3ms.

Using the core list directly:

  $s = microtime_float();
  include_once DRUPAL_ROOT . '/includes/iso.inc';
  $countries = _country_get_predefined_list();
  $e = microtime_float();
  dpm("Executed full load in " . (($e - $s) * 1000) . ' ms');

Times were around 3ms and 0.2ms.

What happens if you remove the static in:

function countries_get_countries($property = 'all', $filters = array()) {
-  static $countries = NULL;  
+  $countries = NULL;

You are now caching the cache... increasing memoryload.

In the above code (Drupal 6 style), the variable is calculated once and stored in what you would consider the global function space, like the $GLOBAL array, but different.

The second time the function is called, that value is already set so it is reused rather than recalculated. Thus the 0.01ms response time compared to the 7ms response.

Con: This moves the array into memory (probably happens anyway, and is not that high)
Pros: Fast!

Without that static you would effectively have this running every time:

    global $language;
    $langcode = $language->language;

    //cache_clear_all('countries_get_countries:' . $langcode);
    $cache = cache_get('countries_get_countries:' . $langcode);
    // Make sure cache has data.
    if (isset($cache->data)) {
      $countries = $cache->data;
    }
    else {
      $countries = entity_load_multiple_by_name('country');
      cache_set('countries_get_countries:' . $langcode, $countries);
    }

And the cache_get() would always do a database call.

1. So the static indeed makes sense - I wasn't sure if a cache_get() always does a database call.
2. However, I don't see how/when the new code responds to a change in the countries list, c.q. after a cache refresh.
3. Then about the $filters:
- Would a separate cache per filter make sense? Or will it only increase the static memory. You then might remove the Static in countries_countries_alter() and centralize code. See below code.
- off-topic: the function countries_filter() does an unnecessary recursive call to countries_get_countries(). See code below.

function countries_get_countries($property = 'all', $filters = array()) {
//  static $countries = NULL;
+  static $countries = array();
+  $filter = $filters['enabled'];

//  if (!isset($countries)) {
+  if (!isset($countries[$filter])) {
    global $language;
    $langcode = $language->language;

    //cache_clear_all('countries_get_countries:' . $langcode);
//    $cache = cache_get('countries_get_countries:' . $langcode);
+    $cache = cache_get('countries_get_countries:' . $filter . $langcode);
    // Make sure cache has data.
    if (isset($cache->data)) {
      $countries[$property] = $cache->data;
    }
    else {
      $countries = entity_load_multiple_by_name('country');
      cache_set('countries_get_countries:' . $filter . $langcode, $countries);
    }
  }

  if ($property == 'all') {
    return $countries;
  }

  $filtered_countries = countries_filter($countries, $filters);
  ...
}
...
function countries_filter($countries, $filters = array()) {
+  if (empty($countries)) {
+    $countries = countries_get_countries();
+  }

  if (!empty($filters)) {
    $target_countries = array();
//    foreach (countries_get_countries() as $country) {
+    foreach ($countries as $country) {
      $include = TRUE;
      ...
    }
    $countries = array_intersect_key($countries, $target_countries);
  }
  return $countries;
}

I was posting the concept only. This would cause tests, et al, to fail at the present state. We would need to flush this cache every time that a country was added, updated or deleted.

The filter thing is required as a workaround for some strange workflow, added a year or so back, so fuzzy with the exact issue it resolved, but it is needed for something, somewhere...

I'm not sure about the benefits of caching individual filters.

Right, the benchmarking of Entity Cache has thrown up a nasty surprise.

Unaltered:
Loaded in 41.144132614136 ms
Loaded in 0.38695335388184 ms

Entity Cache integration:
Loaded in 754.01496887207 ms
Loaded in 853.12294960022 ms

This is based on going to a node edit page with country fields, with the following code to output the results:

function microtime_float() {
  list($usec, $sec) = explode(" ", microtime());
  return ((float)$usec + (float)$sec);
}

function countries_get_countries($property = 'all', $filters = array()) {
  $a = microtime_float();
  $countries = entity_load_multiple_by_name('country');
  $b = microtime_float();
  dpm("Loaded in " . (($b - $a) * 1000) . " ms");
  ....

}

It looks like manual caching is by far the best here.

Tonight (in 10 hours) I'll roll a patch for this, basing this off both language caching of all and enabled countries, as I think johnv has a valid point for caching this subset. I know that I have used the countries module to have only 4 countries enabled before, reducing the load by 4/250th if filtered by this flag. However, caching by the other possible combinations could fill up the cache tables, and will bypass.

Follow on issues, ensure that even disabled countries get rendered when displayed, and moving translation to the load, making the countries i18n widget redundant.

Completely manual caching. I dropped the idea of caching individual filters as it made the code a lot messier.

Another stab.

Maybe this should be lower in the chain but that would require a custom EntityController :/

SimpleTest uses curl to do the gets and posts.

This was the reason that these were failing, entity_get_controller('country') within the test methods was a different controller instance and was getting stale data.

On standard pages there is absolutely no difference in any method that I tested with any significant statistical result.

Randomly picking 50 nodes with 5 country fields each, had better results:

              min  mean[+/-sd] median   max
Total:       1111 1136  11.3   1140    1151 (unaltered)
Total:       1133 1143   9.5   1140    1161 (entity cache)
Total:       1120 1132   8.0   1133    1146 (drupal_static)
Total:       1118 1134  10.4   1133    1150 (drupal_static & direct use of cache results)

Node with no country fields

              min  mean[+/-sd] median   max
Total:       1102 1137  12.6   1140    1156
Total:       1120 1143  13.3   1142    1178
Total:       1118 1142  13.8   1138    1171
Total:       1120 1141  11.5   1139    1172

As such, marking as won't fix unless someone can prove some benefit!!