Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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!
Comments
Comment #1
rgarand CreditAttribution: rgarand commentedI'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.
Comment #2
slybud CreditAttribution: slybud commentedI'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.
Comment #3
slybud CreditAttribution: slybud commentedPS : 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
Comment #4
DeFr CreditAttribution: DeFr commentedOk, so, here's what's going on, and why it's slow:
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.
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:
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:
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.
Comment #5
yched CreditAttribution: yched commentedSounds 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 :
(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 :
Comment #6
DeFr CreditAttribution: DeFr commentedWell, 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:
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
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.
Comment #7
KarenS CreditAttribution: KarenS commentedThe 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.
Comment #8
jstnhffmn CreditAttribution: jstnhffmn commentedThe 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!
Comment #9
pacufist CreditAttribution: pacufist commentedIs 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:
Comment #10
DeFr CreditAttribution: DeFr commented@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.
Comment #11
DeFr CreditAttribution: DeFr commentedSo, 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
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.
Comment #12
rgarand CreditAttribution: rgarand commented@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.
Comment #13
DeFr CreditAttribution: DeFr commentedFor 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.
Comment #14
roball CreditAttribution: roball commentedExcellent 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:
However, there is no real problem visible on viewing that node - it displays just fine.
Comment #15
DeFr CreditAttribution: DeFr commented@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.
Comment #16
roball CreditAttribution: roball commentedThanks 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?
Comment #17
Feng-Shui CreditAttribution: Feng-Shui commentedThanks 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!
Comment #18
mshick CreditAttribution: mshick commentedThis 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.
Comment #19
roball CreditAttribution: roball commentedPatch 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.
Comment #20
DeFr CreditAttribution: DeFr commentedI'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.
Comment #21
roball CreditAttribution: roball commentedFantastic - patch in #20 solves that problem and gains good performance. Thanks a lot!
Comment #22
slybud CreditAttribution: slybud commented+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...
Comment #23
roball CreditAttribution: roball commentedAny comments from the CCK developers on this critical issue?
Comment #24
alippai CreditAttribution: alippai commentedRTBC++
Comment #25
yched CreditAttribution: yched commentedThe 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.
Comment #26
roball CreditAttribution: roball commentedCCK 6.x-3.0-alpha3+9-dev (2012-Apr-15) - which includes that commit - does work well for me. Thanks for committing this patch.
Comment #27
DeFr CreditAttribution: DeFr commentedThanks 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 ?
Comment #28
markus_petrux CreditAttribution: markus_petrux commentedSorry 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!
Comment #29
DeFr CreditAttribution: DeFr commentedArgh, 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 :-)
Comment #30
markus_petrux CreditAttribution: markus_petrux commentedThanks! :)
One (rudimentary) method to test this:
Comment #31
yched CreditAttribution: yched commentedOK, 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)
Comment #32
DeFr CreditAttribution: DeFr commented@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 ?
Comment #33
yched CreditAttribution: yched commentedright, pushed now.
Comment #34
cdale CreditAttribution: cdale commentedThis 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 causescontent_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.
Are there any preferences here? Can provided a patch if required for whatever option is desired.
Comment #35
DeFr CreditAttribution: DeFr commentedInstead 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])) {
toelse if (!empty($current[$key]) && is_array($current[$key])) {
should do. Patch attached.Comment #36
cdale CreditAttribution: cdale commentedThat 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.
Comment #37
markus_petrux CreditAttribution: markus_petrux commentedhmm... #35 seems to be using a version of content.module that is not up to date ¿?
Comment #38
DeFr CreditAttribution: DeFr commentedUrgh, 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.
Comment #39
yched CreditAttribution: yched commentedCommitted. Thanks, nice catch !
Comment #41
roball CreditAttribution: roball commentedMay 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?
Comment #42
jstnhffmn CreditAttribution: jstnhffmn commentedIs 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?
Comment #43
roball CreditAttribution: roball commented@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.
Comment #44
sblommers CreditAttribution: sblommers commented#38 doesn't apply cleanly from drupal.org after #31.
1) apply #31
2) apply this take from #38 :
Comment #45
roball CreditAttribution: roball commented@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.
Comment #46
sblommers CreditAttribution: sblommers commented@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.
Comment #47
roball CreditAttribution: roball commentedsblommers, 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.