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.

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

Avoid double cast in types that only override cast #46219

Conversation

jonathanhefner
Copy link
Member

Follow-up to #44625.

In #44625, the SerializeCastValue module was added to allow types to avoid a redundant call to cast when serializing a value for the database. Because it introduced a new method (serialize_cast_value) that was not part of the ActiveModel::Type::Value contract, it was designed to be opt-in. Furthermore, to guard against incompatible serialize and serialize_cast_value implementations in types that override serialize but (unintentionally) inherit serialize_cast_value, types were required to explicitly include the SerializeCastValue module to activate the optimization. i.e. It was not sufficient just to have SerializeCastValue in the ancestor chain.

The SerializeCastValue module is not part of the public API, and there are no plans to change that, which meant user-created custom types could not benefit from this optimization.

This commit changes the opt-in condition such that it is sufficient for the owner of the serialize_cast_value method to be the same or below the owner of the serialize method in the ancestor chain. This means a user-created type that only overrides cast, not serialize, will now benefit from the optimization. For example, a type like:

class DowncasedString < ActiveModel::Type::String
  def cast(value)
    super&.downcase
  end
end

As demonstrated in the benchmark below, this commit does not change the current performance of the built-in Active Model types. However, for a simple custom type like DowncasedString, the performance of value_for_database is twice as fast. For types with more expensive cast operations, the improvement may be greater.

Benchmark script
# frozen_string_literal: true

require "benchmark/ips"
require "active_model"

class DowncasedString < ActiveModel::Type::String
  def cast(value)
    super&.downcase
  end
end

ActiveModel::Type.register(:downcased_string, DowncasedString)

VALUES = {
  my_big_integer: "123456",
  my_boolean: "true",
  my_date: "1999-12-31",
  my_datetime: "1999-12-31 12:34:56 UTC",
  my_decimal: "123.456",
  my_float: "123.456",
  my_immutable_string: "abcdef",
  my_integer: "123456",
  my_string: "abcdef",
  my_time: "1999-12-31T12:34:56.789-10:00",
  my_downcased_string: "AbcDef",
}

TYPES = VALUES.to_h { |name, value| [name, name.to_s.delete_prefix("my_").to_sym] }

class MyModel
  include ActiveModel::API
  include ActiveModel::Attributes

  TYPES.each do |name, type|
    attribute name, type
  end
end

attribute_set = MyModel.new(VALUES).instance_variable_get(:@attributes)

TYPES.each do |name, type|
  attribute = attribute_set[name.to_s]

  Benchmark.ips do |x|
    x.report(type.to_s) { attribute.value_for_database }
  end
end

Before

        big_integer      2.986M (± 1.2%) i/s -     15.161M in   5.078972s
            boolean      2.980M (± 1.1%) i/s -     15.074M in   5.059456s
               date      2.960M (± 1.1%) i/s -     14.831M in   5.011355s
           datetime      1.368M (± 0.9%) i/s -      6.964M in   5.092074s
            decimal      2.930M (± 1.2%) i/s -     14.911M in   5.089048s
              float      2.932M (± 1.3%) i/s -     14.713M in   5.018512s
   immutable_string      3.013M (± 1.3%) i/s -     15.239M in   5.058085s
            integer      1.603M (± 0.8%) i/s -      8.096M in   5.052046s
             string      2.977M (± 1.1%) i/s -     15.168M in   5.094874s
               time      1.338M (± 0.9%) i/s -      6.699M in   5.006046s
   downcased_string      1.394M (± 0.9%) i/s -      7.034M in   5.046972s

After

        big_integer      3.016M (± 1.0%) i/s -     15.238M in   5.053005s
            boolean      2.965M (± 1.3%) i/s -     15.037M in   5.071921s
               date      2.924M (± 1.0%) i/s -     14.754M in   5.046294s
           datetime      1.435M (± 0.9%) i/s -      7.295M in   5.082498s
            decimal      2.950M (± 0.9%) i/s -     14.800M in   5.017225s
              float      2.964M (± 0.9%) i/s -     14.987M in   5.056405s
   immutable_string      2.907M (± 1.4%) i/s -     14.677M in   5.049194s
            integer      1.638M (± 0.9%) i/s -      8.227M in   5.022401s
             string      2.971M (± 1.0%) i/s -     14.891M in   5.011709s
               time      1.454M (± 0.9%) i/s -      7.384M in   5.079993s
   downcased_string      2.939M (± 0.9%) i/s -     14.872M in   5.061100s

Follow-up to rails#44625.

In rails#44625, the `SerializeCastValue` module was added to allow types to
avoid a redundant call to `cast` when serializing a value for the
database.  Because it introduced a new method (`serialize_cast_value`)
that was not part of the `ActiveModel::Type::Value` contract, it was
designed to be opt-in.  Furthermore, to guard against incompatible
`serialize` and `serialize_cast_value` implementations in types that
override `serialize` but (unintentionally) inherit `serialize_cast_value`,
types were required to explicitly include the `SerializeCastValue`
module to activate the optimization.  i.e. It was not sufficient just to
have `SerializeCastValue` in the ancestor chain.

The `SerializeCastValue` module is not part of the public API, and there
are no plans to change that, which meant user-created custom types could
not benefit from this optimization.

This commit changes the opt-in condition such that it is sufficient for
the owner of the `serialize_cast_value` method to be the same or below
the owner of the `serialize` method in the ancestor chain.  This means
a user-created type that only overrides `cast`, **not** `serialize`,
will now benefit from the optimization.  For example, a type like:

  ```ruby
  class DowncasedString < ActiveModel::Type::String
    def cast(value)
      super&.downcase
    end
  end
  ```

As demonstrated in the benchmark below, this commit does not change the
current performance of the built-in Active Model types.  However, for a
simple custom type like `DowncasedString`, the performance of
`value_for_database` is twice as fast.  For types with more expensive
`cast` operations, the improvement may be greater.

**Benchmark**

  ```ruby
  # frozen_string_literal: true

  require "benchmark/ips"
  require "active_model"

  class DowncasedString < ActiveModel::Type::String
    def cast(value)
      super&.downcase
    end
  end

  ActiveModel::Type.register(:downcased_string, DowncasedString)

  VALUES = {
    my_big_integer: "123456",
    my_boolean: "true",
    my_date: "1999-12-31",
    my_datetime: "1999-12-31 12:34:56 UTC",
    my_decimal: "123.456",
    my_float: "123.456",
    my_immutable_string: "abcdef",
    my_integer: "123456",
    my_string: "abcdef",
    my_time: "1999-12-31T12:34:56.789-10:00",
    my_downcased_string: "AbcDef",
  }

  TYPES = VALUES.to_h { |name, value| [name, name.to_s.delete_prefix("my_").to_sym] }

  class MyModel
    include ActiveModel::API
    include ActiveModel::Attributes

    TYPES.each do |name, type|
      attribute name, type
    end
  end

  attribute_set = MyModel.new(VALUES).instance_variable_get(:@attributes)

  TYPES.each do |name, type|
    attribute = attribute_set[name.to_s]

    Benchmark.ips do |x|
      x.report(type.to_s) { attribute.value_for_database }
    end
  end
  ```

**Before**

  ```
          big_integer      2.986M (± 1.2%) i/s -     15.161M in   5.078972s
              boolean      2.980M (± 1.1%) i/s -     15.074M in   5.059456s
                 date      2.960M (± 1.1%) i/s -     14.831M in   5.011355s
             datetime      1.368M (± 0.9%) i/s -      6.964M in   5.092074s
              decimal      2.930M (± 1.2%) i/s -     14.911M in   5.089048s
                float      2.932M (± 1.3%) i/s -     14.713M in   5.018512s
     immutable_string      3.013M (± 1.3%) i/s -     15.239M in   5.058085s
              integer      1.603M (± 0.8%) i/s -      8.096M in   5.052046s
               string      2.977M (± 1.1%) i/s -     15.168M in   5.094874s
                 time      1.338M (± 0.9%) i/s -      6.699M in   5.006046s
     downcased_string      1.394M (± 0.9%) i/s -      7.034M in   5.046972s
  ```

**After**

  ```
          big_integer      3.016M (± 1.0%) i/s -     15.238M in   5.053005s
              boolean      2.965M (± 1.3%) i/s -     15.037M in   5.071921s
                 date      2.924M (± 1.0%) i/s -     14.754M in   5.046294s
             datetime      1.435M (± 0.9%) i/s -      7.295M in   5.082498s
              decimal      2.950M (± 0.9%) i/s -     14.800M in   5.017225s
                float      2.964M (± 0.9%) i/s -     14.987M in   5.056405s
     immutable_string      2.907M (± 1.4%) i/s -     14.677M in   5.049194s
              integer      1.638M (± 0.9%) i/s -      8.227M in   5.022401s
               string      2.971M (± 1.0%) i/s -     14.891M in   5.011709s
                 time      1.454M (± 0.9%) i/s -      7.384M in   5.079993s
     downcased_string      2.939M (± 0.9%) i/s -     14.872M in   5.061100s
  ```
@jonathanhefner jonathanhefner force-pushed the active_model-type-inherit-serialize_cast_value branch 2 times, most recently from c4cff32 to 28ebf3c Compare October 16, 2022 21:08
@jonathanhefner jonathanhefner merged commit c2c7781 into rails:main Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant