Skip to content
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: use aliased buffer for perf stats, add stats #18020

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 6, 2018

Add an aliased buffer for session and stream statistics, add a few more useful metrics

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 6, 2018
doc/api/http2.md Outdated
* `streamCount` {number} The number of `Http2Stream` instances processed by
the `Http2Session`.
* `receivedBytes` {number} The number of bytes received for this `Http2Session`.
* `sentBytes` {number} The number of bytes sent for this `Http2Session`.
Copy link
Member

@addaleax addaleax Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The net & fs APIs use bytesWritten and bytesRead for these values… might be worth being consistent with that?

Edit: Thinking about it, I would have a pretty strong preference for consistency, because that means we might be able to consolidate some of the code around handling C++ streams on the JS side…

enumerable: true,
value: sessionStats[IDX_SESSION_STATS_MAX_CONCURRENT_STREAMS]
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, why not use plain writable properties? Is that what we do in the rest of the perf API, or even spec-mandated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, it's not spec'd to require it.

@addaleax addaleax added http2 Issues or PRs related to the http2 subsystem. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. labels Jan 8, 2018
@addaleax addaleax mentioned this pull request Jan 8, 2018
3 tasks
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good.

@@ -143,6 +143,8 @@ const { settingsBuffer, optionsBuffer } = binding;
// Node.js core as a performance optimization.
const { sessionState, streamState } = binding;

const { sessionStats, streamStats } = binding;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably be merged with the line before.

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this into the http2 namespace? I'm kind of -1 on having perf-hooks require something within http2 itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not following. The collectStats method is in the internal/http2/util.js file. It is used here because we are instrumenting the http2 performance entry object with the http2 specific details, which must be done within this function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the code in perf_hooks is isolated from the rest of core. We are not loading fs, http, net etc to instrument them. Why do we need to load a part of http2?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, ok, so move the collectStats method in to perf_hooks to keep from having to require anything from http2... yeah, I can do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this for http2 while we don't for http or net?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither http or net currently output any performance metrics via the perf_hooks API. That's something that's on my lower priority todo list. This is there for http2 because we're using an aliased buffer to move those data values out to js instead of setting properties on the object from within the C++ code -- it's far more efficient to do it this way. Eventually, once other bits of code are instrumented with perf_hooks output, similar mechanisms will be added. For instance, I'm planning on doing something similar with dns lookups fairly soon.

@jasnell
Copy link
Member Author

jasnell commented Jan 8, 2018

Updated to address the feedback. PTAL

@jasnell
Copy link
Member Author

jasnell commented Jan 8, 2018

@jasnell
Copy link
Member Author

jasnell commented Jan 8, 2018

Add an aliased buffer for session and stream statistics,
add a few more metrics
@jasnell
Copy link
Member Author

jasnell commented Jan 8, 2018

Trying again with a possible fix for the seemingly unrelated failure: https://ci.nodejs.org/job/node-test-pull-request/12450/

@jasnell
Copy link
Member Author

jasnell commented Jan 8, 2018

@apapirovski
Copy link
Member

I'm reasonably certain this is not a flaky test. I ran it on my local environment against master and it succeeded. I ran a test against this PR and it fails.

@jasnell
Copy link
Member Author

jasnell commented Jan 8, 2018

yeah, except this PR doesn't touch that code at all, which is quite strange. The only thing I can think of is that there's some weird interaction with AliasedBuffer.

@jasnell
Copy link
Member Author

jasnell commented Jan 8, 2018

I'm thinking it has something to do with some alignment issue caused by the new aliased buffers. Trying a different fix.

@apapirovski
Copy link
Member

It's not this PR directly. Just including process.binding('http2') causes this test to fail. So I assume something within the binding initialization is not quite right and it's exposed now because it gets included by perf_hooks.

@jasnell
Copy link
Member Author

jasnell commented Jan 8, 2018

perf_hooks is not used at all by the failing test

@apapirovski
Copy link
Member

perf_hooks is not used at all by the failing test

It's indirectly required by bootstrap_node, which requires internal/process which then requires perf_hooks.

Anyway, a simple reproduction on master is:

'use strict';

process.binding('http2');

const common = require('../common');

process.on('beforeExit', common.mustCall(() => {
  setTimeout(common.mustNotCall(), 1).unref();
}));

@addaleax
Copy link
Member

addaleax commented Jan 8, 2018

@apapirovski I can’t reproduce with that :/

And btw, this reminds me of 941c65b

@jasnell
Copy link
Member Author

jasnell commented Jan 8, 2018

Yeah, that was my first thought too @addaleax but I couldn't find anything that was keeping the loop open. Trying something else... CI: https://ci.nodejs.org/job/node-test-pull-request/12453/

@jasnell
Copy link
Member Author

jasnell commented Jan 8, 2018

Ok, lazy loading the process.binding('http') in perf_hooks fixes it, so thank you for that @apapirovski. Unfortunately, that still doesn't explain why it's happening, as there shouldn't be side effects from process.binding('http2') that should cause this. I've looked a couple times now and can't see anything. @addaleax, can I ask you to take a look to see if anything jumps out at you?

@addaleax
Copy link
Member

addaleax commented Jan 8, 2018

Yes, but that will have to wait until tomorrow :)

@Fishrock123
Copy link
Contributor

I also cannot reproduce from master the way @apapirovski has.

@apapirovski
Copy link
Member

@addaleax @Fishrock123 I'm guessing you're both on Windows? I can reproduce on Linux & macOS.

@Fishrock123
Copy link
Contributor

I am currently using macOS 10.12.6.

@jasnell
Copy link
Member Author

jasnell commented Jan 9, 2018

I'm unable to duplicate on Ubuntu VM or Windows. There really should not be anything in the process.binding('http2') that would keep the loop alive. What could be happening is some weirdness with the http2_state mechanism and the aliased buffers since there is an effect there on env. That's the only real change introduced by this PR so that's the most likely culprit.

jasnell pushed a commit to jasnell/node that referenced this pull request Jan 9, 2018
Scheduling a PerformanceGCCallback should not keep the
loop alive but due to the recent switch to using the
native SetImmediate method, it does. Go back to using
uv_async_t and add a regression test.

PR-URL: nodejs#18051
Fixes: nodejs#18047
Refs: nodejs#18020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
jasnell pushed a commit to jasnell/node that referenced this pull request Jan 9, 2018
Scheduling a PerformanceGCCallback should not keep the
loop alive but due to the recent switch to using the
native SetImmediate method, it does. Go back to using
uv_async_t and add a regression test.

PR-URL: nodejs#18051
Fixes: nodejs#18047
Refs: nodejs#18020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Jan 9, 2018

New CI now that #18051 landed: https://ci.nodejs.org/job/node-test-pull-request/12469/

MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
Scheduling a PerformanceGCCallback should not keep the
loop alive but due to the recent switch to using the
native SetImmediate method, it does. Go back to using
uv_async_t and add a regression test.

Backport-PR-URL: #18050
PR-URL: #18051
Fixes: #18047
Refs: #18020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Jan 9, 2018

This should be ready to go

jasnell added a commit that referenced this pull request Jan 9, 2018
Add an aliased buffer for session and stream statistics,
add a few more metrics

PR-URL: #18020
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Jan 9, 2018

Landed in a02fcd2

@jasnell jasnell closed this Jan 9, 2018
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
Add an aliased buffer for session and stream statistics,
add a few more metrics

PR-URL: #18020
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
Scheduling a PerformanceGCCallback should not keep the
loop alive but due to the recent switch to using the
native SetImmediate method, it does. Go back to using
uv_async_t and add a regression test.

Backport-PR-URL: #18050
PR-URL: #18051
Fixes: #18047
Refs: #18020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
Add an aliased buffer for session and stream statistics,
add a few more metrics

PR-URL: #18020
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
Add an aliased buffer for session and stream statistics,
add a few more metrics

PR-URL: nodejs#18020
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Add an aliased buffer for session and stream statistics,
add a few more metrics

Backport-PR-URL: #20456
PR-URL: #18020
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MylesBorins pushed a commit that referenced this pull request May 22, 2018
Scheduling a PerformanceGCCallback should not keep the
loop alive but due to the recent switch to using the
native SetImmediate method, it does. Go back to using
uv_async_t and add a regression test.

PR-URL: #18051
Fixes: #18047
Refs: #18020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Scheduling a PerformanceGCCallback should not keep the
loop alive but due to the recent switch to using the
native SetImmediate method, it does. Go back to using
uv_async_t and add a regression test.

PR-URL: #18051
Fixes: #18047
Refs: #18020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Scheduling a PerformanceGCCallback should not keep the
loop alive but due to the recent switch to using the
native SetImmediate method, it does. Go back to using
uv_async_t and add a regression test.

PR-URL: #18051
Fixes: #18047
Refs: #18020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. perf_hooks Issues and PRs related to the implementation of the Performance Timing API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants