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.

Sign up for GitHub

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

Jump to bottom

ActiveRecord initialization optimizations #29215

Merged
merged 4 commits into from
May 25, 2017
Merged

ActiveRecord initialization optimizations #29215

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a24912c
Performance optimization for AttributeSet#deep_dup
lovitt May 18, 2017
bfc62fe
Performance optimization for ActiveRecord#subclass_from_attributes
lovitt May 18, 2017
63dd12b
Performance optimization for ActiveRecord#column_defaults
lovitt May 18, 2017
fb6ccce
Make #deep_dup use #allocate instead of #new
lovitt May 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions activerecord/lib/active_record/attribute_set.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,7 @@ def freeze
end

def deep_dup
dup.tap do |copy|
copy.instance_variable_set(:@attributes, attributes.deep_dup)
end
self.class.new(attributes.deep_dup)
Copy link
Member

Choose a reason for hiding this comment

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

This is avoiding the wasted shallow dup in initialize_dup below, right?

Does dup.tap do |copy| copy.attributes.transform_values!(&:deep_dup) end bench similarly?

My only hesitation about using new is that while it currently saves an allocation, it seems more likely to become the costlier of the two at some point in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this change is to avoid the wasted shallow dup in initialize_dup.

I think using Class#allocate may be the best approach:

def deep_dup
  self.class.allocate.tap do |copy|
    copy.instance_variable_set(:@attributes, attributes.deep_dup)
  end
end

That seems to do exactly what we want: create a new instance of AttributeSet without calling initialize, so that we can initialize @attributes (and any future instance variables) ourselves within deep_dup and avoid wasted shallow duplication. (This is effectively what Object#dup does under the hood.)

As you would expect, this variation provides the same speedup as the current optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthewd I went ahead and pushed that change. Let me know what you think.

end

def initialize_dup(_)
Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/inheritance.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ def type_condition(table = arel_table)
def subclass_from_attributes(attrs)
attrs = attrs.to_h if attrs.respond_to?(:permitted?)
if attrs.is_a?(Hash)
subclass_name = attrs.with_indifferent_access[inheritance_column]
subclass_name = attrs[inheritance_column] || attrs[inheritance_column.to_sym]
Copy link
Member

Choose a reason for hiding this comment

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

Is inheritance_column guaranteed to be a string here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the default value of inheritance_column is "type", and when the default is overridden, the setter ensures the custom value is a string.


if subclass_name.present?
find_sti_class(subclass_name)
Expand Down
3 changes: 2 additions & 1 deletion activerecord/lib/active_record/model_schema.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ def type_for_attribute(attr_name, &block)
# default values when instantiating the Active Record object for this table.
def column_defaults
load_schema
_default_attributes.to_hash
@column_defaults ||= _default_attributes.to_hash
end

def _default_attributes # :nodoc:
Expand Down Expand Up @@ -466,6 +466,7 @@ def reload_schema_from_cache
@attribute_types = nil
@content_columns = nil
@default_attributes = nil
@column_defaults = nil
@inheritance_column = nil unless defined?(@explicit_inheritance_column) && @explicit_inheritance_column
@attributes_builder = nil
@columns = nil
Expand Down