http2: cleanup and refactoring of http2 internals by jasnell · Pull Request #32884 · nodejs/node

jasnell · 2020-04-16T18:00:34Z

This is a fairly extensive overhaul of much of the http2 c++ internals across multiple commits. This is in preparation for some additional work in plan.

cleanup constants in http2 binding - Rework constants usage for consistency
refactor GetPackedSettings to avoid unnecessary AsyncWrap - Address TODO statement and make packed settings more efficient
simplify settings to reduce duplicate code - Refactor how settings are handled to eliminate duplicated code
improve style consistency and correctness - multiple changes (see commit message for detail)
make EmitStatistics function private - Only used internally for each class
un-nest Http2Settings and Http2Ping - These were unnecessarily nested within Http2Session
refactoring and cleanup of Http2Settings and Http2Ping - Improve object creation and use
avoid ** syntax for readability - Avoid syntax like **session and **stream
avoid most uses of ToChecked/ToLocalChecked - Move to using MaybeLocal where possible
use const references for Http2Priority - Simplify priority implementation and use
remove unnecessary GetStream function - Self explanatory
refactor Http2Scope to use BaseObjectPtr - Simplify implementation
move utility function to anonymous namespace - Self explanatory
refactor and simplify Origins - Use AllocatedBuffer rather than MaybeStackBuffer, simplify use
use BaseObjectPtr for Http2Streams map - Self explanatory
move MemoryInfo impl to cc -- Style and code organization improvements
fixup lint and style issues -- Fixup linting issues from prior commits

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

The error constants were just doing some weird things. Cleanup and improve maintainability. Signed-off-by: James M Snell <jasnell@gmail.com>

Signed-off-by: James M Snell <jasnell@gmail.com>

Use snake_case for getters and setters at c++ level, avoid unnecessary use of enums, use consistent style for exported vs. internal constants, avoid unnecessary memory info reporting, use setters/getters for flags_ for improved code readability Signed-off-by: James M Snell <jasnell@gmail.com>

Signed-off-by: James M Snell <jasnell@gmail.com>

The **session and **stream syntax for getting the underlying nghttp2 pointers is not ideal for readability Signed-off-by: James M Snell <jasnell@gmail.com>

Signed-off-by: James M Snell <jasnell@gmail.com>

* Use an AllocatedBuffer instead of MaybeStackBuffer * Use a const reference instead of pointer Signed-off-by: James M Snell <jasnell@gmail.com>

Signed-off-by: James M Snell <jasnell@gmail.com>

nodejs-github-bot · 2020-04-16T18:01:55Z

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

addaleax · 2020-04-17T11:27:13Z

@jasnell What if you temporarily remove the callback changes here that make them v8::Globals? The approach is generally a memory leak footgun, because if there is any way through which the callback refers back to the containing object, you’ll end up with a leak

jasnell · 2020-04-17T13:03:45Z

I could but I don't believe that's the issue. The memory accounting is being thrown off while attempting to write the data. Didn't get too far investigating it yesterday so continuing today

nodejs-github-bot · 2020-04-17T18:04:54Z

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

jasnell · 2020-04-17T18:38:42Z

Ok, looks like CI is good to go now. :-)

mcollina

lgtm

* cleanup constants in http2 binding The error constants were just doing some weird things. Cleanup and improve maintainability. * simplify settings to reduce duplicate code * improve style consistency and correctness Use snake_case for getters and setters at c++ level, avoid unnecessary use of enums, use consistent style for exported vs. internal constants, avoid unnecessary memory info reporting, use setters/getters for flags_ for improved code readability * make EmitStatistics function private * un-nest Http2Settings and Http2Ping * refactoring and cleanup of Http2Settings and Http2Ping * avoid ** syntax for readability The **session and **stream syntax for getting the underlying nghttp2 pointers is not ideal for readability * use const references for Http2Priority * remove unnecessary GetStream function * refactor Http2Scope to use BaseObjectPtr * move utility function to anonymous namespace * refactor and simplify Origins * Use an AllocatedBuffer instead of MaybeStackBuffer * Use a const reference instead of pointer * use BaseObjectPtr for Http2Streams map * move MemoryInfo impl to cc Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #32884 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

jasnell · 2020-04-21T20:41:56Z

Landed in 91ca221

