GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: optimize ALPN callback by bnoordhuis · Pull Request #44875 · nodejs/node

Conversation

@bnoordhuis

@bnoordhuis bnoordhuis commented Oct 3, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bnoordhuis
src: simplify ALPN code, remove indirection
432ca36
@nodejs-github-bot

nodejs-github-bot commented Oct 3, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Oct 3, 2022
@bnoordhuis
src: optimize ALPN callback
0f58967
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.
@bnoordhuis
bnoordhuis force-pushed the more-better-alpn branch from b036919 to 0f58967 Compare October 3, 2022 11:19
@tniessen

tniessen commented Oct 3, 2022

Copy link
Copy Markdown
Member

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Oct 4, 2022

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell approved these changes Oct 10, 2022
View reviewed changes
@bnoordhuis bnoordhuis added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed needs-ci PRs that need a full CI run. labels Oct 10, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 10, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 10, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis bnoordhuis added the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 19, 2022
@bnoordhuis bnoordhuis mentioned this pull request Oct 19, 2022
tls: Add support for ALPN fallback when no ALPN protocol matches #45075
Closed
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Oct 19, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 19, 2022

Copy link
Copy Markdown
Collaborator
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 [email protected]

[detached HEAD 777dea3416] src: simplify ALPN code, remove indirection
Author: Ben Noordhuis [email protected]
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 [email protected]

[detached HEAD 3465634e85] src: optimize ALPN callback
Author: Ben Noordhuis [email protected]
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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tniessen
tniessen approved these changes Oct 19, 2022
View reviewed changes

@tniessen tniessen left a comment

Copy link
Copy Markdown
Member

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 that the breaking change is unlikely to affect anything.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@richardlau richardlau added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Oct 19, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 19, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 19, 2022

Copy link
Copy Markdown
Collaborator

Landed in 2d0d997...fdadea8

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot closed this Oct 19, 2022
nodejs-github-bot pushed a commit that referenced this pull request Oct 19, 2022
@bnoordhuis @nodejs-github-bot
src: simplify ALPN code, remove indirection
3209d41
PR-URL: #44875
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Oct 19, 2022
@bnoordhuis @nodejs-github-bot
src: optimize ALPN callback
fdadea8
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@bnoordhuis
bnoordhuis deleted the more-better-alpn branch October 19, 2022 23:30
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
@bnoordhuis @RafaelGSS
src: simplify ALPN code, remove indirection
8896195
PR-URL: #44875
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
@bnoordhuis @RafaelGSS
src: optimize ALPN callback
aeaf653
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
v19.1.0 proposal #45269
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@bnoordhuis @RafaelGSS
src: simplify ALPN code, remove indirection
9dc21a1
PR-URL: #44875
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@bnoordhuis @RafaelGSS
src: optimize ALPN callback
e5b1179
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@bnoordhuis @danielleadams
src: simplify ALPN code, remove indirection
007f0bb
PR-URL: #44875
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@bnoordhuis @danielleadams
src: optimize ALPN callback
545e65d
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@bnoordhuis @danielleadams
src: simplify ALPN code, remove indirection
d433d34
PR-URL: #44875
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@bnoordhuis @danielleadams
src: optimize ALPN callback
bf17f8d
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@tniessen tniessen tniessen approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. lib / src Issues and PRs involving general changes in the lib/ or src/ directories.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@bnoordhuis @nodejs-github-bot @tniessen @jasnell @richardlau
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.