[drupal-devel] [feature] Alternative caching for high traffic websites
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; Jeremy@kerneltrap.org
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp;
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds?
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Anonymous Status: patch Looks very good. Please provide patches in one file, though. Gerhard Anonymous Previous comments: ------------------------------------------------------------------------ March 23, 2005 - 04:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 23, 2005 - 04:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 23, 2005 - 04:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 23, 2005 - 04:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: stefan nagtegaal Status: patch First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... stefan nagtegaal Previous comments: ------------------------------------------------------------------------ March 23, 2005 - 05:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 23, 2005 - 05:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 23, 2005 - 05:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 23, 2005 - 05:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 08:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Dries Status: patch Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. Dries Previous comments: ------------------------------------------------------------------------ March 23, 2005 - 06:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 23, 2005 - 06:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 23, 2005 - 06:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 23, 2005 - 06:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 09:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 10:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)...
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 22, 2005 - 23:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 02:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 03:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 05:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: moshe weitzman Status: patch changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. moshe weitzman Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 22, 2005 - 23:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 02:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 03:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 05:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 07:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 22, 2005 - 23:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 02:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 03:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 05:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 07:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 09:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 22, 2005 - 23:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 02:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 03:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 05:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 07:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 09:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 07:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Dries Status: patch The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. Dries Previous comments: ------------------------------------------------------------------------ March 23, 2005 - 06:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 23, 2005 - 06:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 23, 2005 - 06:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 23, 2005 - 06:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 09:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 10:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 12:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 14:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 16:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 14:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 15:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: daryl@spreadfirefox.com Status: patch Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. daryl@spreadfirefox.com Previous comments: ------------------------------------------------------------------------ March 23, 2005 - 04:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 23, 2005 - 04:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 23, 2005 - 04:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 23, 2005 - 04:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 07:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 08:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 10:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 12:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 14:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 12:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 13:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 16:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 22, 2005 - 23:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 02:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 03:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 05:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 07:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 09:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 07:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 08:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 11:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 13:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry. Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 22, 2005 - 23:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 02:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 03:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 05:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 07:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 09:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 07:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 08:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 11:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 13:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 08:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: moshe weitzman Status: patch i think you can simply get rid of the timer variable and assume it to be true. it is harmless - certainly more harmless than its variable lookup! as for the cache enable/disable, i agree that moving it to settings.php is a good idea. moshe weitzman Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 22, 2005 - 23:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 02:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 03:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 05:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 07:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 09:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 07:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 08:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 11:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 13:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 08:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. ------------------------------------------------------------------------ April 2, 2005 - 09:10 : Jeremy@kerneltrap.org daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch Unfortunately my loose cache implementation currently relies on the variable table, and I don't immediately see how to avoid this. All the configuration stuff can be either removed or moved into settings.php as appropriate, but the 'cache_flush' variable is a dynamic timestamp that needs to be shared with all sessions. Currently this is done via the variable table. One idea is to move 'cache_flush' into the cache table... It would still cost a db_query, but certainly with less overhead than processing the entire variable table. Or is that too much of a kludge? Thoughts? Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 22, 2005 - 23:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 02:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 03:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 05:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 07:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 09:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 07:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 08:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 11:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 13:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 08:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. ------------------------------------------------------------------------ April 2, 2005 - 09:10 : Jeremy@kerneltrap.org daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry. ------------------------------------------------------------------------ April 2, 2005 - 14:23 : moshe weitzman i think you can simply get rid of the timer variable and assume it to be true. it is harmless - certainly more harmless than its variable lookup! as for the cache enable/disable, i agree that moving it to settings.php is a good idea.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch Attachment: http://drupal.org/files/issues/cache_0.patch (10.22 KB) It's probably easier to see what I'm talking about with a patch. So, here's an updated patch that removes all variable_get's from bootstrap.inc. It works the same as my earlier loose caching patch, but the cache configuration has been moved from the adminsitrative interface to settings.php. All the variable logic could probabily be moved out of bootstrap.inc back to common.inc once this patch was applied. Specifically, this patch potentially improves upon my earlier patch by removing the overhead of initializing the variable array. I'd love to see some benchmark results to determine if this is worth it. Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 22, 2005 - 23:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 02:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 03:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 05:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 07:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 09:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 07:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 08:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 11:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 13:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 08:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. ------------------------------------------------------------------------ April 2, 2005 - 09:10 : Jeremy@kerneltrap.org daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry. ------------------------------------------------------------------------ April 2, 2005 - 14:23 : moshe weitzman i think you can simply get rid of the timer variable and assume it to be true. it is harmless - certainly more harmless than its variable lookup! as for the cache enable/disable, i agree that moving it to settings.php is a good idea. ------------------------------------------------------------------------ April 2, 2005 - 23:20 : Jeremy@kerneltrap.org Unfortunately my loose cache implementation currently relies on the variable table, and I don't immediately see how to avoid this. All the configuration stuff can be either removed or moved into settings.php as appropriate, but the 'cache_flush' variable is a dynamic timestamp that needs to be shared with all sessions. Currently this is done via the variable table. One idea is to move 'cache_flush' into the cache table... It would still cost a db_query, but certainly with less overhead than processing the entire variable table. Or is that too much of a kludge? Thoughts?
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Dries Status: patch Thanks for investigating this further, Jemery. If variable_get() is already called, I'd prefer to stick with it unless we pay a significant performance penalty for it. Dries Previous comments: ------------------------------------------------------------------------ March 23, 2005 - 06:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 23, 2005 - 06:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 23, 2005 - 06:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 23, 2005 - 06:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 09:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 10:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 12:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 14:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 16:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 14:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 15:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 18:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 20:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 15:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. ------------------------------------------------------------------------ April 2, 2005 - 16:10 : Jeremy@kerneltrap.org daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry. ------------------------------------------------------------------------ April 2, 2005 - 21:23 : moshe weitzman i think you can simply get rid of the timer variable and assume it to be true. it is harmless - certainly more harmless than its variable lookup! as for the cache enable/disable, i agree that moving it to settings.php is a good idea. ------------------------------------------------------------------------ April 3, 2005 - 06:20 : Jeremy@kerneltrap.org Unfortunately my loose cache implementation currently relies on the variable table, and I don't immediately see how to avoid this. All the configuration stuff can be either removed or moved into settings.php as appropriate, but the 'cache_flush' variable is a dynamic timestamp that needs to be shared with all sessions. Currently this is done via the variable table. One idea is to move 'cache_flush' into the cache table... It would still cost a db_query, but certainly with less overhead than processing the entire variable table. Or is that too much of a kludge? Thoughts? ------------------------------------------------------------------------ April 3, 2005 - 07:43 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache_0.patch (10.22 KB) It's probably easier to see what I'm talking about with a patch. So, here's an updated patch that removes all variable_get's from bootstrap.inc. It works the same as my earlier loose caching patch, but the cache configuration has been moved from the adminsitrative interface to settings.php. All the variable logic could probabily be moved out of bootstrap.inc back to common.inc once this patch was applied. Specifically, this patch potentially improves upon my earlier patch by removing the overhead of initializing the variable array. I'd love to see some benchmark results to determine if this is worth it.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Bèr Kessels Status: patch Just a random thought I had lying around for a while: Combine cache and throttle. That would mean we use the cache we do now, but offer per-role cahce refresh, based on the throttle level. Something like: At Trottle limit [6 |v] do not refresh for [X] Anonymous [X] Registered [ ] Advertiser [ ] Administrator means that above level six, anonymous and registered cache will not be refreshed. This can be expanded by a cron wich will refres all cahces anyway. meaning that in the above exmaple, witha cron every ten minutes, anonymous and registered will see only refreshements every ten minutes. Bèr Kessels Previous comments: ------------------------------------------------------------------------ March 23, 2005 - 05:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 23, 2005 - 05:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 23, 2005 - 05:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 23, 2005 - 05:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 08:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 09:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 11:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 13:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 15:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 13:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 14:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 17:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 19:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 14:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. ------------------------------------------------------------------------ April 2, 2005 - 15:10 : Jeremy@kerneltrap.org daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry. ------------------------------------------------------------------------ April 2, 2005 - 20:23 : moshe weitzman i think you can simply get rid of the timer variable and assume it to be true. it is harmless - certainly more harmless than its variable lookup! as for the cache enable/disable, i agree that moving it to settings.php is a good idea. ------------------------------------------------------------------------ April 3, 2005 - 05:20 : Jeremy@kerneltrap.org Unfortunately my loose cache implementation currently relies on the variable table, and I don't immediately see how to avoid this. All the configuration stuff can be either removed or moved into settings.php as appropriate, but the 'cache_flush' variable is a dynamic timestamp that needs to be shared with all sessions. Currently this is done via the variable table. One idea is to move 'cache_flush' into the cache table... It would still cost a db_query, but certainly with less overhead than processing the entire variable table. Or is that too much of a kludge? Thoughts? ------------------------------------------------------------------------ April 3, 2005 - 06:43 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache_0.patch (10.22 KB) It's probably easier to see what I'm talking about with a patch. So, here's an updated patch that removes all variable_get's from bootstrap.inc. It works the same as my earlier loose caching patch, but the cache configuration has been moved from the adminsitrative interface to settings.php. All the variable logic could probabily be moved out of bootstrap.inc back to common.inc once this patch was applied. Specifically, this patch potentially improves upon my earlier patch by removing the overhead of initializing the variable array. I'd love to see some benchmark results to determine if this is worth it. ------------------------------------------------------------------------ April 3, 2005 - 09:51 : Dries Thanks for investigating this further, Jemery. If variable_get() is already called, I'd prefer to stick with it unless we pay a significant performance penalty for it.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Dries Status: patch Ber: personally, I would not involve user roles. At least not until we have a better understanding of how the loose chacing performs and behaves. Maybe in future, I'd add a _simple_ checkbox along these lines: "Always use loose caching when the throttle mechanism is active". Daryl: could you try Jeremy's patch on SFX? Ultimately a mechanism like this ought to be tested on high-traffic websites. Dries Previous comments: ------------------------------------------------------------------------ March 23, 2005 - 06:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 23, 2005 - 06:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 23, 2005 - 06:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 23, 2005 - 06:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 09:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 10:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 12:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 14:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 16:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 14:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 15:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 18:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 20:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 15:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. ------------------------------------------------------------------------ April 2, 2005 - 16:10 : Jeremy@kerneltrap.org daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry. ------------------------------------------------------------------------ April 2, 2005 - 21:23 : moshe weitzman i think you can simply get rid of the timer variable and assume it to be true. it is harmless - certainly more harmless than its variable lookup! as for the cache enable/disable, i agree that moving it to settings.php is a good idea. ------------------------------------------------------------------------ April 3, 2005 - 06:20 : Jeremy@kerneltrap.org Unfortunately my loose cache implementation currently relies on the variable table, and I don't immediately see how to avoid this. All the configuration stuff can be either removed or moved into settings.php as appropriate, but the 'cache_flush' variable is a dynamic timestamp that needs to be shared with all sessions. Currently this is done via the variable table. One idea is to move 'cache_flush' into the cache table... It would still cost a db_query, but certainly with less overhead than processing the entire variable table. Or is that too much of a kludge? Thoughts? ------------------------------------------------------------------------ April 3, 2005 - 07:43 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache_0.patch (10.22 KB) It's probably easier to see what I'm talking about with a patch. So, here's an updated patch that removes all variable_get's from bootstrap.inc. It works the same as my earlier loose caching patch, but the cache configuration has been moved from the adminsitrative interface to settings.php. All the variable logic could probabily be moved out of bootstrap.inc back to common.inc once this patch was applied. Specifically, this patch potentially improves upon my earlier patch by removing the overhead of initializing the variable array. I'd love to see some benchmark results to determine if this is worth it. ------------------------------------------------------------------------ April 3, 2005 - 10:51 : Dries Thanks for investigating this further, Jemery. If variable_get() is already called, I'd prefer to stick with it unless we pay a significant performance penalty for it. ------------------------------------------------------------------------ April 3, 2005 - 12:34 : Bèr Kessels Just a random thought I had lying around for a while: Combine cache and throttle. That would mean we use the cache we do now, but offer per-role cahce refresh, based on the throttle level. Something like: At Trottle limit [6 |v] do not refresh for [X] Anonymous [X] Registered [ ] Advertiser [ ] Administrator means that above level six, anonymous and registered cache will not be refreshed. This can be expanded by a cron wich will refres all cahces anyway. meaning that in the above exmaple, witha cron every ten minutes, anonymous and registered will see only refreshements every ten minutes.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: daryl@spreadfirefox.com Status: patch Dries, we'll be upgrading to the latest and greatest CivicSpace once we get a few modules ported over. I'll try to remember to implement the patch once we're done with that. daryl@spreadfirefox.com Previous comments: ------------------------------------------------------------------------ March 23, 2005 - 04:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 23, 2005 - 04:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 23, 2005 - 04:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 23, 2005 - 04:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 07:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 08:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 10:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 12:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 14:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 12:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 13:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 16:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 18:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 13:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. ------------------------------------------------------------------------ April 2, 2005 - 14:10 : Jeremy@kerneltrap.org daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry. ------------------------------------------------------------------------ April 2, 2005 - 19:23 : moshe weitzman i think you can simply get rid of the timer variable and assume it to be true. it is harmless - certainly more harmless than its variable lookup! as for the cache enable/disable, i agree that moving it to settings.php is a good idea. ------------------------------------------------------------------------ April 3, 2005 - 04:20 : Jeremy@kerneltrap.org Unfortunately my loose cache implementation currently relies on the variable table, and I don't immediately see how to avoid this. All the configuration stuff can be either removed or moved into settings.php as appropriate, but the 'cache_flush' variable is a dynamic timestamp that needs to be shared with all sessions. Currently this is done via the variable table. One idea is to move 'cache_flush' into the cache table... It would still cost a db_query, but certainly with less overhead than processing the entire variable table. Or is that too much of a kludge? Thoughts? ------------------------------------------------------------------------ April 3, 2005 - 05:43 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache_0.patch (10.22 KB) It's probably easier to see what I'm talking about with a patch. So, here's an updated patch that removes all variable_get's from bootstrap.inc. It works the same as my earlier loose caching patch, but the cache configuration has been moved from the adminsitrative interface to settings.php. All the variable logic could probabily be moved out of bootstrap.inc back to common.inc once this patch was applied. Specifically, this patch potentially improves upon my earlier patch by removing the overhead of initializing the variable array. I'd love to see some benchmark results to determine if this is worth it. ------------------------------------------------------------------------ April 3, 2005 - 08:51 : Dries Thanks for investigating this further, Jemery. If variable_get() is already called, I'd prefer to stick with it unless we pay a significant performance penalty for it. ------------------------------------------------------------------------ April 3, 2005 - 10:34 : Bèr Kessels Just a random thought I had lying around for a while: Combine cache and throttle. That would mean we use the cache we do now, but offer per-role cahce refresh, based on the throttle level. Something like: At Trottle limit [6 |v] do not refresh for [X] Anonymous [X] Registered [ ] Advertiser [ ] Administrator means that above level six, anonymous and registered cache will not be refreshed. This can be expanded by a cron wich will refres all cahces anyway. meaning that in the above exmaple, witha cron every ten minutes, anonymous and registered will see only refreshements every ten minutes. ------------------------------------------------------------------------ April 3, 2005 - 10:54 : Dries Ber: personally, I would not involve user roles. At least not until we have a better understanding of how the loose chacing performs and behaves. Maybe in future, I'd add a _simple_ checkbox along these lines: "Always use loose caching when the throttle mechanism is active". Daryl: could you try Jeremy's patch on SFX? Ultimately a mechanism like this ought to be tested on high-traffic websites.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Dries Status: patch Patch no longer applies. Please keep the variable_get()'s in for now. Dries Previous comments: ------------------------------------------------------------------------ March 23, 2005 - 06:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 23, 2005 - 06:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 23, 2005 - 06:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 23, 2005 - 06:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 09:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 10:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 12:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 14:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 16:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 14:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 15:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 18:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 20:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 15:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. ------------------------------------------------------------------------ April 2, 2005 - 16:10 : Jeremy@kerneltrap.org daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry. ------------------------------------------------------------------------ April 2, 2005 - 21:23 : moshe weitzman i think you can simply get rid of the timer variable and assume it to be true. it is harmless - certainly more harmless than its variable lookup! as for the cache enable/disable, i agree that moving it to settings.php is a good idea. ------------------------------------------------------------------------ April 3, 2005 - 06:20 : Jeremy@kerneltrap.org Unfortunately my loose cache implementation currently relies on the variable table, and I don't immediately see how to avoid this. All the configuration stuff can be either removed or moved into settings.php as appropriate, but the 'cache_flush' variable is a dynamic timestamp that needs to be shared with all sessions. Currently this is done via the variable table. One idea is to move 'cache_flush' into the cache table... It would still cost a db_query, but certainly with less overhead than processing the entire variable table. Or is that too much of a kludge? Thoughts? ------------------------------------------------------------------------ April 3, 2005 - 07:43 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache_0.patch (10.22 KB) It's probably easier to see what I'm talking about with a patch. So, here's an updated patch that removes all variable_get's from bootstrap.inc. It works the same as my earlier loose caching patch, but the cache configuration has been moved from the adminsitrative interface to settings.php. All the variable logic could probabily be moved out of bootstrap.inc back to common.inc once this patch was applied. Specifically, this patch potentially improves upon my earlier patch by removing the overhead of initializing the variable array. I'd love to see some benchmark results to determine if this is worth it. ------------------------------------------------------------------------ April 3, 2005 - 10:51 : Dries Thanks for investigating this further, Jemery. If variable_get() is already called, I'd prefer to stick with it unless we pay a significant performance penalty for it. ------------------------------------------------------------------------ April 3, 2005 - 12:34 : Bèr Kessels Just a random thought I had lying around for a while: Combine cache and throttle. That would mean we use the cache we do now, but offer per-role cahce refresh, based on the throttle level. Something like: At Trottle limit [6 |v] do not refresh for [X] Anonymous [X] Registered [ ] Advertiser [ ] Administrator means that above level six, anonymous and registered cache will not be refreshed. This can be expanded by a cron wich will refres all cahces anyway. meaning that in the above exmaple, witha cron every ten minutes, anonymous and registered will see only refreshements every ten minutes. ------------------------------------------------------------------------ April 3, 2005 - 12:54 : Dries Ber: personally, I would not involve user roles. At least not until we have a better understanding of how the loose chacing performs and behaves. Maybe in future, I'd add a _simple_ checkbox along these lines: "Always use loose caching when the throttle mechanism is active". Daryl: could you try Jeremy's patch on SFX? Ultimately a mechanism like this ought to be tested on high-traffic websites. ------------------------------------------------------------------------ April 5, 2005 - 03:53 : daryl@spreadfirefox.com Dries, we'll be upgrading to the latest and greatest CivicSpace once we get a few modules ported over. I'll try to remember to implement the patch once we're done with that.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch Attachment: http://drupal.org/files/issues/cache_1.patch (9.14 KB) Resync patch to CVS HEAD. Original design, uses variable_get in cached page generation. Updated comments regarding cache field in sessions table. A couple notes: - please verify that the PostgreSQL format is correct - a future optimization, perhaps the cache field from the sessions table could be retrieved from the current session rather than as an extra db_query Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 22, 2005 - 23:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 02:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 03:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 05:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 07:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 09:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 07:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 08:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 11:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 13:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 08:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. ------------------------------------------------------------------------ April 2, 2005 - 09:10 : Jeremy@kerneltrap.org daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry. ------------------------------------------------------------------------ April 2, 2005 - 14:23 : moshe weitzman i think you can simply get rid of the timer variable and assume it to be true. it is harmless - certainly more harmless than its variable lookup! as for the cache enable/disable, i agree that moving it to settings.php is a good idea. ------------------------------------------------------------------------ April 2, 2005 - 23:20 : Jeremy@kerneltrap.org Unfortunately my loose cache implementation currently relies on the variable table, and I don't immediately see how to avoid this. All the configuration stuff can be either removed or moved into settings.php as appropriate, but the 'cache_flush' variable is a dynamic timestamp that needs to be shared with all sessions. Currently this is done via the variable table. One idea is to move 'cache_flush' into the cache table... It would still cost a db_query, but certainly with less overhead than processing the entire variable table. Or is that too much of a kludge? Thoughts? ------------------------------------------------------------------------ April 3, 2005 - 00:43 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache_0.patch (10.22 KB) It's probably easier to see what I'm talking about with a patch. So, here's an updated patch that removes all variable_get's from bootstrap.inc. It works the same as my earlier loose caching patch, but the cache configuration has been moved from the adminsitrative interface to settings.php. All the variable logic could probabily be moved out of bootstrap.inc back to common.inc once this patch was applied. Specifically, this patch potentially improves upon my earlier patch by removing the overhead of initializing the variable array. I'd love to see some benchmark results to determine if this is worth it. ------------------------------------------------------------------------ April 3, 2005 - 03:51 : Dries Thanks for investigating this further, Jemery. If variable_get() is already called, I'd prefer to stick with it unless we pay a significant performance penalty for it. ------------------------------------------------------------------------ April 3, 2005 - 05:34 : Bèr Kessels Just a random thought I had lying around for a while: Combine cache and throttle. That would mean we use the cache we do now, but offer per-role cahce refresh, based on the throttle level. Something like: At Trottle limit [6 |v] do not refresh for [X] Anonymous [X] Registered [ ] Advertiser [ ] Administrator means that above level six, anonymous and registered cache will not be refreshed. This can be expanded by a cron wich will refres all cahces anyway. meaning that in the above exmaple, witha cron every ten minutes, anonymous and registered will see only refreshements every ten minutes. ------------------------------------------------------------------------ April 3, 2005 - 05:54 : Dries Ber: personally, I would not involve user roles. At least not until we have a better understanding of how the loose chacing performs and behaves. Maybe in future, I'd add a _simple_ checkbox along these lines: "Always use loose caching when the throttle mechanism is active". Daryl: could you try Jeremy's patch on SFX? Ultimately a mechanism like this ought to be tested on high-traffic websites. ------------------------------------------------------------------------ April 4, 2005 - 20:53 : daryl@spreadfirefox.com Dries, we'll be upgrading to the latest and greatest CivicSpace once we get a few modules ported over. I'll try to remember to implement the patch once we're done with that. ------------------------------------------------------------------------ April 8, 2005 - 07:45 : Dries Patch no longer applies. Please keep the variable_get()'s in for now.
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Dries Status: patch I'd think the information is already there? sess_read() loads all session columns, including the new cache-column, into the $user object. This line might be redundant: $required = db_fetch_object(db_query("SELECT cache FROM {sessions} WHERE uid = %d and sid = '%s'", $user->uid, $sid)); You might also be able to eliminate: db_query("UPDATE {sessions} SET cache = %d WHERE uid = %d AND sid = '%s'", time(), $user->uid, $sid); We already do a very similar query in sess_write(). Dries Previous comments: ------------------------------------------------------------------------ March 23, 2005 - 06:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 23, 2005 - 06:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 23, 2005 - 06:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 23, 2005 - 06:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 09:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 10:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 12:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 14:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 16:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 14:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 15:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 18:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 20:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 15:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. ------------------------------------------------------------------------ April 2, 2005 - 16:10 : Jeremy@kerneltrap.org daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry. ------------------------------------------------------------------------ April 2, 2005 - 21:23 : moshe weitzman i think you can simply get rid of the timer variable and assume it to be true. it is harmless - certainly more harmless than its variable lookup! as for the cache enable/disable, i agree that moving it to settings.php is a good idea. ------------------------------------------------------------------------ April 3, 2005 - 06:20 : Jeremy@kerneltrap.org Unfortunately my loose cache implementation currently relies on the variable table, and I don't immediately see how to avoid this. All the configuration stuff can be either removed or moved into settings.php as appropriate, but the 'cache_flush' variable is a dynamic timestamp that needs to be shared with all sessions. Currently this is done via the variable table. One idea is to move 'cache_flush' into the cache table... It would still cost a db_query, but certainly with less overhead than processing the entire variable table. Or is that too much of a kludge? Thoughts? ------------------------------------------------------------------------ April 3, 2005 - 07:43 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache_0.patch (10.22 KB) It's probably easier to see what I'm talking about with a patch. So, here's an updated patch that removes all variable_get's from bootstrap.inc. It works the same as my earlier loose caching patch, but the cache configuration has been moved from the adminsitrative interface to settings.php. All the variable logic could probabily be moved out of bootstrap.inc back to common.inc once this patch was applied. Specifically, this patch potentially improves upon my earlier patch by removing the overhead of initializing the variable array. I'd love to see some benchmark results to determine if this is worth it. ------------------------------------------------------------------------ April 3, 2005 - 10:51 : Dries Thanks for investigating this further, Jemery. If variable_get() is already called, I'd prefer to stick with it unless we pay a significant performance penalty for it. ------------------------------------------------------------------------ April 3, 2005 - 12:34 : Bèr Kessels Just a random thought I had lying around for a while: Combine cache and throttle. That would mean we use the cache we do now, but offer per-role cahce refresh, based on the throttle level. Something like: At Trottle limit [6 |v] do not refresh for [X] Anonymous [X] Registered [ ] Advertiser [ ] Administrator means that above level six, anonymous and registered cache will not be refreshed. This can be expanded by a cron wich will refres all cahces anyway. meaning that in the above exmaple, witha cron every ten minutes, anonymous and registered will see only refreshements every ten minutes. ------------------------------------------------------------------------ April 3, 2005 - 12:54 : Dries Ber: personally, I would not involve user roles. At least not until we have a better understanding of how the loose chacing performs and behaves. Maybe in future, I'd add a _simple_ checkbox along these lines: "Always use loose caching when the throttle mechanism is active". Daryl: could you try Jeremy's patch on SFX? Ultimately a mechanism like this ought to be tested on high-traffic websites. ------------------------------------------------------------------------ April 5, 2005 - 03:53 : daryl@spreadfirefox.com Dries, we'll be upgrading to the latest and greatest CivicSpace once we get a few modules ported over. I'll try to remember to implement the patch once we're done with that. ------------------------------------------------------------------------ April 8, 2005 - 14:45 : Dries Patch no longer applies. Please keep the variable_get()'s in for now. ------------------------------------------------------------------------ April 8, 2005 - 15:31 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache_1.patch (9.14 KB) Resync patch to CVS HEAD. Original design, uses variable_get in cached page generation. Updated comments regarding cache field in sessions table. A couple notes: - please verify that the PostgreSQL format is correct - a future optimization, perhaps the cache field from the sessions table could be retrieved from the current session rather than as an extra db_query
Issue status update for http://drupal.org/node/19298 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Jeremy@kerneltrap.org Reported by: Jeremy@kerneltrap.org Updated by: Jeremy@kerneltrap.org Status: patch Attachment: http://drupal.org/files/issues/cache_2.patch (9.67 KB) Yes, I was realizing that as I uploaded the last patch, hence the comment. I just didn't have the time to look into it then. Here's an updated patch, now it uses the $user variable instead of adding database queries. I did have to modify sess_write() to save the cache field. Resynched with HEAD. Jeremy@kerneltrap.org Previous comments: ------------------------------------------------------------------------ March 22, 2005 - 23:32 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/database.mysql_1.patch (382 bytes) Drupal's existing caching mechanism doesn't perform well on highly dynamic websites in which the cache is flushed frequently. One example is a site that is under attack by a spambot that is posting spam comments every few seconds, causing all cached pages to be flushed every few seconds. The attached patches provide the following cache methods: 1) CACHE_DISABLED: no caching. This is unchanged from before. 2) CACHE_ENABLED_STRICT: This is Drupal's existing caching method, in which the cache is flushed immediately whenever it contains invalid data. 3) CACHE_ENABLED_LOOSE: This is the new caching method (feel free to suggest a better name, this is the best I could come up with tonight). Essentially, it immediately flushes the cache only for specific users who have modified cached data (whether or not they are logged in), delaying the flushing of data for other users by several minutes. This feature requires a new 'cache' field in the sessions table to track the cache status for each user, whether or not they are logged in. A one line change to the system.module updates the cache configuration settings to include an additional option for loose caching. The rest of the changes are in bootstrap.inc, as follows: - cache_clear_all: if strict caching is enabled, nothing is changed. If loose caching is enabled, we delay the flushing of the entire cache for 5 minutes. Instead, we update this user's cache field in the sessions table so we know any cache pages created before the current time are no longer valid for this user. We only set the global "cache_flush" variable if it's not already set. If the global "cache_flush" variable was set more than 5 minutes ago, we flush all cache pages that were expired when the "cache_flush" variable was set. - cache_get: if strict caching is enabled, nothing is changed. If loose caching is enabled, we first check the global "cache_flush" variable and if older than 5 minutes we do a garbage collection call to cache_clear_all. Next, we see if there is a valid cache entry for us. If the cache entry is older than our session table's "cache" field, we don't use it. This will cause the cache to be regenerated by the current user, and ultimately updated for all users. Attached is the patch to database.mysql. To add manually, execute the following: ALTER TABLE sessions ADD cache int(11) NOT NULL default '0' AFTER timestamp; ------------------------------------------------------------------------ March 22, 2005 - 23:33 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/system.module_5.patch (1.49 KB) Attached is the patch for the system.module, adding the new cache configuration option. ------------------------------------------------------------------------ March 22, 2005 - 23:36 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc.patch (3.62 KB) And finally, the patch for bootstrap.inc. BTW: Do we want to make the 'cache_flush_delay' configurable? Or perhaps just stick with a sane default? Perhaps 60 seconds is more sane than 300 seconds? ------------------------------------------------------------------------ March 22, 2005 - 23:44 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/bootstrap.inc_0.patch (3.61 KB) Updated version of the bootstrap.inc.patch, removing an erroneous "else" that prevented the cache from being properly updated for a specific user after a delayed flush. ------------------------------------------------------------------------ March 23, 2005 - 02:12 : Anonymous Looks very good. Please provide patches in one file, though. Gerhard ------------------------------------------------------------------------ March 23, 2005 - 03:35 : stefan nagtegaal First of all, I do not have huge sites under my juristriction, so maybe my points here aren't valid.. If so, tell me and I'll shut up.. I took a look at your patches, and I truly do not understand why you set more Caching methods in 'admin/system'. I think that it would be better if we stay with the cuurent options (Enabled, Disabled) and let Drupal suggest the way of the caching methods (CACHE_ENABLED_LOOSE and CACHE_ENABLED_SCTRICT). This would make your patch to system.module a little smaller and easier to understand for newbies.. Also, as you said yourself, I don't see why we should make the 'cache_flush_delay' configurable. I mean, how would you know what a good time would be? Couldn't drupal find out the right value for that, so the administrator doesn't have to? Again, I'm not speaking as someone who is in need of something like this, but I can understand some people are.. What I'm trying to tell, is that the idea is probably pretty good only with less options and more automagical cache settings it would be more userfriendly ad doesn't scare off newbies (or me)... ------------------------------------------------------------------------ March 23, 2005 - 05:21 : Dries Looks like this patch can go in soon after the DRUPAL-4-6 has been created. You'll want to update the cache related documentation in system_help() (then again it might not be necessary). I wouldn't bother making this more configurable until we have evaluated/tested this some more. I'm very interested in testing this out or in getting some performance numbers. ------------------------------------------------------------------------ March 23, 2005 - 07:53 : Jeremy@kerneltrap.org When time permits, I will combine the patches into one file, and update the help text. I included them as separate files for now to simplify code review. I also see a logical error I need to fix when performing garbage collection that could cause the cache to flush uneccessarily. I did not make the functionality automatic for two reasons: it makes the code slightly more complex, and it makes the functionality more difficult to benchmark. While I think it would be a good idea, I will not modify the patch to implement automatic cache configuration unless it is requested by Dries or other core developers. To implement automatic cache configuration, it could simply be tied to the throttle. If the throttle is off (ie the throttle module is disabled, or the site is not busy), use the strict cache. If the throttle is on (ie the throttle module is enabled and the site is busy), use the loose cache. Alternatively, we could leave the configuration as it currently is in this patch (with disabled, strict and loose), and the loose caching functionality itself could be tied to the throttle. If the throttle is off, use a low cache_flush_delay (ie 15 seconds). If the throttle is on, use a high cache_flush_delay (ie 300 seconds). The theory being that the cache_flush_delay enforces a minimum cache life, and the busier a site the more it will benefit from a longer minimum cache life. I believe that this latter implementation would be the best for the spam floods I've experienced on my website. ------------------------------------------------------------------------ March 23, 2005 - 09:16 : moshe weitzman changing the cache mode depending on throttle status is a really good idea IMO. This patch is a great step forward for speeding up Drupal for anon users. The biggest innovation in this area since bootstrap, I think. ------------------------------------------------------------------------ March 25, 2005 - 07:30 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache.patch (8.82 KB) Updated patch attached. Fixed a couple small errors and put all patches in one file. I added a patch for database.pgsql and update.inc (though the pgsql portion of update.inc may not be 100% right). This patch does not tie the cache to the throttle, instead only allowing manual configuration. ------------------------------------------------------------------------ April 1, 2005 - 08:19 : Jeremy@kerneltrap.org Patch testimony: I just got hit by a spam attack on KernelTrap this morning (more than 10,000 spam comment posted in less than 30 minutes) which choked my server ("too many database connections"). So, I merged this patch into my 4.5 installation and enabled "loose caching". While I don't have any actual performance numbers, I'm happy to report that a basically non-functional website become comfortably responsive again. ------------------------------------------------------------------------ April 1, 2005 - 11:23 : Dries The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache (i.e. we have to load and prepare all variables). I haven't tested this though so maybe we were already loading the variables. Not too much of a problem, I think, but I just wanted to point out this small caveat/regression. In the code comments, it might be a good idea to document the interaction with the session-table. Again, a minor nitpick. This patch will go into Drupal 4.7. Good work. ------------------------------------------------------------------------ April 1, 2005 - 13:06 : daryl@spreadfirefox.com Chris Messina (factoryjoe) asked me to check this thread out and respond based on my experience with www.spreadfirefox.com. When we were really struggling with load issues and trying to make the site scale, Dries suggested that we disable the automatic cache dump per page load and handle cache dumping in a cron job; this removes all per-request database overhead associated with cache dumping, and it helped us out. Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me. ------------------------------------------------------------------------ April 2, 2005 - 08:56 : Jeremy@kerneltrap.org Dries wrote on April 1, 2005 - 11:23: ""The fact you use variable_get() in cache_get() presumably introduces an extra SQL query for each page request that hits the cache" " I believe that this overhead is already assumed due to two uses of variable_get() in drupal_page_header(). In building each page, we first check 'dev_timer', and then 'cache'. So my use of a variable does not introduce an extra SQL query. That said, it would be nice to get rid of this overhead. Offhand, the easiest way to do so would probably be to make both the 'dev_timer' variable and the 'cache' variable configurable in settings.php instead of from the administrative interface. Why not? It would be nice to get rid of loading the variable table for cached pages... Dries wrote on April 1, 2005 - 11:23: ""In the code comments, it might be a good idea to document the interaction with the session-table." " Okay, I will update the patch. ------------------------------------------------------------------------ April 2, 2005 - 09:10 : Jeremy@kerneltrap.org daryl@spreadfirefox.com wrote on April 1, 2005 - 13:06: ""Running such a cron job every five minutes or so beats the heck out of running it for every request and would seem to achieve roughly the same end that this patch achieves (at least as I read the post -- I haven't looked at the actual cache). I suppose the downside is that those without cron access are out of luck, and the patch I gather sort of approximates a 5-minute cron routine dependent upon page loads to run. Which seems fine to me." " Just to clarify, the patch does not approximate. There is a similarity to what you have described, however in the cron case the cache is dumped every 5 minutes whether it needs it or not, and anyone who posts in that 5 minutes doesn't see the effect of their post until the cron run completes. With this patch, the timer to dump the cache is only started once something triggers a cache flush. After the 5 minutes passes, all cache entries older than 5 minutes will be dumped. (This typically happens the first time cache_get is called after 5 minutes has passed) Additionally, the user who triggered the cache flush will actually stop viewing cached pages so that any changes they made will show up for them immediately in their session. And as an added bonus, every page they now visit will result in the cache for that page being rebuilt, helping to further minimize the load by potentially spreading out over time the cost of rebuilding cache entries. Thus this has performance and usability advantages to simply using a crontab entry. ------------------------------------------------------------------------ April 2, 2005 - 14:23 : moshe weitzman i think you can simply get rid of the timer variable and assume it to be true. it is harmless - certainly more harmless than its variable lookup! as for the cache enable/disable, i agree that moving it to settings.php is a good idea. ------------------------------------------------------------------------ April 2, 2005 - 23:20 : Jeremy@kerneltrap.org Unfortunately my loose cache implementation currently relies on the variable table, and I don't immediately see how to avoid this. All the configuration stuff can be either removed or moved into settings.php as appropriate, but the 'cache_flush' variable is a dynamic timestamp that needs to be shared with all sessions. Currently this is done via the variable table. One idea is to move 'cache_flush' into the cache table... It would still cost a db_query, but certainly with less overhead than processing the entire variable table. Or is that too much of a kludge? Thoughts? ------------------------------------------------------------------------ April 3, 2005 - 00:43 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache_0.patch (10.22 KB) It's probably easier to see what I'm talking about with a patch. So, here's an updated patch that removes all variable_get's from bootstrap.inc. It works the same as my earlier loose caching patch, but the cache configuration has been moved from the adminsitrative interface to settings.php. All the variable logic could probabily be moved out of bootstrap.inc back to common.inc once this patch was applied. Specifically, this patch potentially improves upon my earlier patch by removing the overhead of initializing the variable array. I'd love to see some benchmark results to determine if this is worth it. ------------------------------------------------------------------------ April 3, 2005 - 03:51 : Dries Thanks for investigating this further, Jemery. If variable_get() is already called, I'd prefer to stick with it unless we pay a significant performance penalty for it. ------------------------------------------------------------------------ April 3, 2005 - 05:34 : Bèr Kessels Just a random thought I had lying around for a while: Combine cache and throttle. That would mean we use the cache we do now, but offer per-role cahce refresh, based on the throttle level. Something like: At Trottle limit [6 |v] do not refresh for [X] Anonymous [X] Registered [ ] Advertiser [ ] Administrator means that above level six, anonymous and registered cache will not be refreshed. This can be expanded by a cron wich will refres all cahces anyway. meaning that in the above exmaple, witha cron every ten minutes, anonymous and registered will see only refreshements every ten minutes. ------------------------------------------------------------------------ April 3, 2005 - 05:54 : Dries Ber: personally, I would not involve user roles. At least not until we have a better understanding of how the loose chacing performs and behaves. Maybe in future, I'd add a _simple_ checkbox along these lines: "Always use loose caching when the throttle mechanism is active". Daryl: could you try Jeremy's patch on SFX? Ultimately a mechanism like this ought to be tested on high-traffic websites. ------------------------------------------------------------------------ April 4, 2005 - 20:53 : daryl@spreadfirefox.com Dries, we'll be upgrading to the latest and greatest CivicSpace once we get a few modules ported over. I'll try to remember to implement the patch once we're done with that. ------------------------------------------------------------------------ April 8, 2005 - 07:45 : Dries Patch no longer applies. Please keep the variable_get()'s in for now. ------------------------------------------------------------------------ April 8, 2005 - 08:31 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/cache_1.patch (9.14 KB) Resync patch to CVS HEAD. Original design, uses variable_get in cached page generation. Updated comments regarding cache field in sessions table. A couple notes: - please verify that the PostgreSQL format is correct - a future optimization, perhaps the cache field from the sessions table could be retrieved from the current session rather than as an extra db_query ------------------------------------------------------------------------ April 8, 2005 - 11:08 : Dries I'd think the information is already there? sess_read() loads all session columns, including the new cache-column, into the $user object. This line might be redundant: $required = db_fetch_object(db_query("SELECT cache FROM {sessions} WHERE uid = %d and sid = '%s'", $user->uid, $sid)); You might also be able to eliminate: db_query("UPDATE {sessions} SET cache = %d WHERE uid = %d AND sid = '%s'", time(), $user->uid, $sid); We already do a very similar query in sess_write().
I'm not a fan of the naming of this patch, sorry for bring it up only now. To me, "loose caching" sounds like the system where we wipe the cache often, while "strict caching" would be where we keep the cache for as long as possible and just disable for a few users. I guess the current (opposite) naming comes from the fact that "strict" caching ensures you always see up-to-date pages, while "loose" caching doesn't guarantee that. But there's too much ambiguity IMO. I propse the following naming instead: - Sparse caching: where the cache is wiped often (currently called "strict" ?) - Aggressive caching: where the cache is kept as long as possible (currently called "loose" ?). Sparse correctly conveys that less caching will be done and less hits will result. Aggressive suggests greedy caching at a trade-off of having slightly outdated pages. I can make a patch for this if you agree... Steven Wittens
I'm not a fan of the naming of this patch, sorry for bring it up only now.
You'll be happy to know that I'm not a fan of the naming either...
To me, "loose caching" sounds like the system where we wipe the cache often, while "strict caching" would be where we keep the cache for as long as possible and just disable for a few users. I guess the current (opposite) naming comes from the fact that "strict" caching ensures you always see up-to-date pages, while "loose" caching doesn't guarantee that. But there's too much ambiguity IMO.
I don't see the words loose and strict how you do, but still agree that they're not the best word choices. I guess that's proof that there's too much ambiguity. ;)
I propse the following naming instead: - Sparse caching: where the cache is wiped often (currently called "strict" ?)
I don't really like the term "sparse caching". That would suggest that the alternative is "dense caching". Neither really means anything to me. How about dropping "strict" and "sparse" altogether, and simply having three choices: - no caching - caching - aggresive caching
- Aggressive caching: where the cache is kept as long as possible (currently called "loose" ?).
I do very much like the name "aggresive caching", that's a good description.
Sparse correctly conveys that less caching will be done and less hits will result. Aggressive suggests greedy caching at a trade-off of having slightly outdated pages.
I can make a patch for this if you agree...
Go for it! :) Cheers, -Jeremy
participants (9)
-
Anonymous -
Bèr Kessels
-
daryl
-
Dries
-
Jeremy
-
Jeremy Andrews -
moshe weitzman
-
stefan nagtegaal
-
Steven Wittens