crypto: make createXYZ inlineable by mcollina · Pull Request #16067 · nodejs/node · GitHub

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

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.

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

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

Not inlining Hash into etag because call is recursive

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

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

LGTM.

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

@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.

I have ported all the createXYZ functions.

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.

CI: https://ci.nodejs.org/job/node-test-pull-request/10465/

Please add a quick comment explaining the optimization.

@bengl done.

@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.

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.

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/10523/

Landed as 9bc4f86

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 <bryan@bryanenglish.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 <bryan@bryanenglish.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This does not land cleanly on v8.x

Would someone be willing to backport if it makes sense?

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

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

@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

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

setting semver-major for right now

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

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.

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

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 <bryan@bryanenglish.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Backport in #16446 marked as blocked currently.

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

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 <bryan@bryanenglish.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

mcollina requested a review from jasnell October 7, 2017 18:27

nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Oct 7, 2017

mcollina requested a review from bmeurer October 7, 2017 18:30

mscdex added the performance Issues and PRs related to the performance of Node.js. label Oct 7, 2017

TimothyGu approved these changes Oct 7, 2017

View reviewed changes

bmeurer approved these changes Oct 7, 2017

View reviewed changes

yosuke-furukawa approved these changes Oct 7, 2017

View reviewed changes

mcollina force-pushed the change-hash-constructor branch from 625e27b to a6a2f89 Compare October 7, 2017 18:51

mcollina changed the title crypto: make createHash inlineable crypto: make createXYZ inlineable Oct 7, 2017

cjihrig approved these changes Oct 7, 2017

View reviewed changes

lpinca approved these changes Oct 7, 2017

View reviewed changes

bengl requested changes Oct 7, 2017

View reviewed changes

mcollina force-pushed the change-hash-constructor branch from a6a2f89 to 6042f8e Compare October 7, 2017 21:37

bengl approved these changes Oct 7, 2017

View reviewed changes

jasnell approved these changes Oct 8, 2017

View reviewed changes

evanlucas approved these changes Oct 8, 2017

View reviewed changes

mscdex reviewed Oct 8, 2017

View reviewed changes

crypto: make createXYZ inlineable

4e61acb

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 force-pushed the change-hash-constructor branch from 6042f8e to 4e61acb Compare October 9, 2017 07:30

BridgeAR approved these changes Oct 9, 2017

View reviewed changes

mcollina closed this Oct 9, 2017

mcollina deleted the change-hash-constructor branch October 9, 2017 09:43

MylesBorins added the backport-requested-v8.x label Oct 23, 2017

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

lpinca mentioned this pull request Oct 24, 2017

[v8.x backport] crypto: make createXYZ inlineable #16446

Closed

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

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

MylesBorins added the dont-land-on-v6.x label Dec 19, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

BethGriggs added backported-to-v8.x and removed backport-requested-v8.x labels Jun 19, 2018

crypto: make createXYZ inlineable by mcollina · Pull Request #16067 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: make createXYZ inlineable - #16067

crypto: make createXYZ inlineable#16067mcollina wants to merge 1 commit intonodejs:masternodejs/node:masterfrom mcollina:change-hash-constructormcollina/node:change-hash-constructorCopy head branch name to clipboard

Conversation

mcollina commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mcollina commented Oct 7, 2017

Uh oh!

mscdex commented Oct 7, 2017

Uh oh!

mcollina commented Oct 7, 2017

Uh oh!

mcollina commented Oct 7, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

bmeurer left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 7, 2017

Uh oh!

mcollina commented Oct 7, 2017

Uh oh!

mscdex commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Oct 7, 2017

Uh oh!

bengl Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 7, 2017

Uh oh!

bmeurer commented Oct 8, 2017

Uh oh!

mscdex Oct 8, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 9, 2017

Uh oh!

mcollina commented Oct 9, 2017

Uh oh!

MylesBorins commented Oct 23, 2017

Uh oh!

lpinca commented Oct 24, 2017

Uh oh!

MylesBorins commented Oct 24, 2017

Uh oh!

lpinca commented Oct 24, 2017

Uh oh!

MylesBorins commented Oct 24, 2017

Uh oh!

mcollina commented Oct 24, 2017

Uh oh!

jasnell commented Oct 24, 2017

Uh oh!

lpinca commented Oct 24, 2017

Uh oh!

MylesBorins commented Oct 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

crypto: make createXYZ inlineable#16067
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:change-hash-constructor

mcollina commented Oct 7, 2017 •
edited
Loading

mscdex commented Oct 7, 2017 •
edited
Loading