http2: refactor read mechanism #18030

addaleax · 2018-01-08T03:22:13Z

~~The first commit is from #18020 to avoid merge conflicts.~~

src: introduce internal buffer slice constructor

Add a C++ variant of Buffer.from(arrayBuffer, offset, length).
http2: refactor read mechanism

Refactor the read mechanism to completely avoid copying.

Instead of copying individual DATA frame contents into buffers,
create ArrayBuffer instances for all socket reads and emit
slices of those ArrayBuffers to JS.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included (note that the added test passes independently of this change)
commit message follows commit guidelines

Affected core subsystem(s)

/cc @nodejs/http2

TimothyGu · 2018-01-08T07:37:55Z

src/node_internals.h

@@ -357,6 +357,19 @@ v8::MaybeLocal<v8::Object> New(Environment* env,
 // Mixing operator new and free() is undefined behavior so don't do that.
 v8::MaybeLocal<v8::Object> New(Environment* env, char* data, size_t length);

+inline
+v8::MaybeLocal<v8::Uint8Array> New(Environment* env,


Would it be possible to explicitly note that a HandleScope must have been opened before calling this function?

@TimothyGu I think that’s kind of explicit in the fact that this returns a MaybeLocal; that’s never valid without a HandleScope, right?

(Or, the other way around: The same is true of the other buffer constructors, but we don’t mention it there.)

That's certainly true. But the other Buffer constructors open a HandleScope themselves so I was wondering.

@TimothyGu I removed it in this case because, as far as I can tell, all the temporary handles created here refer to objects that are kept alive be the returned Buffer instance anyway.

If my understanding is correct, I think they could be removed in the other cases as well, but I am not 100 % sure and since some of them are public API I left them the way they were.

TimothyGu · 2018-01-08T07:51:15Z

test/common/index.js

+exports.mustCallAsync = function(fn, exact) {
+  return exports.mustCall((...args) => {
+    fn(...args).then(exports.mustCall());
+  }, exact);


Should the returned function return a promise as well, so that one could chain the functions returned by mustCallAsync?

@TimothyGu yup, done!

mcollina · 2018-01-08T11:14:21Z

lib/perf_hooks.js

@@ -467,6 +469,10 @@ function doNotify() {
 // Set up the callback used to receive PerformanceObserver notifications
 function observersCallback(entry) {
  const type = mapTypes(entry.entryType);
+
+  if (type === NODE_PERFORMANCE_ENTRY_TYPE_HTTP2)
+    collectStats(entry);


I would prefer we didn't define these here, but rather in the HTTP2 part. I think these are also defined for every single instance, which will increase overhead.

@mcollina Can you comment that on @jasnell’s #18020? :)

jasnell

Nice. Did you benchmark this at all? It should yield a nice modest boost.

addaleax · 2018-01-08T17:28:42Z

@jasnell I got about + 3 % in the simple benchmark but I’ll kick off a benchmark CI after #18020 has been merged :)

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`.

Refactor the read mechanism to completely avoid copying. Instead of copying individual `DATA` frame contents into buffers, create `ArrayBuffer` instances for all socket reads and emit slices of those `ArrayBuffer`s to JS.

addaleax · 2018-01-13T14:45:07Z

This is no longer blocked by #18020.

CI: https://ci.nodejs.org/job/node-test-commit/15373/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/93/

addaleax · 2018-01-13T14:54:25Z

@nodejs/benchmarking https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/93/console says

Error: Requested benchmarker 'h2load' is not installed

Is there anything we can do about that?

jasnell · 2018-01-13T17:34:06Z

Ping @nodejs/build

addaleax · 2018-01-18T21:50:27Z

Landed in da30788, 0625627

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

Refactor the read mechanism to completely avoid copying. Instead of copying individual `DATA` frame contents into buffers, create `ArrayBuffer` instances for all socket reads and emit slices of those `ArrayBuffer`s to JS. PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

Refactor the read mechanism to completely avoid copying. Instead of copying individual `DATA` frame contents into buffers, create `ArrayBuffer` instances for all socket reads and emit slices of those `ArrayBuffer`s to JS. PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

Refactor the read mechanism to completely avoid copying. Instead of copying individual `DATA` frame contents into buffers, create `ArrayBuffer` instances for all socket reads and emit slices of those `ArrayBuffer`s to JS. PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-02-27T05:57:21Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. PR-URL: nodejs#18030 Reviewed-By: James M Snell <jasnell@gmail.com>

Refactor the read mechanism to completely avoid copying. Instead of copying individual `DATA` frame contents into buffers, create `ArrayBuffer` instances for all socket reads and emit slices of those `ArrayBuffer`s to JS. PR-URL: nodejs#18030 Reviewed-By: James M Snell <jasnell@gmail.com>

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. Backport-PR-URL: #20456 PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

Refactor the read mechanism to completely avoid copying. Instead of copying individual `DATA` frame contents into buffers, create `ArrayBuffer` instances for all socket reads and emit slices of those `ArrayBuffer`s to JS. Backport-PR-URL: #20456 PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. PR-URL: nodejs#18030 Reviewed-By: James M Snell <jasnell@gmail.com>

Refactor the read mechanism to completely avoid copying. Instead of copying individual `DATA` frame contents into buffers, create `ArrayBuffer` instances for all socket reads and emit slices of those `ArrayBuffer`s to JS. PR-URL: nodejs#18030 Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added blocked PRs that are blocked by other issues or PRs. http2 Issues or PRs related to the http2 subsystem. labels Jan 8, 2018

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 8, 2018

TimothyGu reviewed Jan 8, 2018

View reviewed changes

mcollina reviewed Jan 8, 2018

View reviewed changes

jasnell approved these changes Jan 8, 2018

View reviewed changes

addaleax added 2 commits January 13, 2018 15:37

src: introduce internal buffer slice constructor

406875e

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`.

http2: refactor read mechanism

d6fcbed

Refactor the read mechanism to completely avoid copying. Instead of copying individual `DATA` frame contents into buffers, create `ArrayBuffer` instances for all socket reads and emit slices of those `ArrayBuffer`s to JS.

addaleax removed the blocked PRs that are blocked by other issues or PRs. label Jan 13, 2018

addaleax force-pushed the http2-inbound-nocopy branch from a2c195f to d6fcbed Compare January 13, 2018 14:38

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018

addaleax closed this Jan 18, 2018

addaleax deleted the http2-inbound-nocopy branch January 18, 2018 21:50

addaleax added a commit that referenced this pull request Jan 18, 2018

src: introduce internal buffer slice constructor

da30788

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas pushed a commit that referenced this pull request Jan 30, 2018

src: introduce internal buffer slice constructor

7219a13

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

evanlucas pushed a commit that referenced this pull request Jan 30, 2018

src: introduce internal buffer slice constructor

077bcbd

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added dont-land-on-v6.x labels Feb 27, 2018

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 1, 2018

kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018

src: introduce internal buffer slice constructor

f494635

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. PR-URL: nodejs#18030 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request May 2, 2018

src: introduce internal buffer slice constructor

4e807d6

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. Backport-PR-URL: #20456 PR-URL: #18030 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

src: introduce internal buffer slice constructor

2c5f0d1

Add a C++ variant of `Buffer.from(arrayBuffer, offset, length)`. PR-URL: nodejs#18030 Reviewed-By: James M Snell <jasnell@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: refactor read mechanism #18030

http2: refactor read mechanism #18030

addaleax commented Jan 8, 2018 •

edited

Loading

TimothyGu Jan 8, 2018

addaleax Jan 8, 2018

TimothyGu Jan 8, 2018

addaleax Jan 8, 2018 •

edited

Loading

TimothyGu Jan 8, 2018

addaleax Jan 8, 2018

mcollina Jan 8, 2018

addaleax Jan 8, 2018

jasnell left a comment

addaleax commented Jan 8, 2018

addaleax commented Jan 13, 2018

addaleax commented Jan 13, 2018

jasnell commented Jan 13, 2018

addaleax commented Jan 18, 2018

MylesBorins commented Feb 27, 2018

http2: refactor read mechanism #18030

http2: refactor read mechanism #18030

Conversation

addaleax commented Jan 8, 2018 • edited Loading

Checklist

Affected core subsystem(s)

TimothyGu Jan 8, 2018

Choose a reason for hiding this comment

addaleax Jan 8, 2018

Choose a reason for hiding this comment

TimothyGu Jan 8, 2018

Choose a reason for hiding this comment

addaleax Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

TimothyGu Jan 8, 2018

Choose a reason for hiding this comment

addaleax Jan 8, 2018

Choose a reason for hiding this comment

mcollina Jan 8, 2018

Choose a reason for hiding this comment

addaleax Jan 8, 2018

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

addaleax commented Jan 8, 2018

addaleax commented Jan 13, 2018

addaleax commented Jan 13, 2018

jasnell commented Jan 13, 2018

addaleax commented Jan 18, 2018

MylesBorins commented Feb 27, 2018

addaleax commented Jan 8, 2018 •

edited

Loading

addaleax Jan 8, 2018 •

edited

Loading