drupal_attributes() is within the top five functions of total self cost on any given page (along with php::PDOStatement->execute, theme, and drupal_render). This is another place where we are using is_array() where a more performant (array) cast will improve performance.

 function drupal_attributes(array $attributes = array()) {
   foreach ($attributes as $attribute => &$data) {
-    if (is_array($data)) {
-      $data = implode(' ', $data);
-    }
+    $data = implode(' ', (array)$data);
     $data = $attribute . '="' . check_plain($data) . '"';
   }

I also attempted the following to see if there is significant time being spent rewriting $data.


 function drupal_attributes(array $attributes = array()) {
   foreach ($attributes as $attribute => &$data) {
-    if (is_array($data)) {
-      $data = implode(' ', $data);
-    }
-    $data = $attribute . '="' . check_plain($data) . '"';
+    $data = $attribute . '="' . check_plain(implode(' ', (array)$data)) . '"';
   }
   return $attributes ? ' ' . implode(' ', $attributes) : '';
 }

I then profiled each scenario (drupal core with all modules enabled, all blocks enabled, 10 nodes on the front page, block and page cache off) and compared self cost of drupal_attributes().

original
4.28%
4.62%
4.36%

cast to array
3.63%
3.97%
3.76%

all on one line
3.70%
3.75%
3.69%

I then switched back and forth a few times to confirm. It's not clear that all-on-one-line has any performance benefit, and there is a loss of readability. But there does appear to be about a half percent improvement in casting to an array.

I'll do a benchmark with ab, and upload a patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dalin’s picture

Status: Active » Needs review
FileSize
807 bytes

Without the patch:

$ ab -n100 -c5 http://drupal-head.local/
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking drupal-head.local (be patient).....done


Server Software:        Apache/2.2.12
Server Hostname:        drupal-head.local
Server Port:            80

Document Path:          /
Document Length:        42422 bytes

Concurrency Level:      5
Time taken for tests:   16.092 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      4286900 bytes
HTML transferred:       4242200 bytes
Requests per second:    6.21 [#/sec] (mean)
Time per request:       804.592 [ms] (mean)
Time per request:       160.918 [ms] (mean, across all concurrent requests)
Transfer rate:          260.16 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.2      0       1
Processing:   578  798 105.6    801    1145
Waiting:      569  780 105.4    780    1133
Total:        579  798 105.6    801    1145

Percentage of the requests served within a certain time (ms)
  50%    801
  66%    838
  75%    855
  80%    880
  90%    931
  95%    970
  98%   1048
  99%   1145
 100%   1145 (longest request)

with the patch:

e$ ab -n100 -c5 http://drupal-head.local/
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking drupal-head.local (be patient).....done


Server Software:        Apache/2.2.12
Server Hostname:        drupal-head.local
Server Port:            80

Document Path:          /
Document Length:        42247 bytes

Concurrency Level:      5
Time taken for tests:   16.025 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      4269400 bytes
HTML transferred:       4224700 bytes
Requests per second:    6.24 [#/sec] (mean)
Time per request:       801.233 [ms] (mean)
Time per request:       160.247 [ms] (mean, across all concurrent requests)
Transfer rate:          260.18 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       1
Processing:   517  797 114.3    799    1076
Waiting:      503  772 112.8    773    1061
Total:        517  797 114.3    799    1076

Percentage of the requests served within a certain time (ms)
  50%    799
  66%    835
  75%    884
  80%    898
  90%    944
  95%    995
  98%   1073
  99%   1076
 100%   1076 (longest request)

Which seems to confirm the .5% performance gain.

dalin’s picture

Issue tags: +Performance
Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
808 bytes

That makes a lot of sense. Sweet, sweet, performance.

I just fixed the code style (we always have a space after the cast), don't credit me. RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

catch’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Looks like this could be backported to D6. Nice patch!

dalin’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.03 KB

Well with the back-ported version, if you misuse the function you get odd results:

echo drupal_attributes('string'); // '0="string"'

Which probably isn't a big deal.

mikeytown2’s picture

Re-roll #6 for git.

Tested this and it gives ~ 2-3ms improvement over stock function; after getting called 175 times on a front page load. So the improvement is there, just a very slight improvement.

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.