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

http2: multiple improvements #24063

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Nov 3, 2018

Multiple improvements throughout the http2 js code

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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 3, 2018
@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Nov 3, 2018
@jasnell jasnell changed the title http2: multiple improvements [WIP] http2: multiple improvements Nov 3, 2018
@jasnell
Copy link
Member Author

jasnell commented Nov 3, 2018

@jasnell
Copy link
Member Author

jasnell commented Nov 6, 2018

On this PR... I plan to pull the destructuring changes back out as that is still a bit too expensive and is offsetting the performance improvement from other bits of the code.

@jasnell jasnell force-pushed the http2-js-improvements branch 2 times, most recently from 53105f7 to eeb665e Compare November 7, 2018 19:08
@jasnell
Copy link
Member Author

jasnell commented Nov 7, 2018

@jasnell jasnell removed the wip Issues and PRs that are still a work in progress. label Nov 8, 2018
@jasnell jasnell changed the title [WIP] http2: multiple improvements http2: multiple improvements Nov 8, 2018
@jasnell
Copy link
Member Author

jasnell commented Nov 10, 2018

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

/cc @nodejs/http2

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Nov 15, 2018

Needs a rebase to eliminate conflicts?

@jasnell
Copy link
Member Author

jasnell commented Nov 15, 2018 via email

@jasnell
Copy link
Member Author

jasnell commented Nov 19, 2018

New CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/18763/

@jasnell
Copy link
Member Author

jasnell commented Nov 19, 2018

Ping @nodejs/http2 ... I plan to get this landed by Wednesday this week.

@jasnell
Copy link
Member Author

jasnell commented Nov 19, 2018

@jasnell
Copy link
Member Author

jasnell commented Nov 20, 2018

Make the http2 binding a bit more efficient by setting the callback
functions once when the module is loaded rather than for each
`Http2Session` and `Http2Stream`.
jasnell added a commit that referenced this pull request Nov 21, 2018
PR-URL: #24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
jasnell added a commit that referenced this pull request Nov 21, 2018
Make the http2 binding a bit more efficient by setting the callback
functions once when the module is loaded rather than for each
`Http2Session` and `Http2Stream`.

PR-URL: #24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
jasnell added a commit that referenced this pull request Nov 21, 2018
PR-URL: #24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
@jasnell
Copy link
Member Author

jasnell commented Nov 21, 2018

Landed in e94d16d, ee80aaa, and 6026582

@jasnell jasnell closed this Nov 21, 2018
targos pushed a commit that referenced this pull request Nov 21, 2018
PR-URL: #24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
targos pushed a commit that referenced this pull request Nov 21, 2018
Make the http2 binding a bit more efficient by setting the callback
functions once when the module is loaded rather than for each
`Http2Session` and `Http2Stream`.

PR-URL: #24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
targos pushed a commit that referenced this pull request Nov 21, 2018
PR-URL: #24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Make the http2 binding a bit more efficient by setting the callback
functions once when the module is loaded rather than for each
`Http2Session` and `Http2Stream`.

PR-URL: #24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
@codebytere
Copy link
Member

@jasnell could you backport this to v10.x if you think it's a candidate? I've added the label, but feel free to remove it!

refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Make the http2 binding a bit more efficient by setting the callback
functions once when the module is loaded rather than for each
`Http2Session` and `Http2Stream`.

PR-URL: nodejs#24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants