Repository navigation
Conversation
nodejs-github-bot
added
build
|
btw, side note to @trevnorris and @addaleax this demonstrates one of the methods we can use to evolve |
|
Probably warrants running benchmarks to compare before/after perf? |
|
Also needs a new CI run but will hold off until the OSX build bot is back up |
There was a problem hiding this comment.
Can we convert these to use the module.exports = { ... } format instead? The reason is that at least exports.DEFAULT_ENCODING is accessed quite frequently in the crypto code here and having a faster property lookup would be beneficial.
|
Why does the code need to move to the internal/ directory? |
|
Also, I agree that we need to have some solid benchmark results for these changes. |
@mscdex ... Organization mostly, I'd like to separate the various related classes into separate files under |
|
@jasnell If it's unavoidable then that's ok. I just don't want to start stuffing things in internal/ unless it's necessary, since I thought the idea behind internal/ was mostly to house things that shouldn't be (or shouldn't have been) exposed to end users. |
|
Here are the benchmarks comparing this PR to 7.4.0: https://gist.github.com/jasnell/fe734fdc57ca143daf6c43c2cc1cb95b The results appear to show no significant change in most cases and significant improvement in others. |
|
@mscdex ... updated to use the |
|
Ping @nodejs/crypto |
|
I'd like to run the benchmarks on this first. |
|
@mscdex ... definitely. I did one run on the benchmarks here: https://gist.github.com/jasnell/fe734fdc57ca143daf6c43c2cc1cb95b Please let me know if these align with the results you get. |
sam-github
left a comment
sam-github
left a comment
There was a problem hiding this comment.
I'm generally a favour of giant files being broken up, but this just seems to move all the contents from lib/crypto.js to lib/internal/crypto/crypto.js, is it a first step to dividing the classes into their own files?
There was a problem hiding this comment.
Its semver-major when a property gets replaced by a pair of accessor functions, isn't it?
There was a problem hiding this comment.
Yes, I thought I had already labeled it that way but looks like I missed it. Added!
There was a problem hiding this comment.
Isn't this a type change? Before, typeof crypto.createHash() === crypto.Hash, but now it doesn't? Why not exports.createHash = exports.Hash = internalCrypto.Hash;?
There was a problem hiding this comment.
is the setHasInstance() supposed to fixup the types after the fact?
There was a problem hiding this comment.
The setHasInstance() allows instanceof to continue working. crypto.createHash() === crypto.Hash should still work here. The two are still aliases of one another. Both will return an instance of the internal Hash class, and the setHasInstance() sets it up so that instanceof keeps working as expected.
There was a problem hiding this comment.
please sort the require lines
There was a problem hiding this comment.
its hard to tell, are there any changes in this code, or is just being moved?
There was a problem hiding this comment.
Moved and refactored in the ES6 class. There are no functional changes with the exception of a small bug fix.
Yes. The idea here is to ease the maintenance burden over time, but in chunks rather than all at once. |
jasnell
added
the
semver-major
|
Given that all the code is being removed from lib/crypto.js, so that no PR touching that file after this change can backport past it, and a split of lib/internal/crypto.js into more small files will create a similar backport barrier, I'd favour doing the split. Its mostly textual, I'd think, the big file is just a series of indepenent code, well suited to being broken up. I'm a bit worried about the backport barrier, but I guess we can't ever improve the code base if we don't bit some bullets. The bullet would be more palatable if the move to internal reduced the API surface exposed to node users, but I don't think crypto has any unnecessary API surface (other than _toBuf?) to remove. |
|
+1... I was going to suggest deprecating the exposed |
What is bug? I can't find it. I don't think a commit should both move code, and fix bugs, its not possible to see the bug-fixing code change in the diff, and the commit message doesn't mention a bug fix, though the PR description does. Is it possible to PR the bug fix seperately, so it can be backported and visible in history? @jasnell any comment on my last comment? I prefer one break-any-backports event, rather than two: why not break lib/internal/crypto.js up immediately into the file-per-algorithm that is intended? |
|
Other than my two concerns above, I'm in favour of this (in case it wasn't clear). |
|
I also like the idea of deprecating _toBuf (in a follow-on PR), so it can be deprecated in 8.x, and removed in a subsequent major. |
|
@sam-github ... splitting up the files is reasonable. I'll update this PR with that change. The bug fixed is minor, see https://github.com/nodejs/node/pull/11138/files#diff-437b078f0e3aa5b13790315b3813fd4fL518 Basically, there's an argument that was being checked to see if the value was a number, but then the check was ignored after. It looks like there was an intent at some point to support number arguments but it wasn't completed. I checked the documentation and passing in a number for the argument is not described. |
009c3c8 to
ad006bf
Compare
|
@sam-github ... updated with the files split out. |
There was a problem hiding this comment.
Can we change these to use the module.exports = { ... } format instead?
There was a problem hiding this comment.
As well as any other (similar) pre-existing uses of exports.* = ...
There was a problem hiding this comment.
I suppose we could. It would require quite a bit more moving things around but it's not out of the question
There was a problem hiding this comment.
On an unrelated note.... I think we should deprecate this (along with the same in Decipheriv). I don't know why this was ever added and it's not even documented.
There was a problem hiding this comment.
Yep, I was planning on doing so in a separate follow-up PR.
0c4aad1 to
995a6b4
Compare
|
@mscdex ... ok, refactored to use the |
There was a problem hiding this comment.
I think we can add these special properties during module.exports creation by using Object.create() instead with all properties.
There was a problem hiding this comment.
Would doing so give any perf advantage?
There was a problem hiding this comment.
Well, the point of doing the module.exports = { ... } is to avoid hidden classes, so these last few Object.defineProperties() are creating hidden classes AFAIK (akin to what was being done before -- exports.foo = bar).
|
@mscdex ... updated with the Object.create suggestion! |
There was a problem hiding this comment.
Sorry, I meant that these properties should go in the object passed as the second argument. This first argument is for defining the prototype, the second is for defining on the object itself.
For the prototype argument, I guess we should use Object.prototype or whatever to be on the safe side, although I would've preferred null.
There was a problem hiding this comment.
I reverted that last change... not sure I'm +1 on doing it the way you're suggesting. Will think about it some more
556e22e to
49ec8ac
Compare
49ec8ac to
629ea29
Compare
|
@nodejs/ctc @nodejs/crypto @sam-github ... updated this and rebased. PTAL |
|
Failure in CI is unrelated. |
|
some benchmark results after refactor: |
|
The stream regressions are a bit concerning. Have you had a chance to find out why it slows down (so much) in those particular cases? |
|
That result appears to be rather inconsistent. I'll be running a few more benchmarks on a couple of different platforms to see if those results hold up. |
Refactor the crypto module to make the code more readable and maintainable. Leverages ES6 classes. No test changes are required. Existing code should keep working. Provide improved inspect support for crypto classes. Remove dead code in ECDH.prototype.getPublicKey with an unused arg type check.
629ea29 to
846fe06
Compare
| assert.throws(() => { | ||
| crypto.createECDH(); | ||
| }, /^TypeError: "curve" argument should be a string$/); | ||
| }, /^TypeError: ECDH curve name must be a string$/); |
There was a problem hiding this comment.
Why is this semver-major? It looked like an internal organization that made no API changes, except for this error message change. Can the error message be made backwards compatible, so we can backport everything else, and just do a semver-major PR afterwards to update the error message for 8.x?
|
Are there reasons for this to be semver-major, other than the error message change? |
|
It's marked major defensively until I can be sure it's not. If/when this
lands I will be working on a backport pr
…On Mon, Feb 20, 2017 at 6:14 PM Sam Roberts ***@***.***> wrote:
Are there reasons for this to be semver-major, other than the error
message change?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11138 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAa2eQxzLY5Xzmr7C4592_lefo4UYxuaks5redgMgaJpZM4L1_ag>
.
|
|
Marking this as in progress. It needs a rebase and I need to figure out the perf regression in the stream api fstill |
|
@jasnell it might be interesting to look into this again and to rerun the benchmarks again due to the changes in v8. |
|
I plan to! It'll be a few weeks tho |
|
Closing this. Opening a new, updated PR with a slightly different take. |
8 participants
Refactor the crypto module to make the code more readable and maintainable. Leverages ES6 classes. No test changes are required. Existing code should keep working.
Provide improved inspect support for crypto classes.
Fix bug in ECDH.prototype.getPublicKey
No changes to tests are required.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
crypto