src: optimize ALPN callback #44875

bnoordhuis · 2022-10-03T11:03:19Z

It doesn't make sense from a performance perspective to retain an
arraybuffer with the ALPN byte string and look it up as a property on
the JS context object for every TLS handshake.

Store the byte string in the C++ TLSWrap object instead. That's both
a lot faster and a lot simpler.

First commit is minor cleanup for readability++.

nodejs-github-bot · 2022-10-03T11:03:23Z

Review requested:

@nodejs/crypto

It doesn't make sense from a performance perspective to retain an arraybuffer with the ALPN byte string and look it up as a property on the JS context object for every TLS handshake. Store the byte string in the C++ TLSWrap object instead. That's both a lot faster and a lot simpler.

tniessen · 2022-10-03T11:42:59Z

It doesn't make sense from a performance perspective to retain an
arraybuffer with the ALPN byte string and look it up as a property on
the JS context object for every TLS handshake.

Store the byte string in the C++ TLSWrap object instead. That's both
a lot faster and a lot simpler.

Is this technically breaking in that users were previously able to modify the ArrayBuffer in-between handshakes?

bnoordhuis · 2022-10-04T17:42:02Z

I suppose, but it's an undocumented property on an undocumented property. Seems both very unlikely and not deserving of much sympathy.

nodejs-github-bot · 2022-10-10T22:25:49Z

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

nodejs-github-bot · 2022-10-19T18:40:07Z

Commit Queue failed

- Loading data for nodejs/node/pull/44875 ✔ Done loading data for nodejs/node/pull/44875 ----------------------------------- PR info ------------------------------------ Title src: optimize ALPN callback (#44875) Author Ben Noordhuis (@bnoordhuis) Branch bnoordhuis:more-better-alpn -> nodejs:main Labels c++, lib / src Commits 2 - src: simplify ALPN code, remove indirection - src: optimize ALPN callback Committers 1 - Ben Noordhuis PR-URL: https://github.com/nodejs/node/pull/44875 Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/44875 Reviewed-By: James M Snell -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 03 Oct 2022 11:03:19 GMT ✔ Approvals: 1 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/44875#pullrequestreview-1136253893 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-10-10T22:25:49Z: https://ci.nodejs.org/job/node-test-pull-request/47165/ - Querying data for job/node-test-pull-request/47165/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD 9926b89a99..19f3973828 main -> origin/main ✔ origin/main is now up-to-date main is out of sync with origin/main. Mismatched commits: - 6ff429777a meta: move one or more collaborators to emeritus - 19f3973828 meta: move one or more collaborators to emeritus -------------------------------------------------------------------------------- HEAD is now at 19f3973828 meta: move one or more collaborators to emeritus ✔ Reset to origin/main - Downloading patch for 44875 From https://github.com/nodejs/node * branch refs/pull/44875/merge -> FETCH_HEAD ✔ Fetched commits as 19f397382810..0f5896716196 -------------------------------------------------------------------------------- Auto-merging src/crypto/crypto_common.cc [main b6f6501ff8] src: simplify ALPN code, remove indirection Author: Ben Noordhuis Date: Mon Oct 3 13:00:53 2022 +0200 3 files changed, 4 insertions(+), 12 deletions(-) Auto-merging src/env_properties.h [main 821948a439] src: optimize ALPN callback Author: Ben Noordhuis Date: Mon Oct 3 13:00:53 2022 +0200 5 files changed, 19 insertions(+), 35 deletions(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: simplify ALPN code, remove indirection

PR-URL: #44875
Reviewed-By: James M Snell jasnell@gmail.com

[detached HEAD 777dea3416] src: simplify ALPN code, remove indirection
Author: Ben Noordhuis info@bnoordhuis.nl
Date: Mon Oct 3 13:00:53 2022 +0200
3 files changed, 4 insertions(+), 12 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: optimize ALPN callback

It doesn't make sense from a performance perspective to retain an
arraybuffer with the ALPN byte string and look it up as a property on
the JS context object for every TLS handshake.

Store the byte string in the C++ TLSWrap object instead. That's both
a lot faster and a lot simpler.

PR-URL: #44875
Reviewed-By: James M Snell jasnell@gmail.com

[detached HEAD 3465634e85] src: optimize ALPN callback
Author: Ben Noordhuis info@bnoordhuis.nl
Date: Mon Oct 3 13:00:53 2022 +0200
5 files changed, 19 insertions(+), 35 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/3284097315

tniessen

Agreed that the breaking change is unlikely to affect anything.

nodejs-github-bot · 2022-10-19T19:26:44Z

Landed in 2d0d997...fdadea8