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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

onelittleant’s picture

onelittleant’s picture

Status: Active » Needs review
azinck’s picture

This looks great -- very smart. I'll give it a test.

alexweber’s picture

Got a whitespace notice when applying against HEAD but the patch does seem to work as advertised!

alexweber’s picture

As 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...

drupov’s picture

Status: Needs review » Reviewed & tested by the community

#1 works great.

Cannot say anything about performance yet, but will report if anything strange occurs.

Thanks

leo_g’s picture

thank you, this solved a lot!
dont have any performance issues at all!
regars,
leo

alcroito’s picture

Confirming, this exports a lot more strong arm variables and field instance settings, that previously were not appearing.

ChristianAdamski’s picture

Confirming this solves major pain in my current project. Feature overrides missing before are available now.

alcroito’s picture

I'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.

azinck’s picture

@Placinta: Agreed. You can hardly use it at all without this patch. Can't believe this wasn't fixed ages ago.

ChristianAdamski’s picture

Well, it was only reported 3 months ago. Cudos to onelittleant to actually trace this down AND provide a patch.

onelittleant’s picture

There 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.

mpotter’s picture

Sorry 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.

mpotter’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 9d1c015

  • mpotter committed 9d1c015 on authored by onelittleant
    Issue #2232999 by onelittleant: Fixed Missing overrides due to recursive...

Status: Fixed » Closed (fixed)

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

MiroslavBanov’s picture

Status: Closed (fixed) » Needs review
FileSize
3.12 KB

I 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.

MiroslavBanov’s picture

This 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.

azinck’s picture

Patch #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.

alexweber’s picture

Just took it for a spin and it's definitely quite a bit faster than the previous iteration, good work! :) +1 from me...

ethanhinson’s picture

+1...

Loading/Recreating a feature module with overrides no longer exceeds max execution time.

alcroito’s picture

+1 from me as well.

rv0’s picture

Still get
PHP Fatal error: Maximum execution time of 240 seconds exceeded in features_override.export.inc on line 285

deviantintegral’s picture

The 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.

Elijah Lynn’s picture

Would it be a good idea to try to get _features_override_is_ref_to() into Core for a wider benefit?

drupal_is_ref_to()

MiroslavBanov’s picture

@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.

sylus’s picture

Status: Needs review » Reviewed & tested by the community

This final patch actually allowed me to open up the modules page and showed a significant by 0.75 improvement in wall time.

mpotter’s picture

I'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.

mpotter’s picture

Status: Reviewed & tested by the community » Needs work

I 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

mpotter’s picture

Status: Needs work » Needs review
FileSize
6.46 KB

Here 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.

mpotter’s picture

Would 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)

MiroslavBanov’s picture

Unfortunately, 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.

  • mpotter committed 609e6d3 on 7.x-2.x
    Issue #2232999 by MiroslavBanov, mpotter, onelittleant, deviantintegral...
mpotter’s picture

Status: Needs review » Fixed

OK, 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.

Status: Fixed » Closed (fixed)

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