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

Little perfomance fix for Array#split. #24723

Merged
merged 1 commit into from Apr 26, 2016

Conversation

Projects
None yet
6 participants
@lvl0nax
Contributor

lvl0nax commented Apr 25, 2016

class Array
  # current behavior
  def split(value = nil)
    if block_given?
      inject([[]]) do |results, element|
        if yield(element)
          results << []
        else
          results.last << element
        end

        results
      end
    else
      results, arr = [[]], self.dup
      until arr.empty?
        if (idx = arr.index(value))
          results.last.concat(arr.shift(idx))
          arr.shift
          results << []
        else
          results.last.concat(arr.shift(arr.size))
        end
      end
      results
    end
  end

  # proposed behavior
  def split2(value = nil)
    if block_given?
      inject([[]]) do |results, element|
        if yield(element)
          results << []
        else
          results.last << element
        end

        results
      end
    else
      arr = self.dup
      result = []
      while (idx = arr.index(value))
        result << arr.shift(idx)
        arr.shift
      end
      result << arr
    end
  end
end
arr = (1..12).to_a

Benchmark.ips do |x|
     x.report('before') { arr.split(3) }
     x.report('after') { arr.split2(3) }
end
          Calculating -------------------------------------
          before    40.770k i/100ms
           after    58.464k i/100ms
          -------------------------------------------------
          before    629.568k (± 5.0%) i/s -      3.180M
           after      1.159M (± 4.5%) i/s -      5.788M
@prathamesh-sonpatki

This comment has been minimized.

@vipulnsward

View changes

activesupport/lib/active_support/core_ext/array/grouping.rb Outdated
end
end
results
(idx = index(value)) ? [first(idx), last(size-idx-1)] : [self]

This comment has been minimized.

@vipulnsward

vipulnsward Apr 25, 2016

Member

: [self.dup]

This comment has been minimized.

@lvl0nax

lvl0nax Apr 25, 2016

Contributor

Thnx. Fixed.

@lvl0nax lvl0nax force-pushed the lvl0nax:array_split_fix branch Apr 25, 2016

@hechen0

This comment has been minimized.

hechen0 commented Apr 25, 2016

change the original split behavior.

a
=> [1, 2, 3, 0, 123, 12, 3, 0]
a.split(0)
=> [[1, 2, 3], [123, 12, 3], []]
a.split2(0)
=> [[1, 2, 3], [123, 12, 3, 0]]

@vipulnsward

This comment has been minimized.

Member

vipulnsward commented Apr 25, 2016

Yes, was just going to say that. cc @amatsuda
@lvl0nax can you check if it is possible to achieve current behaviour? If not, current behaviour will need to stay.

Also add a test case for scenario @hechen0 mentioned.

@lvl0nax lvl0nax force-pushed the lvl0nax:array_split_fix branch Apr 25, 2016

@lvl0nax

This comment has been minimized.

Contributor

lvl0nax commented Apr 25, 2016

@vipulnsward I have achieved current behavior and added test case for scenario @hechen0 mentioned.

@kaspth

View changes

activesupport/test/core_ext/array/grouping_test.rb Outdated
assert_equal [[1, 2], [5, 5], [4, 6, 2, 1], []], a.split(3)
assert_equal [[1, 2, 3], [], [3, 4, 6, 2, 1, 3]], a.split(5)
assert_equal [[1, 2], [], [], [], [4, 6, 2, 1], []], a.split { |i| i == 3 || i == 5 }
assert_equal [1, 2, 3, 5, 5, 3, 4, 6, 2, 1, 3], a

This comment has been minimized.

@kaspth

kaspth Apr 25, 2016

Member

Are you testing for mutation? Otherwise this doesn't really make sense :)

This comment has been minimized.

@lvl0nax

lvl0nax Apr 25, 2016

Contributor

Yes. I'm testing mutation. But this is because all previous tests check the mutation too.

@kaspth

This comment has been minimized.

Member

kaspth commented Apr 25, 2016

How does the benchmark perform with this newer-new version?

@lvl0nax

This comment has been minimized.

Contributor

lvl0nax commented Apr 25, 2016

@kaspth Benchmark in description was updated with PR.

      Calculating -------------------------------------
      before    40.770k i/100ms
       after    58.464k i/100ms
      -------------------------------------------------
      before    629.568k (± 5.0%) i/s -      3.180M
       after      1.159M (± 4.5%) i/s -      5.788M
@jeremy

This comment has been minimized.

Member

jeremy commented Apr 26, 2016

Looking good. Do you feel behavior is backward compatible now?

Could you include the benchmark result in the commit message?

Little perfomance fix for Array#split.
Calculating -------------------------------------
before    40.770k i/100ms
after    58.464k i/100ms
-------------------------------------------------
before    629.568k (± 5.0%) i/s -      3.180M
after      1.159M (± 4.5%) i/s -      5.788M

@lvl0nax lvl0nax force-pushed the lvl0nax:array_split_fix branch to ffb1df5 Apr 26, 2016

@lvl0nax

This comment has been minimized.

Contributor

lvl0nax commented Apr 26, 2016

@jeremy, I think new behavior is backward compatible now. And yes, benchmark result is now included in the commit message.

@jeremy jeremy merged commit ffb1df5 into rails:master Apr 26, 2016

jeremy added a commit that referenced this pull request Apr 26, 2016

Merge pull request #24723 from lvl0nax/array_split_fix
Little perfomance fix for Array#split.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment