FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: improve http2 code a bit by jasnell · Pull Request #23984 · nodejs/node · GitHub

/ node Public

http2: improve http2 code a bit - #23984

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:http2-improvements3
ConversationCommits (1)ChecksFiles changed
Closed

http2: improve http2 code a bit#23984
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:http2-improvements3

Conversation

jasnell commented Oct 30, 2018

Copy link
Copy Markdown
Member

Multiple general improvements to http2 internals for readability and efficiency

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

nodejs-github-bot commented Oct 30, 2018

Copy link
Copy Markdown
Collaborator

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1429/pipeline

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 30, 2018
http2: improve http2 code a bit
d408b51 
Multiple general improvements to http2 internals for
readability and efficiency
jasnell force-pushed the http2-improvements3 branch from 9c996f2 to d408b51 Compare October 30, 2018 23:24

jasnell commented Nov 1, 2018

Copy link
Copy Markdown
Member Author

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

jasnell commented Nov 2, 2018

Copy link
Copy Markdown
Member Author

ping @nodejs/http2

mcollina approved these changes Nov 3, 2018
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Are there any performance benefits?

Trott commented Nov 4, 2018

Copy link
Copy Markdown
Member

Landed in 7825045

Trott closed this Nov 4, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 4, 2018
http2: improve http2 code a bit
7825045 
Multiple general improvements to http2 internals for
readability and efficiency

PR-URL: nodejs#23984
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Nov 5, 2018
http2: improve http2 code a bit
c7f471a 
Multiple general improvements to http2 internals for
readability and efficiency

PR-URL: #23984
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

GrosSacASac commented Nov 6, 2018

Copy link
Copy Markdown
Contributor

From https://github.com/nodejs/node/blob/7825045ee695e9e5c048133255a3b614e04c98d3/lib/internal/http2/util.js
Is it a good idea to use Object.entries loop in this case ?

it would replace

 const keys = Object.keys(map);

  let i;
  let key;
  let value;

  for (i = 0; i < keys.length; i++) {
    key = keys[i];
    value = map[key];
  }

with something like

  Object.entries(map).forEach(([key, value]) => {
  
  });

GrosSacASac commented Nov 6, 2018

Copy link
Copy Markdown
Contributor

node/lib/internal/http2/util.js

Line 486 in 7825045

ret += `${key}\0${value}\0`;

Is it a good idea to push into an array, and join all the strings at the end, instead of doing string concatenation each time in the loop ?

jasnell commented Nov 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Using entries and forEach is quite a bit more expensive performance wise, as is join, because it forces additional unnecessary iterations over the values.

GrosSacASac commented Nov 6, 2018

Copy link
Copy Markdown
Contributor

What is the meaning of irritations in this context ?

jasnell commented Nov 6, 2018

Copy link
Copy Markdown
Member Author

Sorry, that was a phone autocorrect error... Lol

I meant additional unnecessary iterations 😂

GrosSacASac commented Nov 7, 2018

Copy link
Copy Markdown
Contributor

The most natural way to write a program should result in top-of-the-line runtime performance. In places where performance is king, the optimal code should be clearly expressible. Should we open issue in the engine ?

jasnell commented Nov 7, 2018

Copy link
Copy Markdown
Member Author

The v8 team is continually making improvements to the performance in this area, and I suspect they will continue to do so. We'll get there eventually.

BridgeAR mentioned this pull request Nov 14, 2018
Merged
This was referenced Nov 15, 2018
Closed
Closed
Closed
Closed
Closed
beevelop mentioned this pull request Nov 16, 2018
Closed

codebytere commented Nov 29, 2018

Copy link
Copy Markdown
Member

@jasnell do you want to potentially backport this?

jasnell commented Nov 29, 2018 via email

Copy link
Copy Markdown
Member Author

codebytere commented Nov 29, 2018

Copy link
Copy Markdown
Member

@jasnell do you have the bandwidth for this atm?

jasnell commented Nov 29, 2018

Copy link
Copy Markdown
Member Author

Not so much. Will take a few weeks for me to get to it

BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
http2: improve http2 code a bit
00f153d 
Multiple general improvements to http2 internals for
readability and efficiency

[This backport applied to v10.x cleanly.]

Backport-PR-URL: #29123
PR-URL: #23984
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs mentioned this pull request Aug 15, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
http2: improve http2 code a bit
b095e35 
Multiple general improvements to http2 internals for
readability and efficiency

[This backport applied to v10.x cleanly but had several
merge conflicts on v8.x.]

PR-URL: #23984
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This was referenced Aug 15, 2019
Closed
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
addaleax addaleax approved these changes
trivikr trivikr approved these changes
apapirovski Awaiting requested review from apapirovski
+1 more reviewer
ryzokuken ryzokuken approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL