When we upgraded from 6.x-2.9 to 6.x-3.x-dev I noticed a severe drop in page performance. We used 1 nested fieldgroup and page generation time shot from 400-500 ms to 900-1100 ms.

I'm not sure if this is a known issue but I assume you'd like the feedback.

We've downgraded again to 6.x-2.9 and performance is restored.

Kind regards!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rgarand’s picture

Priority: Normal » Major

I've seen this on two sites - they are unusable with cck 3 but work with 2.9. I did some profiling and found close to 1m calls to the same functions when loading a page (I believe it was going around between content_get_nested_elements and element_children to unpack recursive data). This was with a lot of fields and groups, but there is still a huge difference in performance. I haven't seen if it's very noticeable with less fields.

slybud’s picture

I'm jumping into this thread because it seems unactive whereas it is very important.

Since nested fieldgroups (nice feature btw) appeared in cck3, we've been experiencing huge performance loss on production sites, with content types using many cck's and groups (maybe 30 fields on content types), with some build modes...

cachegrind also lead us to this guilty function : content_get_nested_elements(), which is called recursively a zillion times.

This should really be a major concern for the performance gap is huge and can't be optimized.

slybud’s picture

PS : we've been using cck3 on production sites for 2 years, it's the first time I'm beginning to consider using cck2.
Losing multigroups is nothing compared to a slow site and there is nothing you can do about that with all the servers in the world

DeFr’s picture

Ok, so, here's what's going on, and why it's slow:

  1. By design, content_get_nested_elements needs to traverse all of the array it got as an argument to find all the possible matching keys tha
    t are somewhere in the array. In the case that matters, the calls that occurs during the node rendering, the array that goes in is $node->content, which will contains at least N entries, one for each field.
  2. Right now, during the rendering, content_get_nested_elements is called for every field associated to the content type, at least two time per fields by content_fields (once in the 'alter' case, and another time in the 'preprocess node' case). So the function is directly called 2N times
  3. The function is recursive, and function calls in PHP are slow.

In summary, the code is using a slow process for something that's at least O(N²), which smells like trouble.

A few things to note:

  • Even if you exclude the field and set it's formatter to hidden, using the Managed display for you node type, the field informations are added to the $node->content array, which means that you can't limit the impact this way.
  • If the field is in a group, the location of the field in the array will have changed between the two content_get_nested_elements call.
  • A few content_get_nested_elements caller don't really know how to deal with the fact that they may be returned more than one elements, and use array_shift to get only the first result (see content.node_form.inc and content_multigroup.node_form.inc)
  • Given that the recursion happens over the whole array, the nesting deepness of the field doesn't really matter. Even if you don't have any fieldgroup on your node, the performance will be the same.

I'm not really clear on the use cases that would legitimately make content_get_nested_elements return more than one result, I guess the goal is to be able to use a field on more than one place in the node view output ?

Currently, I don't see a way to work around this without tweaking something at the API level. Here are a few way forwards I see:

  1. Change the API to turn content_get_nested_elements into content_get_nested_element, which will only return the first result it finds. That would allow the function to avoid recursing everytime over the whole array. For optimal performance, the algorithm should also change from DFS to BFS, and then one just has to be carefull not to nest the field too deeply to get acceptable performance.
  2. Change the API to make content_get_nested_elements accept an array of field_name, and return an array of elements' array. Then, instead of walking over the whole array 2*N times, "only" do it 2 times, and use this traversal to find at the same time all our elements.
  3. Change the API more radically, and ask modules that change the field location to call something like content_set_element_location($field_name, $parents). Then, use those informations to go straight at the element, and bypass every search.

If we go with either solution 1 or 2, I think that tweaking the algorithm to be non recursive, using a table queue, could help a lot.

I'm willing to work on a patch, but, we need to get a consencus on what the best plan of action would be first.

For the record, right now, on a rather high end server with ~40 fields, the rendering of a node takes ~1.3s, which means that we're at 13s for rendering a list of 10 teasers.

