report: add support for Workers by addaleax · Pull Request #31386 · nodejs/node · GitHub

Only the last commit is concerned with the report feature itself. The other commits are work leading up to it, making features like this easier to add in general, and related cleanup.

src: better encapsulate native immediate list

Refactor for clarity and reusability. Make it more obvious that the
list is a FIFO queue.

src: exclude C++ SetImmediate() from count

There is no real reason to manage a count manually, given that
checking whether there are C++ callbacks is a single pointer
comparison.

This makes it easier to add other kinds of native C++ callbacks
that are managed in a similar way.

src: add a threadsafe variant of SetImmediate()

Add a variant of SetImmediate() that can be called from any thread.
This allows removing the AsyncRequest abstraction and replaces it
with a more generic mechanism.

src: remove AsyncRequest

Remove AsyncRequest from the source code, and replace its
usage with threadsafe SetImmediate() calls. This has the
advantage of being able to pass in any function, rather than
one that is defined when the AsyncRequest is “installed”.

This necessitates two changes:

The stopping flag (which was only used in one case and ignored
in the other) is now a direct member of the Environment class.
Workers no longer have their own libuv handles, requiring
manual management of their libuv ref count.

As a drive-by fix, the can_call_into_js variable was turned
into an atomic variable. While there have been no bug reports,
the flag is set from Stop(env) calls, which are supposed to
be possible from any thread.

src: add interrupts to Environments/Workers

Allow doing what V8’s v8::Isolate::RequestInterrupt() does for V8.
This also works when there is no JS code currently executing.

src: move MemoryInfo() for worker code to .cc files

This is a) the right thing to do anyway because these functions
can not be inlined by the compiler and b) avoids compilation warnings
in the following commit.

report: add support for Workers

Include a report for each sub-Worker of the current Node.js instance.

This adds a feature that is necessary for eventually making the report
feature stable, as was discussed during the last collaborator summit.

Refs: openjs-foundation/summit#240

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

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

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

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

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

Will try to find time to look at the technical aspects. Left a few comments re. tests.

Also maybe bump the NODE_REPORT_VERSION?

node/src/node_report.cc

Line 22 in 4187fcb

constexpr int NODE_REPORT_VERSION = 1;

I'm a bit unsure on the criterea for bumping the version (#28121 (comment)), but this does add a new section and header value.

cc @boneskull as a tool author that consumes the report.

I'm a bit unsure on the criterea for bumping the version (#28121 (comment)), but this does add a new section and header value.

Actually reskimming the PR it looks like the support for workers is to nest reports inside the report so this probably does need a version bump.

@addaleax - this is awesome!

should we inhibit workers from taking reports? do you see any potential downside of doing that?
placement of the workers section: right now you are placing it after the libuv section. Given that the worker's report section is more or less identical to that of main thread, does it make sense to have the worker's sections as siblings to the main, as opposed to a component?
the main thread is blocked while workers are iterated and report collected, with the block duration proportional to the thread count, should we document this?
some process wide sections (such as the header, environment, shared libs etc.) are replicated for each worker. While this is not a problem now, I guess we could refactor GetReport that records sections that are env specific and then compose a master report by combining env specific sections and process specific ones?

also, I am finding it difficult to relate the other commits with add support for Workers - can you pls help a little? especially the remove AsyncRequest and its dependent commits.

I'm a bit unsure on the criterea for bumping the version (#28121 (comment)), but this does add a new section and header value.

Actually reskimming the PR it looks like the support for workers is to nest reports inside the report so this probably does need a version bump.

@richardlau Done, although I agree that there should be documentation on the circumstances under which the version is bumped – otherwise it doesn’t seem like a useful value.

@addaleax - this is awesome!

@gireeshpunathil Thanks! :)

should we inhibit workers from taking reports? do you see any potential downside of doing that?

I think the question should be whether there are any upsides to inhibiting workers from taking reports. I don’t see any, and I’m not a fan of arbitrary restrictions, so I’d go with “no” for now. (I.e. the “obvious” downside is that it makes the report feature less powerful.)

placement of the workers section: right now you are placing it after the libuv section. Given that the worker's report section is more or less identical to that of main thread, does it make sense to have the worker's sections as siblings to the main, as opposed to a component?

I don’t think so. Workers are sub-objects of their parent threads, and in particular, listing them as siblings would lose the tree structure of Workers, i.e. a Worker started by a Worker would be indistinguishable from a Worker started by the main thread in the case of listing them as siblings (unless information like the parent thread id or similar is added).

the main thread is blocked while workers are iterated and report collected, with the block duration proportional to the thread count, should we document this?

I’ve added a section to the report docs, PTAL 👍

some process wide sections (such as the header, environment, shared libs etc.) are replicated for each worker. While this is not a problem now, I guess we could refactor GetReport that records sections that are env specific and then compose a master report by combining env specific sections and process specific ones?

Yes, I’ve thought about that too. For now, duplicating the information seemed somewhat reasonable because it means that any Worker’s report itself is a fully valid report that can be looked at independently, but I also see the benefit of excluding per-process information.

(And in particular, for the environment variables, it’s not really correct to just use the “global” env vars, imo. But then again, that problem seems independent from this PR – we’re already doing this for reports generated from Workers.)

also, I am finding it difficult to relate the other commits with add support for Workers - can you pls help a little? especially the remove AsyncRequest and its dependent commits.

As the PR description says – it’s work leading up to it and related cleanup. Removing AsyncRequest doesn’t need to happen here, but it is closely related to adding the interrupt feature that the Worker reports use, so it fits in here as general cleanup.

re: worker taking reports - because of this

env->ForEachWorker([&](Worker* w) {

will it loose out taking report for the main thread?

re: worker taking reports - because of this
env->ForEachWorker([&](Worker* w) {
will it loose out taking report for the main thread?

Reports taken from Workers don’t include parent threads, that’s correct. I would also consider this the expected behaviour.

Regarding versioning, we really ought to have used semver for the versioning model on the format. Perhaps it's not too late to switch?

Regarding versioning, we really ought to have used semver for the versioning model on the format. Perhaps it's not too late to switch?

It was originally going to use semver, but I was asked to change it in #28121 (comment).

Let's move the versioning question to nodejs/diagnostics#349, it's definitely bigger than this PR.

Fwiw, this is weirdly breaking one specific HTTP/2 test on Windows. I'm investigating, but that means that this PR still needs at least a minor fixup.

I've pushed a commit that seems to resolve the Windows issue for me, locally (hopefully CI will confirm that). It restores behaviour that was present before the first commit in this PR, where the native SetImmediate() queue only ran callbacks that were scheduled before the queue was being run (and not during it).

The HTTP/2 implementation seems to rely on that in some way, but honestly, that seems like a bug waiting to happen (and maybe even the reason for all the Windows HTTP/2 flakiness?), so I'd like to investigate more tomorrow.

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

This test was broken by de2c68c. Refs: #31386 PR-URL: #31494 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Notable changes: * async_hooks: * add executionAsyncResource (Matteo Collina) #30959 * doc: * add ronag to collaborators (Robert Nagy) #31498 * crypto: * add crypto.diffieHellman (Tobias Nießen) #31178 * add DH support to generateKeyPair (Tobias Nießen) #31178 * simplify DH groups (Tobias Nießen) #31178 * add key type 'dh' (Tobias Nießen) #31178 * report: * add support for Workers (Anna Henningsen) #31386 * src: * move MemoryInfo() for worker code to .cc files (Anna Henningsen) #31386 * add interrupts to Environments/Workers (Anna Henningsen) #31386 * remove AsyncRequest (Anna Henningsen) #31386 * add a threadsafe variant of SetImmediate() (Anna Henningsen) #31386 * exclude C++ SetImmediate() from count (Anna Henningsen) #31386 * better encapsulate native immediate list (Anna Henningsen) #31386 * test: * skip keygen tests on arm systems (Tobias Nießen) #31178

Notable changes: * async_hooks: * add executionAsyncResource (Matteo Collina) #30959 * doc: * add ronag to collaborators (Robert Nagy) #31498 * crypto: * add crypto.diffieHellman (Tobias Nießen) #31178 * add DH support to generateKeyPair (Tobias Nießen) #31178 * simplify DH groups (Tobias Nießen) #31178 * add key type 'dh' (Tobias Nießen) #31178 * report: * add support for Workers (Anna Henningsen) #31386 * test: * skip keygen tests on arm systems (Tobias Nießen) #31178

Notable changes: * async_hooks: * add executionAsyncResource (Matteo Collina) #30959 * doc: * add ronag to collaborators (Robert Nagy) #31498 * crypto: * add crypto.diffieHellman (Tobias Nießen) #31178 * add DH support to generateKeyPair (Tobias Nießen) #31178 * simplify DH groups (Tobias Nießen) #31178 * add key type 'dh' (Tobias Nießen) #31178 * report: * add support for Workers (Anna Henningsen) #31386 * test: * skip keygen tests on arm systems (Tobias Nießen) #31178 PR-URL: #31837

Notable changes: * async_hooks * add executionAsyncResource (Matteo Collina) #30959 * crypto * add crypto.diffieHellman (Tobias Nießen) #31178 * add DH support to generateKeyPair (Tobias Nießen) #31178 * simplify DH groups (Tobias Nießen) #31178 * add key type 'dh' (Tobias Nießen) #31178 * test * skip keygen tests on arm systems (Tobias Nießen) #31178 * perf_hooks * add property flags to GCPerformanceEntry (Kirill Fomichev) #29547 * process * report ArrayBuffer memory in `memoryUsage()` (Anna Henningsen) #31550 * readline * make tab size configurable (Ruben Bridgewater) #31318 * report * add support for Workers (Anna Henningsen) #31386 * worker * add ability to take heap snapshot from parent thread (Anna Henningsen) #31569 * added new collaborators * add ronag to collaborators (Robert Nagy) #31498 PR-URL: #31837

@addaleax should this go to 12? It's not a clean bp - Ill add the label but feel free to swap it.

This test was broken by de2c68c. Refs: #31386 PR-URL: #31494 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

addaleax added worker Issues and PRs related to Worker support. report Issues and PRs related to process.report. labels Jan 16, 2020

addaleax requested a review from gireeshpunathil January 16, 2020 19:43

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 16, 2020

addaleax added the wip Issues and PRs that are still a work in progress. label Jan 16, 2020

addaleax removed the wip Issues and PRs that are still a work in progress. label Jan 16, 2020

richardlau reviewed Jan 16, 2020

View reviewed changes

Comment thread test/common/report.js Outdated

Comment thread test/common/report.js Outdated

richardlau mentioned this pull request Jan 17, 2020

Discussion: Criteria for upgrading diagnostic reporting from experimental to stable #26293

Closed

gireeshpunathil reviewed Jan 17, 2020

View reviewed changes

Comment thread src/node_report.cc Outdated

Comment thread src/node_report.cc Outdated

gireeshpunathil approved these changes Jan 17, 2020

View reviewed changes

jasnell approved these changes Jan 17, 2020

View reviewed changes

cjihrig approved these changes Jan 17, 2020

View reviewed changes

addaleax mentioned this pull request Jan 17, 2020

reportVersion semantics are not defined nodejs/diagnostics#349

Closed

codebytere mentioned this pull request Feb 17, 2020

v13.9.0 proposal #31837

Merged

codebytere added the backport-requested-v12.x label Mar 15, 2020

addaleax mentioned this pull request Mar 15, 2020

src,http2: simplify native immediate queue running and harden http2 test #31502

Closed

3 tasks

addaleax mentioned this pull request Mar 16, 2020

[v12.x] backport BaseObjectPtr, SetImmediate(), Worker+report #32301

Closed

addaleax added backport-open-v12.x and removed backport-requested-v12.x labels Mar 16, 2020

codebytere mentioned this pull request Apr 3, 2020

v12.16.2 proposal #32313

Merged

report: add support for Workers by addaleax · Pull Request #31386 · nodejs/node · GitHub

Uh oh!

Uh oh!

report: add support for Workers - #31386

report: add support for Workers#31386addaleax wants to merge 7 commits intonodejs:masternodejs/node:masterfrom addaleax:async-requestaddaleax/node:async-requestCopy head branch name to clipboard

Conversation

addaleax commented Jan 16, 2020

src: better encapsulate native immediate list

src: exclude C++ SetImmediate() from count

src: add a threadsafe variant of SetImmediate()

src: remove AsyncRequest

src: add interrupts to Environments/Workers

src: move MemoryInfo() for worker code to .cc files

report: add support for Workers

Checklist

Uh oh!

nodejs-github-bot commented Jan 16, 2020

Uh oh!

nodejs-github-bot commented Jan 16, 2020

Uh oh!

nodejs-github-bot commented Jan 16, 2020

Uh oh!

nodejs-github-bot commented Jan 16, 2020

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

richardlau commented Jan 17, 2020

Uh oh!

Uh oh!

Uh oh!

gireeshpunathil commented Jan 17, 2020

Uh oh!

gireeshpunathil commented Jan 17, 2020

Uh oh!

addaleax commented Jan 17, 2020

Uh oh!

gireeshpunathil commented Jan 17, 2020

Uh oh!

addaleax commented Jan 17, 2020

Uh oh!

jasnell commented Jan 17, 2020

Uh oh!

cjihrig commented Jan 17, 2020

Uh oh!

addaleax commented Jan 17, 2020

Uh oh!

addaleax commented Jan 17, 2020

Uh oh!

addaleax commented Jan 17, 2020

Uh oh!

nodejs-github-bot commented Jan 17, 2020

Uh oh!

codebytere commented Mar 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

report: add support for Workers#31386
addaleax wants to merge 7 commits into
nodejs:masterfrom
addaleax:async-request