Skip to content
  • Kevin Dew's avatar
    322e6284
    Remove override of ActiveModel#attribute_names · 322e6284
    Kevin Dew authored
    In https://github.com/rails/rails/pull/43036 an optimisation was applied
    to ActiveModel#Serialization to speed up the generation of a
    serialized_hash by avoiding loading the subjects attributes by using an
    attribute_names method. A fallback method,
    ActiveModel::Serialization#attribute_names` was added as #attribute_names
    isn't part of the public API of ActiveModel.
    
    Unfortunately, this fallback method happens to override the ActiveRecord
    method (as ActiveModel::Serialization is a later mixin than
    ActiveRecord::AttributeMethods), so this change didn't actually provide
    an optimisation - the full attribute data was loaded as per [1]
    
    This change also, in our case, produced some severe performance issues
    as it introduced an N+1 query in a situation where we had one gem,
    Globalize [2], which adds in dynamic attributes that are loaded by a query;
    and another gem, Transitions [3], that checks attribute names at
    initialization. The combination of these meant that for every model that
    was initialized an extra query would run - no matter what includes or
    eager_load steps were in place. This rapidly hindered our applications'
    performance and meant we had to rollback the Rails 7 upgrade.
    
    Following rafaelfranca's suggestion [4] this adds a
    `attribute_names_for_serialization` method to Serialization modules in
    ActiveRecord and ActiveModel. This allows the ActiveRecord one to
    override the ActiveModel fallback and thus be optimised.
    
    Some basic benchmarks of this follow - they use code from
    https://github.com/ollietreend/rails-demo and have some pretty large
    arrays set as serialized attributes [5] to demonstrate impacts.
    
    Loading attribute names:
    
    Rails 7.0.2.3
    
    ```
    > Benchmark.ms { Widget.all.map(&:attribute_names) }
      Widget Load (131.1ms)  SELECT "widgets".* FROM "widgets"
    => 20108.852999983355
    ```
    
    This patch
    
    ```
    > Benchmark.ms { Widget.all.map(&:attribute_names) }
      Widget Load (144.0ms)  SELECT "widgets".* FROM "widgets"
    => 237.96699999365956
    ```
    
    Using serializable_hash:
    
    Rails 7.0.2.3
    
    ```
    > widgets = Widget.all.to_a; Benchmark.ms { widgets.map { |w| w.serializable_hash(only: []) } }
      Widget Load (133.3ms)  SELECT "widgets".* FROM "widgets"
    => 22071.45000001765
    ```
    
    This patch
    
    ```
    > widgets = Widget.all.to_a; Benchmark.ms { widgets.map { |w| w.serializable_hash(only: []) } }
      Widget Load (83.5ms)  SELECT "widgets".* FROM "widgets"
    => 67.9039999959059
    ```
    
    [1]: https://github.com/rails/rails/blob/eeb2cfb6864169d7b9cb78744d44b7da17b2b288/activemodel/lib/active_model/serialization.rb#L151-L154
    [2]: https://github.com/globalize/globalize
    [3]: https://github.com/troessner/transitions
    [4]: https://github.com/rails/rails/pull/44770#pullrequestreview-922209612
    [5]: https://github.com/ollietreend/rails-demo/blob/525f88887bb1605a72f7b944b59b0d0e204f92aa/db/seeds.rb
    322e6284
    Remove override of ActiveModel#attribute_names
    Kevin Dew authored
    In https://github.com/rails/rails/pull/43036 an optimisation was applied
    to ActiveModel#Serialization to speed up the generation of a
    serialized_hash by avoiding loading the subjects attributes by using an
    attribute_names method. A fallback method,
    ActiveModel::Serialization#attribute_names` was added as #attribute_names
    isn't part of the public API of ActiveModel.
    
    Unfortunately, this fallback method happens to override the ActiveRecord
    method (as ActiveModel::Serialization is a later mixin than
    ActiveRecord::AttributeMethods), so this change didn't actually provide
    an optimisation - the full attribute data was loaded as per [1]
    
    This change also, in our case, produced some severe performance issues
    as it introduced an N+1 query in a situation where we had one gem,
    Globalize [2], which adds in dynamic attributes that are loaded by a query;
    and another gem, Transitions [3], that checks attribute names at
    initialization. The combination of these meant that for every model that
    was initialized an extra query would run - no matter what includes or
    eager_load steps were in place. This rapidly hindered our applications'
    performance and meant we had to rollback the Rails 7 upgrade.
    
    Following rafaelfranca's suggestion [4] this adds a
    `attribute_names_for_serialization` method to Serialization modules in
    ActiveRecord and ActiveModel. This allows the ActiveRecord one to
    override the ActiveModel fallback and thus be optimised.
    
    Some basic benchmarks of this follow - they use code from
    https://github.com/ollietreend/rails-demo and have some pretty large
    arrays set as serialized attributes [5] to demonstrate impacts.
    
    Loading attribute names:
    
    Rails 7.0.2.3
    
    ```
    > Benchmark.ms { Widget.all.map(&:attribute_names) }
      Widget Load (131.1ms)  SELECT "widgets".* FROM "widgets"
    => 20108.852999983355
    ```
    
    This patch
    
    ```
    > Benchmark.ms { Widget.all.map(&:attribute_names) }
      Widget Load (144.0ms)  SELECT "widgets".* FROM "widgets"
    => 237.96699999365956
    ```
    
    Using serializable_hash:
    
    Rails 7.0.2.3
    
    ```
    > widgets = Widget.all.to_a; Benchmark.ms { widgets.map { |w| w.serializable_hash(only: []) } }
      Widget Load (133.3ms)  SELECT "widgets".* FROM "widgets"
    => 22071.45000001765
    ```
    
    This patch
    
    ```
    > widgets = Widget.all.to_a; Benchmark.ms { widgets.map { |w| w.serializable_hash(only: []) } }
      Widget Load (83.5ms)  SELECT "widgets".* FROM "widgets"
    => 67.9039999959059
    ```
    
    [1]: https://github.com/rails/rails/blob/eeb2cfb6864169d7b9cb78744d44b7da17b2b288/activemodel/lib/active_model/serialization.rb#L151-L154
    [2]: https://github.com/globalize/globalize
    [3]: https://github.com/troessner/transitions
    [4]: https://github.com/rails/rails/pull/44770#pullrequestreview-922209612
    [5]: https://github.com/ollietreend/rails-demo/blob/525f88887bb1605a72f7b944b59b0d0e204f92aa/db/seeds.rb
Loading