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

subscribing.

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.

@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..

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.

To be honest, I don't entirely understand the description of "critical" on that page, but if "critical" means we should not release Drupal 7 until this is resolved, then this issue is critical as per #320331-126: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks and remaining discussion on that issue. Turning url inbound rewriting from a handler to an alter hook resulted in a site with statistics module enabled having 500% or worse performance degradation for serving cached pages, and according to webchick, this is unacceptable. This issue can be marked fixed if we roll that other one back. But I don't accept that that's the only path towards fixing this.

Better issue title I think, and one that got me thinking about an alternate way to solve the problem.

I don't think it's necessary for hook_boot() to have a fully normalized $_GET['q']. I think it's reasonable for us to say that the process of normalizing $_GET['q'] requires module involvement, and therefore doesn't happen until full bootstrap. I'm not too keen on making hook_url_inbound_alter() a bootstrap level hook (#6 - #8), unless some other compelling use-case forces us into that.

The issue with statistics module is more focused to hook_exit() during cached pages. This should have a normalized $_GET['q']. So, how about we add a "path" field to the "cache_page" table, and cache the normalized path along with the page content? Then either within drupal_serve_page_from_cache() or right after calling it from _drupal_bootstrap_page_cache(), we set $_GET['q'] to that cached value.

Thoughts? If it sounds good, anyone up for rolling such a patch?

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.

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.

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'd be on-board with either of the two options in #20. While I think #13/#20.1 is cool, there's something to be said for invoking the principle of "solve the issue in the simplest way possible". Therefore, #20.2 is probably the better way to go. It was suggested in #4, and I don't even remember why I didn't like the idea back then. I think that like gpk (#6), I was concerned about the consequences of extra bootstrap modules. But, on thinking about it more, there really shouldn't be very many modules that implement hook_url_inbound_alter(). So +1 to that approach. #13/#20.1 can wait for another use-case, perhaps not until D8 (though maybe in D7 contrib).

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?

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

Agreed. This is awesome. The patch leads to some bugs in statistics_exit() for cached pages. I'm working on adding tests for that. I have confidence that it will be easy to make those tests pass.

The last submitted patch, 692044-cached-page-metadata-31.patch, failed testing.

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.

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

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.

Benchmarks of anonymous user visiting the front page after a clean "standard profile" install (no content):

Statistics Off
without page cache: HEAD = 41.4ms, Patch = 41.3ms
with page cache: HEAD = 6.6ms, Patch = 6.6ms

Statistics On (access log and count content views enabled)
without page cache: HEAD = 43.0ms, Patch = 42.8ms
with page cache: HEAD = 32.8ms, Patch = 8.5ms

Each number is the "ab" (Apache Benchmark) average of 500 requests.

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.

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

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