-
Notifications
You must be signed in to change notification settings - Fork 2
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
chore(NODE-5455): benchmark FLE #16
Conversation
As predicted throughput has not changed with this change, we do not cross an FFI or js/native boundary when calling the accessor functions, we can still adopt the fields if that's considered "modernizing" our use of libmongocrypt |
bd2dab8
to
2431e57
Compare
2431e57
to
6decb68
Compare
Switching from cryptoCallbacks to native crypto: #18 shows good signs of being a large perf improvement |
@@ -0,0 +1,87 @@ | |||
import crypto from 'node:crypto'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of duplicating these here, can we add the driver as a dev dependency and use the hooks defined in the driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the desire to de-dupe but I think that's overkill. I imagine we'll delete the ones in the driver after switching to native crypto, which would make this script no longer work with a ^
caret dependency on the driver. Additionally, those callbacks aren't public driver API so even if they don't get removed an innocent refactor could break an import here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh but this looks like a perfect opportunity to move these backs into the bindings source code (i.e. next to index.ts)? I still believe that moving these into the driver was a mistake, if we need them here in the tests that's just yet another reason to move them back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine we'll delete the ones in the driver after switching to native crypto
There's probably going to be a need for JS crypto callbacks until something like the alternative approach mentioned in the ticket here is implemented – We can't use libmongocrypt's native crypto integration on macOS and Windows in mongosh because we need to be able to toggle the FIPS mode switch for those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move these backs into the bindings source code (i.e. next to index.ts)?
Not opposed, we still need to leave the callbacks in the driver so bindings versions below the next release continue working.
We can't use libmongocrypt's native crypto integration on macOS and Windows
I think reintroducing them to the src will fit better in #18 since currently I've essentially prevented callbacks from working unless you build from libmongocrypt src using nocrypto.
I will ping you on that PR when it is ready.
something like the alternative approach mentioned in the ticket here is implemented
What's this referencing? Are you referring to what #18 is attempting, using builtin OpenSSL? Or something specific with FIPS mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to use the opensll that's bundled and exposed with Nodejs on all platforms for the crypto hooks (i.e., c++ crypto callbacks). That would take the place of #18.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this morning's discussion I think we have what we need here for this PR. In the following PR where we implement C++ callbacks we can also bring in the JS callbacks if we desire. Is that accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't bring the JS callbacks over, that's breaking. But other than that, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't bring the JS callbacks over, that's breaking
Why is it breaking (for mongodb-client-encryption)?
Description
What is changing?
Is there new documentation needed for these changes?
No
What is the motivation for this change?
Determine what the performance of FLE is.
Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript