Skip to content
This repository has been archived by the owner on Aug 11, 2020. It is now read-only.

C++-side refactor #126

Closed
wants to merge 12 commits into from
Closed

C++-side refactor #126

wants to merge 12 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 21, 2019

quic: remove typedef enum

Using typedef enum is a C-ism.

quic: remove user_data from quic buffer callback

Using std::function and an opaque pointer are pretty much
antithetical. This commit chooses the more C++-y variant, i.e.
std::function.

quic: cleanup node_quic_buffer.h

Prefer inline functions over macros, use explicit nullptr checks,
do not mark functions with implicit inline linkage as inline,
re-use code where possible, etc.

src: cleanup histogram code

Move methods that do not need to be inline into their own file,
replace std::function with a template variant for performance,
remove inline for functions with implicit inline linkage.

quic: remove opaque pointer from Timer callback

Use the more C++-y std::function approach which renders opaque
pointers unnecessary.

quic: cleanup QuicSession code
  • Do not create new v8::Function objects, both for performance
    and because it is unclear whether that leaks memory
    (Node.js issue 28988).
  • Do not put functions into the inline header that do not need to
    be there or that do not make sense as inline functions
    (in particular, callbacks that are provided to C libraries can
    not be realized as inline functions by the compiler).
  • Remove implicit/redundant inline qualifiers and add appropriate
    const qualifiers.
  • Remove unnecessary this-> prefixes.
  • Return early for JS failures rather than accumulating them.
  • Minor stylistic changes.
src: refactor memory tracker implementation

Use a template class instead of virtual methods to allow inlining
the subclass methods, as well as other minor cleanups.

http2: use shared memory tracking implementation

The extensive testing done on http2 makes it easier to make sure
the implementation is correct (and doesn’t diverge unnecessarily).

quic: remove QuicStreamListener

This listener currently had the drawbacks of leaking memory
when an error is encountered and was otherwise equivalent to
the default listener.

quic: more general C++ cleanup

Use default initializers, const qualifiers, minor style changes
to match more common style in the codebase.

Using `std::function` and an opaque pointer are pretty much
antithetical. This commit chooses the more C++-y variant, i.e.
`std::function`.
Using `typedef enum` is a C-ism.
Prefer inline functions over macros, use explicit nullptr checks,
do not mark functions with implicit inline linkage as inline,
re-use code where possible, etc.
Move methods that do not need to be inline into their own file,
replace `std::function` with a template variant for performance,
remove `inline` for functions with implicit inline linkage.
Use the more C++-y `std::function` approach which renders opaque
pointers unnecessary.
- Do not create new `v8::Function` objects, both for performance
  and because it is unclear whether that leaks memory
  (Node.js issue 28988).
- Do not put functions into the inline header that do not need to
  be there or that do not make sense as inline functions
  (in particular, callbacks that are provided to C libraries can
  not be realized as inline functions by the compiler).
- Remove implicit/redundant `inline` qualifiers and add appropriate
  `const` qualifiers.
- Remove unnecessary `this->` prefixes.
- Return early for JS failures rather than accumulating them.
- Minor stylistic changes.
Use a template class instead of virtual methods to allow inlining
the subclass methods, as well as other minor cleanups.
The extensive testing done on http2 makes it easier to make sure
the implementation is correct (and doesn’t diverge unnecessarily).
This listener currently had the drawbacks of leaking memory
when an error is encountered and was otherwise equivalent to
the default listener.
Use default initializers, `const` qualifiers, minor style changes
to match more common style in the codebase.
src/node_quic_util.h Outdated Show resolved Hide resolved
src/histogram.cc Show resolved Hide resolved
addaleax added a commit that referenced this pull request Sep 23, 2019
Using `std::function` and an opaque pointer are pretty much
antithetical. This commit chooses the more C++-y variant, i.e.
`std::function`.

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2019
Using `typedef enum` is a C-ism.

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2019
Prefer inline functions over macros, use explicit nullptr checks,
do not mark functions with implicit inline linkage as inline,
re-use code where possible, etc.

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2019
Move methods that do not need to be inline into their own file,
replace `std::function` with a template variant for performance,
remove `inline` for functions with implicit inline linkage.

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2019
Use the more C++-y `std::function` approach which renders opaque
pointers unnecessary.

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2019
- Do not create new `v8::Function` objects, both for performance
  and because it is unclear whether that leaks memory
  (Node.js issue 28988).
