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.

Sign up for GitHub

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

Jump to bottom

doc: add diagnostics WG session summary #240

Merged
merged 1 commit into from
Jan 8, 2020
Merged

doc: add diagnostics WG session summary #240

merged 1 commit into from
Jan 8, 2020

Conversation

gireeshpunathil
Copy link
Member

christian-bromann
christian-bromann approved these changes Jan 7, 2020
View reviewed changes
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

👍

@christian-bromann christian-bromann merged commit a383650 into openjs-foundation:master Jan 8, 2020
@addaleax addaleax mentioned this pull request Jan 16, 2020
Closed
4 tasks
addaleax added a commit to addaleax/node that referenced this pull request Jan 21, 2020
@addaleax
report: add support for Workers
9de09f3 
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
addaleax added a commit to nodejs/node that referenced this pull request Jan 21, 2020
@addaleax
src: better encapsulate native immediate list
9e4977f 
Refactor for clarity and reusability. Make it more obvious that the
list is a FIFO queue.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 Jan 21, 2020
@addaleax
src: exclude C++ SetImmediate() from count
29693b5 
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.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 Jan 21, 2020
@addaleax
src: add a threadsafe variant of SetImmediate()
79a45e2 
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.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 Jan 21, 2020
@addaleax
src: remove AsyncRequest
de2c68c 
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.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 Jan 21, 2020
@addaleax
src: add interrupts to Environments/Workers
c78c277 
Allow doing what V8’s `v8::Isolate::RequestInterrupt()` does for V8.
This also works when there is no JS code currently executing.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 Jan 21, 2020
@addaleax
src: move MemoryInfo() for worker code to .cc files
880b47d 
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.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 Jan 21, 2020
@addaleax
report: add support for Workers
2606e1e 
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

PR-URL: #31386
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this pull request Feb 17, 2020
@addaleax @codebytere
src: better encapsulate native immediate list
53e566b 
Refactor for clarity and reusability. Make it more obvious that the
list is a FIFO queue.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this pull request Feb 17, 2020
@addaleax @codebytere
src: exclude C++ SetImmediate() from count
74a7cdb 
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.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this pull request Feb 17, 2020
@addaleax @codebytere
src: add a threadsafe variant of SetImmediate()
600e96e 
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.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this pull request Feb 17, 2020
@addaleax @codebytere
src: remove AsyncRequest
f8c45b2 
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.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this pull request Feb 17, 2020
@addaleax @codebytere
src: add interrupts to Environments/Workers
0cacc1f 
Allow doing what V8’s `v8::Isolate::RequestInterrupt()` does for V8.
This also works when there is no JS code currently executing.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this pull request Feb 17, 2020
@addaleax @codebytere
src: move MemoryInfo() for worker code to .cc files
f56de5a 
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.

PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this pull request Feb 17, 2020
@addaleax @codebytere
report: add support for Workers
1bcf2f9 
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

PR-URL: #31386
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: better encapsulate native immediate list
973f8ef 
Refactor for clarity and reusability. Make it more obvious that the
list is a FIFO queue.

PR-URL: nodejs#31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: exclude C++ SetImmediate() from count
5e1bcae 
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.

PR-URL: nodejs#31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: add a threadsafe variant of SetImmediate()
f1aeed5 
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.

PR-URL: nodejs#31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: remove AsyncRequest
3f9d1dd 
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.

PR-URL: nodejs#31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: add interrupts to Environments/Workers
b0ce668 
Allow doing what V8’s `v8::Isolate::RequestInterrupt()` does for V8.
This also works when there is no JS code currently executing.

PR-URL: nodejs#31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: move MemoryInfo() for worker code to .cc files
4e1a192 
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.

PR-URL: nodejs#31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
report: add support for Workers
a9c4fe5 
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

PR-URL: nodejs#31386
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: better encapsulate native immediate list
5df9698 
Refactor for clarity and reusability. Make it more obvious that the
list is a FIFO queue.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: exclude C++ SetImmediate() from count
aafb224 
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.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: add a threadsafe variant of SetImmediate()
748a530 
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.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: remove AsyncRequest
7c2c068 
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.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: add interrupts to Environments/Workers
22bf867 
Allow doing what V8’s `v8::Isolate::RequestInterrupt()` does for V8.
This also works when there is no JS code currently executing.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: move MemoryInfo() for worker code to .cc files
c2176e1 
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.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
report: add support for Workers
87cfbb2 
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

Backport-PR-URL: #32301
PR-URL: #31386
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeekage codeekage codeekage approved these changes

@christian-bromann christian-bromann christian-bromann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Summit Topic: Node.js diagnostics
3 participants
@gireeshpunathil @christian-bromann @codeekage