FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

perf_hooks,http2: add clearEntries to remove http2 entries by jasnell · Pull Request #18046 · nodejs/node · GitHub

/ node Public

perf_hooks,http2: add clearEntries to remove http2 entries - #18046

Closed
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:http2-perf-clearhttp2
ConversationCommits (2)ChecksFiles changed
Closed

perf_hooks,http2: add clearEntries to remove http2 entries#18046
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:http2-perf-clearhttp2

Conversation

jasnell commented Jan 8, 2018

Copy link
Copy Markdown
Member

Add missing clearHttp2 method to perf_hooks.performance to
remove the http2 entries from the master timeline to prevent
that from being a memory leak.

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, perf_hooks

jasnell 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
perf_hooks,http2: add clearHttp2 to remove http2 entries
b3eebe2 
Add missing clearHttp2 method to `perf_hooks.performance` to
remove the http2 entries from the master timeline to prevent
that from being a memory leak.
jasnell force-pushed the http2-perf-clearhttp2 branch from 09dfda6 to b3eebe2 Compare January 9, 2018 20:38

jasnell commented Jan 9, 2018

Copy link
Copy Markdown
Member Author

ping @nodejs/http2

mcollina reviewed Jan 10, 2018
View reviewed changes
Comment thread lib/perf_hooks.js

clearHttp2() {
this[kClearEntry]('http2');
}

mcollina Jan 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I do not think this path is viable, as we will add clearNet(), clearHttp(), clearHTTPS() and so on. How about clear(string)?

jasnell Jan 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

+1.

Comment thread test/parallel/test-http2-perf_hooks.js Outdated

process.on('exit', () => {
// There shouldn't be any http2 entries left over.
assert.strictEqual(performance.getEntries().length, 1);

mcollina Jan 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

what is the one entry that is left in there? Can we match that instead?

jasnell Jan 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The nodeTiming entry and yes.

jasnell mentioned this pull request Jan 10, 2018
Closed
4 tasks
jasnell changed the title perf_hooks,http2: add clearHttp2 to remove http2 entries perf_hooks,http2: add clearEntries to remove http2 entries Jan 10, 2018

jasnell commented Jan 10, 2018

Copy link
Copy Markdown
Member Author

@mcollina ... updated

mcollina reviewed Jan 10, 2018
View reviewed changes
Comment thread lib/perf_hooks.js
}

clearEntries(name) {
this[kClearEntry](name);

mcollina Jan 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

do we still need the Symbol now that this is exposed? Should we perform any validation?

jasnell Jan 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The symbol form takes the additional name argument, which is not strictly necessary here, so I'd rather keep that. Validation is unnecessary since it would just be a non-op if some other value is passed.

mcollina approved these changes Jan 10, 2018
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

jasnell commented Jan 10, 2018

Copy link
Copy Markdown
Member Author

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

jasnell added a commit that referenced this pull request Jan 11, 2018
perf_hooks,http2: add performance.clear()
20fe04f 
Add missing clear() method to `perf_hooks.performance` to
remove the entries from the master timeline to prevent
that from being a memory leak.

PR-URL: #18046
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell commented Jan 11, 2018

Copy link
Copy Markdown
Member Author

Landed in 20fe04f

jasnell closed this Jan 11, 2018
evanlucas added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 30, 2018
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
perf_hooks,http2: add performance.clear()
645be73 
Add missing clear() method to `perf_hooks.performance` to
remove the entries from the master timeline to prevent
that from being a memory leak.

PR-URL: #18046
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
evanlucas mentioned this pull request Jan 30, 2018
Merged
evanlucas added a commit that referenced this pull request Jan 30, 2018
2018-01-31, Version 9.5.0 (Current)
d665cf6 
Notable changes:

* cluster
  - add cwd to cluster.settings (cjihrig) [#18399](#18399)
* deps
  - upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260)
* meta
  - add Leko to collaborators (Leko) [#18117](#18117)
  - add vdeturckheim as collaborator (vdeturckheim) [#18432](#18432)
* n-api
  - expose n-api version in process.versions (Michael Dawson) [#18067](#18067)
* perf_hooks
  - add performance.clear() (James M Snell) [#18046](#18046)
* stream
  - avoid writeAfterEnd() while ending (陈刚) [#18170](#18170)

PR-URL: #18464
evanlucas added a commit that referenced this pull request Jan 31, 2018
2018-01-31, Version 9.5.0 (Current)
5072454 
Notable changes:

* cluster
  - add cwd to cluster.settings (cjihrig) [#18399](#18399)
* deps
  - upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260)
* meta
  - add Leko to collaborators (Leko) [#18117](#18117)
  - add vdeturckheim as collaborator (vdeturckheim) [#18432](#18432)
* n-api
  - expose n-api version in process.versions (Michael Dawson) [#18067](#18067)
* perf_hooks
  - add performance.clear() (James M Snell) [#18046](#18046)
* stream
  - avoid writeAfterEnd() while ending (陈刚) [#18170](#18170)

PR-URL: #18464
evanlucas added a commit that referenced this pull request Feb 1, 2018
2018-01-31, Version 9.5.0 (Current)
83d0f4c 
Notable changes:

* cluster
  - add cwd to cluster.settings (cjihrig) [#18399](#18399)
* deps
  - upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260)
* meta
  - add Leko to collaborators (Leko) [#18117](#18117)
  - add vdeturckheim as collaborator (vdeturckheim) [#18432](#18432)
* n-api
  - expose n-api version in process.versions (Michael Dawson) [#18067](#18067)
* perf_hooks
  - add performance.clear() (James M Snell) [#18046](#18046)
* stream
  - avoid writeAfterEnd() while ending (陈刚) [#18170](#18170)

PR-URL: #18464
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
perf_hooks,http2: add performance.clear()
6a24afd 
Add missing clear() method to `perf_hooks.performance` to
remove the entries from the master timeline to prevent
that from being a memory leak.

PR-URL: nodejs#18046
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
perf_hooks,http2: add performance.clear()
6bdfb1f 
Add missing clear() method to `perf_hooks.performance` to
remove the entries from the master timeline to prevent
that from being a memory leak.

Backport-PR-URL: #20456
PR-URL: #18046
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
2018-01-31, Version 9.5.0 (Current)
6dc8400 
Notable changes:

* cluster
  - add cwd to cluster.settings (cjihrig) [nodejs#18399](nodejs#18399)
* deps
  - upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260)
* meta
  - add Leko to collaborators (Leko) [nodejs#18117](nodejs#18117)
  - add vdeturckheim as collaborator (vdeturckheim) [nodejs#18432](nodejs#18432)
* n-api
  - expose n-api version in process.versions (Michael Dawson) [nodejs#18067](nodejs#18067)
* perf_hooks
  - add performance.clear() (James M Snell) [nodejs#18046](nodejs#18046)
* stream
  - avoid writeAfterEnd() while ending (陈刚) [nodejs#18170](nodejs#18170)

PR-URL: nodejs#18464
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes

Assignees

No one assigned

Labels

http2 Issues or PRs related to the http2 subsystem. 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.

3 participants

Back | FazBrowse Home | New Git URL