http2: track memory allocated by nghttp2 #21374
Closed
Conversation
This file contains 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
addaleax
added
blocked
addaleax
force-pushed
the
track-memory-http2
branch
from
f8aa056
to
922f2ec
Compare
|
Woo! |
jasnell
approved these changes
Jun 17, 2018
trivikr
added
the
c++
apapirovski
approved these changes
Jun 17, 2018
TimothyGu
reviewed
Jun 18, 2018
src/node_http2.h
Outdated
| // Tell our custom memory allocator that this rcbuf is independent of | ||
| // this session now, and may outlive it. | ||
| void StopTrackingRcbuf(nghttp2_rcbuf* buf); | ||
|
|
||
| // Returns the current session memory including the current size of both | ||
| // the inflate and deflate hpack headers, the current outbound storage |
There was a problem hiding this comment.
Might want to change the hpack headers part, as it is no longer visible in code?
TimothyGu
approved these changes
Jun 18, 2018
addaleax
force-pushed
the
track-memory-http2
branch
from
922f2ec
to
50a9d42
Compare
Provide a custom memory allocator for nghttp2, and track memory allocated by the library with it. This makes the used-memory-per-session estimate more accurate, and allows us to track memory leaks either in nghttp2 itself or, more likely, through faulty usage on our end. It also allows us to make the per-session memory limit more accurate in the future; currently, we are not handling this in an ideal way, and instead let nghttp2 allocate what it wants, even if that goes over our limit. Refs: nodejs#21373 Refs: nodejs#21336
addaleax
force-pushed
the
track-memory-http2
branch
from
50a9d42
to
fa8d967
Compare
|
Rebased, this should no longer be blocked |
addaleax
added
the
author ready
TimothyGu
approved these changes
Jun 21, 2018
| if (mem != nullptr) { | ||
| // Adjust the memory info counter. | ||
| session->current_nghttp2_memory_ += size - previous_size; | ||
| *reinterpret_cast<size_t*>(mem) = size; |
There was a problem hiding this comment.
It took me a while to realize how this would behave for a size of 0: our UncheckedRealloc implementation will return nullptr in that case, to fill in an implementation-defined gap in the system realloc(). It might be helpful to clarify that.
ChALkeR
added
the
memory
|
Landed in 15c627f |
apapirovski
pushed a commit
that referenced
this pull request
Jun 25, 2018
Provide a custom memory allocator for nghttp2, and track memory allocated by the library with it. This makes the used-memory-per-session estimate more accurate, and allows us to track memory leaks either in nghttp2 itself or, more likely, through faulty usage on our end. It also allows us to make the per-session memory limit more accurate in the future; currently, we are not handling this in an ideal way, and instead let nghttp2 allocate what it wants, even if that goes over our limit. PR-URL: #21374 Refs: #21373 Refs: #21336 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
targos
pushed a commit
that referenced
this pull request
Jun 25, 2018
Provide a custom memory allocator for nghttp2, and track memory allocated by the library with it. This makes the used-memory-per-session estimate more accurate, and allows us to track memory leaks either in nghttp2 itself or, more likely, through faulty usage on our end. It also allows us to make the per-session memory limit more accurate in the future; currently, we are not handling this in an ideal way, and instead let nghttp2 allocate what it wants, even if that goes over our limit. PR-URL: #21374 Refs: #21373 Refs: #21336 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Merged
kjin
pushed a commit
to kjin/node
that referenced
this pull request
Aug 23, 2018
Provide a custom memory allocator for nghttp2, and track memory allocated by the library with it. This makes the used-memory-per-session estimate more accurate, and allows us to track memory leaks either in nghttp2 itself or, more likely, through faulty usage on our end. It also allows us to make the per-session memory limit more accurate in the future; currently, we are not handling this in an ideal way, and instead let nghttp2 allocate what it wants, even if that goes over our limit. PR-URL: nodejs#21374 Refs: nodejs#21373 Refs: nodejs#21336 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
kjin
pushed a commit
to kjin/node
that referenced
this pull request
Sep 25, 2018
Provide a custom memory allocator for nghttp2, and track memory allocated by the library with it. This makes the used-memory-per-session estimate more accurate, and allows us to track memory leaks either in nghttp2 itself or, more likely, through faulty usage on our end. It also allows us to make the per-session memory limit more accurate in the future; currently, we are not handling this in an ideal way, and instead let nghttp2 allocate what it wants, even if that goes over our limit. PR-URL: nodejs#21374 Refs: nodejs#21373 Refs: nodejs#21336 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
kjin
pushed a commit
to kjin/node
that referenced
this pull request
Oct 16, 2018
Provide a custom memory allocator for nghttp2, and track memory allocated by the library with it. This makes the used-memory-per-session estimate more accurate, and allows us to track memory leaks either in nghttp2 itself or, more likely, through faulty usage on our end. It also allows us to make the per-session memory limit more accurate in the future; currently, we are not handling this in an ideal way, and instead let nghttp2 allocate what it wants, even if that goes over our limit. PR-URL: nodejs#21374 Refs: nodejs#21373 Refs: nodejs#21336 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
BethGriggs
pushed a commit
that referenced
this pull request
Oct 17, 2018
Provide a custom memory allocator for nghttp2, and track memory allocated by the library with it. This makes the used-memory-per-session estimate more accurate, and allows us to track memory leaks either in nghttp2 itself or, more likely, through faulty usage on our end. It also allows us to make the per-session memory limit more accurate in the future; currently, we are not handling this in an ideal way, and instead let nghttp2 allocate what it wants, even if that goes over our limit. Backport-PR-URL: #22850 PR-URL: #21374 Refs: #21373 Refs: #21336 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Provide a custom memory allocator for nghttp2, and track
memory allocated by the library with it.
This makes the used-memory-per-session estimate more
accurate, and allows us to track memory leaks either
in nghttp2 itself or, more likely, through faulty
usage on our end.
It also allows us to make the per-session memory limit
more accurate in the future; currently, we are not
handling this in an ideal way, and instead let nghttp2
allocate what it wants, even if that goes over our limit.
Refs: #21373
Refs: #21336
[blocked by both of these fixes]
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes