Improve performance of pathauto_cleanstring() and pathauto_get_placeholders()

Constructive and drama free posts are more likely to be taken seriously. I look forward to the suggestions for improvement.

Static caching added to pathauto_punctuation_chars() on all three branches:
http://drupal.org/cvs?commit=385382
http://drupal.org/cvs?commit=385384
http://drupal.org/cvs?commit=385386

This seems like an obvious improvement so I don't mind you committing it. I feel like the flow here could have been a little more like the ideal of posting a patch and waiting at least 24 hours before committing.

Sorry about that Greg. Sometimes I need to be subtly reminded to post patches instead of directly committing.

@donquixote - does this "fix" the problem in your opinion?

@dave - no worries, it's a really small thing.

Here's another patch against HEAD to add some static caching to pathauto_cleanstring().
1. Statically cache if transliteration is enabled and the transliteration module is enabled.
2. Statically cache the punctuation and default variables into an array of keys of search values, and values of replacements. Then any following calls only have to call str_replace().

Yes, new features will always need to be patched for 7.x-1.x first. Once committed they can be packported to 6.x-2.x.

I recently installed XHProf and have been systematically profiling my site. I was recently reviewing page load times related to node creation, and was surprised to see that out of about 1000ms worth of processing, about 800ms were spent in pathauto_nodeapi() and auto_nodetitle_nodeapi() -- about 400ms for each. Digging deeper I found that pathauto_cleanstring() and pathauto_get_placeholders() were the functions primarily responsible, in both cases, for the long load times.

I am definitely interested in any efforts to optimize these functions, and would be happy to contribute if I can help in any way.

In my opinion, Dave's patch #23 is a better solution since it isn't as drastic of a change. It's also ready for D7 and just needs some improvement to figure out what if anything it's doing to the simpletests.

In general, it's true that token generation is slow in D5/D6 and much faster in D7.

A further optimization could be to not translate the 32 token descriptions at all if not on the pathauto settings page. This would save 32 translate database calls on each node_edit_form, node_insert and node_update call.

Token descriptions are not fetched on token replacement, so I'm not sure where #29 is coming from.

#30: I'm using the 2011-01-13 7.x-1.0-beta1 version. I could not apply the most recent patch from this thread, but after applying it manually, the patch (from #23) seems not to change this.

The call tree (upwards):
pathauto_punctuation_chars()
pathauto_cleanstring()
pathauto_clean_token_values()
token replace() (pathauto_clean_token_values passed as callback)
pathauto_create_alias()

called by one of these hooks:
pathauto_form_alter() or
pathauto_node_update_alias() or
pathauto_taxonomy_term_update_alias() or
pathauto_user_update_alias() or
pathauto_blog_update_alias()

Call tree starting form pathauto.admin.inc:
pathauto_punctuation_chars()

pathauto_settings_form() or
pathauto_settings_form_validate()

Thus on every content create, update, and (node) update form the descriptions are fetched and translated without need. By moving the call to t() to the actual use of the 'name' entry, the texts do not get translated on the other cases.

Oops, I now see where the confusion is coming from (#30 in response to #29). I meant to say 32 punctuation descriptions as gets translated in pathauto_punctuation_chars().

I came to this issue by searching on pathauto_punctuation_chars. But looking at my suggestion and the direction this thread went after #5, it might be better to make it a separate issue. Moreover, the patch for my suggestion won''t influence the patch over here. Therefore see #1043424: Prevent translations when not used

subscribe for 6.x. Looking at a cachegrind of cron.

pathauto_cleanstring get's called 34,778 times on a cron run via _feedapi_invoke_refresh. _feedapi_invoke_refresh only gets called 37 times though. The direct caller is pathauto_clean_token_values which gets called 316 times. This function eats up over a minute for us.

Looking at the code pathauto_cleanstring seems like it could use a static for 6.x

Have yet to put this patch through production to get performance numbers

With my 6.x patch function got called 38,402 times but it took under 3 seconds to run (total time). Big improvement with over a minute.

Re-testing and benchmarking this after #1182710: Improve performance of pathauto_get_placeholders() landed which reduces the amount of calls to pathauto_cleanstring() a lot.

Patch attached for 6.x-2.x and benchmarked:

ab -n 50 http://mysql.drupal6dev.local/test.php

test.php:

require_once './includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
$result = db_query("SELECT nid FROM {node}");
$nids = array();
while ($nid = db_result($result)) {
  $nids[] = $nid;
}
// FYI: count($nids) == 200
pathauto_node_update_alias_multiple($nids, 'bulkupdate');

BEFORE PATCH
------------

Full executions of pathauto_cleanstring(): 505
memory_get_peak_usage(TRUE): 16.75 MB

Requests per second:    0.34 [#/sec] (mean)
Time per request:       2962.172 [ms] (mean)
Time per request:       2962.172 [ms] (mean, across all concurrent requests)
Transfer rate:          0.16 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:  2709 2961 765.2   2785    7110
Waiting:     2709 2961 765.2   2785    7110
Total:       2709 2961 765.2   2785    7110


AFTER PATCH
-----------

Full executions of pathauto_cleanstring(): 221
memory_get_peak_usage(TRUE): 17 MB

Requests per second:    0.35 [#/sec] (mean)
Time per request:       2861.152 [ms] (mean)
Time per request:       2861.152 [ms] (mean, across all concurrent requests)
Transfer rate:          0.16 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:  2682 2860 441.9   2780    5848
Waiting:     2682 2860 441.7   2780    5847
Total:       2682 2860 441.9   2780    5848

Patch re-rolled for D7 to use the fast-version of drupal_static().

Committed to all three branches:
http://drupalcode.org/project/pathauto.git/commit/e126025
http://drupalcode.org/project/pathauto.git/commit/a698011
http://drupalcode.org/project/pathauto.git/commit/bc31549

I'm going to file a separate issue for the cleanstring optimizations that I attempted.

Going to re-attempt the optimizations now in the same issue...

Benchmarks for #39 running with MAMP, a D7 install with various patterns for each node type with 1-5 tokens for each.

ab -n 100 http://mysql.drupal7dev.local/test.php

test.php:

define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
$nids = db_query("SELECT nid FROM {node}")->fetchCol(); // FYI: count($nids) == 200
pathauto_node_update_alias_multiple($nids, 'bulkupdate');

BEFORE PATCH
------------

Requests per second:    1.16 [#/sec] (mean)
Time per request:       860.577 [ms] (mean)
Time per request:       860.577 [ms] (mean, across all concurrent requests)
Transfer rate:          0.37 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   814  860  42.5    851    1020
Waiting:      813  859  42.5    851    1020
Total:        814  860  42.5    851    1020


AFTER PATCH
-----------

Requests per second:    1.23 [#/sec] (mean)
Time per request:       812.486 [ms] (mean)
Time per request:       812.486 [ms] (mean, across all concurrent requests)
Transfer rate:          0.39 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   782  811  34.8    799     983
Waiting:      781  811  34.8    799     983
Total:        782  811  34.8    799     983

I'll take -50ms any day!

Committed to 7.x-1.x and 6.x-2.x:
http://drupalcode.org/project/pathauto.git/commit/e8c8363
http://drupalcode.org/project/pathauto.git/commit/cf99997