Skip to content
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: make createXYZ inlineable #16067

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Oct 7, 2017

This commit increase by around 10% hot code paths that are hitting
createXYZ functions. Before this change the createXYZ called the XYZ
constructor without new.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

@mcollina mcollina requested a review from jasnell October 7, 2017 18:27
@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Oct 7, 2017
@mcollina
Copy link
Member Author

mcollina commented Oct 7, 2017

This is just an example. We should do the same for all the rest of the createXYZ methods in crypto, and maybe in other places as well.

@mcollina mcollina requested a review from bmeurer October 7, 2017 18:30
@mscdex
Copy link
Contributor

mscdex commented Oct 7, 2017

Yes, I think we should be doing it for the rest as well.

@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Oct 7, 2017
@mcollina
Copy link
Member Author

mcollina commented Oct 7, 2017

The reason it was not inlining was with the following output:

Not inlining Hash into etag because call is recursive

@mcollina
Copy link
Member Author

mcollina commented Oct 7, 2017

@mscdex do you prefer if we do all of them here, or we do this into separate PRs?

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

With the understanding that this is going to be done with all the other creation functions, LGTM.

Copy link
Member

@bmeurer bmeurer left a comment

Choose a reason for hiding this comment

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

LGTM.

For the record: We removed the recursive inlining restriction recently.

@mcollina
Copy link
Member Author

mcollina commented Oct 7, 2017

@bmeurer is this worth doing then? In which version of V8 this is going to be part of?
The problem I describe is still present in node master.

@mcollina mcollina changed the title crypto: make createHash inlineable crypto: make createXYZ inlineable Oct 7, 2017
@mcollina
Copy link
Member Author

mcollina commented Oct 7, 2017

I have ported all the createXYZ functions.

@mscdex
Copy link
Contributor

mscdex commented Oct 7, 2017

I think it's worth doing for now, especially for node v8.x which may not see the version of V8 where the fixed is applied.

@mscdex
Copy link
Contributor

mscdex commented Oct 7, 2017

lib/crypto.js Outdated
@@ -79,23 +79,67 @@ const {
} = require('internal/crypto/util');
const Certificate = require('internal/crypto/certificate');

function createHash(algorithm, options) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a quick comment explaining the optimization.

@mcollina
Copy link
Member Author

mcollina commented Oct 7, 2017

@bengl done.

@bmeurer
Copy link
Member

bmeurer commented Oct 8, 2017

@mcollina I just noticed that we still disallow direct recursive inlining (as of 08bfcb293cfff2d71bcfb28fae8679d0b29a3d5c), so even though indirect recursion is handled, direct recursion is not.

These changes are in V8 6.2. So for Node 8 it definitely makes sense to land this change.

lib/crypto.js Outdated
@@ -79,23 +79,69 @@ const {
} = require('internal/crypto/util');
const Certificate = require('internal/crypto/certificate');

// These function are needed because Hash() calls new Hash()
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nits:

s/function/helper functions/

Also, might use more generic wording like 'These helper functions are needed because the constructors can use new, in which case V8 cannot inline the recursive constructor call' or something along those lines.

This commit increase by around 10% hot code paths that are hitting
createXYZ functions. Before this change the createXYZ called the XYZ
constructor without new.
@mcollina
Copy link
Member Author

mcollina commented Oct 9, 2017

@mcollina
Copy link
Member Author

mcollina commented Oct 9, 2017

Landed as 9bc4f86

@mcollina mcollina closed this Oct 9, 2017
@mcollina mcollina deleted the change-hash-constructor branch October 9, 2017 09:43
mcollina added a commit that referenced this pull request Oct 9, 2017
This commit increase by around 10% hot code paths that are hitting
createXYZ functions. Before this change the createXYZ called the XYZ
constructor without new.

PR-URL: #16067
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
This commit increase by around 10% hot code paths that are hitting
createXYZ functions. Before this change the createXYZ called the XYZ
constructor without new.

PR-URL: nodejs/node#16067
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins
Copy link
Contributor

This does not land cleanly on v8.x

Would someone be willing to backport if it makes sense?

@lpinca
Copy link
Member

lpinca commented Oct 24, 2017

assert.strictEqual(crypto.createHash, crypto.Hash);

et al. throw with this change, should we backport anyway?

@MylesBorins
Copy link
Contributor

@lpinca if it is breaking than this should likely be tagged semver-major, if not then I don't see why we shouldn't backport

@lpinca
Copy link
Member

lpinca commented Oct 24, 2017

@MylesBorins yes exactly I'm not sure if it's a breaking change or not.

@MylesBorins MylesBorins added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 24, 2017
@MylesBorins
Copy link
Contributor

setting semver-major for right now

/cc @nodejs/tsc to chime in. Feel free to change tag

@mcollina
Copy link
Member Author

According to our doc, this is patch. if people are calling createHash with new, probably they are doing somethig wrong. This can go in 8. Or we can bake it for a bit on 9 and then backport to 8.
Most of the discussion in this issue is about landing it in 8.

@jasnell
Copy link
Member

jasnell commented Oct 24, 2017

I'm +1 on it being semver-patch.

lpinca pushed a commit to lpinca/node that referenced this pull request Oct 24, 2017
This commit increase by around 10% hot code paths that are hitting
createXYZ functions. Before this change the createXYZ called the XYZ
constructor without new.

PR-URL: nodejs#16067
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@lpinca
Copy link
Member

lpinca commented Oct 24, 2017

Backport in #16446 marked as blocked currently.

@MylesBorins MylesBorins removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 24, 2017
@MylesBorins
Copy link
Contributor

dropping semver-major. Thinking we should let this bake in 9.x a bit first

gibfahn pushed a commit that referenced this pull request Dec 19, 2017
This commit increase by around 10% hot code paths that are hitting
createXYZ functions. Before this change the createXYZ called the XYZ
constructor without new.

PR-URL: #16067
Backport-PR-URL: #16446
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
This commit increase by around 10% hot code paths that are hitting
createXYZ functions. Before this change the createXYZ called the XYZ
constructor without new.

PR-URL: #16067
Backport-PR-URL: #16446
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.