yched’s picture

Priority: Major » Critical

Sounds critical indeed. The content_get_nested_elements() function definitely seems suboptimal

I'm not familiar with the code and features in branch 3.x, and I'm not 100% sure of the right fix. 2 possible directions for optimisation :

1- Stop scanning the whole render array once we found one match - I'm not sure there really are cases where the $field_name entry would be found in several spots in the array.

That means changing the content_get_nested_elements() function to :

function &content_get_nested_elements(&$form, $field_name) {
  foreach (element_children($form) as $key) {
    if ($key === $field_name) {
      return array(&$form[$key]);
    }
    elseif (is_array($form[$key])) {
      $nested_form = &$form[$key];
      if ($sub_elements = &content_get_nested_elements($nested_form, $field_name)) {
        return $sub_elements;
      }
    }
  }
  return array();
}

(the function still returns an array so that we don't break the calling code)

This might break multigroups, though, dunno, didn't test. PLEASE TEST :-)

2- When recursing on sub parts of the render array, only look within actual possible containers (i.e fieldgroups or multigroup containers), instead of . The best way to do this would probably be to collect the list of groups names and replace the elseif with :

elseif (in_array($key, $possible_group_names)) {
DeFr’s picture

Well, Multigroup is in the "don't know what to do with multiple values, so I'm just gonna use array_shift to get the first one"-camp, so I'm pretty sure it's not the use case that would be broken by the return early case. ;-) That's pretty much one of the problem though, I can't really figure out what would legitimately put more than one instance of the field in the array, which make it hard to test.

I didn't want to post it earlier, because of not knowing what would break, but since you starded, I think I should mention we have been playing on a development server with something along the lines of your proposed patch in #1, with a slight tweak to make the first run go faster: we explicitely check for isset($form[$key]) outside the loop, which gives:

function &content_get_nested_elements(&$form, $field_name) {
  if (isset($form[$field_name])) {
    return array(&$form[$field_name]);
  }
  foreach (element_children($form) as $key) {
    if (is_array($form[$key])) {
      $nested_form = &$form[$key];
      if ($sub_elements = &content_get_nested_elements($nested_form, $field_name, $parents)) {
        return $sub_elements;
      }
    }
  }
  return array();
}

It gives a notable performance improvement (~35%), but it still feels like it could be improved even further: the array will still be walked deeply untill the value is found, which could be avoided by switching to a BFS algorithm. And I'm still not sure there's not something that it breaks horribly :-)

Hurm, in fact, writing this comment, I've just wrote

function &content_get_nested_elements(&$form, $field_name) {
  $queue = array();
  $queue[] = $form;
  while ($current = array_shift($queue)) {
    if (isset($current[$field_name])) {
      return array(&$current[$field_name]);
    }
    foreach (element_children($current) as $key) {
      if (is_array($current[$key])) {
        $queue[] = &$current[$key];
      }
    }
  }
  return array();
}

which would be a BFS search that does seems to be another improvement that cuts down the rendering time by ~65%, but this /does/ break unlimited value, I think the reference get dropped in array_shift.

KarenS’s picture

The use case for finding more than one instance of the element are places where we used it to do things like alter all the 'nid' sub-elements of a field (to do things like disable those form elements). In that case we needed to find the field in the form, then iterate over that, keeping the references intact, to find all the 'nid' children and alter them. I don't think CCK is using that kind of example anywhere, but that's one of the ways I've seen it used.

Another alternative, since this function has been in the wild for a year or so already and changing it in any way will definitely break production sites that I know are using it. Leave this function alone, but create another more efficient one and just worry about making it as efficient as possible for the ways it is being used by CCK, with no worries about whether it works for anything else, then change all internal CCK use to refer to that more efficient function.

jstnhffmn’s picture

The array_shift manual notes that "All numerical array keys will be modified to start counting from zero." Are these steps necessary here? Perhaps a function can be written to do everything but renumber the array. (Just an idea to reduce the number of possible steps.)

