Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Looking through core cachegrinds, I noticed that every call to drupal_render() calls drupal_process_attached() and drupal_process_states() - these functions then only operate if (!empty($elements['#attached/states'])).
Since those functions are almost exclusively called by drupal_render(), apart from the one place where drupal_add_library() sets #attached itself then calls drupal_process_attached() immediately afterwards, it makes sense to move the if statement into drupal_render() itself, and save ourselves a couple hundred function calls on most pages.
I don't expect this to have any effect in benchmarks, maybe a tiny one with micro benchmarking, but neither can it cause any harm, and it removes profiler noise.
| Comment | File | Size | Author |
|---|---|---|---|
| drupal_render.patch | 2.06 KB | catch | |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedIMO, this adds noise for people who read drupal_render(). Also, it isn't really drupal_render()'s business to write that condition. That belongs in the process function IMO. Unless benchmarks prove we want this, I vote a small -1.
Most profiler reports like webgrind let you omit functions that take less than x% of request time so I bet the profiler noise issue is easily mitigated.
Comment #2
catchI did microbenchmarks before/after and there's no measurable difference - at least not with a basic array('#prefix' => 'foo', '#markup' => 'foo', '#suffix' => foo), so this is purely a matter of taste.
However if we followed this pattern everywhere, all our functions would have if (empty($var)) { return; } up top - I think it's better for the caller to be responsible. I also think we have to get out of this mindset of "it's just an extra function call" for critical path functions - look at the check_plain() issue.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedWhy would we gum up every caller when we can only gum up the function which actually has the knowledge of whats supposed to be empty. Putting the empty inside the function is better encapsulation. Seems to me that empty inside the function improves readability and encapsulation.
In my taste, we only have to get out of that mindset when benchmarks make it worthwhile. Otherwise, readability prevails.
We're at a stalemate. Need more opinions.
And, many many thanks for working on performance.
Comment #4
chx CreditAttribution: chx commentedThere is no benefit and it violates encapsulation.
Comment #5
Crell CreditAttribution: Crell commentedExcept that we have many other instances where moving a check outside of a function gives us a small boost. We even just committed one: #619566: Don't invoke hook_url_outbound_alter() if there's nothing to invoke
Comment #6
chx CreditAttribution: chx commentedOK, I am convinced
that's the same. drcs starts with a huge if and could do this check but we decided to check in drupal_render instead. That seems to be a pattern in drupal_render anyways even if the other property checks are not performance. It's not something I would be happy to do in other places but drupal_render already has this "if property call function" pattern.
Comment #7
casey CreditAttribution: casey commentedadded tag
Comment #8
catchDon't mean to play tag pong, but there's zero measurable performance impact here, so trying to keep the performance queue tidy.
Comment #9
webchickI went ahead and committed this. It seems like it cleans the code up slightly, though I know Moshe isn't nuts about it. chx points out that this is consistent with how we handle this in other places.