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

perf_hooks: add property flags to GCPerformanceEntry #29547

Closed
wants to merge 6 commits into from
Closed

perf_hooks: add property flags to GCPerformanceEntry #29547

wants to merge 6 commits into from

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Sep 13, 2019

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

perf_hooks have kind for GC, but do not have flags

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 13, 2019
fanatid
fanatid commented Sep 13, 2019
View reviewed changes
doc/api/perf_hooks.md Outdated Show resolved Hide resolved
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 16, 2019
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. labels Nov 30, 2019
@nodejs-github-bot
Copy link
Collaborator

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

Trott
Trott reviewed Dec 6, 2019
View reviewed changes
doc/api/perf_hooks.md Outdated Show resolved Hide resolved
@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 6, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Dec 6, 2019

This could use more reviews. /cc @nodejs/performance

@addaleax
Copy link
Member

addaleax commented Dec 6, 2019

This LGTM code-wise, but it doesn’t really seem to document what these flags indicate (although I guess the same is true for .kind…)

joyeecheung
joyeecheung reviewed Dec 8, 2019
View reviewed changes
doc/api/perf_hooks.md

When `performanceEntry.entryType` is equal to `'gc'`, the `performance.flags`
property contain additional information about garbage collection operation.
The value may be one of:
Copy link
Member

Choose a reason for hiding this comment

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

If we are exposing this we at least need a note similar to the one in https://nodejs.org/api/v8.html#v8_v8_getheapspacestatistics

Neither the ordering of heap spaces, nor the availability of a heap space can be guaranteed as the statistics are provided via the V8 GetHeapSpaceStatistics function and may change from one V8 version to the next.

As compared to the GC kind the flags here seem to be even less stable.

This also lacks explanation and they are less self-explanatory compared to the gc kinds. I guess we could link to the v8 docs of GCCallbackFlags at least for reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This flags provided via V8 GCCallbackFlags enum and may change from one V8 version to next.

Looks good for adding after constants list?

@BridgeAR
Copy link
Member

This needs a rebase.

@fanatid fanatid force-pushed the perf_hooks-add-flags-for-gc branch from 86b0aa0 to 36b01b7 Compare January 12, 2020 05:52
@fanatid
Copy link
Contributor Author

fanatid commented Jan 12, 2020

Rebased.

@nodejs-github-bot
Copy link
Collaborator

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 12, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

Trott
Trott reviewed Feb 2, 2020
View reviewed changes
doc/api/perf_hooks.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@jasnell
Copy link
Member

jasnell commented Feb 3, 2020

Failure in CI does not appear relevant but kicked off a new CI just in case

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

addaleax pushed a commit that referenced this pull request Feb 4, 2020
@fanatid @addaleax
perf_hooks: add property flags to GCPerformanceEntry
018c3e8 
PR-URL: #29547
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@addaleax
Copy link
Member

addaleax commented Feb 4, 2020

Landed in 018c3e8

@addaleax addaleax closed this Feb 4, 2020
@fanatid fanatid deleted the perf_hooks-add-flags-for-gc branch February 4, 2020 16:43
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@fanatid @codebytere
perf_hooks: add property flags to GCPerformanceEntry
e7fea14 
PR-URL: #29547
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Merged
codebytere added a commit that referenced this pull request Feb 18, 2020
@codebytere
2020-02-18, Version 13.9.0 (Current)
1f0f58c 
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
codebytere added a commit that referenced this pull request Feb 18, 2020
@codebytere
2020-02-18, Version 13.9.0 (Current)
0c3c0e7 
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
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@fanatid @targos
perf_hooks: add property flags to GCPerformanceEntry
055d054 
PR-URL: nodejs#29547
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@fanatid @targos
perf_hooks: add property flags to GCPerformanceEntry
9eeee0d 
PR-URL: #29547
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@targos targos mentioned this pull request May 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@richardlau richardlau richardlau left review comments

@joyeecheung joyeecheung joyeecheung left review comments

@Trott Trott Trott left review comments

@legendecas legendecas legendecas approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@fanatid @nodejs-github-bot @Trott @addaleax @BridgeAR @jasnell @joyeecheung @richardlau @legendecas @targos