Avoiding clone in AR::Base#attributes

rubyonrails-core
1

Hi, I’ve changed the ActiveRecord::Base#attributes method to avoid cloning of objects and increase apps performance. Here is the patch: http://dev.rubyonrails.org/ticket/11047

This sure speeds up the method avoiding Kernel#clone calls, but I don’t know if there is any special case where not cloning makes this unsafe. As koz suggested ( http://groups.google.com/group/rubyonrails-core/browse_thread/thread/faeaed67dee0ff1d/c0b0c81e9a4f6eb3 ) I’ve could not find cloning here being necessary and no test is broken. Any doublecheck, comment or +/-1 is welcome.

Cheers, Juanjo

2

Hi, I've changed the ActiveRecord::Base#attributes method to avoid cloning of objects and increase apps performance. Here is the patch: http://dev.rubyonrails.org/ticket/11047 This sure speeds up the method avoiding Kernel#clone calls, but I don't know if there is any special case where not cloning makes this unsafe. As koz suggested ( http://groups.google.com/group/rubyonrails-core/browse_thread/thread/faeaed67dee0ff1d/c0b0c81e9a4f6eb3 ) I've could not find cloning here being necessary and no test is broken. Any doublecheck, comment or +/-1 is welcome.

The only case that breaks (that I can think of) is where you assign the attributes of one model to another, they'll end up sharing the same instances for their attributes. For example:

  def test_values_copied_with_attributes_assignment     new_topic = Topic.create(:title=>"Some title")     other_topic = Topic.create(new_topic.attributes)     assert_equal new_topic.title, other_topic.title     new_topic.title.gsub!(/Some/, 'The')     assert_equal "The title", new_topic.title     assert_equal "Some title", other_topic.title   end

With your patch that test would fail, both topics would have a title of 'The title'. I'm not particularly worried about this, but it would be best to leave it till 2.1 rather than a 2.0 point release.

Anyone have any objections to this new behaviour?

3

Anyone have any objections to this new behaviour?

I've applied the patch to edge, so if you notice any breakage shout out in this thread.

4

As nobody reports any breakage, it seems natural to implement the same behaviour to attributes_before_type_cast as well.

Here is the patch to avoid cloning in attributes_before_type_cast: http://dev.rubyonrails.org/ticket/11077

Cheers, Juanjo

5

As nobody reports any breakage, it seems natural to implement the same behaviour to attributes_before_type_cast as well.

Here is the patch to avoid cloning in attributes_before_type_cast: http://dev.rubyonrails.org/ticket/11077

Agreed, applied, thanks!