Fix a performance regression in attribute methods
Fix: #52111 Fix: 5dbc7b4 The above commit caused the size of the `CodeGenerator` method cache to explode, because the dynamic namespace is way too granular. But there is actually a much better fix for that, since `alias_attribute` is now generating exactly the same code as the attribute it's aliasing, we can generated it as the canonical method in the cache, and then just define it in the model as the aliased name. This prevent the cache from growing a lot, and even reduce memory usage further as the original attribute and its alias now share the same method cache.
This commit is contained in:
parent
4a4b399830
commit
514d474836
@ -215,9 +215,7 @@ def eagerly_generate_alias_attribute_methods(new_name, old_name) # :nodoc:
|
||||
end
|
||||
|
||||
def generate_alias_attribute_methods(code_generator, new_name, old_name)
|
||||
attribute_method_patterns.each do |pattern|
|
||||
alias_attribute_method_definition(code_generator, pattern, new_name, old_name)
|
||||
end
|
||||
define_attribute_method(old_name, _owner: code_generator, as: new_name)
|
||||
end
|
||||
|
||||
def alias_attribute_method_definition(code_generator, pattern, new_name, old_name) # :nodoc:
|
||||
@ -305,25 +303,38 @@ def define_attribute_methods(*attr_names)
|
||||
# person.name = 'Bob'
|
||||
# person.name # => "Bob"
|
||||
# person.name_short? # => true
|
||||
def define_attribute_method(attr_name, _owner: generated_attribute_methods)
|
||||
def define_attribute_method(attr_name, _owner: generated_attribute_methods, as: attr_name)
|
||||
ActiveSupport::CodeGenerator.batch(_owner, __FILE__, __LINE__) do |owner|
|
||||
attribute_method_patterns.each do |pattern|
|
||||
method_name = pattern.method_name(attr_name)
|
||||
|
||||
unless instance_method_already_implemented?(method_name)
|
||||
generate_method = "define_method_#{pattern.proxy_target}"
|
||||
|
||||
if respond_to?(generate_method, true)
|
||||
send(generate_method, attr_name.to_s, owner: owner)
|
||||
else
|
||||
define_proxy_call(owner, method_name, pattern.proxy_target, pattern.parameters, attr_name.to_s, namespace: :active_model_proxy)
|
||||
end
|
||||
end
|
||||
define_attribute_method_pattern(pattern, attr_name, owner: owner, as: as)
|
||||
end
|
||||
attribute_method_patterns_cache.clear
|
||||
end
|
||||
end
|
||||
|
||||
def define_attribute_method_pattern(pattern, attr_name, owner:, as:) # :nodoc:
|
||||
canonical_method_name = pattern.method_name(attr_name)
|
||||
public_method_name = pattern.method_name(as)
|
||||
|
||||
unless instance_method_already_implemented?(public_method_name)
|
||||
generate_method = "define_method_#{pattern.proxy_target}"
|
||||
|
||||
if respond_to?(generate_method, true)
|
||||
send(generate_method, attr_name.to_s, owner: owner, as: as)
|
||||
else
|
||||
define_proxy_call(
|
||||
owner,
|
||||
canonical_method_name,
|
||||
pattern.proxy_target,
|
||||
pattern.parameters,
|
||||
attr_name.to_s,
|
||||
namespace: :active_model_proxy,
|
||||
as: public_method_name
|
||||
)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
# Removes all the previously dynamically defined methods from the class, including alias attribute methods.
|
||||
#
|
||||
# class Person
|
||||
@ -404,14 +415,19 @@ def attribute_method_patterns_matching(method_name)
|
||||
# Define a method `name` in `mod` that dispatches to `send`
|
||||
# using the given `extra` args. This falls back on `send`
|
||||
# if the called name cannot be compiled.
|
||||
def define_proxy_call(code_generator, name, proxy_target, parameters, *call_args, namespace:)
|
||||
def define_proxy_call(code_generator, name, proxy_target, parameters, *call_args, namespace:, as: name)
|
||||
mangled_name = build_mangled_name(name)
|
||||
|
||||
call_args.map!(&:inspect)
|
||||
call_args << parameters if parameters
|
||||
namespace = :"#{namespace}_#{proxy_target}_#{call_args.join("_")}}"
|
||||
|
||||
define_call(code_generator, name, proxy_target, mangled_name, parameters, call_args, namespace: namespace)
|
||||
# We have to use a different namespace for every target method, because
|
||||
# if someone defines an attribute that look like an attribute method we could clash, e.g.
|
||||
# attribute :title_was
|
||||
# attribute :title
|
||||
namespace = :"#{namespace}_#{proxy_target}"
|
||||
|
||||
define_call(code_generator, name, proxy_target, mangled_name, parameters, call_args, namespace: namespace, as: as)
|
||||
end
|
||||
|
||||
def build_mangled_name(name)
|
||||
@ -424,8 +440,8 @@ def build_mangled_name(name)
|
||||
mangled_name
|
||||
end
|
||||
|
||||
def define_call(code_generator, name, target_name, mangled_name, parameters, call_args, namespace:)
|
||||
code_generator.define_cached_method(name, as: mangled_name, namespace: namespace) do |batch|
|
||||
def define_call(code_generator, name, target_name, mangled_name, parameters, call_args, namespace:, as:)
|
||||
code_generator.define_cached_method(mangled_name, as: as, namespace: namespace) do |batch|
|
||||
body = if CALL_COMPILABLE_REGEXP.match?(target_name)
|
||||
"self.#{target_name}(#{call_args.join(", ")})"
|
||||
else
|
||||
|
@ -89,11 +89,11 @@ def attribute_names
|
||||
|
||||
##
|
||||
private
|
||||
def define_method_attribute=(name, owner:)
|
||||
def define_method_attribute=(canonical_name, owner:, as: canonical_name)
|
||||
ActiveModel::AttributeMethods::AttrNames.define_attribute_accessor_method(
|
||||
owner, name, writer: true,
|
||||
owner, canonical_name, writer: true,
|
||||
) do |temp_method_name, attr_name_expr|
|
||||
owner.define_cached_method("#{name}=", as: temp_method_name, namespace: :active_model) do |batch|
|
||||
owner.define_cached_method(temp_method_name, as: "#{as}=", namespace: :active_model) do |batch|
|
||||
batch <<
|
||||
"def #{temp_method_name}(value)" <<
|
||||
" _write_attribute(#{attr_name_expr}, value)" <<
|
||||
|
@ -77,6 +77,13 @@ def eagerly_generate_alias_attribute_methods(_new_name, _old_name) # :nodoc:
|
||||
# alias attributes in Active Record are lazily generated
|
||||
end
|
||||
|
||||
def generate_alias_attribute_methods(code_generator, new_name, old_name) # :nodoc:
|
||||
attribute_method_patterns.each do |pattern|
|
||||
alias_attribute_method_definition(code_generator, pattern, new_name, old_name)
|
||||
end
|
||||
attribute_method_patterns_cache.clear
|
||||
end
|
||||
|
||||
def alias_attribute_method_definition(code_generator, pattern, new_name, old_name)
|
||||
old_name = old_name.to_s
|
||||
|
||||
@ -84,12 +91,7 @@ def alias_attribute_method_definition(code_generator, pattern, new_name, old_nam
|
||||
raise ArgumentError, "#{self.name} model aliases `#{old_name}`, but `#{old_name}` is not an attribute. " \
|
||||
"Use `alias_method :#{new_name}, :#{old_name}` or define the method manually."
|
||||
else
|
||||
method_name = pattern.method_name(new_name).to_s
|
||||
parameters = pattern.parameters
|
||||
|
||||
define_proxy_call(code_generator, method_name, pattern.proxy_target, parameters, old_name,
|
||||
namespace: :proxy_alias_attribute
|
||||
)
|
||||
define_attribute_method_pattern(pattern, old_name, owner: code_generator, as: new_name)
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -8,11 +8,11 @@ module Read
|
||||
|
||||
module ClassMethods # :nodoc:
|
||||
private
|
||||
def define_method_attribute(name, owner:)
|
||||
def define_method_attribute(canonical_name, owner:, as: canonical_name)
|
||||
ActiveModel::AttributeMethods::AttrNames.define_attribute_accessor_method(
|
||||
owner, name
|
||||
owner, canonical_name
|
||||
) do |temp_method_name, attr_name_expr|
|
||||
owner.define_cached_method(name, as: temp_method_name, namespace: :active_record) do |batch|
|
||||
owner.define_cached_method(temp_method_name, as: as, namespace: :active_record) do |batch|
|
||||
batch <<
|
||||
"def #{temp_method_name}" <<
|
||||
" _read_attribute(#{attr_name_expr}) { |n| missing_attribute(n, caller) }" <<
|
||||
|
@ -12,11 +12,11 @@ module Write
|
||||
|
||||
module ClassMethods # :nodoc:
|
||||
private
|
||||
def define_method_attribute=(name, owner:)
|
||||
def define_method_attribute=(canonical_name, owner:, as: canonical_name)
|
||||
ActiveModel::AttributeMethods::AttrNames.define_attribute_accessor_method(
|
||||
owner, name, writer: true,
|
||||
owner, canonical_name, writer: true,
|
||||
) do |temp_method_name, attr_name_expr|
|
||||
owner.define_cached_method("#{name}=", as: temp_method_name, namespace: :active_record) do |batch|
|
||||
owner.define_cached_method(temp_method_name, as: "#{as}=", namespace: :active_record) do |batch|
|
||||
batch <<
|
||||
"def #{temp_method_name}(value)" <<
|
||||
" _write_attribute(#{attr_name_expr}, value)" <<
|
||||
|
@ -1220,8 +1220,10 @@ def some_method_that_is_not_on_super
|
||||
test "aliases to the same attribute name do not conflict with each other" do
|
||||
first_model_object = ToBeLoadedFirst.new(author_name: "author 1")
|
||||
assert_equal("author 1", first_model_object.subject)
|
||||
assert_equal([nil, "author 1"], first_model_object.subject_change)
|
||||
second_model_object = ToBeLoadedSecond.new(title: "foo")
|
||||
assert_equal("foo", second_model_object.subject)
|
||||
assert_equal([nil, "foo"], second_model_object.subject_change)
|
||||
end
|
||||
|
||||
test "#alias_attribute with an overridden original method does not use the overridden original method" do
|
||||
|
@ -9,16 +9,19 @@ def initialize(namespace)
|
||||
@cache = METHOD_CACHES[namespace]
|
||||
@sources = []
|
||||
@methods = {}
|
||||
@canonical_methods = {}
|
||||
end
|
||||
|
||||
def define_cached_method(name, as: name)
|
||||
name = name.to_sym
|
||||
as = as.to_sym
|
||||
@methods.fetch(name) do
|
||||
unless @cache.method_defined?(as)
|
||||
def define_cached_method(canonical_name, as: nil)
|
||||
canonical_name = canonical_name.to_sym
|
||||
as = (as || canonical_name).to_sym
|
||||
|
||||
@methods.fetch(as) do
|
||||
unless @cache.method_defined?(canonical_name) || @canonical_methods[canonical_name]
|
||||
yield @sources
|
||||
end
|
||||
@methods[name] = as
|
||||
@canonical_methods[canonical_name] = true
|
||||
@methods[as] = canonical_name
|
||||
end
|
||||
end
|
||||
|
||||
@ -26,8 +29,10 @@ def apply(owner, path, line)
|
||||
unless @sources.empty?
|
||||
@cache.module_eval("# frozen_string_literal: true\n" + @sources.join(";"), path, line)
|
||||
end
|
||||
@methods.each do |name, as|
|
||||
owner.define_method(name, @cache.instance_method(as))
|
||||
@canonical_methods.clear
|
||||
|
||||
@methods.each do |as, canonical_name|
|
||||
owner.define_method(as, @cache.instance_method(canonical_name))
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -52,8 +57,8 @@ def initialize(owner, path, line)
|
||||
@namespaces = Hash.new { |h, k| h[k] = MethodSet.new(k) }
|
||||
end
|
||||
|
||||
def define_cached_method(name, namespace:, as: name, &block)
|
||||
@namespaces[namespace].define_cached_method(name, as: as, &block)
|
||||
def define_cached_method(canonical_name, namespace:, as: nil, &block)
|
||||
@namespaces[namespace].define_cached_method(canonical_name, as: as, &block)
|
||||
end
|
||||
|
||||
def execute
|
||||
|
Loading…
Reference in New Issue
Block a user