Permalink
Browse files

Improve the performance of `save` and friends

The biggest source of the performance regression in these methods
occurred because dirty tracking required eagerly materializing and type
casting the assigned values. In the previous commits, I've changed dirty
tracking to perform the comparisons lazily. However, all of this is moot
when calling `save`, since `changes_applied` will be called, which just
ends up eagerly materializing everything, anyway. With the new mutation
tracker, it's easy to just compare the previous two hashes in the same
lazy fashion.

We will not have aliasing issues with this setup, which is proven by the
fact that we're able to detect nested mutation.

Before:
    User.create! 2.007k (± 7.1%) i/s -     10.098k

After:
    User.create! 2.557k (± 3.5%) i/s -     12.789k

Fixes #19859
  • Loading branch information...
sgrif committed Sep 24, 2015
1 parent 8a811c8 commit 136fc65c9b8b66e1fb56f3a17f0d1fddff9b4bd0
@@ -203,7 +203,7 @@ def changes_include?(attr_name)
# Returns +true+ if attr_name were changed before the model was saved,
# +false+ otherwise.
def previous_changes_include?(attr_name)
@previously_changed.include?(attr_name)
previous_changes.include?(attr_name)
end
# Removes current changes and makes them accessible through +previous_changes+.
@@ -225,7 +225,7 @@ def attribute_change(attr)
# Handles <tt>*_previous_change</tt> for +method_missing+.
def attribute_previous_change(attr)
@previously_changed[attr] if attribute_previously_changed?(attr)
previous_changes[attr] if attribute_previously_changed?(attr)

This comment has been minimized.

Show comment
Hide comment
@claudiob

claudiob May 8, 2018

Member

Hello @sgrif 🙇 I think that this method can now be rewritten from:

def attribute_previous_change(attr)
  previous_changes[attr] if attribute_previously_changed?(attr)
end

to:

def attribute_previous_change(attr)
  previous_changes[attr]
end

without losing performance but let me know if that's correct.


Calling

previous_changes[attr] if attribute_previously_changed?(attr)

is equivalent to calling

previous_changes[attr] if previous_changes.include?(attr)

When this commit 136fc65 was made, Active Record had its own previous_changes method, added here below. However, that method has been recently removed from the codebase, so previous_changes is now only the method defined in Active Model as:

def previous_changes
  @previously_changed ||= ActiveSupport::HashWithIndifferentAccess.new
  @previously_changed.merge(mutations_before_last_save.changes)
end

Since we are dealing with a memoized Hash, there is probably no need to check if .include?(attr_name) before trying to fetch [attr] for it.

Does that make sense? Did I miss anything? Thanks!

@claudiob

claudiob May 8, 2018

Member

Hello @sgrif 🙇 I think that this method can now be rewritten from:

def attribute_previous_change(attr)
  previous_changes[attr] if attribute_previously_changed?(attr)
end

to:

def attribute_previous_change(attr)
  previous_changes[attr]
end

without losing performance but let me know if that's correct.


Calling

previous_changes[attr] if attribute_previously_changed?(attr)

is equivalent to calling

previous_changes[attr] if previous_changes.include?(attr)

When this commit 136fc65 was made, Active Record had its own previous_changes method, added here below. However, that method has been recently removed from the codebase, so previous_changes is now only the method defined in Active Model as:

def previous_changes
  @previously_changed ||= ActiveSupport::HashWithIndifferentAccess.new
  @previously_changed.merge(mutations_before_last_save.changes)
end

Since we are dealing with a memoized Hash, there is probably no need to check if .include?(attr_name) before trying to fetch [attr] for it.

Does that make sense? Did I miss anything? Thanks!

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 2, 2018

Member

@claudiob why not try it in a PR and see if the tests fail? 😄

@kaspth

kaspth Jul 2, 2018

Member

@claudiob why not try it in a PR and see if the tests fail? 😄

end
# Handles <tt>*_will_change!</tt> for +method_missing+.
@@ -51,12 +51,12 @@ def initialize_dup(other) # :nodoc:
end
def changes_applied
super
@previous_mutation_tracker = @mutation_tracker
store_original_attributes
end
def clear_changes_information
super
@previous_mutation_tracker = nil
store_original_attributes
end
@@ -89,6 +89,10 @@ def changes
end
end
def previous_changes
previous_mutation_tracker.changes
end
def attribute_changed_in_place?(attr_name)
@mutation_tracker.changed_in_place?(attr_name)
end
@@ -119,6 +123,10 @@ def store_original_attributes
@mutation_tracker = @mutation_tracker.now_tracking(@attributes)
end
def previous_mutation_tracker
@previous_mutation_tracker ||= NullMutationTracker.new
end
def cache_changed_attributes
@cached_changed_attributes = changed_attributes
yield
@@ -13,6 +13,14 @@ def changed_values
end
end
def changes
attr_names.each_with_object({}.with_indifferent_access) do |attr_name, result|
if changed?(attr_name)
result[attr_name] = [original_attributes.fetch_value(attr_name), attributes.fetch_value(attr_name)]
end
end
end
def changed?(attr_name)
attr_name = attr_name.to_s
modified?(attr_name) || changed_in_place?(attr_name)
@@ -52,4 +60,25 @@ def clean_copy_of(attributes)
end
end
end
class NullMutationTracker
def changed_values
{}
end
def changes
{}
end
def changed?(*)
false
end
def changed_in_place?(*)
false
end
def forget_change(*)
end
end
end

0 comments on commit 136fc65

Please sign in to comment.