src: deprecate global COUNTER_* functions by jasnell · Pull Request #22485 · nodejs/node

jasnell · 2018-08-23T17:21:10Z

To support Performance Counters on Windows, a number of
global COUNTER_ methods were added that are undocumented
and really only intended to be used internally by Node.js.

~~This replaces those with an internalBinding that is only
enabled when the --with-perfctr configuration option is
used and adds a runtime deprecation for the existing global
functions.

Intent is to remove the globals after one major release
cycle with the runtime deprecations.~~

Update: This removes the perfctr support given that it's apparently been non-functional for a while now and no one noticed. The global js functions are replaced with deprecated non-ops that can be removed in the next major release cycle.

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

nodejs-github-bot · 2018-08-23T17:21:11Z

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/650/pipeline

jasnell · 2018-08-23T17:21:40Z

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

jasnell · 2018-08-23T19:37:13Z

~~CI: https://ci.nodejs.org/job/node-test-pull-request/16700/~~
CI: https://ci.nodejs.org/job/node-test-pull-request/16701/

jasnell · 2018-08-23T21:06:11Z

Relevant failures in CI. Looking into it.

jasnell · 2018-08-23T21:56:57Z

Ok, should be a working CI now: https://ci.nodejs.org/job/node-test-pull-request/16703/

jasnell · 2018-08-23T23:52:48Z

CI is good.

jasnell · 2018-08-23T23:53:40Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1520/

jasnell · 2018-08-27T21:13:15Z

New CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/16796/

joaocgreis

Rubber-stamp LGTM. Removing the global functions LGTM. However, I'm not sure this should land, given the reasons detailed in nodejs/diagnostics#223 (comment) .

mcollina · 2018-09-01T06:56:36Z

Can you just increase this by 1?

addaleax · 2018-09-02T11:47:04Z

This would need at least a rebase (but it sounds like it might not happen at all?)

jasnell · 2018-09-04T18:17:42Z

I will be refactoring this to:

Remove the counter support entirely
Temporarily leave in the globals as deprecated non-ops that can be removed in 12.

jasnell · 2018-09-06T15:42:42Z

@joaocgreis @cjihrig @BridgeAR ... this PR has been significantly updated to just remove the perfctr support entirely and replace the global js functions with deprecated, non-enumerable non-ops. Given that the three of you signed off on the prior version, please take another look.

joaocgreis · 2018-09-10T22:35:55Z

There was an issue with git-rebase, resumed: https://ci.nodejs.org/view/All/job/node-test-pull-request/17109/

jasnell · 2018-09-10T23:57:39Z

Flaky failure on Windows... resuming: https://ci.nodejs.org/view/All/job/node-test-pull-request/17110/

To support Performance Counters on Windows, a number of global `COUNTER_` methods were added that are undocumented and really only intended to be used internally by Node.js. Unfortunately, the perfctr support apparently hasn't even worked for quite a while and no one has even complained. This removes the perfctr support and replaces the global functions with deprecated non-ops for now, with the intent of removing those outright in the next major release cycle.

jasnell · 2018-09-12T14:23:56Z

Unrelated failure in windows again... running a full new CI after rebase... https://ci.nodejs.org/job/node-test-pull-request/17143/

addaleax · 2018-09-17T15:46:18Z

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

Trott · 2018-09-18T19:38:17Z

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/17300/

danbev · 2018-09-19T09:16:14Z

Landed in 9d71e6a.

To support Performance Counters on Windows, a number of global `COUNTER_` methods were added that are undocumented and really only intended to be used internally by Node.js. Unfortunately, the perfctr support apparently hasn't even worked for quite a while and no one has even complained. This removes the perfctr support and replaces the global functions with deprecated non-ops for now, with the intent of removing those outright in the next major release cycle. PR-URL: #22485 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com>

jasnell · 2018-09-19T12:52:30Z

Thank you @danbev ...

It's frustrating that it took 5 re-runs of the ci to get this landed.

Notable changes: * Build * FreeBSD 10 is no longer supported. [#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * `crypto` * PEM-level encryption is now supported. [#23151](#23151) * An API for key pair generation has been added. [#22660](#22660) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * `http2` * An event will be emitted when a `PING` frame is received. [#23009](#23009) * Support for the `ORIGIN` frame has been added. [#22956](#22956) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Promises * A new `multipleResolves` event will be emitted when a Promise is resolved (or rejected) more than once. [#22218](#22218) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[nodejs#22617](nodejs#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [nodejs#21316](nodejs#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [nodejs#21649](nodejs#21649) * `console.time()` will no longer reset a timer if it already exists. [nodejs#20442](nodejs#20442) * Dependencies * V8 has been updated to 7.0. [nodejs#22754](nodejs#22754) * `fs` * The `fs.read()` method now requires a callback. [nodejs#22146](nodejs#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[nodejs#20735](nodejs#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [nodejs#20270](nodejs#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [nodejs#22951](nodejs#22951) * Internal * Windows performance-counter support has been removed. [nodejs#22485](nodejs#22485) * The `--expose-http2` command-line option has been removed. [nodejs#20887](nodejs#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [nodejs#20002](nodejs#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [nodejs#22281](nodejs#22281) * `util.inspect()` output size is limited to 128 MB by default. [nodejs#22756](nodejs#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [nodejs#21914](nodejs#21914)

nodejs/node#22485

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](nodejs/node#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](nodejs/node#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](nodejs/node#20442) * Dependencies * V8 has been updated to 7.0. [#22754](nodejs/node#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](nodejs/node#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](nodejs/node#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](nodejs/node#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](nodejs/node#22951) * Internal * Windows performance-counter support has been removed. [#22485](nodejs/node#22485) * The `--expose-http2` command-line option has been removed. [#20887](nodejs/node#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](nodejs/node#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](nodejs/node#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](nodejs/node#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](nodejs/node#21914)

nodejs/node#22485

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](nodejs/node#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](nodejs/node#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](nodejs/node#20442) * Dependencies * V8 has been updated to 7.0. [#22754](nodejs/node#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](nodejs/node#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](nodejs/node#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](nodejs/node#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](nodejs/node#22951) * Internal * Windows performance-counter support has been removed. [#22485](nodejs/node#22485) * The `--expose-http2` command-line option has been removed. [#20887](nodejs/node#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](nodejs/node#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](nodejs/node#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](nodejs/node#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](nodejs/node#21914)

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 23, 2018

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 23, 2018

jasnell mentioned this pull request Aug 23, 2018
Performance Counter, --with-perfctr nodejs/diagnostics#223
Closed

BridgeAR approved these changes Aug 23, 2018
View reviewed changes

jasnell force-pushed the deprecate_global_counters branch 2 times, most recently from 93a47ec to 9cd812dCompare August 23, 2018 21:55

jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 23, 2018

cjihrig approved these changes Aug 24, 2018
View reviewed changes

jasnell requested a review from a teamAugust 27, 2018 21:00

jasnell force-pushed the deprecate_global_counters branch from 9cd812d to 6615cd1Compare August 27, 2018 21:10

joaocgreis approved these changes Aug 31, 2018
View reviewed changes

mcollina reviewed Sep 1, 2018
View reviewed changes

Comment threadtest/parallel/test-bootstrap-modules.js Outdated
mcollinaSep 1, 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.
Can you just increase this by 1?

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 2, 2018

BridgeAR added the wip Issues and PRs that are still a work in progress. label Sep 5, 2018

jasnell force-pushed the deprecate_global_counters branch 2 times, most recently from d4c0b90 to 5cd1ce4Compare September 6, 2018 15:38

jasnell removed the wip Issues and PRs that are still a work in progress. label Sep 6, 2018

jasnell force-pushed the deprecate_global_counters branch from 5cd1ce4 to 5574a5eCompare September 6, 2018 15:42

jasnell force-pushed the deprecate_global_counters branch from 5574a5e to ac71e30Compare September 6, 2018 15:43

joaocgreis approved these changes Sep 10, 2018
View reviewed changes

jasnell force-pushed the deprecate_global_counters branch from 2d67d1f to 4e6ef4dCompare September 12, 2018 14:23

jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2018

danbev closed this Sep 19, 2018

jasnell mentioned this pull request Oct 2, 2018
11.0.0 proposal #23230
Closed
4 tasks

deepak1556 added a commit to electron/node that referenced this pull request Oct 25, 2018
build: [gn] remove node_use_perfctr
1f8ce6a
nodejs/node#22485

MarshallOfSound pushed a commit to electron/node that referenced this pull request Oct 25, 2018
build: [gn] remove node_use_perfctr
522042f
nodejs/node#22485

refack mentioned this pull request Nov 17, 2018
Upon reviewing the installation process it rolls back and reports a fatal error. #9181
Closed

deepak1556 added a commit to electron/node that referenced this pull request Dec 10, 2018
build: [gn] remove node_use_perfctr
e43c403
nodejs/node#22485

Uh oh!

Uh oh!

Conversation

jasnell commented Aug 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Aug 23, 2018

Uh oh!

jasnell commented Aug 23, 2018

Uh oh!

jasnell commented Aug 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Aug 23, 2018

Uh oh!

jasnell commented Aug 23, 2018

Uh oh!

jasnell commented Aug 23, 2018

Uh oh!

jasnell commented Aug 23, 2018

Uh oh!

jasnell commented Aug 27, 2018

Uh oh!

joaocgreis left a comment

Choose a reason for hiding this comment

Uh oh!

mcollinaSep 1, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 2, 2018

Uh oh!

jasnell commented Sep 4, 2018

Uh oh!

jasnell commented Sep 6, 2018

Uh oh!

joaocgreis commented Sep 10, 2018

Uh oh!

jasnell commented Sep 10, 2018

Uh oh!

jasnell commented Sep 12, 2018

Uh oh!

addaleax commented Sep 17, 2018

Uh oh!

Trott commented Sep 18, 2018

Uh oh!

danbev commented Sep 19, 2018

Uh oh!

jasnell commented Sep 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jasnell commented Aug 23, 2018•
edited
Loading

jasnell commented Aug 23, 2018•
edited
Loading