Issue status update for http://drupal.org/node/24260 Project: Drupal Version: 4.6.0 Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Yrlec Updated by: walkah Status: patch +1 in principle... however, rather than see this get committed directly, I'd rather see a "caching framework" evolve... we've also had other talks about making drupal more squid friendly, in addition to using memcached, etc. i'm willing to help put some brainpower into this... walkah Previous comments: ------------------------------------------------------------------------ June 3, 2005 - 14:52 : Yrlec Attachment: http://drupal.org/files/issues/newcache.php (3.82 KB) I'm not sure if this is the best place to post, please let me know if I should post somewhere else. I've done some changes to bootstrap.inc to make it use the semaphore cache of either eAccelerator or Turck mmCache instead of the database to handle the caching. I've not done any benchmarking but I'm quiet sure that it will speed things up. I'm afraid that I didn't base my code on the latest version so I apologize if my changes aren't compatible with the current cvs-version. As you can see, if neither eAccelerator nor Turck mmCache isn't available on the system the old caching mechanism will be used. The changes are in cache_get, cache_set and cache_clear_all. I'm sorry I didn't post a diff but I think it will be quiet easy to implement anyway.

Issue status update for http://drupal.org/node/24260 Project: Drupal Version: 4.6.0 Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Yrlec Updated by: Yrlec Status: patch Dear Dries, I am bit surprised by your reply. I get the feeling that you do not want people to contribute. - Ok I forgot to changing the camel-casing on one variable, is that such a big deal? - I already said that I did not use the HEAD but if you look at the code you will see that it is quiet easy to implement anyway (just copy the upper "if" of each function). - Just because it may not be a major performance boost does that mean it is a bad thing? IMHO all performance improvments should be implemented since performance is a major issue for many users. -What has that got to do with it? Even if you use Squid you will benefit from caching static content in memory, neither thing has to exclude the other. I love Drupal and I thought I could try and give something back by contributing but I know I realize my help is probably not wanted. Yrlec Previous comments: ------------------------------------------------------------------------ June 3, 2005 - 19:52 : Yrlec Attachment: http://drupal.org/files/issues/newcache.php (3.82 KB) I'm not sure if this is the best place to post, please let me know if I should post somewhere else. I've done some changes to bootstrap.inc to make it use the semaphore cache of either eAccelerator or Turck mmCache instead of the database to handle the caching. I've not done any benchmarking but I'm quiet sure that it will speed things up. I'm afraid that I didn't base my code on the latest version so I apologize if my changes aren't compatible with the current cvs-version. As you can see, if neither eAccelerator nor Turck mmCache isn't available on the system the old caching mechanism will be used. The changes are in cache_get, cache_set and cache_clear_all. I'm sorry I didn't post a diff but I think it will be quiet easy to implement anyway. ------------------------------------------------------------------------ June 3, 2005 - 20:31 : walkah +1 in principle... however, rather than see this get committed directly, I'd rather see a "caching framework" evolve... we've also had other talks about making drupal more squid friendly, in addition to using memcached, etc. i'm willing to help put some brainpower into this... ------------------------------------------------------------------------ June 4, 2005 - 07:16 : Dries - The variable names are not consistent with our coding style (no camel casing). - The code won't work against HEAD. - This needs to be benchmarked before it can be committed. I don't think it will buy us much. - I agree with James that the squid changes might be worth looking at too (forgot the URL).

