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

Perfomance fix for ActiveSupport Enumerable#index_by #25008

Merged
merged 1 commit into from May 14, 2016

Conversation

Projects
None yet
4 participants
@lvl0nax
Contributor

lvl0nax commented May 13, 2016

module Enumerable
  # current behaviour
  def index_by
    if block_given?
      Hash[map { |elem| [yield(elem), elem] }]
    else
      to_enum(:index_by) { size if respond_to?(:size) }
    end
  end

  # new behaviour
  def index_by2
    if block_given?
      result = {}
      each {|elem| result[yield(elem)] = elem }
      result
    else
      to_enum(:index_by) { size if respond_to?(:size) }
    end
  end
end

ExpandedPayment = Struct.new(:dollars, :cents)
arr = [
  ExpandedPayment.new(5, 99),
  ExpandedPayment.new(15, 199),
  ExpandedPayment.new(25, 299),
  ExpandedPayment.new(5, 99),
  ExpandedPayment.new(15, 1199),
  ExpandedPayment.new(25, 2299),
  ExpandedPayment.new(55, 5399)
]
Benchmark.ips do |x|
  x.report('before')   { arr.index_by(&:dollars) }
  x.report('after')    { arr.index_by2(&:dollars) }
  x.compare!
end
Calculating -------------------------------------
              before    34.731k i/100ms
               after    48.206k i/100ms
-------------------------------------------------
              before    508.451k (± 1.2%) i/s -      2.570M
               after    720.068k (± 0.9%) i/s -      3.615M

Comparison:
               after:   720067.6 i/s
              before:   508451.1 i/s - 1.42x slower
@lvl0nax

This comment has been minimized.

Contributor

lvl0nax commented May 13, 2016

@rafaelfranca

View changes

activesupport/lib/active_support/core_ext/enumerable.rb Outdated
@@ -34,7 +34,9 @@ def sum(identity = nil, &block)
# => { "Chade- Fowlersburg-e" => <Person ...>, "David Heinemeier Hansson" => <Person ...>, ...}
def index_by
if block_given?
Hash[map { |elem| [yield(elem), elem] }]
result = {}
each {|elem| result[yield(elem)] = elem }

This comment has been minimized.

@rafaelfranca

rafaelfranca May 14, 2016

Member

space after the {

This comment has been minimized.

@lvl0nax

lvl0nax May 14, 2016

Contributor

Fixed. Thnx.

Perfomance fix for Enumerable#index_by
Calculating -------------------------------------
              before    34.731k i/100ms
               after    48.206k i/100ms
-------------------------------------------------
              before    508.451k (± 1.2%) i/s -      2.570M
               after    720.068k (± 0.9%) i/s -      3.615M
Comparison:
               after:   720067.6 i/s
              before:   508451.1 i/s - 1.42x slower

@rafaelfranca rafaelfranca merged commit 4046ac7 into rails:master May 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cben

This comment has been minimized.

Contributor

cben commented Nov 2, 2017

For others that may be looking at this wondering if it's important to upgrade (or patch) an app that's still on 5.0:
I run the benchmark with 100K items, from mostly same to mostly distinct keys
=> I'm seeing stable ≤1.4x factor (approaching "same-ish" on the mostly distinct end). Nothing super-linear.
(MRI 2.3.4)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment