src: deprecate global COUNTER_* and remove perfctr

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>
nodejs · Sep 19, 2018 · 9d71e6a · 9d71e6a
1 parent 6e746f1
commit 9d71e6a
Show file tree

Hide file tree

Showing 23 changed files with 33 additions and 871 deletions.
diff --git a/configure.py b/configure.py
@@ -455,11 +455,6 @@
 
 parser.add_option_group(http2_optgroup)
 
-parser.add_option('--with-perfctr',
-    action='store_true',
-    dest='with_perfctr',
-    help='build with performance counters (default is true on Windows)')
-
 parser.add_option('--without-dtrace',
     action='store_true',
     dest='without_dtrace',
@@ -475,11 +470,6 @@
     dest='without_npm',
     help='do not install the bundled npm (package manager)')
 
-parser.add_option('--without-perfctr',
-    action='store_true',
-    dest='without_perfctr',
-    help='build without performance counters')
-
 # Dummy option for backwards compatibility
 parser.add_option('--with-snapshot',
     action='store_true',
@@ -1018,14 +1008,6 @@ def configure_node(o):
   else:
     o['variables']['node_use_etw'] = 'false'
 
-  # By default, enable Performance counters on Windows.
-  if flavor == 'win':
-    o['variables']['node_use_perfctr'] = b(not options.without_perfctr)
-  elif options.with_perfctr:
-    raise Exception('Performance counter is only supported on Windows.')
-  else:
-    o['variables']['node_use_perfctr'] = 'false'
-
   o['variables']['node_with_ltcg'] = b(options.with_ltcg)
   if flavor != 'win' and options.with_ltcg:
     raise Exception('Link Time Code Generation is only supported on Windows.')

diff --git a/lib/_http_client.js b/lib/_http_client.js
@@ -285,7 +285,6 @@ util.inherits(ClientRequest, OutgoingMessage);
 
 ClientRequest.prototype._finish = function _finish() {
   DTRACE_HTTP_CLIENT_REQUEST(this, this.connection);
-  COUNTER_HTTP_CLIENT_REQUEST();
   OutgoingMessage.prototype._finish.call(this);
 };
 
@@ -554,7 +553,6 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
   }
 
   DTRACE_HTTP_CLIENT_RESPONSE(socket, req);
-  COUNTER_HTTP_CLIENT_RESPONSE();
   req.res = res;
   res.req = req;
 

diff --git a/lib/_http_server.js b/lib/_http_server.js
@@ -140,7 +140,6 @@ util.inherits(ServerResponse, OutgoingMessage);
 
 ServerResponse.prototype._finish = function _finish() {
   DTRACE_HTTP_SERVER_RESPONSE(this.connection);
-  COUNTER_HTTP_SERVER_RESPONSE();
   OutgoingMessage.prototype._finish.call(this);
 };
 
@@ -624,7 +623,6 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {
 
   res.shouldKeepAlive = keepAlive;
   DTRACE_HTTP_SERVER_REQUEST(req, socket);
-  COUNTER_HTTP_SERVER_REQUEST();
 
   if (socket._httpMessage) {
     // There are already pending outgoing res, append.

diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
@@ -172,11 +172,11 @@
       NativeModule.require('internal/process/esm_loader').setup();
     }
 
+    const { deprecate } = NativeModule.require('internal/util');
     {
       // Install legacy getters on the `util` binding for typechecking.
       // TODO(addaleax): Turn into a full runtime deprecation.
       const { pendingDeprecation } = process.binding('config');
-      const { deprecate } = NativeModule.require('internal/util');
       const utilBinding = internalBinding('util');
       const types = internalBinding('types');
       for (const name of [
@@ -195,6 +195,31 @@
       }
     }
 
+    // TODO(jasnell): The following have been globals since around 2012.
+    // That's just silly. The underlying perfctr support has been removed
+    // so these are now deprecated non-ops that can be removed after one
+    // major release cycle.
+    if (process.platform === 'win32') {
+      function noop() {}
+      const names = [
+        'NET_SERVER_CONNECTION',
+        'NET_SERVER_CONNECTION_CLOSE',
+        'HTTP_SERVER_REQUEST',
+        'HTTP_SERVER_RESPONSE',
+        'HTTP_CLIENT_REQUEST',
+        'HTTP_CLIENT_RESPONSE'
+      ];
+      for (var n = 0; n < names.length; n++) {
+        Object.defineProperty(global, `COUNTER_${names[n]}`, {
+          configurable: true,
+          enumerable: false,
+          value: deprecate(noop,
+                           `COUNTER_${names[n]}() is deprecated.`,
+                           'DEP00XX')
+        });
+      }
+    }
+
     perf.markMilestone(NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE);
 
     setupAllowedFlags();

diff --git a/lib/net.js b/lib/net.js
@@ -615,7 +615,6 @@ Socket.prototype._destroy = function(exception, cb) {
   cb(exception);
 
   if (this._server) {
-    COUNTER_NET_SERVER_CONNECTION_CLOSE(this);
     debug('has server');
     this._server._connections--;
     if (this._server._emitCloseIfDrained) {
@@ -1522,7 +1521,6 @@ function onconnection(err, clientHandle) {
   socket._server = self;
 
   DTRACE_NET_SERVER_CONNECTION(socket);
-  COUNTER_NET_SERVER_CONNECTION(socket);
   self.emit('connection', socket);
 }
 

diff --git a/node.gyp b/node.gyp
@@ -4,7 +4,6 @@
     'v8_trace_maps%': 0,
     'node_use_dtrace%': 'false',
     'node_use_etw%': 'false',
-    'node_use_perfctr%': 'false',
     'node_no_browser_globals%': 'false',
     'node_code_cache_path%': '',
     'node_use_v8_platform%': 'true',
@@ -285,11 +284,6 @@
                   'sources': [
                     'tools/msvs/genfiles/node_etw_provider.rc'
                   ],
-                }],
-                [ 'node_use_perfctr=="true"', {
-                  'sources': [
-                    'tools/msvs/genfiles/node_perfctr_provider.rc',
-                   ],
                 }]
               ],
             }],
@@ -569,28 +563,6 @@
             }],
           ],
         }],
-        [ 'node_use_perfctr=="true"', {
-          'defines': [ 'HAVE_PERFCTR=1' ],
-          'dependencies': [ 'node_perfctr' ],
-          'include_dirs': [
-            'src',
-            'tools/msvs/genfiles',
-            '<(SHARED_INTERMEDIATE_DIR)' # for node_natives.h
-          ],
-          'sources': [
-            'src/node_win32_perfctr_provider.h',
-            'src/node_win32_perfctr_provider.cc',
-            'src/node_counters.cc',
-            'src/node_counters.h',
-          ],
-          'conditions': [
-            ['node_intermediate_lib_type != "static_library"', {
-              'sources': [
-                'tools/msvs/genfiles/node_perfctr_provider.rc',
-              ],
-            }],
-          ],
-        }],
         [ 'node_use_dtrace=="true"', {
           'defines': [ 'HAVE_DTRACE=1' ],
           'dependencies': [
@@ -714,30 +686,6 @@
         } ]
       ]
     },
-    # generate perf counter header and resource files
-    {
-      'target_name': 'node_perfctr',
-      'type': 'none',
-      'conditions': [
-        [ 'node_use_perfctr=="true"', {
-          'actions': [
-            {
-              'action_name': 'node_perfctr_man',
-              'inputs': [ 'src/res/node_perfctr_provider.man' ],
-              'outputs': [
-                'tools/msvs/genfiles/node_perfctr_provider.h',
-                'tools/msvs/genfiles/node_perfctr_provider.rc',
-                'tools/msvs/genfiles/MSG00001.BIN',
-              ],
-              'action': [ 'ctrpp <@(_inputs) '
-                          '-o tools/msvs/genfiles/node_perfctr_provider.h '
-                          '-rc tools/msvs/genfiles/node_perfctr_provider.rc'
-              ]
-            },
-          ],
-        } ]
-      ]
-    },
     {
       'target_name': 'node_js2c',
       'type': 'none',
@@ -758,9 +706,6 @@
             [ 'node_use_dtrace=="false" and node_use_etw=="false"', {
               'inputs': [ 'src/notrace_macros.py' ]
             }],
-            [ 'node_use_perfctr=="false"', {
-              'inputs': [ 'src/noperfctr_macros.py' ]
-            }],
             [ 'node_debug_lib=="false"', {
               'inputs': [ 'tools/nodcheck_macros.py' ]
             }],
@@ -989,9 +934,6 @@
             'HAVE_OPENSSL=1',
           ],
         }],
-        [ 'node_use_perfctr=="true"', {
-          'defines': [ 'HAVE_PERFCTR=1' ],
-        }],
         ['v8_enable_inspector==1', {
           'sources': [
             'test/cctest/test_inspector_socket.cc',

diff --git a/src/node.cc b/src/node.cc
@@ -31,10 +31,6 @@
 #include "node_context_data.h"
 #include "tracing/traced_value.h"
 
-#if defined HAVE_PERFCTR
-#include "node_counters.h"
-#endif
-
 #if HAVE_OPENSSL
 #include "node_crypto.h"
 #endif
@@ -2152,10 +2148,6 @@ void LoadEnvironment(Environment* env) {
   InitDTrace(env, global);
 #endif
 
-#if defined HAVE_PERFCTR
-  InitPerfCounters(env, global);
-#endif
-
   // Enable handling of uncaught exceptions
   // (FatalException(), break on uncaught exception in debugger)
   //

diff --git a/src/node_counters.cc b/src/node_counters.cc