search

dcodeIO / bcrypt.js

Optimized bcrypt in plain JavaScript with zero dependencies.
Other
3.47k stars 264 forks source link

Adding addon option #60

Closed octaviansoldea closed 6 years ago

octaviansoldea commented 7 years ago

Dear Reviewer

This pull request is a proposal for an optional add-on. The code still runs if the add-on cannot be loaded. Using Node.js, version 7.4.0, and adding this add-on option, it is possible to optimize BCrypt.js with about 36% both in sync and async cases. For example, here are some of the measurements I got using tests/bench.js on my machine:

Using 8 rounds

Baseline

Baseline

Baseline

Baseline

In this context, I mention my machine is an Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz.

Could you please tell if you accept my solution? Do you have any feedback?

I am looking forward to hearing from you.

Best regards, Octavian

dcodeIO commented 7 years ago

Well, the point of bcryptjs is to provide a plain JS version of bcrypt, which is a native module already. Both bcrypt and bcryptjs expose pretty much the same API. Isn't that effectively identical to what you are proposing?

For example, one could add bcrypt as an optional dependency and fall back to bcryptjs where the former isn't available. It might be a neat idea to make a dedicated fallback package doing exactly this, though, if there isn't one already. The same thing could also be done with your code by packaging it, make it depend on bcryptjs and override the respective methods if the native binding is available just before exporting.

octaviansoldea commented 7 years ago

Dear Reviewer, Hi Daniel

Thank you for your fast answer. Please allow me to come back with an answer after I perform some analysis and formulate arguments for a follow up.

Best regards, Octavian

octaviansoldea commented 7 years ago

Dear Reviewer, Hi Daniel

First of all, thank you for your feedback. In this context, I modified the code such that it overrides the the implementation before export time.

In order to better motivate the adding of this overriding procedure, I would like to mention the following. The baseline code does not compile bcrypt correctly. Before a week, bcrypt was slower than bcyptjs. However, in the last downloads and installs I also observed that bcrypt is compiled for debug mode. Therefore, I accessed manually the bcrypt module and compiled it for release. In the current version, BCrypt module is indeed faster than BCrypt.js. However, the new add-on I propose manifests even better results, at more than 5%. Taking into account the consistency of the measurements, this 5% is very statistically significant.

In this context, I refer to FallBack scenario, being the code that resembles the baseline at the possible extent, i.e. when the addon is not available. The speed of the FallBack scenario is comparable with the baseline, eventually slightly slower, however, most probably not very statistically significant.

Here are some of the results regarding execution on my machine, which is an Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz.

Using 9 rounds

Baseline: Salt: $2a$09$.t3VbHsoKNiImbBCftRQGO

FallBack Salt: $2a$09$/38mREjEYo.RWnhGsk2MDu

AddOn Salt: $2a$09$qZnZDkMQhIFkafeM8uK5Ue

Using 15 rounds

Baseline Salt: $2a$15$RDnYtByjTMTTFpoMgsNon.

FallBack Salt: $2a$15$rHN1WHx.P2MZqvoYrZNxeu

AddOn Salt: $2a$15$qrdw4X75vtavCv731pWRp.

Could you please provide feedback? I am looking forward to hearing from you.

Best regards, Octavian

dcodeIO commented 7 years ago

I'd recommend creating a new package for this matter, i.e. named "bcryptjs-native", that then loads "bcryptjs" and overrides the relevant parts with native functionality and re-exports the modified library. This way around, a user can decide to use the native addon right from their package.json.

octaviansoldea commented 7 years ago

Dear Reviewer, Hi Daniel

Thank you for your message and feedback. I would like to tell that I want very much to follow the indicated path. However, I would like to ask a bit of help and clarification.

Does your suggestion means that applications that use BCrypt.js will not need a change, except, a version indication? Or is the intention that these applications change their dependencies?

Since it is my first time dealing with such a change, would you be so kind to indicate, maybe you know an example of a previous work? It will be a bit easier for me to follow such a pattern.

I am looking forward to hearing from you.

Best regards, Octavian

dcodeIO commented 7 years ago

The reason why I am proposing a separate package is that not all environments have a (working) C++ compiler. That's basically why bundling a native addon with this package isn't an option. As mentioned earlier, this package is all about being a JS-only bcrypt implementation. I must also admit that I am being a bit careful here because of possible security relevance of such a big change.

Does your suggestion means that applications that use BCrypt.js will not need a change, except, a version indication? Or is the intention that these applications change their dependencies?

Applications would have to change their dependencies to use another package.

Since it is my first time dealing with such a change, would you be so kind to indicate, maybe you know an example of a previous work? It will be a bit easier for me to follow such a pattern.

I don't have a reference at hand, but one option would be:

  1. Create a new package bcryptjs-native-cpp that contains the native functionality only (just a helper library)
  2. Create a new package bcryptjs-native that depends on bcryptjs and, as an optional dependency (in case compilation fails), on bcrypt-native-cpp.

Now, when bcryptjs-native is installed, it tries to install bcryptjs, which will always succeed, and bcryptjs-native-cpp, which might fail depending on the environment (hence: optional dependency). Then, at runtime, when bcryptjs-native is required by a dependent, it (not the dependent) first checks if the native-cpp module is available and if so, exports bcryptjs with the native parts injected, or, otherwise, exports just bcryptjs. This way around, installing bcryptjs-native would always succeed and users of the package wouldn't have to worry because all the checking is done by bcryptjs-native already.

Keep in mind that there are other viable solutions, probably. That's just how I'd do it. (I also wonder if any such package doing this with bcrypt and bcryptjs already exists.)

octaviansoldea commented 7 years ago

Dear Reviewer, Hi Daniel

Thank you for your message and feedback again. I will try to implement the modifications proposed following your guideline. In this context, could you please leave this communication channel/pull request open until I implement them? Maybe I will have some more questions regarding this module.

Thank you in advance and wishing you a nice week-end! Octavian

octaviansoldea commented 6 years ago

Hi Daniel

First, thank you very much for your kind recommendations and letting this pull request open. Due to several analyses I have done, I decided I will not implement a BCrypt module and I am changing my focus on other projects. Please feel free to close this pull request.

Of course, I will be more than happy to keep in touch for future.

Best regards, Octavian

Ruffio commented 6 years ago

If this PR is not going to be implemented, then it should be rejected. It doesn't look like there will be more activity around this one...