A site with statistics module enabled is much slower in serving cached pages in D7 than in D6

subscribing.

What does hook_url_inbound_alter() need to make it run successfully, apart from drupal_alter() being available?

Mostly it was drupal_alter() and the assumption that since this is the opposite of hook_url_outbound_alter(), it should run the same bootstrap. We could make a copy of it and put it into bootstrap.inc (bootstrap_alter). Also in the test function we had used user_load_by_name() but I guess that could be converted to a simpler query: SELECT uid FROM {users} WHERE name = :name instead of relying on user.module being loaded. I guess if a module implementation of hook_url_inbound_alter() needed a full bootstrap it could do so itself and we can add BOOTSTRAP_PATH back in.

Other changes required to revert removing the bootstrap:
- Add 'url_inbound_alter' to http://api.drupal.org/api/function/bootstrap_hooks/7
- Add to the hook documentation that a full bootstrap is not available.

I'd also like to see more benchmarks with the current code and statistics module enabled. Ones I did indicated little difference. Recent ones indicate huge difference.

@5: my hunch in #320331-121: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks was that the page cache probably wasn't turned on for your tests.

Re 4:
> - Add 'url_inbound_alter' to http://api.drupal.org/api/function/bootstrap_hooks/7
Given that the 'url_inbound_alter' hook is only required during a cached page response if a module (e.g. statistics) has upped the bootstrap phase to DRUPAL_BOOTSTRAP_PATH, can we distinguish between boot/exit/watchdog and url_inbound_alter? Otherwise we could be loading modules unnecessarily. The bootstrap column in {system} is already a full-blown int so there's certainly room to store the info..

It would needed to be added to bootstrap_hooks since it's invoked prior to all modules loaded. Otherwise modules that do implement url_inbound_alter() but not the other bootstrap hooks would never be run.

I was thinking that in many cases this could cause such modules to be loaded unnecessarily, however possibly it would require too big an API change at this stage to finesse the loading of these modules. Same could also be said for modules implementing hook_watchdog - in 99% of cached page requests they will not be needed.

Not critical. Please read and understand Priority levels of Issues, thanks.

Yes, we either fix this, or we roll back hook_url_*_alter(). Note that the hooks also cause potentially infinite recursion if you call url() inside taxonomy_term_load() per #712076: Infinite loop possibility and performance problems in forum_url_outbound_alter().

We already have something for ESI tie in in drupal_render_cache_set(), couldn't the same pattern be used here? http://drupal.org/node/651902

So far we have two critical bugs open due to hook_url_*_alter() - I think it should just be rolled back to be honest, consequences were not thought through, all the fixes create pain.

I would rather drop statistics module from core than take away url_alter hook for all developers. logging all page views in mysql is a pretty bad idea, and we should just acknowledge that. it can live on in contrib for folks who can't seem to find google analytics or apache logs or any other alternative.

Quoting Moshe in #16: logging all page views in mysql is a pretty bad idea, and we should just acknowledge that. it can live on in contrib for folks who can't seem to find google analytics or apache logs or any other alternative.

I completely agree with this statement, having wondered repeatedly how Drupal's Statistics module fits into the general web access statistics world.
While I am only superficially familiar with Drupal's API and other internals, I am keenly aware of performance issues caused by disk access patterns. The less the better, and dropping Statistics from core would seem to fit the bill.

To be honest, I think this is getting way to complex. What about rolling back hook_url_*_alter() ?

I agree with Moshe and others that we should keep hook_url_inbound/outbound_alter(), mainly from a strategic perspective.

Hopping through the API and bootstrap process, I'd see two potential solutions:

1) Add a {cache_page}.data column to store additional information (here, $_GET['q'], drupal_get_title()) with each entry. If done properly, that would most likely be interesting and helpful for other use-cases. Potential data handling (getter/setter) functions: hook_boot() and hook_exit(). Alternatively, or additionally, cache_get() and cache_set() could and probably should invoke a hook_cache_get|set_BIN().

2) Revert the removal of DRUPAL_BOOTSTRAP_PATH and turn hook_url_inbound_alter() into a bootstrap hook. Not sure why that was removed in the first place. There may be other use-cases other than statistics that require knowledge of the current system path prior to DRUPAL_BOOSTRAP_FULL.

@Dries - in #16 I suggested that the hook is more important to the framework than statistics module. Are you in disagreement based on your suggestion in #18?

At the risk of sticking my nose in ...

> the hook is more important to the framework than statistics module
part of the tension here is I think that large/busy sites have different requirements and performance issues from small/quieter sites. At the moment as far as I can make out Drupal is trying to work for both, which creates certain challenges.

> live on in contrib for folks who can't seem to find google analytics or apache logs or any other alternative.
I use both of those, but they have different uses than statistics module. And I imagine that alternatives (i.e. contrib modules yet to be written..??) would have the same problem with the bootstrap. The point about statistics module is that you just turn it on if you need it. However I agree it might ultimately have a better time in contrib - it's not had much love of late.

Another workaround might be to store the requested URL in {accesslog} which could then be used to get the normalised path for a cached response. However this wouldn't provide the additional benefits for other potential use cases (#20 option 1).

I like sun's option #2. Add back DRUPAL_BOOTSTRAP_PATH and make hook_url_inbound_alter() a bootstrap hook. Then we don't need to make any schema changes just to accommodate statistics.module

We absolutely need to fix #699440: Add bundle support to entity_uri callback to remove performance overhead of forum_url_outbound_alter() (regression from D6) whether we were to rollback the alter hooks or not, so that in itself isn't an argument for a rollback.

http://api.drupal.org/api/function/locale_url_outbound_alter/7 is a proper use of the hook IMO - or at least afaik the code is extremely similar to the Drupal 6 code (which I can't locate at the moment).

So if these two are fixed, I'm in favour of leaving the hook in - the issue less the hook as such, more that we converted too many special cases to it for which it's not really suited.

just roll back the !@#$%^&* hook. there was a good reason it never was a hook and it should still not be a hook. it's pointless to be a hook and it causes a major performance hit. what's so hard?

Why not simply removing this old crap that is {cache}.headers (that is used *only* for page cache), and simply implement the page cache in a more reasonable manner.

This allows us to pass some additional meta-data in the cached page, solving the issue at hand.

This thing is like weed, it goes everywhere. Removed more 'headers'.

Very nice work. Thats how you slay a critical!

Waiting for bot and perhaps more feedback before rtbc.

Looks good to me, thanks for the additional tests and the fixes.

Looks great here too. Will need to document the API/schema change but it's a small one, and the schema change won't cause actual breakage anyway.

