Fix a performance regression in attribute methods

Fix: rails#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.

Author: byroot

activemodel/lib/active_model/attribute_methods.rb

@@ -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,22 +303,35 @@ 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)
+            define_attribute_method_pattern(pattern, attr_name, owner: owner, as: as)
+          end
+          attribute_method_patterns_cache.clear
+        end
+      end
 
-            unless instance_method_already_implemented?(method_name)
-              generate_method = "define_method_#{pattern.proxy_target}"
+      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)
 
-              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
+        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
-          attribute_method_patterns_cache.clear
         end
       end
 
@@ -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

activemodel/lib/active_model/attributes.rb

@@ -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)" <<

activerecord/lib/active_record/attribute_methods.rb

@@ -77,19 +77,21 @@ 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
 
         if !abstract_class? && !has_attribute?(old_name)
           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
 

activerecord/lib/active_record/attribute_methods/read.rb

@@ -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) }" <<

activerecord/lib/active_record/attribute_methods/write.rb

@@ -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)" <<

activerecord/test/cases/attribute_methods_test.rb

@@ -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

activesupport/lib/active_support/code_generator.rb

@@ -9,25 +9,30 @@ 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
 
       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