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().
Comment | File | Size | Author |
---|---|---|---|
#20 | drinit45.patch | 1.28 KB | moshe weitzman |
#16 | drinit.patch | 1.9 KB | moshe weitzman |
#14 | drstats.patch | 3.91 KB | moshe weitzman |
#11 | get_normal.txt | 1.25 KB | chx |
#9 | drboot2.45.patch | 4.74 KB | moshe weitzman |
Comments
Comment #1
Dries CreditAttribution: Dries commentedLooks 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?
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedsounds good to me.
Comment #3
(not verified) CreditAttribution: commentedDoes this fix the "statistics are not logged for cached pages" problem?
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedyes 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.
Comment #5
jhriggs CreditAttribution: jhriggs commentedsystem.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.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedHere 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.
Comment #7
jhriggs CreditAttribution: jhriggs commentedNote 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...
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedi 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.
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedand now, with a patch. sigh. sorry for the noise.
Comment #10
TDobes CreditAttribution: TDobes commentedComment #11
chx CreditAttribution: chx commentedWell, this breaks i18n among others so... another patch, this moves drupal_get_normal_path from common.inc to bootstrap.inc.
Comment #12
chx CreditAttribution: chx commentedOps, forgot to set to patch.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedabout 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.
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedhere 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.
Comment #15
Dries CreditAttribution: Dries commentedThis would break the 'track node' feature. Won't fix.
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedthis 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
Comment #17
gordon CreditAttribution: gordon commented+1
However does this re-execute the bootstrap modules init processes for a second time?
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedno it doesn't ever execute init hook twice on a request
Comment #19
gordon CreditAttribution: gordon commentedI 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.
Comment #20
moshe weitzman CreditAttribution: moshe weitzman commentedsame patch for 4.5. the other one won't apply because of insignificant differences between the 2 versions.
Comment #21
moshe weitzman CreditAttribution: moshe weitzman commentedmore 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.
Comment #22
Dries CreditAttribution: Dries commentedCommitted to both HEAD and DRUPAL-4-5.
Comment #23
(not verified) CreditAttribution: commented