For some reason _hosting_package_instances_load_languages() is statically cached but _hosting_package_instances_load() isn't?

Also, neither have any persistent caching which should be totally implementable since the only time the results for a given query could change is when something changes in the hosting_package_instance, hosting_package, node, or hosting_platform tables we can just clear what we need to on updates/inserts.

Changes to the UI that would provide more comprehensive information on the sites/platforms being managed by Aegir have previously been blocked due to performance concerns #1853588: Sort Install profiles on Site add form so why not profile at least some basic changes that could speed things up?

Hostmaster has no dedicated persistent cache table afaik, which this would obviously rely on, in part. I'll add that as a separate issue and link to it from here.

Members fund testing for the Drupal project. Drupal Association Learn more


thedavidmeister’s picture

Can't do persistent caching without this first #1875446: Cache tables for hosting_package. Can do static caching first though since they're still separate things in D6 and needed to be implemented in tandem.

thedavidmeister’s picture

Status: Active » Needs work
3.05 KB

Patch attached for static caching. I don't actually think this will speed up page loads all that much as the query is likely different every/most times it is called on a single page load. It does get us a mechanism for generating a unique cache ID that will translate well to what we'll need to do set/get in a persistent cache.

This patch hasn't been tested yet, I just wrote it based on similar implementations I've done for other modules.

thedavidmeister’s picture

Here's a slight refactor of the last patch. Removes the $multiple flag from _hosting_package_instances_load() as it isn't being used anywhere other than the wrapper functions and leaving it in forces us to either admit that it's doing nothing or split our cache into two for identical queries. I added a $reset flag instead to allow for the cache to be rebuilt externally.

This refactor means that hosting_package_instances_load() and hosting_packages_instance_load() have a shared cache, and do the same thing but hosting_package_instance_load() handles returning the first result rather than leaving it up to the cache building function. This mimics the way that node_load() works in d7 by calling node_load_multiple().

Also, it's rolled against the latest 6.x-2.x as the previous patch now fails to apply against that branch.

anarcat’s picture

This is interesting. Have you done benchmarks or some functional testing to get a feel of whether or not this actually improves performance? There seems to be a significant different between enabling a drupal_static() cache and making a whole new cache table...

thedavidmeister’s picture

drupal_static() doesn't exist in Drupal 6. This patch is just using the static PHP keyword.

I haven't profiled it yet, and there's no persistent cache in the patch in #3. Possibly worth splitting static and persistent caching into two issues in hindsight...

The amount of time that a persistent cache saves over a static cache is proportional to the number of times the function is called on each page load.

anarcat’s picture

Benchmarks would be nice.

A little code review for ya:

+++ b/modules/hosting/package/hosting_package.instance.incundefined
+++ b/modules/hosting/package/hosting_package.instance.incundefined
@@ -95,8 +95,15 @@ function hosting_package_instance_create(&$instance) {

@@ -95,8 +95,15 @@ function hosting_package_instance_create(&$instance) {
  * @see hosting_package_instances_load()
-function hosting_package_instance_load($param) {
-  return _hosting_package_instances_load($param);
+function hosting_package_instance_load($param, $reset = FALSE) {
+  $instance = hosting_package_instances_load($param, $reset);
+  // $instance could be FALSE or an empty array.
+  if (!empty($instance)){
+    $instance = reset($instance);
+  }
+  return $instance;

This looks weird to me. What is $reset for? Shouldn't it be documented in the phpdoc above?

Also, why do we reset() a non-empty array? The comment seems unrelated...

+++ b/modules/hosting/package/hosting_package.instance.incundefined
@@ -107,57 +114,79 @@ function hosting_package_instance_load($param) {
-function _hosting_package_instances_load($param, $multiple = FALSE) {
+function _hosting_package_instances_load($param, $reset = FALSE) {

we seem to be changing the API there. have you researched to see if $multiple is actually in use? This would need to be documented in the upgrade path, at the very least.

thedavidmeister’s picture

I have researched to see if $multiple is actually in use. It isn't used by anything that I didn't refactor.

The $reset is technically not required, but it is the Drupal 6 "equivalent" of the functionality provided by drupal_static() in Drupal 7. Documentation for the approach that I took is which is pretty standard afaik.

reset() returns the first item in the array, which simplifies what $multiple used to do when it was FALSE but allows multiple and not multiple to share the same cache. See for an example in Drupal 7 core.

anarcat’s picture

Cool, that seems all good responses. Could you just make sure you have a phpdoc for $reset?

Also, i guess the same applies as in #1875446: Cache tables for hosting_package - benchmarks are important here. :) if we're just trying to skip some joins, maybe the proper fix is to add aliases on those tables...

thedavidmeister’s picture

Actually, I see static caching as the first step towards persistent caching in D6 - the core cache in D7 automatically statically caches everything it looks up but this has to be done ourselves in D6.

For me personally, I'd like to see benchmarking of what happens when you use a non-db based cache system as well as just whether there's an improvement in referencing a cache table over running some joins.

My experience elsewhere is that you can often improve performance pulling things from something like memcache, apc, or a memory file system style cache than the database regardless of the specifics of the query being run, but *only* if the lookups are available through a call to cache_get().

I'll re-roll the patch above with better docs.

ergonlogic’s picture

Version: 6.x-2.x-dev » 7.x-3.x-dev
Issue summary: View changes
Issue tags: -

New features need to be implemented in Aegir 3.x, then we can consider back-porting to Aegir 2.x.