Another idea: What if the $form element was serialized and string parsing was done instead of recursion?

Serialized output follows this pattern:

type_of_index:str_len(index):"index";type_of_content:str_len(content):"content";

Where type_of_index or type_of_content is 's' for string, 'a' for array, 'i' for integer, etc.

For example, if the parameter $field_name was equal to 'test', then you would first look for the string 's:4:"test";'. If this string is found then it is followed by something like 's:50:a:2{string of length 50};'. There is not a lot of complex parsing to be done here. And if you want to look through the child elements then you simply parse the substring. You should be able to unserialize these substrings into the elements you need.

Is string parsing any faster? I'm not an expert; more an idea man!

pacufist’s picture

Is it a good idea to create an index for $form ? We can build flat array once, and than use it each time.

I don`t know algorithms to build such indexes for hierarchical arrays, but we can start from this:

<?php

function &content_get_nested_elements(&$form, $field_name) {
  $elements = array();

  if(!isset($form['#content_nested_elements_index'])){
    $index = array();
    content_build_nested_elements_index($index, $form);
    $form['#content_nested_elements_index'] = &$index;
  }

  if(isset($form['#content_nested_elements_index'][$field_name])){
    $elements = $form['#content_nested_elements_index'][$field_name];
  }

  return $elements;
}

function &content_build_nested_elements_index(&$index, &$form) {

  foreach (element_children($form) as $key) {
    $index[$key][] = &$form[$key];
    if(is_array($form[$key])) {
      content_build_nested_elements_index($index,$form[$key]);
    }
  }
 
}

?>
DeFr’s picture

@Pacufist in #9: The problem is that the elements can (and realy does if we're talking of a field in a fieldgroup) move between the different content_get_nested_elements calls for the same name, so the index needs to be invalidated... at a point that's not easy to discover from inside the function, because most of the caller of content_get_nested_elements in the critical node rendering path actually use the fact that the return value is by reference to tweak the array.

@jstnhffmn in #8: array_shift in fact doesn't return a reference, so, a custom function that does the same thing needs to be written, of which the renumbering can be omitted easily enough. That's what I have locally, but it breaks content multigroup multiple value handling for some reason I haven't yet figured out.

Wrt serializing the array, I've just played a bit with it, and simply serializing the array (without trying to find anything in it or extracting it) is roughly 2.5 times slower than walking the first few levels of the array and returning the first match ; that's because the serialization needs to take the whole array into account, and it's rather deep and contains whole copies of the node object at various place stored in '#node'; manually walking the object and filtering the keys with element_children bypass those #node properties, but the serialization process needs to export it. It also means we would get false positives, because of the corresponding entries in the node object.

DeFr’s picture

Status: Active » Needs review
FileSize
4.62 KB

So, I decided to take the opportunity of a long train ride to explore the solution 2 I mentionned in #5, which was to basically walk the whole array only once per operation type, instead of once per operation type per field, and the result are rather encouraging.

Benchmark is

$n = node_load(3);
timer_start('view');
for ($i = 0; $i < 10; $i++) {
  $node = clone $n;
  node_view($node);
}
var_dump(timer_stop('view'));

running on a netbook, using only core + cck, with a specially crafted Story content type that got 40 text fields added with content_field_instance_create.

The benchmark is launched with drush php-script.

The goal of the loop is both to try to round up the measurement a bit, and to simulate what the performance impact on a default frontpage with 10 teasers might be.

All that being said, here are the results:

Before patch: 17400 ± 600ms
After patch: 1570 ± 40ms

So, we're talking about taking about 10x less time on node views, without changing the contract of always
returning all the matching elements in the array.

As is, I'm not completely fond of the need to hook into _content_field_invoke_default and add an additional optionnal parameters to content_field, but that seems like the last logical place to have something that needs to operate on all fields (the other approach would be to have static variables in content_field for $op = 'alter' / 'preprocess_node', and make the call for all the fields when the variable is still NULL)

I've checked that both unlimited fields and content multigroup still seems to work after the patch is applied.

Still left to do if we got with this is tweaking the function doc' to make it obvious that an array of name can be given as the second argument (and that you really should do it whenever you can) and maybe tweaking the other callers of content_get_nested_elements in CCK to use this pattern. Before doing that though, I'd rather check if this approach is going to fly with you, CCK maintainers :-)

In the misc improvements area, the code could probably be made more D7-ish by having a new content_get_nested_elements_multiple. I have that in a local branch in git, but somehow the performance regresssed (1570ms => 1780ms for the benchmark) and I haven't figured out why yet, so, I'm attaching a patch of the last known good commit.

rgarand’s picture

@DeFR: that sounds very promising :) On the test it might be useful to load and view 10 different nodes since a view could easily do that.

DeFr’s picture

For the record, the patch in #11 breaks the "add more" button of multigroups when they are nested in a fieldgroup; the patch changes the order in which the array is walked, and somehow the last form_builder call in content_multigroup_add_more_js adds another key in the $form array with the name of the $group. Then

$group_form = array_shift(content_get_nested_elements($form, $group_name)); (content_multigroup.node_form.inc L834)

get the newly added entry instead of the one that really should be rendered, and things break. An easy fix is to switch the array_shift to an array_pop, but I'd rather stop the guessing of which value should be taken if possible.

roball’s picture

Title: Poor performance nested fieldgroups » Poor performance of nested fieldgroups
Status: Needs review » Reviewed & tested by the community

Excellent DeFr!

Your patch magically did it! I have successfully applied it to 6.x-3.x-dev (2011-Aug-02). I have a content type with approx. 1000 single select fields, spread over approx. 250 (nested) groups. Creating or editing nodes of that type was no problem, but trying to view such nodes took some 2 mins and 20 secs to load! After applying the patch, the node load time decreased from 180 to 5 seconds, so it gained a performance boost of the factor 36 :-)

I have not found any side effects yet. I'm not using Content Multigroup so that's no problem for me. Solving this problem is more crucial and this should be addresses asap IMO even if it introduces a small problem to Multigroup.

Thanks again.

Update 2012-01-11:

One day after applying the patch, I have digged into the dblog and found the following PHP errors triggered when a node of a certain content type was viewed:

Invalid argument supplied for foreach() in /etc/drupal6/all/modules/cck/content.module on line 857.
Invalid argument supplied for foreach() in /etc/drupal6/all/modules/cck/content.module on line 892.

However, there is no real problem visible on viewing that node - it displays just fine.

DeFr’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.93 KB

@roball: Thanks for testing and confirming that this patch helped on performance :-) Can you check if those warning also occurs without the patch being applied ? It looks like you have a field that CCK tries to render and which isn't in the $node->content array ; the patch has a fallback, and tries to find it again in the array but that second lookup also seems to fail (which probably means the performance could improve again in your case without that fallback) .

I'm attaching a patch that's quite basically #11 and the fix for the compatibility problem with the Add more button on nested multigroups, and with a small phpdoc for the new content_shift function.

Dear CCK maintainers, I would welcome any comment on either the global approach or the details.

roball’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
63.26 KB

Thanks DeFr. The two error messages mentioned at #14 definitely only occur while the patch is applied, but only when loading a node of a certain content type. I have attached the Export data of that type ("event") for you to import and try.

I am totally sure because I first loaded one node of that type while the patch was applied, and observed these 2 errors. Then I reverted the patch and reloaded that node. No more errors. Finally I applied your new patch from #15, reloaded that node again, and the 2 errors were there again.

However, your new patch still works fine on the performance boost. Thus I am changing the status of this issue to RTBTC. But I cannot say how it works with multigroups. Can this please test someone else?

Feng-Shui’s picture

Thanks very much all, just ran into this problem today, views started crawling and tracked things down to content_get_nested_elements.

Patch has applied cleanly and wall time has dropped by 90%. Have a site with a lot of Display Suite and a few multi-groups, things seem to be happy, will keep testing and report back any issues.

Running 6.x-3.0-alpha3

Thanks!

mshick’s picture

This brought several sites I work on to a crashing halt. Cachegrind got me on the track to finding this thread, seeing the content_get_nested_elements() function invoked something like 70000 times on a single page load. The patches here solved the problem for me.

Devs, please get a fix like this into the project. This is a killer.

roball’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
12.55 KB

Patch is working fine, however the problem mentioned at #14 is somehow annoying. Now these errors occur on all my sites. On one when a node of the attached content type is viewed.

DeFr’s picture

Status: Needs work » Needs review
FileSize
6.01 KB

I've deployed the attached patch on a rather big production site that suffered from this problem.

@roball, can you check that this version of the patch fixes your problem ? It should also improves a bit more the performance.

roball’s picture

Status: Needs review » Reviewed & tested by the community

Fantastic - patch in #20 solves that problem and gains good performance. Thanks a lot!

slybud’s picture

+1 for the rtbc.

this patch was deployed on big production sites (one of France's leading radio site) suffering from this major performance downsizing => boost performance gained with no side effects

Everyone happy : customer, sysad, cachegrind...

roball’s picture

Any comments from the CCK developers on this critical issue?

alippai’s picture

RTBC++

yched’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
5.54 KB
6.54 KB

The approach is not ideally elegant, but given the BC constraints and my lack of familiarity with the 3.x branch, this is more than good enough for me. Kudos & many thanks @DeFr for attacking this, and thanks all for the feedback.

I made a couple changes (added comments, refactored/streamlined a bit, renamed comment_shift() to a helper _func), and committed the attached patch (interdiff with #20 attached).

I could use some confirmation that my changes didn't break anything before rolling a new release, though.

roball’s picture

CCK 6.x-3.0-alpha3+9-dev (2012-Apr-15) - which includes that commit - does work well for me. Thanks for committing this patch.

DeFr’s picture

Thanks a lot to you for commiting this @yched !

The patch that was commited looks fine, there's only one little side-effect that was maybe not planned: using array_fill_keys means that the code won't work on PHP < 5.2. That's fine on D7 where it's part of the core requirement, but that's not the case for D6. Given that all the versions up to PHP 5.2 were deprecated by upstream, that's probably not a big deal, but maybe adding at least a php=5.2.0 to the .info would be a good idea ?

markus_petrux’s picture

Status: Fixed » Active
FileSize
622 bytes

Sorry to re-open, but the latest version of the patch is broken, because it does not return references to the elements found.

Keeping this critical because it has broken all our custom form alters. :_(

Please, see attached patch. Thanks!

DeFr’s picture

Status: Active » Needs review
FileSize
664 bytes

Argh, sorry not to have catch this while reviewing the interdiff.

Confirming both the bug, which is critical because it's not only breaking custom formatters but also all the "Add more content" multigroup buttons and the fix.

The patch is missing its header informations though, which means that patch will refuse to apply it. Attaching a mini re-roll including it. Switching to Needs Review :-)

markus_petrux’s picture

Thanks! :)

One (rudimentary) method to test this:

$array = array(
  'item-0' => array(
    'item-0-0' => array(
      'item-0-0-0' => array(0),
      'item-0-0-1' => array(0),
    ),
    'item-0-1' => array(
      'item-0-1-0' => array(0),
      'item-0-1-1' => array(0),
    ),
  ),
  'item-1' => array(
    'item-1-0' => array(
      'item-1-0-0' => array(0),
      'item-1-0-1' => array(0),
    ),
    'item-1-1' => array(
      'item-1-1-0' => array(0),
      'item-1-1-1' => array(0),
    ),
  ),
);
$key = 'item-1-1-0';

print '<pre>$array:' . check_plain(var_export($array, TRUE)) . '</pre>';

$elements = &content_get_nested_elements($array, $key);
print '<pre>$elements:' . check_plain(var_export($elements, TRUE)) . '</pre>';

if (!empty($elements[0])) {
  $elements[0][0] = 1;
  print '<pre>$elements:' . check_plain(var_export($elements, TRUE)) . '</pre>';
}

print '<pre>$array:' . check_plain(var_export($array, TRUE)) . '</pre>';
yched’s picture

Status: Needs review » Fixed
FileSize
6.6 KB

OK, sorry about that. Fixed both #27 (good call on array_fill_keys) and #28-#29. Thanks folks !

Attached is the cumulative patch for the changes that got in for the issue (#25 + fixes)

DeFr’s picture

@yched: The commit doesn't seem to have been pushed to the git repository yet, I guess it's normal and you're testing a few other patches before pushing them all ?

yched’s picture

right, pushed now.

cdale’s picture

Status: Fixed » Needs work

This patch has introduced a regression as while ($current = &_content_shift($queue)) { will evaluate to false when the current item in the queue is an empty array, which causes content_get_nested_elements() to return before queue has been fully processed.

Not sure which approach to solve this is preferred, but one could do any of the following to fix this.

while ($current = &_content_shift($queue) || !empty($queue)) {

while ($current = &_content_shift($queue) || (is_array($current) && !empty($queue))) {

while (!empty($queue)) {
  $current = &_content_shift($queue);

Are there any preferences here? Can provided a patch if required for whatever option is desired.

DeFr’s picture

Status: Needs work » Needs review
FileSize
800 bytes

Instead of checking the element when dequeuing it, I think it would look nicer to check that the array isn't empty before adding it to the queue. Simply changing the else if (is_array($current[$key])) { to else if (!empty($current[$key]) && is_array($current[$key])) { should do. Patch attached.

cdale’s picture

Status: Needs review » Reviewed & tested by the community

That works too. Kicking myself that I didn't even consider that. It's a bit obvious hey.

Patch in #35 solves the problem I was facing. Let's get it in.

markus_petrux’s picture

Status: Reviewed & tested by the community » Needs work

hmm... #35 seems to be using a version of content.module that is not up to date ¿?

DeFr’s picture

Status: Needs work » Needs review
FileSize
813 bytes

Urgh, sorry, applied the patch on the wrong git tree, and that one was lagging behind, without the $element => $current[$key] correction. Patch against lastest git code attached.

yched’s picture

Status: Needs review » Fixed

Committed. Thanks, nice catch !

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

roball’s picture

May we expect a new alpha or even a beta release of 6.x-3.x so we don't have to rely on dev snapshots to get a critical bug fixed?

jstnhffmn’s picture

Is there a version of CCK for Drupal 6 that I can install that resolves the issues in this thread as well as http://drupal.org/node/800314? (I have the alpha version with various patches applied that I've come across.)

Does my version of Drupal (6.19) need to be updated to work properly with the most recent CCK module?

roball’s picture

@jstnhffmn: the latest 6.x-3.x-dev snapshot - which is 6.x-3.0-alpha3+13-dev (2012-May-04) - works fine for me solving this issue. BTW, it would be useful to have a new alpha (6.x-3.0-alpha4) released.

The issue #800314: FileField disappears after upload using Multigroup must be fixed in the FileField module, not in CCK.

And you should always use the latest official D6 version (currently 6.26) before posing D6 problems into the issue queue.

sblommers’s picture

#38 doesn't apply cleanly from drupal.org after #31.
1) apply #31
2) apply this take from #38 :

roball’s picture

@sblommers: You do NOT need to apply any patch - the code has already been committed. Just download http://ftp.drupal.org/files/projects/cck-6.x-3.x-dev.tar.gz and you are fine.

sblommers’s picture

@roball: We use cck in production and install everything using drush, we cannot afford using dev versions. The latest release is alpha_3 and the patches are needed until a new version is released.

@maintainer: please release a new version, something like alpha4.

roball’s picture

sblommers, sure I would also like to have a new release like 6.x-3.0-alpha4. Maybe it helps posting into #1171518: Please release CCK 6.x-3.0-alpha4.