-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
crypto: refactor to avoid unsafe array iteration #37364
Conversation
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/948/ EDIT: Overall perf looks good.
|
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 am beginning to wonder if we could use some kind of transpiler instead to avoid making our source code less readable and harder to understand, for marginal benefits.
} else if (!allowKeyObject) { | ||
return ArrayPrototypeSlice(types, 0, 5); | ||
} | ||
return types; |
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.
This is so much less intuitive than the original implementation...
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.
Agreed, I have considered using .push
to add the entries instead of slicing them out, but ended up doing it that way to improve the performance.
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.
😐
if (name !== 'AES-KW') | ||
ArrayPrototypePush(checkUsages, 'encrypt', 'decrypt'); | ||
|
||
const usagesSet = new SafeSet(keyUsages); | ||
if (hasAnyNotIn(usagesSet, ...checkUsages)) { | ||
if (ReflectApply(hasAnyNotIn, null, checkUsages)) { |
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.
These call sites of hasAnyNotIn
are much harder to read now, for no real benefit.
It might be better to simply change hasAnyNotIn
to accept an array as the second argument instead of a variable number of arguments.
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.
OK, I'll try that, if it doesn't have a negative perf impact I'm all for it.
break; | ||
} | ||
if (hasAnyNotIn(usages, ...checkSet)) { | ||
if (ReflectApply(hasAnyNotIn, null, args)) { |
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.
Dito.
break; | ||
} | ||
if (hasAnyNotIn(usages, ...checkSet)) { | ||
if (hasAnyNotIn(usages, check)) { |
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.
Dito.
break; | ||
} | ||
} | ||
if (hasAnyNotIn(usages, ...checkSet)) { | ||
if (ReflectApply(hasAnyNotIn, null, args)) { |
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.
Dito.
break; | ||
} | ||
} | ||
if (hasAnyNotIn(usages, ...checkSet)) { | ||
if (ReflectApply(hasAnyNotIn, null, args)) { |
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.
Dito.
Blocked by #37433. |
Actually, I don't think it's useful to wait for #37433, I'll resolve the git conflict there. |
PR-URL: nodejs#37364 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
4384da5
to
08a2383
Compare
Landed in 08a2383 |
PR-URL: #37364 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
No description provided.