Non-unique cache-ids generated for features-based panels.

I am running into a problem that this patch can partly address. We have a number of panel pages with typically 5-10 panes in them. All of these pages are coming via Features and are considered to be 'in code' from the context of panels. Since the panels definitions are in code and not in the database there is no numerical display ID (did) nor pane ID (pid) for each pane. This means when using simple cache on a pane that there is not a unique cache key as each panel defined in a feature has display ID of 'new' and each pane within that panel page has a pane ID of 'new-X' where X is a number reflecting the order of the panes defined in the exported panel page in the feature.

So one could easily end up with a simple cache id of 'panels_simple_cache:new:new-1' for the first pane of a panel page from a feature which would of course be the exact same cache id used for the first pane of a panel page from another feature.

The simple cache code references a display owner and owner id that can be included in a cache id but none of the panel panes that come out of my feature have any displays so they have nothing else to distinguish one panel pane from another pane in another panel page in the same position.

If you add the display->cache_key as in the patch that's one way to add a unique identifier to the cache id. In terms of maximum size for a cache id there is a limit that needs to be accounted for. Regular DB caching has a field size for the cid of 255. If using Memcache there is also an issue with cid size as was covered in #525400: long keys get truncated.

So if caching with Memcache module the cache id size is taken care of but for DB caching there is nothing that checks the cid length so it would probably be prudent to account for it in the simple cache plugin.

The caching for Views already uses md5 to generate a key that captures all the necessary context for the view in question so there is a precedent for it already in Earl's other big masterpiece.

There is a similar problem in panels_simple_cache_clear_cache() in that if the display ID is not unique ('new') then an attempt to clear an entire display you could actually clear all displays if all displays are of storage type 'in code' and like those from features have no display ID.

function panels_simple_cache_clear_cache($display) {
  $cid = 'panels_simple_cache';

  // This is used in case this is an in-code display, which means did will be something like 'new-1'.
  if (isset($display->owner) && isset($display->owner->id)) {
    $cid .= ':' . $display->owner->id;
  }
  $cid .= ':' . $display->did;

  cache_clear_all($cid, 'cache', TRUE);
}

And here's a patch for at least the pane level caching.

I've not actively used patch #4 yet however it looks like it should be a significant improvements.

A thought... Why are you using an array, serializing to a string and THEN MD5ing? Why not just start with an empty string and concatenate values onto it (much like $id)? I've not benchmarked - but SURELY string concatenation is more efficient than array allocation and serialization?

It's also work noting that this effects the D6 branch too...

For reference...

<?php
echo "Starting\n--------\n\n";

$start = microtime(TRUE);
for ($i = 0; $i < 1000000; $i++) {
  $t = '';
  $t .= ':'. 'AAA';
  $t .= ':'. 'BBB';
  $t .= ':'. 'CCC';
  $t .= ':'. 'DDD';
  $t .= ':'. 'EEE';
  $t .= ':'. 'FFF';
  $t .= ':'. time();
  $t = md5($t);
}

$end = microtime(TRUE);
echo 'String Concat Time: '. ($end - $start);
echo "\n\n\n";



$start = microtime(TRUE);
for ($i = 0; $i < 1000000; $i++) {
  $t = array();
  $t[] = 'AAA';
  $t[] = 'BBB';
  $t[] = 'CCC';
  $t[] = 'DDD';
  $t[] = 'EEE';
  $t[] = 'FFF';
  $t[] = time();
  $t = md5(serialize($t));
}

$end = microtime(TRUE);
echo 'Array Concat Time: '. ($end - $start);
echo "\n\n\n";

The result is:

Starting
--------

String Concat Time: 1.3722338676453


Array Concat Time: 3.3130779266357

Array serializing is around 2.5x slower (depending on run, it sometimes spiked as high as 3x) than string concatenation.

Admittedly - this is over 1 million runs, so the impact is relatively small.

EDIT: Then again... Thinking about it - this function will get called several times per (un-page-cached) page (once per pane, for example). Say you have 10 panes and a busy site (eg 30 requests a second)... That's 300 requests per second. I guess that adds up ;) Based on that, I think that means every 1 hour you would waste 2 seconds on processing time ;-) heh...

EDIT 2 : A suggested by StewSnooze, I tested json_encode as an alternative to serialize. Interestingly, json_encode is faster, but still significantly slower than string concatenation.

Mostly for simplicity. serialize is one command I can use that I know is reliable on all structures no matter what's in it. Anything else is going to be risky if the structure isn't what we expect.

The implications of this bug does in no way deem it a 'minor' status. Also, I think the patches attached are not complete as they do not include the necessary corresponding changes to the panels_simple_cache_clear_cache function.

Attaching a simpler version of the patch in #4. I keep the concatenation instead to make the patch much shorter.

Not sure if panels_simple_cache_clear_cache really needs a change.

Isn't this the same issue as #1349118: Simple Caching ID scheme will lead to conflicts with exported Panels definitions which has already been committed to D7 but just needs to be backported to D6?

I reviewed the patches from both issues and yes, #1349118 makes this redundant.