Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

immutablize properly as we deep merge #6362

Merged
merged 1 commit into from Sep 1, 2017

Conversation

lamont-granquist
Copy link
Contributor

@lamont-granquist lamont-granquist commented Aug 29, 2017

removes a lot of excess dup'ing that was going on in deep merge

this forks our deep merge implementation since people were using deep merge as an external API, internally we no longer use the old deep merge class.

the attribute version of deep merging is about to mutate into unrecognizability though, and is no longer an external api.

@lamont-granquist lamont-granquist force-pushed the lcg/deep-merge-immutablizing branch 2 times, most recently from 86ba18a to c582643 Compare September 1, 2017 01:35
@lamont-granquist lamont-granquist requested a review from a team September 1, 2017 01:35
Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I presume there's a decent speed-up with this implementation since it's not doing a lot of the duping that Chef::Mixin::DeepMerge does?

@jaymzh
Copy link
Collaborator

jaymzh commented Sep 1, 2017

@adamleff yup. Particularly on attribute-heavy users like us (Facebook).

@adamleff
Copy link
Contributor

adamleff commented Sep 1, 2017

@jaymzh I think I grossly underestimated the amount of attributes you all use 🙂 I hope this helps you out!

@lamont-granquist
Copy link
Contributor Author

yeah it has a pile of speed up. its still not actually sufficient, so its likely that i'm going to have to redo how deep merging works and how the merged cache works completely.

the fix in #5360 did a whole lot of extra work, but i think now we're down to the fact that properly immutablizing through arrays is the rest of the perf hit that facebook is seeing before and after that PR. so i think this is as fast as we can make attributes work via 'patching' and the next step is throwing out the top-level-key merge cache and doing it at every level on-demand.

@lamont-granquist lamont-granquist merged commit 36e9b60 into master Sep 1, 2017
@lamont-granquist lamont-granquist deleted the lcg/deep-merge-immutablizing branch September 1, 2017 16:58
@chef chef locked and limited conversation to collaborators Feb 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants