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

Add default: support for ActiveSupport::CurrentAttributes.attribute #50677

Merged
merged 1 commit into from Jan 10, 2024

Conversation

seanpdoyle
Copy link
Contributor

Detail

Extend the .attribute class method to accept a :default option for its list of attributes:

class Current < ActiveSupport::CurrentAttributes
  attribute :counter, default: 0
end

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rafaelfranca
Copy link
Member

Does this decrease the performance of the creation of those objects in any meaningful way?

@seanpdoyle
Copy link
Contributor Author

@rafaelfranca these are the benchmark results:

Warming up --------------------------------------
          no default    76.698k i/100ms
  default from value    79.369k i/100ms
   default from proc    79.499k i/100ms
Calculating -------------------------------------
          no default    767.078k (± 0.4%) i/s -      3.912M in   5.099426s
  default from value    778.670k (± 3.4%) i/s -      3.968M in   5.102782s
   default from proc    779.728k (± 1.9%) i/s -      3.975M in   5.099794s

Comparison:
   default from proc:   779727.8 i/s
  default from value:   778670.0 i/s - same-ish: difference falls within error
          no default:   767078.2 i/s - same-ish: difference falls within error

source:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activesupport", path: "."

  gem "benchmark-ips"
end

require "active_support/code_generator"
require "active_support/current_attributes"

class Current < ActiveSupport::CurrentAttributes
  attribute :counter
end

class CurrentWithDefaultValue < ActiveSupport::CurrentAttributes
  attribute :counter, default: 0
end

class CurrentWithDefaultProc < ActiveSupport::CurrentAttributes
  attribute :counter, default: -> { 0 }
end

Benchmark.ips do |x|
  x.report("no default") do
    instance = Current.new
    instance.counter = 0
    instance.counter += 1
    instance.reset
  end
  x.report("default from value") do
    instance = CurrentWithDefaultValue.new
    instance.counter += 1
    instance.reset
  end
  x.report("default from proc") do
    instance = CurrentWithDefaultProc.new
    instance.counter += 1
    instance.reset
  end
  x.compare!
end

@seanpdoyle seanpdoyle force-pushed the current-attributes-defaults branch 3 times, most recently from 2bda7bd to 8f8f359 Compare January 9, 2024 21:04
Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing. Can you rebase please?

Extend the `.attribute` class method to accept a `:default` option for
its list of attributes:

```ruby
class Current < ActiveSupport::CurrentAttributes
  attribute :counter, default: 0
end
```

Internally, `ActiveSupport::CurrentAttributes` will maintain a
`.defaults` class attribute to determine default values during instance
initialization.
Comment on lines +221 to +225
value =
case default
when Proc then default.call
else default.dup
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To compare to prior art, this is the code that Active Model uses when determining the default:

def value_before_type_cast
if user_provided_value.is_a?(Proc)
@memoized_value_before_type_cast ||= user_provided_value.call
else
@user_provided_value
end
end

@rafaelfranca rafaelfranca merged commit 1db9705 into rails:main Jan 10, 2024
3 of 4 checks passed
@seanpdoyle seanpdoyle deleted the current-attributes-defaults branch January 10, 2024 00:57
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Jan 11, 2024
Follow-up to rails#50677.

Prior to this commit, all `ActiveSupport::CurrentAttributes` subclasses
stored their default values in the same `Hash`, causing default values
to leak between classes.  This commit ensures each subclass maintains a
separate `Hash`.

This commit also simplifies the resolution of default values, replacing
the `merge_defaults!` method with `resolve_defaults`.
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

2 participants