Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

http2: shrink memory to match read data #26201

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Perform a shrinking Realloc() so that less data is used for HTTP2 reads.

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

Perform a shrinking `Realloc()` so that less data is
used for HTTP2 reads.
@addaleax addaleax added http2 Issues or PRs related to the http2 subsystem. lts-watch-v8.x labels Feb 19, 2019
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v6.x labels Feb 19, 2019
@addaleax
Copy link
Member Author

@addaleax
Copy link
Member Author

CI failures should be fixed, new CI: https://ci.nodejs.org/job/node-test-pull-request/20896/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 20, 2019
@addaleax
Copy link
Member Author

addaleax commented Feb 20, 2019

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/20922/ (:heavy_check_mark:)

(This needs another review, 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

@addaleax
Copy link
Member Author

Landed in 83e1b97

@addaleax addaleax closed this Feb 21, 2019
@addaleax addaleax deleted the http2-shrink-realloc branch February 21, 2019 14:53
addaleax added a commit that referenced this pull request Feb 21, 2019
Perform a shrinking `Realloc()` so that less data is
used for HTTP2 reads.

PR-URL: #26201
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax added a commit that referenced this pull request Feb 21, 2019
Perform a shrinking `Realloc()` so that less data is
used for HTTP2 reads.

PR-URL: #26201
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
Perform a shrinking `Realloc()` so that less data is
used for HTTP2 reads.

PR-URL: #26201
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
Lazily allocate `ArrayBuffer`s for the contents of DATA frames.
Creating `ArrayBuffer`s is, sadly, not a cheap operation with V8.

This is part of performance improvements to mitigate CVE-2019-9513.

Together with the previous commit, these changes improve throughput
in the adversarial case by about 100 %, and there is little more
that we can do besides artificially limiting the rate of incoming
metadata frames (i.e. after this patch, CPU usage is virtually
exclusively in libnghttp2).

[This backport also applies changes from 83e1b97 and required
some manual work due to the lack of `AllocatedBuffer` on v10.x.]

Refs: #26201

Backport-PR-URL: #29123
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 15, 2019
Lazily allocate `ArrayBuffer`s for the contents of DATA frames.
Creating `ArrayBuffer`s is, sadly, not a cheap operation with V8.

This is part of performance improvements to mitigate CVE-2019-9513.

Together with the previous commit, these changes improve throughput
in the adversarial case by about 100 %, and there is little more
that we can do besides artificially limiting the rate of incoming
metadata frames (i.e. after this patch, CPU usage is virtually
exclusively in libnghttp2).

[This backport also applies changes from 83e1b97 and required
some manual work due to the lack of `AllocatedBuffer` on v10.x.
More work was necessary for v8.x, including copying utilities
for `util.h` from more recent Node.js versions.]

Refs: #26201

Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
author ready PRs 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++. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants