Hi,

I've been trying to track down some performance problems on our site and discovered that the combination of the "me" module and tokens is playing a significant part. It seems the "me" module likes to pass paths through token_replace() to replace any global tokens, which causes all global tokens to be fetched on every page. Because we have some modules that produce somewhat expensive global tokens (like Ubercart), this was significantly increasing query count and load time.

Since we don't have any tokens in paths anyway, it seems wasteful for token_replace() to load up all this data if it's not going to use it, so I've crafted a small patch that will skip all that hard work if the string doesn't contain any tokens anyway. I can't see this being detrimental in any way?

Another optimisation that would be nice would be to token_scan() the string before substitution (like D7 does), and only call out to those modules that supply the tokens that are in the string, rather than fetching all the tokens of that type. Some token_values() calls are quite expensive, and often superfluous.

Patch attached for review.

Thanks

Files: 
CommentFileSizeAuthor
#10 1307890-token-fix-api-array-handling-9.patch1.17 KBELC
PASSED: [[SimpleTest]]: [MySQL] 196 pass(es).
[ View ]
#8 1307890-skip-if-no-tokens-in-string.patch3.6 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1307890-skip-if-no-tokens-in-string_1.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#6 1307890-skip-if-no-tokens-in-string.patch3.6 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] 183 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#5 1307890-skip-if-no-tokens-in-string.patch3.6 KBDave Reid
PASSED: [[SimpleTest]]: [MySQL] 184 pass(es).
[ View ]
token.module-skipload.patch683 bytesneilnz
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch token.module-skipload.patch. See the log in the details link for more information.
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, token.module-skipload.patch, failed testing.

Status:Needs work» Needs review

The patch seems like it could be a good improvement.

Another optimisation that would be nice would be to token_scan() the string before substitution (like D7 does), and only call out to those modules that supply the tokens that are in the string, rather than fetching all the tokens of that type. Some token_values() calls are quite expensive, and often superfluous.

We can't change the current API in this fashion. Encourage people to upgrade to Drupal 7 since tokens are generated on-demand now with the improved API in 7.

Sure, but a half-way optimisation that just module_invoke()'s hook_token_values() only on the modules that provide the tokens in the string would still avoid lots of unnecessary token production that just goes to waste. I realise the API doesn't allow passing in the specific tokens, but even if you only need to call token_values on 2 modules instead of 20, it could be a bit of a speed-up without breaking API compatibility.

This shouldn't be too hard to implement given that token_list should give a cacheable catalog of tokens to modules that supply them. I guess the caveat is there may be some naughty modules that provide tokens without declaring them, or provide token values that were listed by another module, but they should be using the alter hook to do that anyway.

I'm happy to take a shot at making a patch for this if you're interested. I've still got a lot invested in D6 right now, and moving off that will be a gradual process.

I am very interested in this.
preg_replace_callback() is your friend!

This will not only improve performance, but also prevent some of the side effects I find myself running into. That is, it does not really fix these problems, just prevent them from happening on a majority of sites.

StatusFileSize
new3.6 KB
PASSED: [[SimpleTest]]: [MySQL] 184 pass(es).
[ View ]

Revised patch that fixes a bug in token_scan() so that it can be reliably used to check if a string has any replaceable tokens or not.

StatusFileSize
new3.6 KB
FAILED: [[SimpleTest]]: [MySQL] 183 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Re-rolled with additional tests for tokens like [[foo]] and %%foo.

Status:Needs review» Needs work

The last submitted patch, 1307890-skip-if-no-tokens-in-string.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1307890-skip-if-no-tokens-in-string_1.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Status:Needs review» Fixed

Tested manually. Committed #8 to Git: http://drupalcode.org/project/token.git/commit/4fe3959

Priority:Normal» Major
Status:Fixed» Needs review
StatusFileSize
new1.17 KB
PASSED: [[SimpleTest]]: [MySQL] 196 pass(es).
[ View ]

As I found out today after an update, this fix destroys the ability for token_replace to be called with an an array instead of text, resulting is this fun error message:

warning: preg_match_all() expects parameter 2 to be string, array given in [snip]/sites/all/modules/token/token.module on line 705.

Attached is an improvement to fix that issue, however it might not be the best method. It attempts to get the $text_tokens as quickly as possible, without calling preg_match_all a zillion times. I believe this is O(n) instead of O(2n) which is not too bad. There might be faster way but this certainly works.

Update: changing my O() evaluations.. again .. essentially it's one call to implode, and one call to preg_match_all, instead of a loop through all of n with a preg_match_all and all the associated work with doing that lots of times.

The $text parameter has never been supported as an array.

Status:Needs review» Fixed

Priority:Major» Normal

Well it has worked quite well up until now - the custom_breacrumbs module uses it which is what brought the error to my attention.

If it's not meant to be accepting arrays, I'll have to go file a bug with custom_breadcrumbs :/

From custom_breadcrumbs_nodeapi($node, $op, $teaser, $page)

      $titles = preg_split("/[\n]+/", $breadcrumb->titles);
      $paths = preg_split("/[\n]+/", $breadcrumb->paths);
      $titles = module_exists('token') ? token_replace($titles, 'node', $node) : $titles;
      $paths = module_exists('token') ? token_replace($paths, 'node', $node) : $paths;

Don't feel like accepting arrays? ;)