Issue status update for http://drupal.org/node/24260 Project: Drupal Version: 4.6.0 Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Yrlec Updated by: Dries Status: patch Quite the contrary. I'd like to see this patch evolve and work on this together. Dries Previous comments: ------------------------------------------------------------------------ June 3, 2005 - 21:52 : Yrlec Attachment: http://drupal.org/files/issues/newcache.php (3.82 KB) I'm not sure if this is the best place to post, please let me know if I should post somewhere else. I've done some changes to bootstrap.inc to make it use the semaphore cache of either eAccelerator or Turck mmCache instead of the database to handle the caching. I've not done any benchmarking but I'm quiet sure that it will speed things up. I'm afraid that I didn't base my code on the latest version so I apologize if my changes aren't compatible with the current cvs-version. As you can see, if neither eAccelerator nor Turck mmCache isn't available on the system the old caching mechanism will be used. The changes are in cache_get, cache_set and cache_clear_all. I'm sorry I didn't post a diff but I think it will be quiet easy to implement anyway. ------------------------------------------------------------------------ June 3, 2005 - 22:31 : walkah +1 in principle... however, rather than see this get committed directly, I'd rather see a "caching framework" evolve... we've also had other talks about making drupal more squid friendly, in addition to using memcached, etc. i'm willing to help put some brainpower into this... ------------------------------------------------------------------------ June 4, 2005 - 09:16 : Dries - The variable names are not consistent with our coding style (no camel casing). - The code won't work against HEAD. - This needs to be benchmarked before it can be committed. I don't think it will buy us much. - I agree with James that the squid changes might be worth looking at too (forgot the URL). ------------------------------------------------------------------------ June 4, 2005 - 10:07 : Yrlec Dear Dries, I am bit surprised by your reply. I get the feeling that you do not want people to contribute. - Ok I forgot to changing the camel-casing on one variable, is that such a big deal? - I already said that I did not use the HEAD but if you look at the code you will see that it is quiet easy to implement anyway (just copy the upper "if" of each function). - Just because it may not be a major performance boost does that mean it is a bad thing? IMHO all performance improvments should be implemented since performance is a major issue for many users. -What has that got to do with it? Even if you use Squid you will benefit from caching static content in memory, neither thing has to exclude the other. I love Drupal and I thought I could try and give something back by contributing but I know I realize my help is probably not wanted.

Issue status update for http://drupal.org/node/24260 Project: Drupal Version: 4.6.0 Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Yrlec Updated by: chx Status: patch First, don't think that Dries does not want you to contribute, but this has been discussed already. However, I have some concerns. Natrak installed eAccelerator on Drupal at one point and it segfaulted Apache quite often. Natrak, if you read this please comment! chx Previous comments: ------------------------------------------------------------------------ June 3, 2005 - 21:52 : Yrlec Attachment: http://drupal.org/files/issues/newcache.php (3.82 KB) I'm not sure if this is the best place to post, please let me know if I should post somewhere else. I've done some changes to bootstrap.inc to make it use the semaphore cache of either eAccelerator or Turck mmCache instead of the database to handle the caching. I've not done any benchmarking but I'm quiet sure that it will speed things up. I'm afraid that I didn't base my code on the latest version so I apologize if my changes aren't compatible with the current cvs-version. As you can see, if neither eAccelerator nor Turck mmCache isn't available on the system the old caching mechanism will be used. The changes are in cache_get, cache_set and cache_clear_all. I'm sorry I didn't post a diff but I think it will be quiet easy to implement anyway. ------------------------------------------------------------------------ June 3, 2005 - 22:31 : walkah +1 in principle... however, rather than see this get committed directly, I'd rather see a "caching framework" evolve... we've also had other talks about making drupal more squid friendly, in addition to using memcached, etc. i'm willing to help put some brainpower into this... ------------------------------------------------------------------------ June 4, 2005 - 09:16 : Dries - The variable names are not consistent with our coding style (no camel casing). - The code won't work against HEAD. - This needs to be benchmarked before it can be committed. I don't think it will buy us much. - I agree with James that the squid changes might be worth looking at too (forgot the URL). ------------------------------------------------------------------------ June 4, 2005 - 10:07 : Yrlec Dear Dries, I am bit surprised by your reply. I get the feeling that you do not want people to contribute. - Ok I forgot to changing the camel-casing on one variable, is that such a big deal? - I already said that I did not use the HEAD but if you look at the code you will see that it is quiet easy to implement anyway (just copy the upper "if" of each function). - Just because it may not be a major performance boost does that mean it is a bad thing? IMHO all performance improvments should be implemented since performance is a major issue for many users. -What has that got to do with it? Even if you use Squid you will benefit from caching static content in memory, neither thing has to exclude the other. I love Drupal and I thought I could try and give something back by contributing but I know I realize my help is probably not wanted. ------------------------------------------------------------------------ June 4, 2005 - 10:52 : Dries Quite the contrary. I'd like to see this patch evolve and work on this together. ------------------------------------------------------------------------ June 4, 2005 - 13:14 : Steven Please keep in mind that your patch is one of many others in the queue... we can't shower everyone who submits one with praise, it is not a good use of our limited time. And in spite of you not submitting your code in the most ideal form possible (a diff), you had 4 replies already. Your issue hasn't even been up for a whole day. Most of us do this in our spare time, so this is a succesful response. The fact is, Drupal has high standards to keep up. If people point out how your patch does not conform to those, it means they would probably accept it if it did. It is a good sign, trust me. Your change automatically uses eaccelerator/mmcache if they are present, without providing an option to the administrator for this. Is this a reasonable thing to do? I'm not sure how widespread they are, but it is these sort of things which can cause sites to suddenly break when the host's configuration changes. Now as far as the patch goes: * You still call db_encode_blob() and db_decode_blob() even when using the alternate storage mechanism. This is a specific form of encoding suited for the current database type, so it is unnecessary. * All data passed to cache_set must be a string already, so serialization is unnecessary. * You convert CACHE_TEMPORARY to 1 (second?). Is there a reason for using this and not 0 (that would seem more logical to me). The code does not conform to the code style in much more ways than just one name. It looks messy and unorganised. * You do the check for 'function_exists' twice. You can easily re-arrange things to avoid this. * The if statements for checking the type of $expire passed would look much cleaner expressed as a switch statement. * We put spaces after control flow statements (if, switch, while, etc). * We put spaces around the concatenation operator, except between a space and a quote. This is a rule which many forget, but it strikes the perfect balance between keeping code readable, and keeping code compact. * We prefer single quoted strings to double quoted strings, except when it would reduce readability (e.g. when there are single quotes inside). * We prefer proper typography for comments (ending with a period if it is a full sentence) and a space between the comment's marker and its contents. * Having a clean code style keeps the code easy to read, easy to scan visually and easy to search programmatically. By respecting these ideas, any future contributions you make will have a higher chance of being accepted. Please take this as constructive criticism, not a sign that we dislike you or your contribution.

