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

Don't calculate all changes just to check if the param field has changed #667

Merged
merged 1 commit into from
May 24, 2015
Merged

Don't calculate all changes just to check if the param field has changed #667

merged 1 commit into from
May 24, 2015

Conversation

mhodgson
Copy link
Contributor

Starting with Rails 4.2, calculating the changes hash in activerecord is much more expensive since it now tracks in-place changes in json/jsonb/hstore fields. We noticed a ~10x hit to performance due to these changes being calculated on every to_param call on some of our models with large json fields.

This change simply checks for changes in a less expensive way before calculating the changes hash if the param field has indeed changed.

All tests still pass and I didn't see a need to add additional testing.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.31%) to 98.44% when pulling e1f83f7 on GoBoundless:prevent-changes into ccedf1a on norman:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.31%) to 98.44% when pulling e1f83f7 on GoBoundless:prevent-changes into ccedf1a on norman:master.

norman added a commit that referenced this pull request May 24, 2015
Don't calculate all changes just to check if the param field has changed
@norman norman merged commit 256ebe5 into norman:master May 24, 2015
@norman
Copy link
Owner

norman commented May 24, 2015

Merged, thanks!

@mhodgson
Copy link
Contributor Author

Of course, thanks for getting it merged quickly!

@norman
Copy link
Owner

norman commented Jun 1, 2015

This is now out in 5.2.0.beta.1. No timetable for stable release yet, but probably within a couple weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants