Skip to content

unlock the gvl when calculating hashes / salts#124

Merged
tenderlove merged 4 commits intobcrypt-ruby:masterfrom
tenderlove:nogvl
Nov 27, 2019
Merged

unlock the gvl when calculating hashes / salts#124
tenderlove merged 4 commits intobcrypt-ruby:masterfrom
tenderlove:nogvl

Conversation

@tenderlove
Copy link
Copy Markdown
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.

@tenderlove tenderlove force-pushed the nogvl branch 2 times, most recently from e02b148 to c800cf0 Compare October 28, 2015 01:01
@compwron
Copy link
Copy Markdown

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.

@tenderlove
Copy link
Copy Markdown
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
Copy Markdown
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.

4 participants