Issue status update for http://drupal.org/node/24260 Project: Drupal Version: 4.6.0 Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Yrlec Updated by: Kjartan Status: patch eAccelerator has problems with sites that have a high load and at some point will screw up on randon pieces of PHP code and cause segfaults till apache is restarted. From what I last heard they are making progress and probably has something to do with locking and eAccelerator not being fully thread safe. Zend Optimizer seems to work without problems, so using that till something better comes up. Also just quickly looking at this code wouldn't the cache size theoretically be small than when using MySQL? Admins can set a limit on shared memory storage, from the docs on eaccelerator_put it seems that once its full you can't store any new data (function returns false). On Drupal.org I usually see the cache table to be anywhere from 20 to 80 megs, not sure what the default values of eAccelerator are. Kjartan Previous comments: ------------------------------------------------------------------------ June 3, 2005 - 21:52 : Yrlec Attachment: http://drupal.org/files/issues/newcache.php (3.82 KB) I'm not sure if this is the best place to post, please let me know if I should post somewhere else. I've done some changes to bootstrap.inc to make it use the semaphore cache of either eAccelerator or Turck mmCache instead of the database to handle the caching. I've not done any benchmarking but I'm quiet sure that it will speed things up. I'm afraid that I didn't base my code on the latest version so I apologize if my changes aren't compatible with the current cvs-version. As you can see, if neither eAccelerator nor Turck mmCache isn't available on the system the old caching mechanism will be used. The changes are in cache_get, cache_set and cache_clear_all. I'm sorry I didn't post a diff but I think it will be quiet easy to implement anyway. ------------------------------------------------------------------------ June 3, 2005 - 22:31 : walkah +1 in principle... however, rather than see this get committed directly, I'd rather see a "caching framework" evolve... we've also had other talks about making drupal more squid friendly, in addition to using memcached, etc. i'm willing to help put some brainpower into this... ------------------------------------------------------------------------ June 4, 2005 - 09:16 : Dries - The variable names are not consistent with our coding style (no camel casing). - The code won't work against HEAD. - This needs to be benchmarked before it can be committed. I don't think it will buy us much. - I agree with James that the squid changes might be worth looking at too (forgot the URL). ------------------------------------------------------------------------ June 4, 2005 - 10:07 : Yrlec Dear Dries, I am bit surprised by your reply. I get the feeling that you do not want people to contribute. - Ok I forgot to changing the camel-casing on one variable, is that such a big deal? - I already said that I did not use the HEAD but if you look at the code you will see that it is quiet easy to implement anyway (just copy the upper "if" of each function). - Just because it may not be a major performance boost does that mean it is a bad thing? IMHO all performance improvments should be implemented since performance is a major issue for many users. -What has that got to do with it? Even if you use Squid you will benefit from caching static content in memory, neither thing has to exclude the other. I love Drupal and I thought I could try and give something back by contributing but I know I realize my help is probably not wanted. ------------------------------------------------------------------------ June 4, 2005 - 10:52 : Dries Quite the contrary. I'd like to see this patch evolve and work on this together. ------------------------------------------------------------------------ June 4, 2005 - 13:14 : Steven Please keep in mind that your patch is one of many others in the queue... we can't shower everyone who submits one with praise, it is not a good use of our limited time. And in spite of you not submitting your code in the most ideal form possible (a diff), you had 4 replies already. Your issue hasn't even been up for a whole day. Most of us do this in our spare time, so this is a succesful response. The fact is, Drupal has high standards to keep up. If people point out how your patch does not conform to those, it means they would probably accept it if it did. It is a good sign, trust me. Your change automatically uses eaccelerator/mmcache if they are present, without providing an option to the administrator for this. Is this a reasonable thing to do? I'm not sure how widespread they are, but it is these sort of things which can cause sites to suddenly break when the host's configuration changes. Now as far as the patch goes: * You still call db_encode_blob() and db_decode_blob() even when using the alternate storage mechanism. This is a specific form of encoding suited for the current database type, so it is unnecessary. * All data passed to cache_set must be a string already, so serialization is unnecessary. * You convert CACHE_TEMPORARY to 1 (second?). Is there a reason for using this and not 0 (that would seem more logical to me). The code does not conform to the code style in much more ways than just one name. It looks messy and unorganised. * You do the check for 'function_exists' twice. You can easily re-arrange things to avoid this. * The if statements for checking the type of $expire passed would look much cleaner expressed as a switch statement. * We put spaces after control flow statements (if, switch, while, etc). * We put spaces around the concatenation operator, except between a space and a quote. This is a rule which many forget, but it strikes the perfect balance between keeping code readable, and keeping code compact. * We prefer single quoted strings to double quoted strings, except when it would reduce readability (e.g. when there are single quotes inside). * We prefer proper typography for comments (ending with a period if it is a full sentence) and a space between the comment's marker and its contents. * Having a clean code style keeps the code easy to read, easy to scan visually and easy to search programmatically. By respecting these ideas, any future contributions you make will have a higher chance of being accepted. Please take this as constructive criticism, not a sign that we dislike you or your contribution. ------------------------------------------------------------------------ June 4, 2005 - 17:04 : chx First, don't think that Dries does not want you to contribute, but this has been discussed already. However, I have some concerns. Natrak installed eAccelerator on Drupal at one point and it segfaulted Apache quite often. Natrak, if you read this please comment! ------------------------------------------------------------------------ June 4, 2005 - 19:10 : TDobes chx: I have been using PHP5 + eAccelerator 0.9.2a (and just upgraded to 0.9.3) for a few months on a couple of my Drupal sites without any trouble. My sites certainly aren't as large or as high-traffic as Drupal.org, though. (I'm not sure whether it matters, but I'm using lighttpd instead of Apache.) Anyway, it's possible that a lot of eAccelerator bugs have been fixed recently.

