$_GET['q'] is not properly initialized when serving cached pages. The following code is unreachable when serving a cached page:

  // Initialize $_GET['q'] prior to loading modules and invoking hook_init().
  if (!empty($_GET['q'])) {
    $_GET['q'] = drupal_get_normal_path(trim($_GET['q'], '/'));
  }
  else {
    $_GET['q'] = drupal_get_normal_path(variable_get('site_frontpage', 'node'));
  }

This results in statistics_exit() not counting reads from anonymous users in presence of a path alias. The check if ((arg(0) == 'node') && is_numeric(arg(1)) && arg(2) == '') { (from statistics_exit())will fail when a path alias is active. In that case, arg(0) will be 'my-path-alias' and not 'node'. The solution is to move the initialization code, however, it introduces some database overhead.

CommentFileSizeAuthor
#27 arg.patch229 byteschx
#22 path.inc_4.patch15.31 KBchx
#19 path.inc_3.patch14.91 KBchx
#18 path.inc_2.patch9.72 KBchx
#16 init_path_0.patch6.11 KBJaza
#13 path.inc_1.patch15.56 KBJaza
#10 path.inc_0.patch13.57 KBchx
#7 path.inc.patch17.5 KBchx
#6 init_path.patch2.01 KBCrell
q.patch1.61 KBdries

Comments

dries’s picture

dries commented
Status: Active » Needs review
chx’s picture

chx commented

As far as I can remember, two people have fiddled with the bootstrap process: moshe and me. $_GET['q'] init code was in common.inc before the bootstrap rehaul which was not included when serving cached pages.

Therefore, I am turning to moshe -- what do you think?

dries’s picture

dries commented

Maybe we need to introduce a function, that contains this code. Then the function can be called from statistics_exit(), as well as from the current location. If the statistics.module is not enabled, you won't suffer from any overhead. Slightly uglier code-wise, but faster execution-wise.

moshe weitzman’s picture

moshe weitzman commented
Version: 4.6.5 » x.y.z

a new function makes sense. no time to do a full review right now.

dries’s picture

dries commented
Priority: Normal » Critical

Going to bump the priority of this. It doesn't break your site, however, it does render statistics.module semi-useless.

Crell’s picture

Crell commented
StatusFileSize
new init_path.patch2.01 KB

Ask and ye shall receive.

I'm not 100% happy with the function name, but it basically just moves that block out to a static-cache run-once function, and then calls it from both _drupal_bootstrap_full() and statistics_exit(). Total performance cost: 1 function call, 2 if statistics is enabled.

chx’s picture

chx commented
Assigned: Unassigned » chx
StatusFileSize
new path.inc.patch17.5 KB

This function should live in bootstrap. And it'll bring in some other functions from common.inc . But somewhere we should stop the growing of bootstrap. It was 644 lines in 4.6 and now it inches towards 900 lines. We know it's a performance hit to tokenize all that. Therefore I moved all these to path.inc and introduced a new bootstrap stage. if you are not running stat module then it'll be a performance boost without a doubt.

chx’s picture

chx commented

And I moved page_set_cache to common -- if you are serving cached pages you do not want to set the cache. You only to want set the cache if you have already built it.

chx’s picture

chx commented

Even one more thing -- yes, I discussed this w/ moshe. The name path.inc was even suggested by him.

chx’s picture

chx commented
StatusFileSize
new path.inc_0.patch13.57 KB

My form.inc changes sneaked in.

dries’s picture

dries commented

I'll go with Crell's patch and leave path.inc for 4.8. Looks like it needs to be rerolled then.

chx’s picture

chx commented

But Crell's patch won't work. He adds the function common.inc and that's not included. And you'll need to move drupal_get_normal_path to bootstrap as well. That does not sound good. I will try to benchmark a cached situationw/ 4.6 and HEAD but I can't promise anything, when I tried benching it fluctuated so widely that it was unusable.

Jaza’s picture

Jaza commented
StatusFileSize
new path.inc_1.patch15.56 KB

Updated chx's patch to have full doxygen comments for each function in path.inc. Apart from that, it looks good. Only, now that Dries has refused path.inc for 4.7 (quite understandable), looks like I'll have to update Crell's last patch instead.

chx’s picture

chx commented

Not so fast. He promised a revisit :)

Jaza’s picture

Jaza commented

