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.
Comment | File | Size | Author |
---|---|---|---|
#7 | drupal-drupal_attributes-961908-7.patch | 643 bytes | mikeytown2 |
#6 | 961908_drupal_attributes_d6.diff | 1.03 KB | dalin |
#3 | 961908_drupal_attributes.diff | 808 bytes | Damien Tournoud |
#1 | 961908_drupal_attributes.diff | 807 bytes | dalin |
Comments
Comment #1
dalinWithout the patch:
with the patch:
Which seems to confirm the .5% performance gain.
Comment #2
dalinComment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat 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.
Comment #4
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #5
catchLooks like this could be backported to D6. Nice patch!
Comment #6
dalinWell with the back-ported version, if you misuse the function you get odd results:
Which probably isn't a big deal.
Comment #7
mikeytown2 CreditAttribution: mikeytown2 commentedRe-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.