Merge pull request rails#52118 from Shopify/fix-code-generator-bloat

Fix a performance regression in attribute methods

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