drupal works hard to include as little code as possible before determining whether a cached page may be served to an anon user. this is called the 'bootstrap'.

however, current drupal has a logic error where it includes all modules right after printing out the cached page. this pointlessly increases server load. the error was to call module_invoke_all() when invoking the required bootstrap hooks (init and exit). module_invoke_all() is forbidden during a bootstrap because it includes all the modules, and thus defeats the whole bootstrap optimization.

the attached patch replaces two calls to module_invoke_all() with two calls to the newly created bootstrap_invoke_all().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

Looks like this patch might clash with the multi-site configuration patch. I'll apply your patch to the DRUPAL-4-5 branch, and then we'll want to merge it with the multi-site configuration patch for HEAD. Doable?

moshe weitzman’s picture

sounds good to me.

Anonymous’s picture

Does this fix the "statistics are not logged for cached pages" problem?

moshe weitzman’s picture

yes it fixes that problem. the only time we don't log is when we serve a '304 Not Modified'. I understand that this response usually is served to RSS aggregators and crawlers.

jhriggs’s picture

Priority: Normal » Critical
FileSize
607 bytes

system.module still needs bootstrap_hooks() which was removed by this patch. The attached patch adds it again. Setting to critical, because admin/modules is currently broken in 4.5.

moshe weitzman’s picture

FileSize
11.35 KB

Here is a new patch for HEAD. In addition to the first patch, it moves some new required functions from common.inc to bootstrap.inc. we also fix some calls within statistics_exit() so that logging while serving a cached page.

The bootstrap_hooks.patch patch uploaded by jhriggs should be applied to 4.5 (thanks). No need to apply that one to HEAD since its changes are in the drboot2.patch here.

jhriggs’s picture

Note that this patch severely broke the 4.5 branch. If I drop to the previous revision of bootstrap.inc, all is fine. When I update to this version, though, almost all of the site is broken. Menu items are missing, nodes can't be viewed, and even the home page (/node) results in a 404. I am not using page cache, and I am logged in. This is not good...

moshe weitzman’s picture

i fear that jhriggs is correct. The attached patch for 4.5 fixes things. It supercedes jhrigg's patch in this issue, as it includes the boostrap_hooks() function.

only 1 line in module_list() was incorrect, and the error was subtle; unsetting a static variable within a function doesn't totally eliminate the contents of the variable.

the other bits in this patch just move functions into bootstrap.inc so that statistics_exit() works for cached pages.

moshe weitzman’s picture

FileSize
4.74 KB

and now, with a patch. sigh. sorry for the noise.

TDobes’s picture

chx’s picture

FileSize
1.25 KB

Well, this breaks i18n among others so... another patch, this moves drupal_get_normal_path from common.inc to bootstrap.inc.

chx’s picture

Ops, forgot to set to patch.

moshe weitzman’s picture

Priority: Critical » Normal

about the most recent patch proposed by chx - i am now thinking that none of the path/alias functions belong in bootstrap.inc. the bootstrap should only have the code needed to determine whether or not to serve a cached page. i recently moved several of .these functions into bootstrap in order to get statistics_exit() working again But I now see that statistics_exit() can be simplified so it doesn't need any path/alias calls. i will likely submit a new patch (for HEAD only) which simplifies statistics_exit() and moves those functions back to common.inc

about i18n - I don't think the init hook is the right place to do your magic. perhaps hook_menu(!may_cache) is better. or maybe this code is executed immediately upon including the module. i'm not too sure about that though - i18n requires some studying to really grok.

moshe weitzman’s picture

FileSize
3.91 KB

here is the patch which simplifies statistics_exit() by not aliasing a drupal path before inserting ito the access log. theres no benefit that i can see by doing this.

this patch also moves 2 path functions back to common.inc. We really don't want to load the aliases from the DB during a bootstrap, since they can't afect the page anyway.

please consider for HEAD only.

Dries’s picture

This would break the 'track node' feature. Won't fix.

moshe weitzman’s picture

FileSize
1.9 KB

this patch lets you actually do something useful with the init hook. a recent patch to 4.5 and HEAD made this patch run too early in the request (for non-cached pages).

please review and apply to 4.5 and HEAD

gordon’s picture

+1

However does this re-execute the bootstrap modules init processes for a second time?

moshe weitzman’s picture

no it doesn't ever execute init hook twice on a request

gordon’s picture

I see it now, It only executes that bootstrap_invoke_all('init'); if it has a cached page. I really hope to see this functionality back in.

What would be nice though is to pass an additional parameter to indicate if it is being called from cached page or not. This would mean that with some modules they may want to not do anything in the init hook if the page is cached.

moshe weitzman’s picture

FileSize
1.28 KB

same patch for 4.5. the other one won't apply because of insignificant differences between the 2 versions.

moshe weitzman’s picture

more than likely, you don't shouldn't use the init hook if you not going to take action upon cached pages. instead, you should use hook_menu(!$may_cache).

lets wait on that param for now, since we are about to launch 4.5.1. You can easily learn if we are in a bootstrap by checking for the presence of any function defined in common.inc. If not found, we are in bootstrap.

Dries’s picture

Committed to both HEAD and DRUPAL-4-5.

Anonymous’s picture