Skip to content

http2: cleanup and refactoring of http2 internals#32884

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.

Sign up for GitHub

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

Jump to bottom
Closed
jasnell wants to merge 18 commits into from
Closed

http2: cleanup and refactoring of http2 internals #32884

jasnell wants to merge 18 commits into from

Conversation

@jasnell
Copy link
Member

@jasnelljasnell commented Apr 16, 2020

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

jasnell added 17 commits April 16, 2020 10:51
@jasnell
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 <[email protected]>
@jasnell
http2: refactor GetPackedSettings to avoid unnecessary AsyncWrap
219e339
Signed-off-by: James M Snell <[email protected]>
@jasnell
http2: simplify settings to reduce duplicate code
5499c7c
Signed-off-by: James M Snell <[email protected]>
@jasnell
http2: improve style consistency and correctness
dd06df1
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 <[email protected]>
@jasnell
http2: make EmitStatistics function private
a2efaa7
Signed-off-by: James M Snell <[email protected]>
@jasnell
http2: un-nest Http2Settings and Http2Ping
b23cecc
Signed-off-by: James M Snell <[email protected]>
@jasnell
http2: refactoring and cleanup of Http2Settings and Http2Ping
681bfe4
Signed-off-by: James M Snell <[email protected]>
@jasnell
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 <[email protected]>
@jasnell
http2: avoid most uses to ToChecked/ToLocalChecked
e39420f
Signed-off-by: James M Snell <[email protected]>
@jasnell
http2: use const references for Http2Priority
e8a2f5e
Signed-off-by: James M Snell <[email protected]>
@jasnell
http2: remove unnecessary GetStream function
52fcf89
Signed-off-by: James M Snell <[email protected]>
@jasnell
http2: refactor Http2Scope to use BaseObjectPtr
7aefc8f
Signed-off-by: James M Snell <[email protected]>
@jasnell
http2: move utility function to anonymous namespace
d9c5a40
Signed-off-by: James M Snell <[email protected]>
@jasnell
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 <[email protected]>
@jasnell
http2: use BaseObjectPtr for Http2Streams map
4ccd690
Signed-off-by: James M Snell <[email protected]>
@jasnell
http2: move MemoryInfo impl to cc
fe77e23
Signed-off-by: James M Snell <[email protected]>
@jasnell
http2: fixup lint and style issues
764c71d
Signed-off-by: James M Snell <[email protected]>
@nodejs-github-botnodejs-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
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 16, 2020

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

addaleax
addaleax reviewed Apr 16, 2020
View reviewed changes
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jasnell

This comment has been minimized.

Sign in to view
@jasnelljasnellforce-pushed the refactor-http2-internals branch from 9603c7a to 639f426CompareApril 16, 2020 23:11
@addaleax
Copy link
Member

addaleax commented Apr 17, 2020

@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
Copy link
MemberAuthor

jasnell commented Apr 17, 2020

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

@jasnelljasnellforce-pushed the refactor-http2-internals branch 2 times, most recently from 28c9cc2 to 04415cfCompareApril 17, 2020 16:11
@jasnelljasnellforce-pushed the refactor-http2-internals branch from 8045d42 to 1342b24CompareApril 17, 2020 18:04
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 17, 2020

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

@jasnelljasnell requested a review from addaleaxApril 17, 2020 18:38
@jasnell
Copy link
MemberAuthor

jasnell commented Apr 17, 2020

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

mcollina
mcollina approved these changes Apr 17, 2020
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@jasnelljasnell 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 added a commit that referenced this pull request Apr 21, 2020
@jasnell
http2: refactor and cleanup http2
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 <[email protected]> PR-URL: #32884 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
@jasnell
Copy link
MemberAuthor

jasnell commented Apr 21, 2020

Landed in 91ca221

@jasnelljasnell closed this Apr 21, 2020
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
@jasnell@BethGriggs
http2: refactor and cleanup http2
6fc4d17
* 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 <[email protected]> PR-URL: #32884 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
@BethGriggsBethGriggs mentioned this pull request Apr 27, 2020
Merged
@joyeecheungjoyeecheung mentioned this pull request Jun 8, 2020
Closed
3 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@fhinkelfhinkelfhinkel approved these changes

@addaleaxaddaleaxAwaiting requested review from addaleax

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues 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.

7 participants

@jasnell@nodejs-github-bot@addaleax@mcollina@fhinkel@targos@BridgeAR