+++ includes/bootstrap.inc	18 May 2010 01:10:40 -0000
@@ -1134,7 +1138,7 @@ function drupal_serve_page_from_cache(st
-      // $cache->data is already gzip'ed, so make sure zlib.output_compression
+      // $cache->data['body'] is already gzip'ed, so make sure zlib.output_compression
       // does not compress it once more.

Exceeds 80 chars.

+++ includes/bootstrap.inc	18 May 2010 01:10:40 -0000
@@ -1142,11 +1146,12 @@ function drupal_serve_page_from_cache(st
+  // Print the page.
+  print $cache->data['body'];

+++ includes/common.inc	18 May 2010 01:10:41 -0000
@@ -4518,28 +4518,32 @@ function drupal_page_set_cache() {
+        'body' => ob_get_clean(),

Wouldn't 'html', 'markup', or 'content' be a more appropriate array key instead of 'body'?

+++ modules/statistics/statistics.module	18 May 2010 01:10:45 -0000
@@ -51,9 +51,23 @@ function statistics_help($path, $arg) {
+  // If this is running for a cached page using the page_cache_without_database
+  // configuration, then we may not have bootstrapped the system variables yet.
+  // Websites running non-database page caches will almost certainly not have
+  // the statistics module enabled, but just in case, bootstrap the system
+  // variables. This wipes out most if not all of the performance gain from a
+  // non-database page cache, but if the statistics module is enabled, then it
+  // is also likely to have the 'statistics_enable_access_log' variable set, in
+  // which case, we'll be bootstrapping to the session phase anyway. In summary,
+  // we do not invoke an expensive full bootstrap, but for any site that does
+  // not want to bootstrap beyond DRUPAL_BOOTSTRAP_PAGE_CACHE for cached pages,

Awesome explanation + information. However, can we shorten all sentences, while keeping the information?

"When serving cached pages with the 'page_cache_without_database' configuration, system variables need to be loaded. This is a major performance decrease for non-database page caches, but with Statistics module, it is likely to also have 'statistics_enable_access_log' enabled, in which case we need to bootstrap to the session phase anyway."

+++ modules/statistics/statistics.module	18 May 2010 01:10:45 -0000
@@ -51,9 +51,23 @@ function statistics_help($path, $arg) {
+  // do not enable the statistics module. ¶

1) The "Statistics" module (proper name).

2) Trailing white-space.

+++ modules/statistics/statistics.module	18 May 2010 01:10:45 -0000
@@ -51,9 +51,23 @@ function statistics_help($path, $arg) {
+    // We need this to use the arg() function.
+    require_once DRUPAL_ROOT . '/' . variable_get('path_inc', 'includes/path.inc');

I wonder whether we shouldn't move arg() into bootstrap.inc or something, too? It's a few lines only, and we already have a group of path-related functions in bootstrap.inc it could be moved into. (IIRC, not having arg() available early was a WTF for me a couple of times already in the past)

Alternatively, we could simply inline $arg = explode('/', $_GET['q']); here.

I'd prefer to do the former though (move arg()).

+++ modules/update/update.install	18 May 2010 01:10:45 -0000
@@ -167,3 +167,10 @@ function update_update_7000() {
\ No newline at end of file

Missing newline.

Powered by Dreditor.

New version, integrating all of @sun's comments, except one: I stick with 'body', which is the HTTP terminology (headers / body).

+++ includes/bootstrap.inc
@@ -1134,19 +1134,24 @@ function drupal_serve_page_from_cache(stdClass $cache) {
+  // Restore the metadata that was cached with the page.
+  $_GET['q'] = $cache->data['path'];
+  drupal_set_title($cache->data['title'], PASS_THROUGH);

I wonder whether we shouldn't move this into drupal_page_get_cache() or _drupal_bootstrap_page_cache(), so as to make both available to hook_boot() and hook_exit()?

Powered by Dreditor.

I need to give this a closer review to verify the smaller details but from a conceptual level this looks pretty clever.

It would be interesting to see some performance results.

I'm OK with 'body' -- that seems to be official term according to the HTTP specification. See http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.

Thanks, looks good.

AFAICS, there shouldn't be any measurable positive or negative performance impact from this patch.

If there wasn't, then this issue wouldn't be critical - ideally someone should benchmark cached pages with statistics module on and off. Agreed that without statistics on this ought to be no change though.

Reviewed and looks very solid. Committed to CVS HEAD. Thanks.

Some upgrade issues:

In update_prepare_d7_bootstrap() we are creating table cache_bootstrap with headers field. We then drop it in an update hook. Better to just create it correctly in the first place.

Also, cache_block needs to have its header field dropped.

Good catch!

Committed to CVS HEAD. Moving to HEAD to HEAD project as this will break HEAD to HEAD upgrades.

Let's just open a new issue: #803574: update for A site with statistics module enabled is much slower in serving cached pages in D7 than in D6

Does anyone happen to remember why statistics module needs to bootstrap to the session phase in statistics_exit()? If the request didn't already bootstrap to session phase - i.e. it's being served from cache - then this would generate a "bogus" session ID that can be logged by statistics module. But this doesn't seem particularly useful.

In recent versions of PHP, initializing session after serving the page throws a PHP warning, which I was researching in #3009351: "session_id(): Cannot change session id". I don't use statistics module so not clear what it's trying to achieve.