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

32% Faster Object#try #33747

Merged
merged 1 commit into from
Aug 29, 2018
Merged

32% Faster Object#try #33747

merged 1 commit into from
Aug 29, 2018

Conversation

schneems
Copy link
Member

@schneems schneems commented Aug 29, 2018

Here’s the micro benchmark:

module ActiveSupport
  module NewTryable #:nodoc:
    def try(*a, &b)
      return unless a.empty? || respond_to?(a.first)
      return public_send(*a, &b) unless a.empty?

      return nil unless block_given?
      return instance_eval(&b) if b.arity == 0
      yield self
    end

    def try!(*a, &b)
      return public_send(*a, &b) if !a.empty?
      return nil unless block_given?
      return instance_eval(&b) if b.arity == 0
      yield self
    end
  end
end


module ActiveSupport
  module OldTryable #:nodoc:
    def try(*a, &b)
      try!(*a, &b) if a.empty? || respond_to?(a.first)
    end

    def try!(*a, &b)
      if a.empty? && block_given?
        if b.arity == 0
          instance_eval(&b)
        else
          yield self
        end
      else
        public_send(*a, &b)
      end
    end
  end
end

class FooNew
  include ActiveSupport::NewTryable

  def foo
  end
end

class FooOld
  include ActiveSupport::OldTryable

  def foo
  end
end 


foo_new = FooNew.new
foo_old = FooOld.new

require 'benchmark/ips'

Benchmark.ips do |x|
  x.report("old") { foo_old.try(:foo) }
  x.report("new") { foo_new.try(:foo) }
  x.compare!
end

# Warming up --------------------------------------
#                  old   144.178k i/100ms
#                  new   172.371k i/100ms
# Calculating -------------------------------------
#                  old      2.181M (± 8.0%) i/s -     10.813M in   5.001419s
#                  new      2.889M (± 7.7%) i/s -     14.479M in   5.051760s

# Comparison:
#                  new:  2888691.7 i/s
#                  old:  2180740.7 i/s - 1.32x  slower

Also reduces memory. On https://www.codetriage.com i’m seeing 1.5% fewer object allocations per request (in object count).

Before:

Total allocated: 1014475 bytes (8525 objects)

After:

Total allocated: 1015499 bytes (8389 objects)

Why?

Most of the savings come from getting rid of an arg splat *awhen calling try! from within try. This cause an extra array to be allocated. A tiny savings comes from removing the extra method call.

Did you consider?

A logical question might be why not get rid of the splat in public_send(*a, &b) for the case where there's only a single symbol and for some reason it's not as big of a savings. No idea why.

BTW if anyone shows up and wants to performance code golf, please use the microbenchmark to test your own ideas, don't post "did you consider ..." because that's kind of a jerk move to make someone else test your hypothesis for you (unless your in Rails core and you get a pass). Plus you'll get valuable benchmarking experience!

@matthewd
Copy link
Member

I follow the gain from inlining try! (though given we generally avoid calling either, especially in hot spots, I'm not sure how valuable it really is).

I don't see why try! is changing though. Is that just cosmetic?

A logical question might be why not get rid of the splat in public_send(*a, &b) for the case where there's only a single symbol and for some reason it's not as big of a savings. No idea why.

I suspect you'll find the saving is in avoiding the splat capture in try!, rather than the splat expansion in the call: that's the one that allocates, after all.

@schneems
Copy link
Member Author

I suspect you'll find the saving is in avoiding the splat capture in try!, rather than the splat expansion in the call: that's the one that allocates, after all.

Ahh, that makes sense. Thanks. My first "DRY" attempt of fixing this was to introduce a private method without the splat, but I like this approach better.

I don't see why try! is changing though. Is that just cosmetic?

This version was easier for me to see that both versions are identical. I'll change it back to the original logic.

Here’s the micro benchmark:

```ruby
module ActiveSupport
  module NewTryable #:nodoc:
    def try(*a, &b)
      return unless a.empty? || respond_to?(a.first)
      return public_send(*a, &b) unless a.empty?

      return nil unless block_given?
      return instance_eval(&b) if b.arity == 0
      yield self
    end

    def try!(*a, &b)
      return public_send(*a, &b) if !a.empty?
      return nil unless block_given?
      return instance_eval(&b) if b.arity == 0
      yield self
    end
  end
end


module ActiveSupport
  module OldTryable #:nodoc:
    def try(*a, &b)
      try!(*a, &b) if a.empty? || respond_to?(a.first)
    end

    def try!(*a, &b)
      if a.empty? && block_given?
        if b.arity == 0
          instance_eval(&b)
        else
          yield self
        end
      else
        public_send(*a, &b)
      end
    end
  end
end

class FooNew
  include ActiveSupport::NewTryable

  def foo
  end
end

class FooOld
  include ActiveSupport::OldTryable

  def foo
  end
end 


foo_new = FooNew.new
foo_old = FooOld.new

require 'benchmark/ips'

Benchmark.ips do |x|
  x.report("old") { foo_old.try(:foo) }
  x.report("new") { foo_new.try(:foo) }
  x.compare!
end

# Warming up --------------------------------------
#                  old   144.178k i/100ms
#                  new   172.371k i/100ms
# Calculating -------------------------------------
#                  old      2.181M (± 8.0%) i/s -     10.813M in   5.001419s
#                  new      2.889M (± 7.7%) i/s -     14.479M in   5.051760s

# Comparison:
#                  new:  2888691.7 i/s
#                  old:  2180740.7 i/s - 1.32x  slower
```

Also reduces memory. On https://www.codetriage.com i’m seeing 1.5% fewer object allocations per request (in object count).

Before:

Total allocated: 1014475 bytes (8525 objects)

After:

Total allocated: 1015499 bytes (8389 objects)
@schneems
Copy link
Member Author

schneems commented Aug 29, 2018

Updated to use original logic inside the method. Still preserving the removal of the splat expansion and the reduction in method calls.

@schneems schneems merged commit 96d7504 into rails:master Aug 29, 2018
@sgrif
Copy link
Contributor

sgrif commented Aug 29, 2018

There's definitely another 20% win by pulling out the method name into its own argument. Will open a followup PR

@sgrif sgrif mentioned this pull request Aug 29, 2018
schneems pushed a commit that referenced this pull request Aug 29, 2018
Following up on #33747, this takes things a step further by pulling out
the method name from the arguments array, letting us skip an allocation
in the case where there are no arguments -- notably, this also no longer
*requires* the splat to be an array, allowing us to benefit from
optimizations in Jruby (and maybe MRI in the future) of skipping the
array allocation entirely.

Benchmark results:

```
Warming up --------------------------------------
                 old   179.987k i/100ms
                 new   199.201k i/100ms
Calculating -------------------------------------
                 old      3.029M (± 1.6%) i/s -     15.299M in   5.052417s
                 new      3.657M (± 1.2%) i/s -     18.326M in   5.012648s

Comparison:
                 new:  3656620.7 i/s
                 old:  3028848.3 i/s - 1.21x  slower
```
@schneems schneems mentioned this pull request Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants