Implement Duration#negative? without calling public_send

[Morriar]

Motivation / Background

Inspired by @casperisfine change in #49909.

For codebases using ActiveSupport::Duration extensively, checking if the duration is negative currently requires going through the method_missing mechanism and a `public_send`` call which can be quite slow.

Instead, we can implement the method natively and get the check executed almost 4x faster.

Here's some benchmark:

ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [arm64-darwin21]
Warming up --------------------------------------
           negative?   588.000  i/100ms
    negative_native?     2.544k i/100ms
Calculating -------------------------------------
           negative?      6.738k (± 1.9%) i/s -     34.104k in   5.063211s
    negative_native?     25.804k (± 1.3%) i/s -    129.744k in   5.028821s

Comparison:
           negative?:     6738.1 i/s
    negative_native?:    25804.4 i/s - 3.83x  (± 0.00) faster

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'benchmark-ips'
  gem 'activesupport'
end

require 'active_support/all'

class ActiveSupport::Duration
  def negative_native?
    @value < 0
  end
end

values = 1000.times.map do |i|
  rand(-100..100).hours
end

puts RUBY_DESCRIPTION
Benchmark.ips do |x|
  x.report("negative?") do
    values.each do |v|
      v.negative?
    end
  end

  x.report("negative_native?") do
    values.each do |v|
      v.negative_native?
    end
  end

  x.compare!(order: :baseline)
end

Detail

This pull request implements ActiveSupport::Duration#negative? without relying on method_missing and public_send.

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.

[casperisfine]

Can you also benchmark using delegate :negative?, to: :@value?

This will be slower than your solution, but I have an idea how to optimize it that would allow to more easily eagerly define much more than just negative? without lots of extra code.

[casperisfine]

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'benchmark-ips'
  gem 'activesupport'
end

require 'active_support/all'

class ActiveSupport::Duration
  delegate :negative?, to: :@value, prefix: :delegate
  def negative_native?
    @value < 0
  end

  def raw_delegate_negative?
    @value.negative?
  end
end

values = 1000.times.map do |i|
  rand(-100..100).hours
end

puts RUBY_DESCRIPTION
Benchmark.ips do |x|
  x.report("negative?") do
    values.each do |v|
      v.negative?
    end
  end

  x.report("delegate_negative?") do
    values.each do |v|
      v.delegate_negative?
    end
  end

  x.report("raw_delegate") do
    values.each do |v|
      v.raw_delegate_negative?
    end
  end

  x.report("negative_native?") do
    values.each do |v|
      v.negative_native?
    end
  end

  x.compare!(order: :baseline)
end

ruby 3.2.2 (2023-03-30 revision e51014f9c0) [arm64-darwin22]
Warming up --------------------------------------
           negative?   548.000  i/100ms
  delegate_negative?   921.000  i/100ms
        raw_delegate   972.000  i/100ms
    negative_native?     2.180k i/100ms
Calculating -------------------------------------
           negative?      5.088k (±21.9%) i/s -     22.468k in   5.079056s
  delegate_negative?      8.907k (±15.4%) i/s -     41.445k in   5.059330s
        raw_delegate     18.692k (±18.6%) i/s -     83.592k in   5.085456s
    negative_native?     20.672k (±15.8%) i/s -     98.100k in   5.067596s

Comparison:
           negative?:     5087.9 i/s
    negative_native?:    20671.6 i/s - 4.06x  faster
        raw_delegate:    18692.2 i/s - 3.67x  faster
  delegate_negative?:     8907.3 i/s - 1.75x  faster

ruby 3.2.2 (2023-03-30 revision e51014f9c0) +YJIT [arm64-darwin22]
Warming up --------------------------------------
           negative?   313.000  i/100ms
  delegate_negative?   550.000  i/100ms
        raw_delegate     3.201k i/100ms
    negative_native?     1.898k i/100ms
Calculating -------------------------------------
           negative?      5.867k (±19.9%) i/s -     24.727k in   5.003801s
  delegate_negative?     10.095k (±14.4%) i/s -     47.300k in   5.052609s
        raw_delegate     29.698k (±21.1%) i/s -    134.442k in   5.134902s
    negative_native?     38.962k (±14.3%) i/s -    180.310k in   5.039054s

Comparison:
           negative?:     5866.7 i/s
    negative_native?:    38962.3 i/s - 6.64x  faster
        raw_delegate:    29697.5 i/s - 5.06x  faster
  delegate_negative?:    10095.0 i/s - 1.72x  faster

So answering my own question, right now delegate is still much slower (because of blind argument delegation), but if we feed it the method signature we can make is close enough to your solution.

So we could implement a ClassDelegator style of thing that generate proper delegators for all Numeric methods. Making the solution more generic.

[nvasilevski]

There is no rubocop rule for that but generally for the same reason why we prefer assert_predicate over assert we should be using assert_not_predicate as it produces a better error message. Even though assert_not_predicate may not sound very natural I think having better test failure message overweights

Suggested change

	assert_not 1.hours.negative?
	assert_not 1.days.negative?
	assert_not (1.weeks - 1.days).negative?
	assert_not_predicate 1.hours, :negative?
	assert_not_predicate 1.days, :negative?
	assert_not_predicate (1.weeks - 1.days), :negative?

[skipkayhil]

So we could implement a ClassDelegator style of thing that generate proper delegators for all Numeric methods. Making the solution more generic.

(In case you're suggesting we delegate all Numeric methods without a way to except) there are many Numeric methods that don't make sense on a Duration and I think should be deprecated ref #45437 (the least controversial probably being Duration#days type methods).

[casperisfine]

In case you're suggesting we delegate all Numeric methods without a way to except

Yeah, we can perfectly pick and chose the ones that make sense.