Issue status update for http://drupal.org/node/24260 Project: Drupal Version: 4.6.0 Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Yrlec Updated by: Yrlec Status: patch Steven, I didn't expect a "shower of praise", I had just hoped for a bit more positive attitude. Take walkah as I good example of how I had hoped you would all be like; you don't have to be negative to come with some constructive input. I had also hoped you would spend less time on rather subjective nitpicking. You have some very valid arguments but they sort of disappear when you start to argument on why my code looks messy and unorganised because of a missing space between the comment slashes and the comment. I also find it quiet ironic that you spend so much time explaining that this is your spare-time why you don't have time to answer. A simple thanks instead would have saved both you and me a lot of time. To save us all some time I will not argument against the rather subjective and unrelated points but instead try and answer the questions which I find valid. * Your change automatically uses eaccelerator/mmcache if they are present, without providing an option to the administrator for this. Is this a reasonable thing to do? Sounds like a good idea to implement some kind of option to disable it. * You still call db_encode_blob() and db_decode_blob() even when using the alternate storage mechanism. This is a specific form of encoding suited for the current database type, so it is unnecessary. Ok, I'll remove them. * All data passed to cache_set must be a string already, so serialization is unnecessary. Same there, I'll remove them. However eAccelerator's documentation claims that it can segfault if you don't serialize. * You convert CACHE_TEMPORARY to 1 (second?). Is there a reason for using this and not 0 (that would seem more logical to me). If you read your code you will see that CACHE_PERMANENT is set to 0 so converting CACHE_TEMPORARY to 0 would be quiet stupid, wouldn't it? Also if you read eAccelerator's documentation you see that 0 is equivalent to "never". Which I assume means that it never "dies". * You do the check for 'function_exists' twice. You can easily re-arrange things to avoid this. Sure, I'll fix that. * We prefer single quoted strings to double quoted strings, except when it would reduce readability (e.g. when there are single quotes inside). Sure, I'll fix that. * Having a clean code style keeps the code easy to read, easy to scan visually and easy to search programmatically. Is that why you have your opening and closing brackets on different horizontal levels, which forces the eye to move both vertically horizontally to match two brackets? Sorry for being sarcastic but honestly, you call my code unorganised when your caching mechanism is placed in a file called bootstrap.inc? Let he who is without sin... Yrlec Previous comments: ------------------------------------------------------------------------ June 3, 2005 - 19:52 : Yrlec Attachment: http://drupal.org/files/issues/newcache.php (3.82 KB) I'm not sure if this is the best place to post, please let me know if I should post somewhere else. I've done some changes to bootstrap.inc to make it use the semaphore cache of either eAccelerator or Turck mmCache instead of the database to handle the caching. I've not done any benchmarking but I'm quiet sure that it will speed things up. I'm afraid that I didn't base my code on the latest version so I apologize if my changes aren't compatible with the current cvs-version. As you can see, if neither eAccelerator nor Turck mmCache isn't available on the system the old caching mechanism will be used. The changes are in cache_get, cache_set and cache_clear_all. I'm sorry I didn't post a diff but I think it will be quiet easy to implement anyway. ------------------------------------------------------------------------ June 3, 2005 - 20:31 : walkah +1 in principle... however, rather than see this get committed directly, I'd rather see a "caching framework" evolve... we've also had other talks about making drupal more squid friendly, in addition to using memcached, etc. i'm willing to help put some brainpower into this... ------------------------------------------------------------------------ June 4, 2005 - 07:16 : Dries - The variable names are not consistent with our coding style (no camel casing). - The code won't work against HEAD. - This needs to be benchmarked before it can be committed. I don't think it will buy us much. - I agree with James that the squid changes might be worth looking at too (forgot the URL). ------------------------------------------------------------------------ June 4, 2005 - 08:07 : Yrlec Dear Dries, I am bit surprised by your reply. I get the feeling that you do not want people to contribute. - Ok I forgot to changing the camel-casing on one variable, is that such a big deal? - I already said that I did not use the HEAD but if you look at the code you will see that it is quiet easy to implement anyway (just copy the upper "if" of each function). - Just because it may not be a major performance boost does that mean it is a bad thing? IMHO all performance improvments should be implemented since performance is a major issue for many users. -What has that got to do with it? Even if you use Squid you will benefit from caching static content in memory, neither thing has to exclude the other. I love Drupal and I thought I could try and give something back by contributing but I know I realize my help is probably not wanted. ------------------------------------------------------------------------ June 4, 2005 - 08:52 : Dries Quite the contrary. I'd like to see this patch evolve and work on this together. ------------------------------------------------------------------------ June 4, 2005 - 11:14 : Steven Please keep in mind that your patch is one of many others in the queue... we can't shower everyone who submits one with praise, it is not a good use of our limited time. And in spite of you not submitting your code in the most ideal form possible (a diff), you had 4 replies already. Your issue hasn't even been up for a whole day. Most of us do this in our spare time, so this is a succesful response. The fact is, Drupal has high standards to keep up. If people point out how your patch does not conform to those, it means they would probably accept it if it did. It is a good sign, trust me. Your change automatically uses eaccelerator/mmcache if they are present, without providing an option to the administrator for this. Is this a reasonable thing to do? I'm not sure how widespread they are, but it is these sort of things which can cause sites to suddenly break when the host's configuration changes. Now as far as the patch goes: * You still call db_encode_blob() and db_decode_blob() even when using the alternate storage mechanism. This is a specific form of encoding suited for the current database type, so it is unnecessary. * All data passed to cache_set must be a string already, so serialization is unnecessary. * You convert CACHE_TEMPORARY to 1 (second?). Is there a reason for using this and not 0 (that would seem more logical to me). The code does not conform to the code style in much more ways than just one name. It looks messy and unorganised. * You do the check for 'function_exists' twice. You can easily re-arrange things to avoid this. * The if statements for checking the type of $expire passed would look much cleaner expressed as a switch statement. * We put spaces after control flow statements (if, switch, while, etc). * We put spaces around the concatenation operator, except between a space and a quote. This is a rule which many forget, but it strikes the perfect balance between keeping code readable, and keeping code compact. * We prefer single quoted strings to double quoted strings, except when it would reduce readability (e.g. when there are single quotes inside). * We prefer proper typography for comments (ending with a period if it is a full sentence) and a space between the comment's marker and its contents. * Having a clean code style keeps the code easy to read, easy to scan visually and easy to search programmatically. By respecting these ideas, any future contributions you make will have a higher chance of being accepted. Please take this as constructive criticism, not a sign that we dislike you or your contribution. ------------------------------------------------------------------------ June 4, 2005 - 15:04 : chx First, don't think that Dries does not want you to contribute, but this has been discussed already. However, I have some concerns. Natrak installed eAccelerator on Drupal at one point and it segfaulted Apache quite often. Natrak, if you read this please comment! ------------------------------------------------------------------------ June 4, 2005 - 17:10 : TDobes chx: I have been using PHP5 + eAccelerator 0.9.2a (and just upgraded to 0.9.3) for a few months on a couple of my Drupal sites without any trouble. My sites certainly aren't as large or as high-traffic as Drupal.org, though. (I'm not sure whether it matters, but I'm using lighttpd instead of Apache.) Anyway, it's possible that a lot of eAccelerator bugs have been fixed recently. ------------------------------------------------------------------------ June 6, 2005 - 07:37 : Kjartan eAccelerator has problems with sites that have a high load and at some point will screw up on randon pieces of PHP code and cause segfaults till apache is restarted. From what I last heard they are making progress and probably has something to do with locking and eAccelerator not being fully thread safe. Zend Optimizer seems to work without problems, so using that till something better comes up. Also just quickly looking at this code wouldn't the cache size theoretically be small than when using MySQL? Admins can set a limit on shared memory storage, from the docs on eaccelerator_put it seems that once its full you can't store any new data (function returns false). On Drupal.org I usually see the cache table to be anywhere from 20 to 80 megs, not sure what the default values of eAccelerator are. ------------------------------------------------------------------------ June 6, 2005 - 14:16 : walkah KJ - true, but we're seeing much better behaviour out of eaccel 0.9.3 .. Yerlec - please don't be put off by such comments, call it "constructive criticism". I'm interested in working on this as well, but agree that making this eaccel / turck specific is probably less desirable as there are several different options for caching , and ideally we'd like a framework to accomodate as many as possible.

Issue status update for http://drupal.org/node/24260 Project: Drupal Version: 4.6.0 Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Yrlec Updated by: Yrlec Status: patch Junyor, Dries sent me an IM and apologized so my reply here was not directed to him (as you can see in the top row of the reply). Personally I think that being constructive means you come with solutions instead of just pointing out the problems but let's just drop that discussion and instead focus what further improvements we can do. Yrlec Previous comments: ------------------------------------------------------------------------ June 3, 2005 - 19:52 : Yrlec Attachment: http://drupal.org/files/issues/newcache.php (3.82 KB) I'm not sure if this is the best place to post, please let me know if I should post somewhere else. I've done some changes to bootstrap.inc to make it use the semaphore cache of either eAccelerator or Turck mmCache instead of the database to handle the caching. I've not done any benchmarking but I'm quiet sure that it will speed things up. I'm afraid that I didn't base my code on the latest version so I apologize if my changes aren't compatible with the current cvs-version. As you can see, if neither eAccelerator nor Turck mmCache isn't available on the system the old caching mechanism will be used. The changes are in cache_get, cache_set and cache_clear_all. I'm sorry I didn't post a diff but I think it will be quiet easy to implement anyway. ------------------------------------------------------------------------ June 3, 2005 - 20:31 : walkah +1 in principle... however, rather than see this get committed directly, I'd rather see a "caching framework" evolve... we've also had other talks about making drupal more squid friendly, in addition to using memcached, etc. i'm willing to help put some brainpower into this... ------------------------------------------------------------------------ June 4, 2005 - 07:16 : Dries - The variable names are not consistent with our coding style (no camel casing). - The code won't work against HEAD. - This needs to be benchmarked before it can be committed. I don't think it will buy us much. - I agree with James that the squid changes might be worth looking at too (forgot the URL). ------------------------------------------------------------------------ June 4, 2005 - 08:07 : Yrlec Dear Dries, I am bit surprised by your reply. I get the feeling that you do not want people to contribute. - Ok I forgot to changing the camel-casing on one variable, is that such a big deal? - I already said that I did not use the HEAD but if you look at the code you will see that it is quiet easy to implement anyway (just copy the upper "if" of each function). - Just because it may not be a major performance boost does that mean it is a bad thing? IMHO all performance improvments should be implemented since performance is a major issue for many users. -What has that got to do with it? Even if you use Squid you will benefit from caching static content in memory, neither thing has to exclude the other. I love Drupal and I thought I could try and give something back by contributing but I know I realize my help is probably not wanted. ------------------------------------------------------------------------ June 4, 2005 - 08:52 : Dries Quite the contrary. I'd like to see this patch evolve and work on this together. ------------------------------------------------------------------------ June 4, 2005 - 11:14 : Steven Please keep in mind that your patch is one of many others in the queue... we can't shower everyone who submits one with praise, it is not a good use of our limited time. And in spite of you not submitting your code in the most ideal form possible (a diff), you had 4 replies already. Your issue hasn't even been up for a whole day. Most of us do this in our spare time, so this is a succesful response. The fact is, Drupal has high standards to keep up. If people point out how your patch does not conform to those, it means they would probably accept it if it did. It is a good sign, trust me. Your change automatically uses eaccelerator/mmcache if they are present, without providing an option to the administrator for this. Is this a reasonable thing to do? I'm not sure how widespread they are, but it is these sort of things which can cause sites to suddenly break when the host's configuration changes. Now as far as the patch goes: * You still call db_encode_blob() and db_decode_blob() even when using the alternate storage mechanism. This is a specific form of encoding suited for the current database type, so it is unnecessary. * All data passed to cache_set must be a string already, so serialization is unnecessary. * You convert CACHE_TEMPORARY to 1 (second?). Is there a reason for using this and not 0 (that would seem more logical to me). The code does not conform to the code style in much more ways than just one name. It looks messy and unorganised. * You do the check for 'function_exists' twice. You can easily re-arrange things to avoid this. * The if statements for checking the type of $expire passed would look much cleaner expressed as a switch statement. * We put spaces after control flow statements (if, switch, while, etc). * We put spaces around the concatenation operator, except between a space and a quote. This is a rule which many forget, but it strikes the perfect balance between keeping code readable, and keeping code compact. * We prefer single quoted strings to double quoted strings, except when it would reduce readability (e.g. when there are single quotes inside). * We prefer proper typography for comments (ending with a period if it is a full sentence) and a space between the comment's marker and its contents. * Having a clean code style keeps the code easy to read, easy to scan visually and easy to search programmatically. By respecting these ideas, any future contributions you make will have a higher chance of being accepted. Please take this as constructive criticism, not a sign that we dislike you or your contribution. ------------------------------------------------------------------------ June 4, 2005 - 15:04 : chx First, don't think that Dries does not want you to contribute, but this has been discussed already. However, I have some concerns. Natrak installed eAccelerator on Drupal at one point and it segfaulted Apache quite often. Natrak, if you read this please comment! ------------------------------------------------------------------------ June 4, 2005 - 17:10 : TDobes chx: I have been using PHP5 + eAccelerator 0.9.2a (and just upgraded to 0.9.3) for a few months on a couple of my Drupal sites without any trouble. My sites certainly aren't as large or as high-traffic as Drupal.org, though. (I'm not sure whether it matters, but I'm using lighttpd instead of Apache.) Anyway, it's possible that a lot of eAccelerator bugs have been fixed recently. ------------------------------------------------------------------------ June 6, 2005 - 07:37 : Kjartan eAccelerator has problems with sites that have a high load and at some point will screw up on randon pieces of PHP code and cause segfaults till apache is restarted. From what I last heard they are making progress and probably has something to do with locking and eAccelerator not being fully thread safe. Zend Optimizer seems to work without problems, so using that till something better comes up. Also just quickly looking at this code wouldn't the cache size theoretically be small than when using MySQL? Admins can set a limit on shared memory storage, from the docs on eaccelerator_put it seems that once its full you can't store any new data (function returns false). On Drupal.org I usually see the cache table to be anywhere from 20 to 80 megs, not sure what the default values of eAccelerator are. ------------------------------------------------------------------------ June 6, 2005 - 14:16 : walkah KJ - true, but we're seeing much better behaviour out of eaccel 0.9.3 .. Yerlec - please don't be put off by such comments, call it "constructive criticism". I'm interested in working on this as well, but agree that making this eaccel / turck specific is probably less desirable as there are several different options for caching , and ideally we'd like a framework to accomodate as many as possible. ------------------------------------------------------------------------ June 7, 2005 - 10:36 : Yrlec Steven, I didn't expect a "shower of praise", I had just hoped for a bit more positive attitude. Take walkah as I good example of how I had hoped you would all be like; you don't have to be negative to come with some constructive input. I had also hoped you would spend less time on rather subjective nitpicking. You have some very valid arguments but they sort of disappear when you start to argument on why my code looks messy and unorganised because of a missing space between the comment slashes and the comment. I also find it quiet ironic that you spend so much time explaining that this is your spare-time why you don't have time to answer. A simple thanks instead would have saved both you and me a lot of time. To save us all some time I will not argument against the rather subjective and unrelated points but instead try and answer the questions which I find valid. * Your change automatically uses eaccelerator/mmcache if they are present, without providing an option to the administrator for this. Is this a reasonable thing to do? Sounds like a good idea to implement some kind of option to disable it. * You still call db_encode_blob() and db_decode_blob() even when using the alternate storage mechanism. This is a specific form of encoding suited for the current database type, so it is unnecessary. Ok, I'll remove them. * All data passed to cache_set must be a string already, so serialization is unnecessary. Same there, I'll remove them. However eAccelerator's documentation claims that it can segfault if you don't serialize. * You convert CACHE_TEMPORARY to 1 (second?). Is there a reason for using this and not 0 (that would seem more logical to me). If you read your code you will see that CACHE_PERMANENT is set to 0 so converting CACHE_TEMPORARY to 0 would be quiet stupid, wouldn't it? Also if you read eAccelerator's documentation you see that 0 is equivalent to "never". Which I assume means that it never "dies". * You do the check for 'function_exists' twice. You can easily re-arrange things to avoid this. Sure, I'll fix that. * We prefer single quoted strings to double quoted strings, except when it would reduce readability (e.g. when there are single quotes inside). Sure, I'll fix that. * Having a clean code style keeps the code easy to read, easy to scan visually and easy to search programmatically. Is that why you have your opening and closing brackets on different horizontal levels, which forces the eye to move both vertically horizontally to match two brackets? Sorry for being sarcastic but honestly, you call my code unorganised when your caching mechanism is placed in a file called bootstrap.inc? Let he who is without sin... ------------------------------------------------------------------------ June 7, 2005 - 12:02 : Junyor Yrlec: Dries was being constructive. By posting his comments, he was implying that he wanted you to continue working on this patch and gave you concrete suggestions that would help your patch get integrated into core. One of the reasons Drupal's code is clean and consistent quality control from the core team. Instead of having the coding style of hundreds of developers, there's one consistent code style for all of core. In other words, let's get this cleaned up and into core!