We were unable to get features override to recognize all overrides available for export and cause all overridden features to show a default state. We tracked down the issue to a logic bug in the _features_override_remove_recursion() function in features_override.export.inc. Array comparison in that function would recognize two arrays with the same elements and keys (eg. two fields with the same display settings) as creating a recursive loop and drop one of the objects. As a result overrides are either not available for export or are exported with missing elements.
The attached patch I think resolves the issue. This may resolve some other issues in the queue as well.
On loading a feature create screen for my override feature I counted the total elements that features override had available to it after passing the 314 items through features_override_remove_recursion():
pre-patch: 12163 total elements
post-patch: 14371 total elements
the delta is 2208 elements or about 15%
This would suggest that features override is missing in this case 15% of the available overridden data without the patch (eg. field settings arrays with the same structure and values on different field instances).
Application of this patch to sites with existing overrides modules may cause more elements to be auto-detected when re-generating those features.
Comments
Comment #1
onelittleant CreditAttribution: onelittleant commentedComment #2
onelittleant CreditAttribution: onelittleant commentedComment #3
azinck CreditAttribution: azinck commentedThis looks great -- very smart. I'll give it a test.
Comment #4
alexweber CreditAttribution: alexweber commentedGot a whitespace notice when applying against HEAD but the patch does seem to work as advertised!
Comment #5
alexweber CreditAttribution: alexweber commentedAs a follow-up after using this patch for a while it's worth noting that whereas it does expose new overrides and does solve problems, it also does incur a significant performance penalty, making the Features UI on a large site with lots of exportable components not quite unusable, but close...
Comment #6
drupov CreditAttribution: drupov commented#1 works great.
Cannot say anything about performance yet, but will report if anything strange occurs.
Thanks
Comment #7
leo_g CreditAttribution: leo_g commentedthank you, this solved a lot!
dont have any performance issues at all!
regars,
leo
Comment #8
alcroito CreditAttribution: alcroito commentedConfirming, this exports a lot more strong arm variables and field instance settings, that previously were not appearing.
Comment #9
ChristianAdamski CreditAttribution: ChristianAdamski commentedConfirming this solves major pain in my current project. Feature overrides missing before are available now.
Comment #10
alcroito CreditAttribution: alcroito commentedI'm not even sure how people are using features overrides without this patch. In distributions (Commons for me) it didn't work almost at all.
Comment #11
azinck CreditAttribution: azinck commented@Placinta: Agreed. You can hardly use it at all without this patch. Can't believe this wasn't fixed ages ago.
Comment #12
ChristianAdamski CreditAttribution: ChristianAdamski commentedWell, it was only reported 3 months ago. Cudos to onelittleant to actually trace this down AND provide a patch.
Comment #13
onelittleant CreditAttribution: onelittleant commentedThere is a performance hit from this patch to be sure, but only when comparing / generating features and not during normal operation. It would be useful to profile the recursive array walking function to try to determine if any efficiency can be gained in the logic there. The issue is really that the complexity of the algorithm grows exponentially with the number of elements in the exported features arrays... so the bigger (more stuff in) the feature, the exponentially more complex the calculation.
Comment #14
mpotter CreditAttribution: mpotter commentedSorry for the delay on this. I've been playing with this on Open Atrium and it definitely fixes some issues. Will roll a new release soon after a bit more testing.
Comment #15
mpotter CreditAttribution: mpotter commentedCommitted to 9d1c015
Comment #18
MiroslavBanov CreditAttribution: MiroslavBanov commentedI did face significant performance problem with this. I have a huge project with 75 features enabled with various configuration exported in them. I have about 30 content types, 50 field bases, several hundred field instances, ungodly amount of permissions, various other entity types exported - fieldable panel panes, beans, taxonomies, file entities. Also - panels pages, panelizer displays, views, openlayer maps, block settings, etc, etc.
I installed features_override to handle differences between two profiles - one "base profile", and one "extension profile" that has additional functionality. I needed features_override to do some adjustments to some variables and field visibility settings. I still have only one "override" feature enabled. Locally, features-list took 37 seconds on average. Of that time, the "override" feature took 28 seconds on it's features_get_storage() call, and 26 seconds of that time was in features_override_remove_recursion(). The problem is that not just feature-list is getting slower, but each feature-revert is also getting slower, So if I try to revert many features at the same time, it is quite noticeable performance impact.
It appears that more granular features have better performance. For instance - it is convenient for me to have all permissions in one feature, but I have field_permissions and other modules that add many, and the permission feature alone is adding 5 seconds to the "override" feature.
Here is a patch that reduces some of the load for me, I managed to reduce the time for the "override" feature in half, so it took 14 seconds. I hope that someone will come up with a more optimized solution.
Comment #19
MiroslavBanov CreditAttribution: MiroslavBanov commentedThis is another try - seems that the "override" feature is now taking only 7.5 seconds.
Edit: I think it will be a good idea to create a test that will ensure that references are removed, and that equal arrays that are not references are not removed.
Comment #20
azinck CreditAttribution: azinck commentedPatch #19 is working well for me. Big performance improvement vs. what was committed in #16.
Hoping to hear from some others before marking as RTBC.
Comment #21
alexweber CreditAttribution: alexweber commentedJust took it for a spin and it's definitely quite a bit faster than the previous iteration, good work! :) +1 from me...
Comment #22
ethanhinson CreditAttribution: ethanhinson commented+1...
Loading/Recreating a feature module with overrides no longer exceeds max execution time.
Comment #23
alcroito CreditAttribution: alcroito commented+1 from me as well.
Comment #24
rv0 CreditAttribution: rv0 commentedStill get
PHP Fatal error: Maximum execution time of 240 seconds exceeded in features_override.export.inc on line 285
Comment #25
deviantintegral CreditAttribution: deviantintegral commentedThe patch in #19 is working wonderfully for me. It reduces function calls to _features_override_is_ref_to() by an order of magnitude, and in an anecdotal test on a large site with many features improves performance by around 225%.
Here's an updated patch that is only code style and documentation improvements.
Comment #26
Elijah LynnWould it be a good idea to try to get _features_override_is_ref_to() into Core for a wider benefit?
drupal_is_ref_to()
Comment #27
MiroslavBanov CreditAttribution: MiroslavBanov commented@Elijah Lynn
I think this is unlikely to be needed. I know that devel uses something similar (Krumo marker). Other than that, I have not seen any need for similar function / logic. I can't imagine I'll ever need to use this myself.
Comment #28
sylus CreditAttribution: sylus commentedThis final patch actually allowed me to open up the modules page and showed a significant by 0.75 improvement in wall time.
Comment #29
mpotter CreditAttribution: mpotter commentedI'll try to get #25 into a new release soon. We are using similar recursive code in Features itself right now and it also shows this same performance issue, so I'll be trying to apply this solution to both modules.
Comment #30
mpotter CreditAttribution: mpotter commentedI want to rework the patch here to reflect what we did recently in Features that worked well #2543306: drush_features_list() slow - about 220 seconds
Comment #31
mpotter CreditAttribution: mpotter commentedHere is a new patch for review that uses the formaldehyde added to Features in #2543306: drush_features_list() slow - about 220 seconds. It also refactors Features Override to use the new functions that were moved into Features itself.
Comment #32
mpotter CreditAttribution: mpotter commentedWould love to get people's benchmarking tests on the patch in #31. I'd love to do a new release with this.
NOTE: You'll need the -dev version of Features to test this (or Features >= 2.7)
Comment #33
MiroslavBanov CreditAttribution: MiroslavBanov commentedUnfortunately, I no longer use feature overrides, so my tests are probably not very helpful. I still have the module installed, and so I will give the performance stats:
With patch #19
time drush cc all
real 0m39.018s
user 0m16.437s
sys 0m0.913s
time drush fl
real 0m29.289s
user 0m9.037s
sys 0m0.345s
With patch #31
time drush cc all
real 0m35.657s
user 0m15.163s
sys 0m0.708s
time drush fl
real 0m27.778s
user 0m8.735s
sys 0m0.294s
So new patch is faster.
I haven't looked at whether there are any actual differences between what is exported, but if it is in Features module, it makes perfect sense to reuse that for Features override. I am leaving it at Needs review.
Comment #35
mpotter CreditAttribution: mpotter commentedOK, I want to get a new release of this for Atrium this week, so going ahead with committed this to 609e6d3. We've had a couple more people try it and haven't found any negative side effects and it's definitely faster.
Thanks to all who helped on this issue! Between this and Features 2.7 I think it's a *lot* more useable now.