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

[ActiveModel] Improve the performance of mass assignments #29447

Merged
merged 3 commits into from Jun 15, 2017

Conversation

@shotat
Copy link
Contributor

@shotat shotat commented Jun 14, 2017

Summary

Improve the performance of ActiveModel initializer with removing duplicated string object generation.

Other Information

simple benchmarks for ActiveModel::new

1.2x faster than the current implementation.

before     31.132k (± 5.2%) i/s -    157.781k in   5.083369s
after      37.170k (± 4.7%) i/s -    185.614k in   5.006230s
@shotat shotat changed the title Improve the performance of ActiveModel mass assignment [ActiveModel] Improve the performance of mass assignments Jun 14, 2017
@jonathanhefner
Copy link
Member

@jonathanhefner jonathanhefner commented Jun 14, 2017

Although I honestly enjoy micro-optimizing, this change will probably have no effect outside of a microbenchmark. Have you tested it in a larger context?

Also, String#freeze would be more idiomatic. EDIT: It was late, and I wasn't thinking straight. String#freeze isn't really applicable when using interpolated strings.

@shotat
Copy link
Contributor Author

@shotat shotat commented Jun 14, 2017

@jonathanhefner
Thanks. I agree with your opinion. There might be no effect on a typical Rails app.

However, ActiveModel is used in various contexts independent of Rails.
This change benefits on batch processing like data format conversions using ActiveModel.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jun 14, 2017

Thank for the pull request but could you just put # frozen_string_literal: true to the top of the file? Or the performance improvement come from avoiding to do the interpolation twice?

@kaspth
Copy link
Member

@kaspth kaspth commented Jun 14, 2017

What's the diff if we use quoted symbols :#{k}=?

@shotat
Copy link
Contributor Author

@shotat shotat commented Jun 15, 2017

@rafaelfranca
Thanks, I just put # frozen_string_literal: true.
The problem was unnecessary repeated string interpolations as you mentioned.

@kaspth
Thanks. I noticed that quoted symbols are better as a whole.
Following reflection methods(respond_to? and public_send) handles symbols faster than strings.

 after     40.172k (± 2.6%) i/s -    203.149k in   5.060347s
before     32.679k (± 4.0%) i/s -    163.929k in   5.025078s
@rafaelfranca rafaelfranca merged commit e57d8d0 into rails:master Jun 15, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shotat shotat deleted the shotat:feature/enhance_active_model branch Jun 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants