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

unlock the gvl when calculating hashes / salts #124

Merged
merged 4 commits into from
Nov 27, 2019

Conversation

tenderlove
Copy link
Collaborator

Holding on to the GVL means we can't do anything in parallel. Since
these are CPU-only operations that do not involve the ruby interpreter,
we can safely unlock the GVL when calculating hashes.

This program demonstrates the difference:

require 'bcrypt'
require 'thread'

GUESSES = (ENV['GUESSES'] || 100).to_i
THREADS = (ENV['THREADS'] || 1).to_i

p GUESSES: GUESSES, THREADS: THREADS

password = BCrypt::Password.create 'hello world!'

queue = Queue.new
GUESSES.times { queue << "x" * 90 }
THREADS.times { queue << nil }
THREADS.times.map {
  Thread.new {
    while guess = queue.pop
      password == guess
    end
  }
}.each(&:join)

Without this patch:

[aaron@TC bcrypt-ruby (master)]$ time THREADS=4 ruby test.rb
{:GUESSES=>100, :THREADS=>4}

real    0m30.014s
user    0m29.739s
sys 0m0.153s

With the patch:

[aaron@TC bcrypt-ruby (master)]$ time THREADS=4 ruby -Ilib test.rb
{:GUESSES=>100, :THREADS=>4}

real    0m12.293s
user    0m42.382s
sys 0m0.169s

If you run this program with the patch, you can see Ruby use nearly 400% CPU,
as long as you have a 4 core machine.

Sending this as a PR because I'm not sure if the feature detection is enough to work on all versions of Ruby we support and I figured I'd let the CI test it. If something fails, I'll add better feature detection.

@compwron
Copy link

compwron commented Dec 4, 2015

So is this mergeable yet? :)
Sending this as a PR because I'm not sure if the feature detection is enough to work on all versions of Ruby we support and I figured I'd let the CI test it. If something fails, I'll add better feature detection.

ext/mri/bcrypt_ext.c Outdated Show resolved Hide resolved
@tenderlove
Copy link
Collaborator Author

I'm going to merge this. I should have merged it a long time ago. 🙇‍♂️

Bechmarks in 2019:

Without this patch:

[aaron@tc-lan-adapter ~/g/bcrypt-ruby (master)]$ time env THREADS=8 ruby -I lib bench.rb
{:GUESSES=>100, :THREADS=>8}
       18.93 real        18.60 user         0.13 sys

With this patch:

[aaron@tc-lan-adapter ~/g/bcrypt-ruby (nogvl)]$ time env THREADS=8 ruby -I lib bench.rb
{:GUESSES=>100, :THREADS=>8}
        3.20 real        19.00 user         0.12 sys

Holding on to the GVL means we can't do anything in parallel.  Since
these are CPU-only operations that do not involve the ruby interpreter,
we can safely unlock the GVL when calculating hashes.

This program demonstrates the difference:

```ruby
require 'bcrypt'
require 'thread'

GUESSES = (ENV['GUESSES'] || 100).to_i
THREADS = (ENV['THREADS'] || 1).to_i

p GUESSES: GUESSES, THREADS: THREADS

password = BCrypt::Password.create 'hello world!'

queue = Queue.new
GUESSES.times { queue << "x" * 90 }
THREADS.times { queue << nil }
THREADS.times.map {
  Thread.new {
    while guess = queue.pop
      password == guess
    end
  }
}.each(&:join)
```

Without this patch:

```
[aaron@TC bcrypt-ruby (master)]$ time THREADS=4 ruby test.rb
{:GUESSES=>100, :THREADS=>4}

real	0m30.014s
user	0m29.739s
sys	0m0.153s
```

With the patch:

```
[aaron@TC bcrypt-ruby (master)]$ time THREADS=4 ruby -Ilib test.rb
{:GUESSES=>100, :THREADS=>4}

real	0m12.293s
user	0m42.382s
sys	0m0.169s
```

If you run this program with the patch, you can see Ruby use nearly 400% CPU,
as long as you have a 4 core machine.
The crypt library is almost certainly using `malloc` and not `xmalloc`.
We should use `free` instead of `xfree` to ensure the malloc / free
calls are correctly balanced.
@tenderlove tenderlove merged commit 69f2941 into bcrypt-ruby:master Nov 27, 2019
@tenderlove tenderlove deleted the nogvl branch November 27, 2019 23:38
@sergey-alekseev
Copy link
Contributor

@tenderlove may be mention this in CHANGELOG?

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

4 participants