Port Path Cache to Drupal 7

A big +1 here, for D7 and its pluggable field storage this patch actually helps a lot. In an odd twist of events when you take a bunch of the "large" queries off MySQL, the small ones start to pile up. (who could have guessed...) The url_aliases queries are the top queries on stock D7 + MongoDB in my early testing. This patch to path cache evicts them from the lists and got me 50 more requests per second.

catch++

The cache is happy to get and set cached paths in a variety of lanuguages and frequently will end up with language_default. However, pathcache_clear_cache only clears the original path language plus LANGUAGE_NONE, resulting in caches not being cleared for, say, language_default(), if language_default() is not the same as the language of the saved path alias, which could be eg. LANGUAGE_NONE.

The only change in this patch from the previous is to clear the cache for the requested path in all configured languages.

Reworked patch in #15 to match today's Drupal 7.x HEAD

Haven't tested it yet, though...

Re-rolled against branch 6.x-1.x of git.drupalcode.org/project/pathcache.git

Re-rolled after the Great Git Migration.

Okay, I finally re-read the note at the top about path.inc being swappable.

I still think that the module should maintain a patch instead of a copy of path.inc, though.

Re-rolled accordingly, and added install hooks to set the path_inc variable.

subscribe

subscribe

subscribe

subscribe

It might also be nice to submit a patch to include it into d8 core.

would be harmful with the database backend

Even if it were an optional module that is turned off by default?

How about a d8 patch that only adds the necessary hooks to path.inc that would enable a d8-version of pathcache to run, if it were enabled?

If I we get CacheArrayObject in from the theme registry issue

Are you referring to this issue?

#1011614: Theme registry can grow too large for MySQL max_allowed_packet and memcache default slab size

Dealing with an issue today and this may indeed also be a core issue.

If I call:

drupal_lookup_path('source', $alias);
drupal_lookup_path('wipe');
drupal_lookup_path('alias', $source);

Then the second call returns nothing this is because I have a path_alias_whitelist defined in my settings.php. Calling wipe results in this not getting repopulated. Amending

if ($action == 'wipe') {
  $cache = array();
  $cache['whitelist'] = drupal_path_alias_whitelist_rebuild();
}

to

if ($action == 'wipe') {
  $cache = array();
}

Results in the $cache array being correctly initialised.

subscribing

subscribe

sub

Subscribing...

subscribing

Subscribing...

subscribe

basicmagic.net, STOP subscribing. Start following.
http://drupal.org/node/1306444

Looking at the path.inc differences between the one from the patch and includes/path.inc, I noticed the following:

+          // Add a cache entry with an empty string - this saves hitting the
+          // db for zero results. We can afford to do this because the
+          // whitelist will prevent attempts to cache paths like comment/$cid.
+          // And because memcache operates an LRU cache. However, only do this
+          // for anonymous users to avoid bloating the cache with paths like
+          // node/n/edit.
+          if ($GLOBALS['user']->uid) {
+            cache_set($path_cid, '', 'cache_path_source');
+          }

The comment makes sense, but the anon check seems to be implemented the wrong way round?

@Berdir - agreed, the comment and the code are opposed.

I'm sure I hit this problem on a site (a little hard to remember now though ...) and ended up logging all the path cache entries and realising that what's described in the comment are not all good assumptions, it's generally necessary to analyse what sort of paths are being hit on your site, what paths you have in the whitelist and what additional paths have been created by enabled modules. In short: the behaviour of this chunk won't actually suit everyone's use case.

Anyway ... back to your point - yes, it's the wrong way round.

Re-rolled.

1. Fixed according to #39.
2. Fixed comments for the install file.
3. Changes to README.txt and path.inc.patch are not included. We can update them after we finish the patch.

This afternoon I tested the Patch in #41. I'm saving at least 20 queries a page according to Devel. I have not noticed any problems at all. I'm not familiar with path cache's implementation and have not reviewed the code, but simply based on usage experience this would appear RTBC for a first pass. Updating README.txt can come afterwards.

I'm seeing a 50+ query improvement on link-heavy pages. I don't see any problems either myself, but I won't RTBC without production environment testing.

subscribing.

Can we get a 7.x branch commit to start here? Even if it's only in development snapshots, it will be easier to provide further feedback and testing.

The patch as it stands does not take into account #30 which prevents whitelist set via variables being populated correctly.

I believe I found a bug in the handling of looking up a source from the cache, and afterwards looking up the alias for the same source..

