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

Speed up xor_byte_strings by 70% #32931

Merged
merged 1 commit into from May 22, 2018

Conversation

Projects
None yet
6 participants
@jeremyevans
Contributor

jeremyevans commented May 18, 2018

Benchmark:

require 'benchmark/ips'
require 'securerandom'

def xor_byte_strings(s1, s2) # :doc:
  s2_bytes = s2.bytes
  s1.each_byte.with_index { |c1, i| s2_bytes[i] ^= c1 }
  s2_bytes.pack("C*")
end

def xor_byte_strings_new(s1, s2) # :doc:
  s2 = s2.dup
  size = s1.bytesize
  i = 0
  while i < size
    s2.setbyte(i, s1.getbyte(i) ^ s2.getbyte(i))
    i += 1
  end
  s2
end

s1 = SecureRandom.random_bytes(32)
s2 = SecureRandom.random_bytes(32)

Benchmark.ips do |x|
  x.report("current"){xor_byte_strings(s1, s2)}
  x.report("new"){xor_byte_strings_new(s1, s2)}
  x.compare!
end

100000.times do |i|
  s3 = SecureRandom.random_bytes(32)
  s4 = SecureRandom.random_bytes(32)
  raise unless xor_byte_strings(s3, s4) == xor_byte_strings_new(s3, s4)
end

Results on ruby 2.5.1:

Warming up --------------------------------------
             current     6.519k i/100ms
                 new    10.508k i/100ms
Calculating -------------------------------------
             current     84.723k (_ 0.4%) i/s -    423.735k in   5.001456s
                 new    145.871k (_ 0.3%) i/s -    735.560k in   5.042606s

Comparison:
                 new:   145870.6 i/s
             current:    84723.4 i/s - 1.72x  slower
Speed up xor_byte_strings by 70%
Benchmark:

```ruby
require 'benchmark'
require 'benchmark/ips'
require 'securerandom'

def xor_byte_strings(s1, s2) # :doc:
  s2_bytes = s2.bytes
  s1.each_byte.with_index { |c1, i| s2_bytes[i] ^= c1 }
  s2_bytes.pack("C*")
end

def xor_byte_strings_new(s1, s2) # :doc:
  s2 = s2.dup
  size = s1.bytesize
  i = 0
  while i < size
    s2.setbyte(i, s1.getbyte(i) ^ s2.getbyte(i))
    i += 1
  end
  s2
end

s1 = SecureRandom.random_bytes(32)
s2 = SecureRandom.random_bytes(32)

Benchmark.ips do |x|
  x.report("current"){xor_byte_strings(s1, s2)}
  x.report("new"){xor_byte_strings_new(s1, s2)}
  x.compare!
end

100000.times do |i|
  s3 = SecureRandom.random_bytes(32)
  s4 = SecureRandom.random_bytes(32)
  raise unless xor_byte_strings(s3, s4) == xor_byte_strings_new(s3, s4)
end
```

Results on ruby 2.5.1:

```
Warming up --------------------------------------
             current     6.519k i/100ms
                 new    10.508k i/100ms
Calculating -------------------------------------
             current     84.723k (_ 0.4%) i/s -    423.735k in   5.001456s
                 new    145.871k (_ 0.3%) i/s -    735.560k in   5.042606s

Comparison:
                 new:   145870.6 i/s
             current:    84723.4 i/s - 1.72x  slower
```
@rails-bot

This comment has been minimized.

rails-bot commented May 18, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@guilleiguaran guilleiguaran requested a review from tenderlove May 21, 2018

@rafaelfranca rafaelfranca merged commit 7ee898c into rails:master May 22, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@printercu

This comment has been minimized.

Contributor

printercu commented May 27, 2018

It's known that block loops are slightly slower, buts it's idiomatic in ruby. This does not mean that all loops are to be rewritten with while loops. Please consider this solution:

def xor_byte_strings_new(s1, s2) # :doc:
  s2 = s2.dup
  s1.size.times { |i| s2.setbyte(i, s1.getbyte(i) ^ s2.getbyte(i)) }
  s2
end

It's only 20% slower comparing to while version.

@gregmolnar

This comment has been minimized.

Member

gregmolnar commented May 28, 2018

@printercu I think that 20% matters more in this case than keeping the code idiomatic in ruby.

@printercu

This comment has been minimized.

Contributor

printercu commented May 28, 2018

Well, let's start further optimisations from this file. There is #any? call: https://github.com/jeremyevans/rails/blob/8b10a9414dd30817b1fc9c4c8cb7600cca0d15b3/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L295
I've just benchmarked, and while version brings 27% performance improvement. It's almost 1.5 times more than 20% in previous optimisation.

require 'benchmark/ips'
require 'securerandom'

def matches?(x)
  x > 5
end

def with_while(ar) # :doc:
  size = ar.size
  i = 0
  while i < size
    return true if matches?(ar[i])
    i += 1
  end
  false
end

def with_block(ar) # :doc:
  ar.any? { |x| matches?(x) }
end

ar = 10.times.to_a

Benchmark.ips do |x|
  x.report("while"){with_while(ar)}
  x.report("any"){with_block(ar)}
  x.compare!
end
Warming up --------------------------------------
               while   114.070k i/100ms
                 any    96.216k i/100ms
Calculating -------------------------------------
               while      1.780M (± 5.9%) i/s -      8.897M in   5.016393s
                 any      1.397M (±10.5%) i/s -      6.928M in   5.034635s

Comparison:
               while:  1780304.1 i/s
                 any:  1396520.4 i/s - 1.27x  slower
@gregmolnar

This comment has been minimized.

Member

gregmolnar commented May 28, 2018

@printercu you are right. Do you want to open a PR for that?

@printercu

This comment has been minimized.

Contributor

printercu commented May 28, 2018

For what? Changing back to block version or converting everything to while?

@gregmolnar

This comment has been minimized.

Member

gregmolnar commented May 28, 2018

@printercu I would go with the fastest implementation which seems to be the while loop, but I am not the one who makes the call here.

@printercu

This comment has been minimized.

Contributor

printercu commented May 28, 2018

I'm really sorry, it was a joke. Ruby is not a right language for micro-optimisations. It always was about readability and programmer's happiness. Please see my example with #any? again. There are 7 times more code in while-version, and it's far from as readable as block-version.

@gregmolnar

This comment has been minimized.

Member

gregmolnar commented May 28, 2018

I don't agree on that. There are cases where micro-optimisations matter more than readability or the length of the code. If you can make a quite often called chunk of code almost 2x faster while making it less idiomatic, I believe that's the right thing to do.

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