- Do not put functions into the inline header that do not need to
  be there or that do not make sense as inline functions
  (in particular, callbacks that are provided to C libraries can
  not be realized as inline functions by the compiler).
- Remove implicit/redundant `inline` qualifiers and add appropriate
  `const` qualifiers.
- Remove unnecessary `this->` prefixes.
- Return early for JS failures rather than accumulating them.
- Minor stylistic changes.

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2019
Use a template class instead of virtual methods to allow inlining
the subclass methods, as well as other minor cleanups.

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2019
The extensive testing done on http2 makes it easier to make sure
the implementation is correct (and doesn’t diverge unnecessarily).

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2019
This listener currently had the drawbacks of leaking memory
when an error is encountered and was otherwise equivalent to
the default listener.

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2019
Use default initializers, `const` qualifiers, minor style changes
to match more common style in the codebase.

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member Author

Landed in 72997d6...ea095af

@addaleax addaleax closed this Sep 23, 2019
@addaleax addaleax deleted the bananas branch September 23, 2019 21:15
jasnell pushed a commit that referenced this pull request Oct 2, 2019
The extensive testing done on http2 makes it easier to make sure
the implementation is correct (and doesn’t diverge unnecessarily).

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 3, 2019
The extensive testing done on http2 makes it easier to make sure
the implementation is correct (and doesn’t diverge unnecessarily).

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 3, 2019
The extensive testing done on http2 makes it easier to make sure
the implementation is correct (and doesn’t diverge unnecessarily).

PR-URL: #126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit to nodejs/node that referenced this pull request Dec 7, 2019
This implements a memory-tracking allocator that can be used to
provide memory allocation facilities to several thread-safe C
libraries, including nghttp2, nghttp3, ngtcp3 and uvwasi.

Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Co-authored-by: James M Snell <jasnell@gmail.com>

PR-URL: #30745
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit to nodejs/node that referenced this pull request Dec 7, 2019
The extensive testing done on http2 makes it easier to make sure
the implementation is correct (and doesn’t diverge unnecessarily).

Refs: nodejs/quic#126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit to nodejs/node that referenced this pull request Dec 7, 2019
PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit to nodejs/node that referenced this pull request Dec 7, 2019
PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit to nodejs/node that referenced this pull request Dec 7, 2019
This:

- Protects against memory leaks in uvwasi.
- Allows tracking the allocated memory in heap dumps.

PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Dec 9, 2019
This implements a memory-tracking allocator that can be used to
provide memory allocation facilities to several thread-safe C
libraries, including nghttp2, nghttp3, ngtcp3 and uvwasi.

Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Co-authored-by: James M Snell <jasnell@gmail.com>

PR-URL: #30745
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Dec 9, 2019
The extensive testing done on http2 makes it easier to make sure
the implementation is correct (and doesn’t diverge unnecessarily).

Refs: nodejs/quic#126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Dec 9, 2019
PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Dec 9, 2019
PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Dec 9, 2019
This:

- Protects against memory leaks in uvwasi.
- Allows tracking the allocated memory in heap dumps.

PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Jan 14, 2020
This implements a memory-tracking allocator that can be used to
provide memory allocation facilities to several thread-safe C
libraries, including nghttp2, nghttp3, ngtcp3 and uvwasi.

Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Co-authored-by: James M Snell <jasnell@gmail.com>

PR-URL: #30745
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Jan 14, 2020
The extensive testing done on http2 makes it easier to make sure
the implementation is correct (and doesn’t diverge unnecessarily).

Refs: nodejs/quic#126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Jan 14, 2020
PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Jan 14, 2020
PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Jan 14, 2020
This:

- Protects against memory leaks in uvwasi.
- Allows tracking the allocated memory in heap dumps.

PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Feb 6, 2020
This implements a memory-tracking allocator that can be used to
provide memory allocation facilities to several thread-safe C
libraries, including nghttp2, nghttp3, ngtcp3 and uvwasi.

Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Co-authored-by: James M Snell <jasnell@gmail.com>

PR-URL: #30745
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Feb 6, 2020
The extensive testing done on http2 makes it easier to make sure
the implementation is correct (and doesn’t diverge unnecessarily).

Refs: nodejs/quic#126
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Feb 6, 2020
PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Feb 6, 2020
PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Feb 6, 2020
This:

- Protects against memory leaks in uvwasi.
- Allows tracking the allocated memory in heap dumps.

PR-URL: #30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants