Skip to content

Adds bcrypt native binding for better login performance #2549

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

Merged
merged 3 commits into from
Aug 19, 2016

Conversation

flovilmart
Copy link
Contributor

@flovilmart flovilmart commented Aug 18, 2016

Fixes #2539

  • Adds optional bcrypt native binding for better performance
  • Defaults salt rounds to 10

 Well... Here are some benchmarks. Ran with ab -n200 -c100 locally 5 times for each module.

  • Baseline (immediately fulfill promises in password.js without bcrypt use): 250 req/sec
  • bcrypt-nodejs (current implementation): 5 req/sec
  • bcryptjs: 5 req/sec
  • twin-bcrypt: 5 req/sec
  • bcrypt (native bindings): 52 req/sec

@codecov-io
Copy link

codecov-io commented Aug 18, 2016

Current coverage is 92.20% (diff: 100%)

Merging #2549 into master will decrease coverage by 0.01%

@@             master      #2549   diff @@
==========================================
  Files            97         97          
  Lines         11814      11817     +3   
  Methods        1454       1454          
  Messages          0          0          
  Branches       1906       1906          
==========================================
+ Hits          10895      10896     +1   
- Misses          919        921     +2   
  Partials          0          0          

Powered by Codecov. Last update 9ecb9a3...9f31d81

@JeremyPlease
Copy link
Contributor

@flovilmart It's important that we use bcrypt native bindings for the compare function too.

@flovilmart
Copy link
Contributor Author

It's gonna be using it but the signature of the methods are the same, so no need to override the method

@@ -15,6 +14,21 @@ function hash(password) {
});
}

try {
bcrypt = require('bcrypt');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we override the bcrypt module altogether, so if it loads, the next bcrypt.compare will correctly call.

I had to do it that way for hash as both modules don't share the same signature.

@ghost
Copy link

ghost commented Aug 19, 2016

@flovilmart updated the pull request - view changes

@ghost
Copy link

ghost commented Aug 19, 2016

@flovilmart updated the pull request - view changes

@drew-gross drew-gross merged commit 3a08ec9 into master Aug 19, 2016
@drew-gross drew-gross deleted the bcrypt-optimization branch August 19, 2016 20:53
caoer added a commit to caoer/parse-server that referenced this pull request Aug 29, 2016
* ParsePlatform/master: (100 commits)
  Only allow basic auth credentials with a known appId (parse-community#2574)
  vk.com provider registered (parse-community#2579)
  chore(package): update parse-server-push-adapter to version 1.1.0 (parse-community#2588)
  vk.com auth data manager implemented (parse-community#2578)
  Fix a typo (parse-community#2563)
  Makes sure routes don't overlap and yield a header set error (parse-community#2559)
  Postgres: $all, $and CLP and more (parse-community#2551)
  Changelog 2.2.18 (parse-community#2558)
  chore(package): update winston-daily-rotate-file to version 1.3.0 (parse-community#2547)
  chore(package): update parse-server-s3-adapter to version 1.0.5 (parse-community#2536)
  Adds bcrypt native binding for better login performance (parse-community#2549)
  chore(package): update mongodb to version 2.2.7 (parse-community#2554)
  Make parse-server cloud code logging closer parse.com legacy (parse-community#2550)
  chore(package): update pg-promise to version 5.3.1 (parse-community#2519)
  Postgres: Operations, Hooks, OAuth login, Files support (parse-community#2528)
  Syncing afterSave/afterDelete trigger calls (Issue parse-community#2489) (parse-community#2499)
  Updated README.md (parse-community#2538)
  Fix capitalization, typo, and grammar mistake (parse-community#2533)
  Update ISSUE_TEMPLATE.md
  fix typo (parse-community#2525)
  ...
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