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: improve http2 code a bit #23984

Closed
wants to merge 1 commit into from

Conversation

@jasnell
Copy link
Member

commented Oct 30, 2018

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
http2: improve http2 code a bit
Multiple general improvements to http2 internals for
readability and efficiency

@jasnell jasnell force-pushed the jasnell:http2-improvements3 branch from 9c996f2 to d408b51 Oct 30, 2018

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2018

@jasnell jasnell requested review from addaleax and apapirovski Nov 1, 2018

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2018

@trivikr
trivikr approved these changes Nov 2, 2018
@mcollina
Copy link
Member

left a comment

LGTM

Are there any performance benefits?

@Trott

This comment has been minimized.

Copy link
Member

commented Nov 4, 2018

Landed in 7825045

@Trott Trott closed this Nov 4, 2018

Trott added a commit to Trott/io.js that referenced this pull request Nov 4, 2018
http2: improve http2 code a bit
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 added a commit that referenced this pull request Nov 5, 2018
http2: improve http2 code a bit
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

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

What is the meaning of irritations in this context ?

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2018

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

I meant additional unnecessary iterations 😂

@GrosSacASac

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Nov 7, 2018

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.

@codebytere

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

@jasnell do you want to potentially backport this?

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

@codebytere

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

@jasnell do you have the bandwidth for this atm?

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

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

BethGriggs added a commit that referenced this pull request Aug 15, 2019
http2: improve http2 code a bit
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 BethGriggs referenced this pull request Aug 15, 2019
BethGriggs added a commit that referenced this pull request Aug 15, 2019
http2: improve http2 code a bit
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.