[drupal-devel] [feature] Improved caching with eAccelerator/turck mmcache

[Steven]

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:   Steven
 Status:       patch

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.




Steven



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.