Here's Crell's patch, updated to work fully within the bootstrap system. This has been submitted instead of the path.inc patch. This patch is not ideal for performance, as it unnecessarily includes in bootstrap the 7 functions that have been moved (in chx's patch) to path.inc. But I'd rather see this get in core (and at least fix the problem), than see nothing get in for 4.7 (in which case statistics.module will be broken for cached sites).

This patch also implements chx's idea of moving page_set_cache() to common.inc.

Jaza’s picture

Jaza commented
StatusFileSize
new init_path_0.patch6.11 KB

And now for the patch.

dries’s picture

dries commented

Upon re-investigating Chx's patch, I think we should create a path.inc. Sorry for the extra work.

Does anyone happen to have performance numbers for this change?

chx’s picture

chx commented
StatusFileSize
new path.inc_2.patch9.72 KB

I can't bench, sorry, I am getting numbers like 908+-615 ms which is totally useless. Updated patch though -- drupal_init_path was called in _drupal_bootstrap_full instead at the DRUPAL_BOOTSTRAP_PATH stage.

chx’s picture

chx commented
StatusFileSize
new path.inc_3.patch14.91 KB

I left out path.inc , repatch.

dries’s picture

dries commented

This patch reduces the size of bootstrap.inc by 20%. The patched bootrap.inc is still 20% bigger than the one in Drupal 4.6. If you have the statistics module enabled, the bootstrap code is 40% bigger than the one in Drupal 4.6.

dries’s picture

dries commented

Did some benchmarks. The new code (patch.inc) seems to be 2-3% slower when statistics are enabled, and is equally fast when statistics are disabled. 2-3% is still within the error-marge but it was consistent behavior across multiple tests. Not what I expected ... Anyone else? To reproduce, access a node page (?q=node/x) because that is when the statistic counters are executed.

chx’s picture

chx commented
StatusFileSize
new path.inc_4.patch15.31 KB

I used a + where there is no doubt that it's needed for cached pages.

  • timer_start It's needed if we want to time a cached page.
  • timer_read I redid timer_stop to reuse this one.
  • timer_stop Also needed if we want to time a cached page.
  • conf_init +
  • drupal_get_filename +
  • variable_init The way cache_get is written, variable subsystem is needed
  • variable_get +
  • variable_set +
  • variable_del 4 lines, does not worth the inconsistency
  • cache_get +
  • cache_set Used only once by variable_init, 5 lines. I do not think we can elegantly kick it to common.inc
  • cache_clear_all + (variable_set dependency)
  • page_get_cache +
  • bootstrap_invoke_all +
  • drupal_load +
  • drupal_page_header +
  • bootstrap_hooks +
  • drupal_unpack + (session.inc !)
  • referer_uri watchdog calls it
  • check_plain + (db error)
  • request_uri +
  • watchdog We could move this one.
  • drupal_set_message +
  • drupal_get_messages +
  • drupal_is_denied +
  • drupal_bootstrap +
  • _drupal_bootstrap +
  • drupal_maintenance_theme This one is called by database* -- how big a problem it is if we mandate an include ./includes/theme.inc before calling this one?

    • New patch: changed timer_stop to be shorter.

    chx’s picture

    chx commented

    Dries, care to share how you tested? I tried ab localhost without any good result.

    moshe weitzman’s picture

    moshe weitzman commented
    Status: Needs review » Reviewed & tested by the community

    I tested this patch for anon and registered users which page cache was enabled. all works well. code looks good too.

    since it fixes the bug described here, lets promote to 'ready to commit'

    dries’s picture

    dries commented
    Status: Reviewed & tested by the community » Fixed

    I benchmarked again to double-check, and Crell's patch is slightly faster. Regardless, I committed chx's patch because it is conceptually cleaner and potentially beneficial in future. The slowdown is 2-3ms per requests, probably the extra overhead of opening a file.

    tomsys’s picture

    tomsys commented

    Hello,

    Using CVS from HEAD

    ..after this .patch .. now there is a warning on http://www.my_drupal_site.com/node/add page

    warning: array_key_exists(): The first argument should be either a string or an integer in <br />/home/www/my_drupal_site/htdocs/modules/node.module on line 1733.

    Just to let you know
    T.

    chx’s picture

    chx commented
    Status: Fixed » Reviewed & tested by the community
    StatusFileSize
    new arg.patch229 bytes

    How the....? Now, there is a return FALSE at the end of arg but for my life I can't figure out why I put it there. Once again, I suspect gremlin involvement.

    dries’s picture

    dries commented
    Status: Reviewed & tested by the community » Fixed

    Committed! Thanks.

    tomsys’s picture

    tomsys commented

    Thank You :)

    T.

    Anonymous’s picture

    (not verified) commented
    Status: Fixed » Closed (fixed)