Definitely a bug in custom_breadcrumbs. $text has always said it is a string and never mixed use.

Odd, so it seems that str_replace() supports $text being array, but the API for token_replace() has never supported it. This would also be broken in Drupal 7 as it has the same behavior. Modules doing the incorrect thing need to be fixed.

It was just so convenient and it worked ... that's why I thought it would be easy enough to fix in token module. IMESHO, the solution I came up with was pretty fly and near cost free ;)

Anyway, for reference, bugs reported and patches filed for

custom_breadcrumbs - #1327960: Invalid use of token_replace API - causes preg_match_all errors in token module
custom_pagers - #1327982: Invalid use of token_replace API - causes preg_match_all errors in token module

I'm sure there's a few more out there doing the same thing.

Same problem here, due to custom_breadcrumbs, and the patch apparently fixed it. Thanks ELC.

Looks like we might have caused a regression with our own rules integration. Patch is available for testing at #1329196: Rules integration broke in 6.x-1.17.

I think it's much better to support $text arg as array than call token_replace in foreach cycle in every module.

Messagin / Notifications modules affected too. (and probably all modules that uses messaging)

Anyway it's wrong time to change the rules.

Sorry, but $text as an array has *never* been supported in the API itself, it just happened to work by accident. You will run into this same problem when porting your module to Drupal 7 so it's better to have things fix themselves now.

@Dave,
do you think it would be realistic in D6 to do a lazy evaluation of hook_token_values() based on the actual tokens found in a string?

If we know which module is responsible for a given token (via hook_token_list, or via cached hook_token_values), then we invoke only that. If we don't know the token, or if it does not work, then we invoke all the other modules' hook_token_values.

This can be done with preg_replace_callback(), or with some other preg_ stuff.
If you say you are interested, and you believe it is realistic, then I might spend some time and prepare a patch.
If you say this is too risky, or you are not interested, then I rather don't want to spend time on it.

Title:token_replace() performance when there's no tokenstoken_replace() performance when there are no tokens
Status:Fixed» Active

It seems issue #1329428: Update Viewfield token replacement for API change is caused by the same "improvement" introduced with this commit.

However I am not using the custom_breadcrumbs or custom_pagers module. How do you suggest to find out what modules could cause this? I would run a recursive grep through the modules directory, but on which search string?

Status:Active» Fixed

I have added some debugging code to 6.x-1.x-dev release that will help trace what function is responsible for calling this incorrectly. Please download the 6.x-1.x-dev release in about 12 hours to help me debug this further.

I mean you break back compatibility. That's ok perhaps in D7 , but many D6-modules wont work with token-6.x-1.17+ (and probably never will get updated).

but $text as an array has *never* been supported in the API itself

I understand your point. But it's possible to add support of $text as an array to the API. It's easiest way for drupal community (and the best way imho)

Status:Fixed» Active

Fully agree to #24. Even if Dave Reid is 100% correct in terms of technical specs, the change in 1.17 would break countless sites which is not nice to the community. There seems to be a lot of stable D6 modules affected, so it is definitely not an option for production sites to hope and wait that all these module authors would update their modules.

I had to revert back to Token 6.x-1.16. If there would be a 6.x-1.18 release that is again compatible with all these modules, I will try again - otherwise have to stick with 6.16. To allow further discussion, I have changed this status to active. When you set it back to fixed, does this mean that you are not willing to make this change compatible and keep it this way?

ehm, agree with #24 and #25.
Technical spec is worth little, if it is weakly enforced.

Status:Active» Fixed

I have committed a follow-up with http://drupalcode.org/project/token.git/commit/f02c914 that will allow token replacement to still work with arrays but it will also still throw a notice because modules which call with an array still need to be fixed. I will reiterate that it was never supposed to be supported. It only worked by accident.

@donquixote Possibly - let's file a new issue for that but frankly at this point I'm almost not wanting to touch token for D6 ever again after this crap.

Thank you for re-supporting the "unsupported" but obviously widespread call, so the existing modules can continue to work with the current version of the Token module.

6.x-1.18 has been released adding the support but warning for array $text parameter.

Perfect - thank you. Just upgraded to 6.x-1.18 and all seems to be fine again. The warnings regarding the wrong use of $text as array should motivate the authors of the affected modules to correct them.

I understand why you have added the logging message but perhaps next time you could consider that it can render a site's logs useless if you are using something like custom breadcrumbs because it quickly fills the watchdog log with an error message (not a warning) for every page hit. And then when I went through the code I saw that the array is handled anyway making the error message spurious.

I have already considered that. Download 6.x-1.x-dev as I have removed the logging message.

No problem - I already commented it out.

So with more explanation, I'm officially backtracking on not supporting $text being an array. I looked back and as of 6.x-1.15 the token_replace() functions officially had it documented that $text could either be a string or an array. This reference to it being an array was removed when I updated the documentation for the functions which I had assumed the array reference was a mistake (which was my fault). At this point in 6.x-1.x-dev (and eventually in the future 6.x-1.19 release) there are no more warnings thrown by token if $text is an array and token replacement should just work as before. I apologize to the end users of Token who encountered those frustrating notices and the module developers who had these bugs reported against their modules. This experience was at least valuable to know which modules were using array token replacement as they would have encountered this problem when porting modules to Drupal 7. Also, in the official API documentation, $text is a string. The parameter being an array is only supported for backwards-compatibility.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.