The code in question is this excerpt:

      if (!isset($cache['map'][$path_language]) || !($source = array_search($path, $cache['map'][$path_language]))) {
        $path_cid = $path_language . ':' . $path;

        if ($cached = cache_get($path_cid, 'cache_path_source')) {
          if (!empty($cached->data)) {
            $cache['map'][$path_language][$source] = $source =  $cached->data;
          }

Say I have an alias 'node/1234' -> 'my-custom-name'

So first I call drupal_get_path('source', 'my-custom-name', $language), it fails to find the $source in the $cache['map'], so we enter the if-condition. We then lookup the source in the cache, so $cached->data == 'node/1234'. This means that $source will be set to node/1234, so it will set $cache['map'][$path_language]['node/1234'] = 'node/1234';

So the next time you then call drupal_get_path('alias', 'node/1234', $language), it will return 'node/1234', because it's now present in the $cache['map'].

I believe the code should be

      if (!isset($cache['map'][$path_language]) || !($source = array_search($path, $cache['map'][$path_language]))) {
        $path_cid = $path_language . ':' . $path;

        if ($cached = cache_get($path_cid, 'cache_path_source')) {
          if (!empty($cached->data)) {
            $source = $cached->data;
            $cache['map'][$path_language][$source] = $path;
          }

This way, it will set $cache['map'][$path_language]['node/1234'] = 'my-custom-name'; which is correct!.

Kind regards
Morten.

subscribing

I know that http://tag1consulting.com/patches/path-cache is in D7 but it's often that I can identify url_alias as one of the most consuming DB offender in a D7 site.

Thus I wonder if there's any movement about this? (here or elsewhere, that is)

I am using the patch from #41 and it works great apart from one small thing:
When I update the entity, then the "Generate automatic URL alias" checkbox becomes unchecked and the alias doesn't get updated automatically.

I believe this happens due to the drupal_get_path_alias($uri['path'], $langcode) function in the pathauto_field_attach_form hook implementation.
It returns cached alias rather than newly updated.

So I suggest to implement hook_entity_update() and update the cache for the alias.

The step to patch the path.inc seems a bit silly, in Drupal-7 you can set a new file for the path.inc include

./common.inc:  require_once DRUPAL_ROOT . '/' . variable_get('path_inc', 'includes/path.inc');

Applied your patch, then tried to apply the path.inc.patch from 7.x-1.x

patching file path.inc
Hunk #1 FAILED at 46.
1 out of 1 hunk FAILED -- saving rejects to file path.inc.rej

The patch on #51 probably should not be doing a query and setting the cache, as this could get into a racing condition with the bit of code that is responsible for setting the cache using that same cid.

function pathcache_entity_update($entity, $type) {
  if(!empty($entity->language) && !empty($entity->path['source']) && !empty($entity->path['pid'])){
    $path_cid = $entity->language.':'.$entity->path['source'];

    $query = db_select('url_alias', 'ua');
    $query->addField('ua', 'alias');
    $query->condition('pid', $entity->path['pid'], '=');
    $query->condition('source', $entity->path['source'], '=');
    $query->condition('language', $entity->language, '=');
    $new_alias = $query->execute()->fetchField();

    if(!empty($new_alias)){
      cache_set($path_cid, $new_alias, 'cache_path_alias');
    }
  }
 }

It would be better if you just cleared the cache bin for that cid i.e.

cache_clear_all($path_cid, 'cache_path_alias');

Or even

pathcache_clear_cache($entity->path['source']);

Thanks, marcelovani.

Replaced

$query = db_select('url_alias', 'ua');
    $query->addField('ua', 'alias');
    $query->condition('pid', $entity->path['pid'], '=');
    $query->condition('source', $entity->path['source'], '=');
    $query->condition('language', $entity->language, '=');
    $new_alias = $query->execute()->fetchField();

    if(!empty($new_alias)){
      cache_set($path_cid, $new_alias, 'cache_path_alias');
    }

with

cache_clear_all($path_cid, 'cache_path_alias');

Works ok to me!

@dgtlmoon, I think the path.inc.patch is only for patching D6 version of path.inc

Sweet, thanks man, have applied the re-rolled patch to 6.x and seems to be functioning, not seeing the usual 400 drupal_lookup_calls in my devel module output, waiting to see if any issues come up

Along with

+++ b/pathcache.module
@@ -6,11 +6,55 @@
+    $path_cid = $entity->language.':'.$entity->path['source'];
+    cache_clear_all($path_cid, 'cache_path_alias');
+  }
 }

we may need to also clear 'cache_path_source'.

Thanks girishmuraly !
Re-rollled the patch

Patch in #59 works great for me.

This looks very stable, why not create a D7 release?

Agree, it works fine and should be committed

I agree. I tested and it worked for me too.

The issue is https://drupal.org/comment/4651040#comment-4651040 still exists and is an on going item that needs fixed.

I disagree with you, marcingy here.

This just means that:

* If you use whitelists in Drupal 7, don't use this module as there is a bug.

**RTBC** again - lets get this in, then deal with the whitelists bug.

I looked into this:

And the only thing needed is:

diff --git a/sites/all/modules/contrib/pathcache/path.inc b/sites/all/modules/contrib/pathcache/path.inc
index f3920e8..606ae8d 100644
--- a/sites/all/modules/contrib/pathcache/path.inc
+++ b/sites/all/modules/contrib/pathcache/path.inc
@@ -78,7 +78,6 @@ function drupal_lookup_path($action, $path = '', $path_language = NULL) {
 
   if ($action == 'wipe') {
-     $cache = array();
+     $cache = NULL;
-    $cache['whitelist'] = drupal_path_alias_whitelist_rebuild();
   }
   elseif ($cache['whitelist'] && $path != '') {
     if ($action == 'alias') {

as marcingy pointed out and I saw now that it needs to be NULL instead of array ...

The reason is that the check above does more:

  if (!isset($cache['whitelist'])) {
    $cache['whitelist'] = variable_get('path_alias_whitelist', NULL);
    if (!isset($cache['whitelist'])) {
      $cache['whitelist'] = drupal_path_alias_whitelist_rebuild();
    }
  }

Again this is no issue when not using 'path_alias_whitelist' variable.

And the check for re-init does use:

if (!isset())

Can someone re-roll the patch, please so we can get this committed?

Can anyone provider a full working module for this. I keep getting errors when I try to apply the patches.

Just that small change is all that is needed?

Nope, both changes are needed.

Remove the:

- $cache = array();
+ $cache = NULL;
- $cache['whitelist'] = drupal_path_alias_whitelist_rebuild();

whitelist as well.

Here is the final patch. I also changed the README for D7 that path.inc should be included.

Can you post files as ready drupal 7 module?

I second the request for a full Drupal 7 module, if this seems to be working properly now. 100's of path requests per page load is a little ridiculous, and is the biggest eater of queries on my pages now.

There shouldn't be hundreds of queries with Drupal 7's default caching implementation. Note that there is a major issue in redirect that is breaking the path cache: #2048137: Canonical redirect breaks path cache

Even with redirect disabled, I saw enough queries in new relic to justify the existence of this module / patch.

I will track down the maintainer of path_cache again and am willing to co-maintain.

@fabianx if you get co-maintainership and need some assistance I or one of the other devs at examiner.com will also be willing to step up as co-maintainer.

Thanks everyone for helping. I was able to reduce my queries by close to 20% on some pages, so this module is a big help.

However I have just found that this D7 patch breaks the "Path Auto" module. All current paths still work, however once the "Path Cache" module is enabled you are no longer able to edit any new paths, and the default paths do not work on new nodes either. Once Path Cache is disabled AND uninstalled, only then does Path Auto work properly again.

There are no error messages on the screen or in the logs to report. I'm not a programmer, so I cannot add anymore than what I just found.

I am a co-maintainer now and will also onboard marcingy as co-maintainer, too per #75. The 7.x-1.x branch should be opened soon. :)

#76: Did you try if installing pathauto_persist would solve that problem?

#75: Added you as a maintainer now, feel free to add more from examiner. I am a strong believer in teams! :)

Thanks Fabianx I have also added slashrsm(https://www.drupal.org/u/slashrsm).

I originally didn't try Pathauto_persist because it seemed like it was fixing a separate issue. I just tried it and it did not resolve the issue.

Hey, great to know we have new maintainers, I have also worked on this patch and it's been a lot of time waiting for this to be committed. I did not have any issue with Path Auto

I don't have access to a shell script/service, so I patched the files "manually". I fear I likely messed something up, so I will be eager to try a proper 7.x-1.x version when it is available and test again.

AAAAAND ....

Fixed!

Thanks so much everyone.

7.x-1.x branch exists now, will create versions, etc. now.

Lets create issues for any issues.

I also fixed as a quick follow-up the whitelist change that again sneaked in ...

Thanks all! Just to note, it must have been a patch issue for me as this new version is working as expected.