* cleanup constants in http2 binding The error constants were just doing some weird things. Cleanup and improve maintainability. * simplify settings to reduce duplicate code * improve style consistency and correctness Use snake_case for getters and setters at c++ level, avoid unnecessary use of enums, use consistent style for exported vs. internal constants, avoid unnecessary memory info reporting, use setters/getters for flags_ for improved code readability * make EmitStatistics function private * un-nest Http2Settings and Http2Ping * refactoring and cleanup of Http2Settings and Http2Ping * avoid ** syntax for readability The **session and **stream syntax for getting the underlying nghttp2 pointers is not ideal for readability * use const references for Http2Priority * remove unnecessary GetStream function * refactor Http2Scope to use BaseObjectPtr * move utility function to anonymous namespace * refactor and simplify Origins * Use an AllocatedBuffer instead of MaybeStackBuffer * Use a const reference instead of pointer * use BaseObjectPtr for Http2Streams map * move MemoryInfo impl to cc Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #32884 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

jasnell added 17 commits April 16, 2020 10:51

http2: cleanup constants in http2 binding
f38c9ea
The error constants were just doing some weird things. Cleanup and improve maintainability. Signed-off-by: James M Snell <jasnell@gmail.com>

http2: refactor GetPackedSettings to avoid unnecessary AsyncWrap
219e339
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: simplify settings to reduce duplicate code
5499c7c
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: make EmitStatistics function private
a2efaa7
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: un-nest Http2Settings and Http2Ping
b23cecc
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: refactoring and cleanup of Http2Settings and Http2Ping
681bfe4
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: avoid ** syntax for readability
0e0a3a2
The **session and **stream syntax for getting the underlying nghttp2 pointers is not ideal for readability Signed-off-by: James M Snell <jasnell@gmail.com>

http2: avoid most uses to ToChecked/ToLocalChecked
e39420f
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: use const references for Http2Priority
e8a2f5e
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: remove unnecessary GetStream function
52fcf89
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: refactor Http2Scope to use BaseObjectPtr
7aefc8f
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: move utility function to anonymous namespace
d9c5a40
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: refactor and simplify Origins
6b86554
* Use an AllocatedBuffer instead of MaybeStackBuffer * Use a const reference instead of pointer Signed-off-by: James M Snell <jasnell@gmail.com>

http2: use BaseObjectPtr for Http2Streams map
4ccd690
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: move MemoryInfo impl to cc
fe77e23
Signed-off-by: James M Snell <jasnell@gmail.com>

http2: fixup lint and style issues
764c71d
Signed-off-by: James M Snell <jasnell@gmail.com>

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 Apr 16, 2020

jasnell requested review from addaleax and mcollina April 16, 2020 18:00

addaleax reviewed Apr 16, 2020
View reviewed changes

This comment has been minimized.
Sign in to view

jasnell force-pushed the refactor-http2-internals branch from 9603c7a to 639f426Compare April 16, 2020 23:11

jasnell force-pushed the refactor-http2-internals branch 2 times, most recently from 28c9cc2 to 04415cfCompare April 17, 2020 16:11

[Squash] fixup
1342b24

jasnell force-pushed the refactor-http2-internals branch from 8045d42 to 1342b24Compare April 17, 2020 18:04

jasnell requested a review from addaleax April 17, 2020 18:38

mcollina approved these changes Apr 17, 2020
View reviewed changes

fhinkel approved these changes Apr 21, 2020
View reviewed changes

jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 21, 2020

jasnell closed this Apr 21, 2020

BethGriggs mentioned this pull request Apr 27, 2020
v14.1.0 proposal #33103
Merged

BridgeAR added the dont-land-on-v13.x label Apr 28, 2020

targos added the backport-blocked-v12.x label May 7, 2020

joyeecheung mentioned this pull request Jun 8, 2020
src: retrieve binding data from the context #33139
Closed
3 tasks

addaleax added dont-land-on-v12.x and removed backport-blocked-v12.x labels Sep 23, 2020

Uh oh!

Uh oh!

Conversation

jasnell commented Apr 16, 2020

Checklist

Uh oh!

nodejs-github-bot commented Apr 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

addaleax commented Apr 17, 2020

Uh oh!

jasnell commented Apr 17, 2020

Uh oh!

nodejs-github-bot commented Apr 17, 2020

Uh oh!

jasnell commented Apr 17, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants