★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21374Nouvelle recherche | Portail wanayoo
Skip to content

/node

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: track memory allocated by nghttp2 #21374

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@apapirovski apapirovski

@jasnell jasnell

@TimothyGu TimothyGu

Assignees
No one assigned
Labels
C++ author ready http2 memory
Projects
None yet
Milestone
No milestone
6 participants
@addaleax @jasnell @apapirovski @TimothyGu @ChALkeR @trivikr
@addaleax
Copy link
Member

addaleax commented Jun 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.

Refs: #21373
Refs: #21336

[blocked by both of these fixes]

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

@addaleax addaleax added blocked http2 labels Jun 16, 2018

@addaleax addaleax force-pushed the addaleax:track-memory-http2 branch from f8aa056 to 922f2ec Jun 16, 2018

@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

jasnell commented Jun 17, 2018

Woo!
@jasnell

jasnell approved these changes Jun 17, 2018
View changes

@trivikr trivikr added the C++ label Jun 17, 2018

@apapirovski

apapirovski approved these changes Jun 17, 2018
View changes

@TimothyGu

TimothyGu reviewed Jun 18, 2018
View changes

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

This comment has been minimized.

Sign in to view
Copy link
@TimothyGu

TimothyGu Jun 18, 2018

Member

Might want to change the hpack headers part, as it is no longer visible in code?

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Jun 20, 2018

Author Member

Yup, done!
@TimothyGu

TimothyGu approved these changes Jun 18, 2018
View changes

@addaleax addaleax force-pushed the addaleax:track-memory-http2 branch from 922f2ec to 50a9d42 Jun 20, 2018

@addaleax
http2: track memory allocated by nghttp2 
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
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
fa8d967

@addaleax addaleax force-pushed the addaleax:track-memory-http2 branch from 50a9d42 to fa8d967 Jun 20, 2018

@addaleax addaleax removed the blocked label Jun 20, 2018

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Jun 20, 2018

Rebased, this should no longer be blocked

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

@addaleax addaleax added the author ready label Jun 20, 2018

@TimothyGu

TimothyGu approved these changes Jun 21, 2018
View changes

Copy link
Member

TimothyGu left a comment

Still LGTM.
src/node_http2.cc
if (mem != nullptr) {
// Adjust the memory info counter.
session->current_nghttp2_memory_ += size - previous_size;
*reinterpret_cast<size_t*>(mem) = size;

This comment has been minimized.

Sign in to view
Copy link
@TimothyGu

TimothyGu Jun 21, 2018

Member

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.

@cTn-dev cTn-dev referenced this pull request Jun 22, 2018

Closed

Possible memory leak in http2 #21332

@ChALkeR ChALkeR added the memory label Jun 22, 2018

@apapirovski

This comment has been minimized.

Sign in to view
Copy link
Member

apapirovski commented Jun 25, 2018

Landed in 15c627f

@apapirovski apapirovski closed this Jun 25, 2018

apapirovski added a commit that referenced this pull request Jun 25, 2018

@addaleax @apapirovski
http2: track memory allocated by nghttp2 
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>
Verified
This commit was signed with a verified signature.
@apapirovski apapirovski Anatoli Papirovski
GPG key ID: 614E2E1ABEB4B2C0 Learn about signing commits
Loading status checks…
15c627f

targos added a commit that referenced this pull request Jun 25, 2018

@addaleax @targos
http2: track memory allocated by nghttp2 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
5a71e79

@targos targos referenced this pull request Jul 3, 2018

Merged

v10.6.0 proposal #21629

kjin added a commit to kjin/node that referenced this pull request Aug 23, 2018

@addaleax @kjin
http2: track memory allocated by nghttp2 
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>
5beded5

kjin added a commit to kjin/node that referenced this pull request Sep 17, 2018

@addaleax @kjin
http2: track memory allocated by nghttp2 
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>
4438f11

kjin added a commit to kjin/node that referenced this pull request Sep 19, 2018

@addaleax @kjin
http2: track memory allocated by nghttp2 
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>
354f59a

kjin added a commit to kjin/node that referenced this pull request Sep 25, 2018

@addaleax @kjin
http2: track memory allocated by nghttp2 
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>
ab9af24

kjin added a commit to kjin/node that referenced this pull request Oct 16, 2018

@addaleax @kjin
http2: track memory allocated by nghttp2 
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>
f8ca564

BethGriggs added a commit to BethGriggs/node that referenced this pull request Oct 16, 2018

@addaleax @BethGriggs
http2: track memory allocated by nghttp2 
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: nodejs#22850
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>
Verified
This commit was signed with a verified signature.
@BethGriggs BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
650fcd9

BethGriggs added a commit that referenced this pull request Oct 17, 2018

@addaleax @BethGriggs
http2: track memory allocated by nghttp2 
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>
Verified
This commit was signed with a verified signature.
@BethGriggs BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
20b72fc

@BethGriggs BethGriggs referenced this pull request Oct 30, 2018

Merged

v8.13.0 proposal #23974

@beevelop beevelop referenced this pull request Nov 20, 2018

Closed

2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs #250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.