Re: [development] coding standard question
1326ms vs. 1382ms is 60ms, and when normal page load times are somewhere around 200ms, it makes a substantial difference.
Even if we disregard the fact that microtime tests have a big stddev,
We shouldn't do that. The posted results are totally worthless without an error estimate.
People, stop wasting everybody's time, read http://drupal.org/node/79237, or preferably an introduction to statistics and a howto on doing experiments in first year phyiscs, then re-do the tests.
imo the tests are pretty pointless. Given that PHP uses the zend hashtable to store all lists of this type (including constants) then this link shows how the table hash key is generated using the DJBX33A algorithm:- http://lxr.php.net/source/ZendEngine2/zend_hash.h#315 Seems to me that upper or lower case won't have an effect on this (not one that microtime is going to perceive anyhow) so this hardly seems worth following up. If you really feel the "need for speed" then the only suggestion is to keep all constant definitions to 8 characters or less. Hardly practical in the real world. --Andy
Hi Andy, This is really interesting. I know very little about the internals of php, but several sites that I read claimed it was less cpu cycles. This is good info. Also, if this is wasting anyones time, please feel free to disregard. You do not need to read nor respond to this. Having a desire to help the project improve performance should hardly be scoffed at or shot down. This is how to discourage people from wanting to help. If my testing is incorrect, please feel free to correct and give information on how to better test. I have spent quite a bit of time on things that many people have felt are "worthless" (Memcache, APC Cache, core cache serialization patch) and have had a good performance impact. It is my area of interest. I apologize if it offends anyone for posting to this list about it. Is there a better place to post about this stuff? Steve Rude andy@spiders-lair.com wrote:
1326ms vs. 1382ms is 60ms, and when normal page load times are somewhere around 200ms, it makes a substantial difference.
Even if we disregard the fact that microtime tests have a big stddev,
We shouldn't do that. The posted results are totally worthless without an error estimate.
People, stop wasting everybody's time, read
http://drupal.org/node/79237, or preferably an introduction to
statistics and a howto on doing experiments in first year phyiscs, then re-do the tests.
imo the tests are pretty pointless. Given that PHP uses the zend hashtable to store all lists of this type (including constants) then this link shows how the table hash key is generated using the DJBX33A algorithm:-
http://lxr.php.net/source/ZendEngine2/zend_hash.h#315
Seems to me that upper or lower case won't have an effect on this (not one that microtime is going to perceive anyhow) so this hardly seems worth following up.
If you really feel the "need for speed" then the only suggestion is to keep all constant definitions to 8 characters or less. Hardly practical in the real world.
--Andy
-- Steve Rude + Lead Developer *AchieveInternet* (800) 618-8777 x 202 http://www.achieveinternet.com
Sorry to add more salt to the open wound, but i am with Steve Rude on this one. when it comes to drupal, we should hang onto every chance to improve performance. even small things like changing upper case TRUE to lowercase. as some have mentioned: in a single request drupal might not have enough such cases to make a difference. but when your site receives a lot of traffic, something as small as this might make a difference.
It won't. Even if the capitalization parsing did trigger a speed hit, the 'worst case scenario' posted earlier in the thread still reveals a small enough difference that it vanishes into the realm of statistical noise. The execution time of a raw chunk of PHP is not where Drupal's problems lie, plain and simple. The problems lie in the memory and database footprint. As an experiment, look at the timing differences, then look at how much time it takes to execute a single SQL query. Think hard on that for a moment. By eliminating a *single SQL query* from your page load, you will save more time than can be saved by all the constant- capitalization-tweaking in the world. Hyper-optimizing for questionable gains due to undocumented idiosyncrasies of a particular language's parser -- especially when there are other well-known speed issues -- is a bad idea. You saw this in eons past with C and other languages, where the 'speed gains' of a switch statement over an if/then/else statement were cited. Those gains vanished as compilers got better, and any gains we'd receive from this capitalization change (if any) would be wiped out by the use of an opcode cache, something that's all but required for any site getting enough traffic to worry about these kinds of incremental gains. Drupal's existing coding standards are geared towards readability and consistency, and the use of TRUE, FALSE, and NULL as capitalized strings is good. I vote for changing coder.module to mirror the defined coding standards. --Jeff
Steve Rude wrote:
I vote for changing coder.module to mirror the defined coding standards.
+1 This was the very point of my original email.
Steve Rude Wow, I don't read the development list for one day, and BOOM!
1. The "performance" review is clearly identified as "under development, use with discretion" 2. The TRUE/true warning is only generated by this "performance review" and not by the "coding standards" review. 3. This performance optimization was submitted by sun (http://drupal.org/node/121388), who had some interesting ideas, but most of which just didn't take hold, the TRUE/true being one of them. The reason that true is supposedly faster than TRUE is that the latter is a define and the former a native constant. Thus when processing TRUE, supposedly, it had to do one additional array lookup in the php C code. However, as many people have pointed out, the time is either minuscule or non-existent. chx says that "php is case agnostic with it's identifiers". I didn't/don't know that this is true. 4. I'll probably remove this rule based on this discussion. I have neglected the coder performance and security reviews. I've considered removing them, but I think they are great ideas and I've been really hoping for more community input (comments and patches)! I was planning on talking about this some at DrupalCon. If you'd like to submit rules patches, please read the documentation: http://drupal.org/node/144172. I think that it's a pretty nice and flexible system. If you know (or can learn) regex's, you can write coder rules. -- Doug Green douggreen@douggreenconsulting.com 904-583-3342 Bringing Ideas to Life with Software Artistry and Invention...
Also, I would like the bikeshed to be blue. --Jeff On Aug 27, 2007, at 7:16 PM, Jeff Eaton wrote:
Drupal's existing coding standards are geared towards readability and consistency, and the use of TRUE, FALSE, and NULL as capitalized strings is good. I vote for changing coder.module to mirror the defined coding standards.
--Jeff
participants (4)
-
andy＠spiders-lair.com -
Doug Green -
Jeff Eaton -
Steve Rude