\Drupal\Core\Render\Renderer::doRender() could be lazier about calling Element::children()

Will profile this tonight, planning on testing 50 comments.

Seems like quite a nice performance boost, let's get this in! Great find @thedavidmeister.

Scenario: Node page as anonymous user with 50 threaded comments displayed. Bartik theme.

=== 8.x..8.x compared (51afe99cebff9..51afeaa2e9325):

ct  : 233,757|233,757|0|0.0%
wt  : 985,069|984,888|-181|-0.0%
cpu : 953,525|953,873|348|0.0%
mu  : 33,683,624|33,683,624|0|0.0%
pmu : 34,205,392|34,205,392|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51afe99cebff9&...

=== 8.x..lazy_children-2012800-1 compared (51afe99cebff9..51afeb006b56a):

ct  : 233,757|232,845|-912|-0.4%
wt  : 985,069|981,646|-3,423|-0.3%
cpu : 953,525|948,915|-4,610|-0.5%
mu  : 33,683,624|33,682,864|-760|-0.0%
pmu : 34,205,392|34,192,296|-13,096|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51afe99cebff9&...

So the thing is is that element_children makes changes to $elements as it passes in the the value by reference and does all sorts of sorting...

So in order to put this in we need proof that nothing needs the changes made by element_children() to $elements.

The green test run in #1 is probably not enough because not many of our tests actually test the order that things appear on the page.

Ah, very good point @alexpott.

…and looking at this again, if nothing else needs the sorting, we could make this:

foreach (element_children($elements, TRUE) as $key) {
  $elements['#children'] .= drupal_render($elements[$key]);
}

But this needs more research and testing.

Most of them are in now. What else should we be waiting on at this point?

element_children() no longer exists. See #2226621: Remove usage of element_info(), element_child() and element_children(). Deprecate element_info_property().

The code still exists, just both in classes now.

Tried to change the title, trying again.

FYI the functions element_child() and element_children() are completely gone from core, it is NOT just the usage that was removed. There's no change notice about this, so you have to grep the code base to confirm, or you have to have code that uses them which was completely broken when the core change was pushed ... :-(

This is the change notice for the deprecation:
https://www.drupal.org/node/2173683

Here's the removal:
https://www.drupal.org/node/2443805

Which probably needed it's own change record or something?

Yeah, major API changes pushed without a change record constitutes sociopathic behavior IMO. In particular anything theme-related, since the Twig stuff is cached on disk so a change like this brings a D8 site down hard (yes, I can turn off this caching, but sometime I forget when I'm doing say the 10th core re-install of the week). I know we don't support beta-to-beta upgrades yet, but really how hard is it to give fair warning that pulling a core commit will require a core reinstall?

Anyway, sorry for the off-topic rant, just wanted to give notice to anyone working on this issue that element_children() is no longer around.

I agree it should've had a CR and it still could. Maybe you can write up the CR on that issue that removed it and we can still publish it so people are aware.

I'm sure the change was made with the best of intentions, many function removals are to allow for proper unit testing. And a bit of a battle between beurocracy vs the pressure to deliver makes it easy to have slips in some areas and CRs and documentation are in need of help as they are the first to be forgotten. So hopefully you can help out in this area?

Maybe we can do this for 8.1.x

Also, this issue shouldn't need a CR as it should be a purely internal change to optimize performance (not changing any logic).

8.7.x branch has many changes. Changes for this patch now are here core/lib/Drupal/Core/Render/Renderer.php:448
and if I correct understand we don't need this issue anymore.

Re-rolled.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Attached patch against Drupal 10.1.x

Patch #27 is not applied for Drupal 10.1.x so Inter-diff file is not added.

Checking patch core/lib/Drupal/Core/Render/Renderer.php...
error: while searching for:
drupal_process_states($elements);
}

// Get the children of the element, sorted by weight.
$children = Element::children($elements, TRUE);

// Initialize this element's #children, unless a #pre_render callback
// already preset #children.
if (!isset($elements['#children'])) {

error: patch failed: core/lib/Drupal/Core/Render/Renderer.php:404
error: core/lib/Drupal/Core/Render/Renderer.php: patch does not apply

This feels more like a feature request to me. Or something that should least have test coverage.

This was profiled 10 years ago but with the new system wonder if it should be profiled again?