Skip to content

Commit

Permalink
lib: move DTRACE_* probes out of global scope
Browse files Browse the repository at this point in the history
The DTRACE_* probes have been global for no really good reason.
Move those into an internalBinding.

PR-URL: #26541
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
  • Loading branch information
jasnell committed Mar 12, 2019
1 parent f2064df commit f9ddbb6
Show file tree
Hide file tree
Showing 14 changed files with 92 additions and 54 deletions.
6 changes: 0 additions & 6 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -281,12 +281,6 @@ module.exports = {
BigInt: 'readable',
BigInt64Array: 'readable',
BigUint64Array: 'readable',
DTRACE_HTTP_CLIENT_REQUEST: 'readable',
DTRACE_HTTP_CLIENT_RESPONSE: 'readable',
DTRACE_HTTP_SERVER_REQUEST: 'readable',
DTRACE_HTTP_SERVER_RESPONSE: 'readable',
DTRACE_NET_SERVER_CONNECTION: 'readable',
DTRACE_NET_STREAM_END: 'readable',
TextEncoder: 'readable',
TextDecoder: 'readable',
queueMicrotask: 'readable',
Expand Down
4 changes: 4 additions & 0 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ const {
} = require('internal/errors').codes;
const { validateTimerDuration } = require('internal/timers');
const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;
const {
DTRACE_HTTP_CLIENT_REQUEST,
DTRACE_HTTP_CLIENT_RESPONSE
} = require('internal/dtrace');

const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/;

Expand Down
4 changes: 4 additions & 0 deletions lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ const {
ERR_INVALID_CHAR
} = require('internal/errors').codes;
const Buffer = require('buffer').Buffer;
const {
DTRACE_HTTP_SERVER_REQUEST,
DTRACE_HTTP_SERVER_RESPONSE
} = require('internal/dtrace');

const kServerResponse = Symbol('ServerResponse');

Expand Down
21 changes: 21 additions & 0 deletions lib/internal/dtrace.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use strict';

const config = internalBinding('config');

const {
DTRACE_HTTP_CLIENT_REQUEST = () => {},
DTRACE_HTTP_CLIENT_RESPONSE = () => {},
DTRACE_HTTP_SERVER_REQUEST = () => {},
DTRACE_HTTP_SERVER_RESPONSE = () => {},
DTRACE_NET_SERVER_CONNECTION = () => {},
DTRACE_NET_STREAM_END = () => {}
} = (config.hasDtrace ? internalBinding('dtrace') : {});

module.exports = {
DTRACE_HTTP_CLIENT_REQUEST,
DTRACE_HTTP_CLIENT_RESPONSE,
DTRACE_HTTP_SERVER_REQUEST,
DTRACE_HTTP_SERVER_RESPONSE,
DTRACE_NET_SERVER_CONNECTION,
DTRACE_NET_STREAM_END
};
4 changes: 4 additions & 0 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ const {
} = require('internal/errors');
const { validateInt32, validateString } = require('internal/validators');
const kLastWriteQueueSize = Symbol('lastWriteQueueSize');
const {
DTRACE_NET_SERVER_CONNECTION,
DTRACE_NET_STREAM_END
} = require('internal/dtrace');

// Lazy loaded to improve startup performance.
let cluster;
Expand Down
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@
'lib/internal/dns/promises.js',
'lib/internal/dns/utils.js',
'lib/internal/domexception.js',
'lib/internal/dtrace.js',
'lib/internal/encoding.js',
'lib/internal/errors.js',
'lib/internal/error-serdes.js',
Expand Down
2 changes: 1 addition & 1 deletion src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
Local<Object> global = context->Global();

#if defined HAVE_DTRACE || defined HAVE_ETW
InitDTrace(env, global);
InitDTrace(env);
#endif

Local<Object> process = env->process_object();
Expand Down
9 changes: 8 additions & 1 deletion src/node_binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@
#define NODE_BUILTIN_PROFILER_MODULES(V)
#endif

#if HAVE_DTRACE || HAVE_ETW
#define NODE_BUILTIN_DTRACE_MODULES(V) V(dtrace)
#else
#define NODE_BUILTIN_DTRACE_MODULES(V)
#endif

// A list of built-in modules. In order to do module registration
// in node::Init(), need to add built-in modules in the following list.
// Then in binding::RegisterBuiltinModules(), it calls modules' registration
Expand Down Expand Up @@ -85,7 +91,8 @@
NODE_BUILTIN_OPENSSL_MODULES(V) \
NODE_BUILTIN_ICU_MODULES(V) \
NODE_BUILTIN_REPORT_MODULES(V) \
NODE_BUILTIN_PROFILER_MODULES(V)
NODE_BUILTIN_PROFILER_MODULES(V) \
NODE_BUILTIN_DTRACE_MODULES(V)

// This is used to load built-in modules. Instead of using
// __attribute__((constructor)), we call the _register_<modname>
Expand Down
4 changes: 4 additions & 0 deletions src/node_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ static void Initialize(Local<Object> target,
READONLY_PROPERTY(target,
"bits",
Number::New(env->isolate(), 8 * sizeof(intptr_t)));

#if defined HAVE_DTRACE || defined HAVE_ETW
READONLY_TRUE_PROPERTY(target, "hasDtrace");
#endif
} // InitConfig

} // namespace node
Expand Down
46 changes: 20 additions & 26 deletions src/node_dtrace.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@

namespace node {

using v8::Context;
using v8::FunctionCallbackInfo;
using v8::GCCallbackFlags;
using v8::GCType;
Expand Down Expand Up @@ -262,31 +263,7 @@ void dtrace_gc_done(Isolate* isolate, GCType type, GCCallbackFlags flags) {
}


void InitDTrace(Environment* env, Local<Object> target) {
HandleScope scope(env->isolate());

static struct {
const char* name;
void (*func)(const FunctionCallbackInfo<Value>&);
} tab[] = {
#define NODE_PROBE(name) #name, name
{ NODE_PROBE(DTRACE_NET_SERVER_CONNECTION) },
{ NODE_PROBE(DTRACE_NET_STREAM_END) },
{ NODE_PROBE(DTRACE_HTTP_SERVER_REQUEST) },
{ NODE_PROBE(DTRACE_HTTP_SERVER_RESPONSE) },
{ NODE_PROBE(DTRACE_HTTP_CLIENT_REQUEST) },
{ NODE_PROBE(DTRACE_HTTP_CLIENT_RESPONSE) }
#undef NODE_PROBE
};

for (size_t i = 0; i < arraysize(tab); i++) {
Local<String> key = OneByteString(env->isolate(), tab[i].name);
Local<Value> val = env->NewFunctionTemplate(tab[i].func)
->GetFunction(env->context())
.ToLocalChecked();
target->Set(env->context(), key, val).FromJust();
}

void InitDTrace(Environment* env) {
#ifdef HAVE_ETW
// ETW is neither thread-safe nor does it clean up resources on exit,
// so we can use it only on the main thread.
Expand All @@ -295,10 +272,27 @@ void InitDTrace(Environment* env, Local<Object> target) {
}
#endif

#if defined HAVE_DTRACE || defined HAVE_ETW
env->isolate()->AddGCPrologueCallback(dtrace_gc_start);
env->isolate()->AddGCEpilogueCallback(dtrace_gc_done);
}

void InitializeDTrace(Local<Object> target,
Local<Value> unused,
Local<Context> context,
void* priv) {
Environment* env = Environment::GetCurrent(context);

#if defined HAVE_DTRACE || defined HAVE_ETW
# define NODE_PROBE(name) env->SetMethod(target, #name, name);
NODE_PROBE(DTRACE_NET_SERVER_CONNECTION)
NODE_PROBE(DTRACE_NET_STREAM_END)
NODE_PROBE(DTRACE_HTTP_SERVER_REQUEST)
NODE_PROBE(DTRACE_HTTP_SERVER_RESPONSE)
NODE_PROBE(DTRACE_HTTP_CLIENT_REQUEST)
NODE_PROBE(DTRACE_HTTP_CLIENT_RESPONSE)
# undef NODE_PROBE
#endif
}

} // namespace node
NODE_MODULE_CONTEXT_AWARE_INTERNAL(dtrace, node::InitializeDTrace)
2 changes: 1 addition & 1 deletion src/node_dtrace.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ typedef struct {

namespace node {

void InitDTrace(Environment* env, v8::Local<v8::Object> target);
void InitDTrace(Environment* env);

} // namespace node

Expand Down
9 changes: 0 additions & 9 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -266,15 +266,6 @@ if (global.gc) {
knownGlobals.push(global.gc);
}

if (global.DTRACE_HTTP_SERVER_RESPONSE) {
knownGlobals.push(DTRACE_HTTP_SERVER_RESPONSE);
knownGlobals.push(DTRACE_HTTP_SERVER_REQUEST);
knownGlobals.push(DTRACE_HTTP_CLIENT_RESPONSE);
knownGlobals.push(DTRACE_HTTP_CLIENT_REQUEST);
knownGlobals.push(DTRACE_NET_STREAM_END);
knownGlobals.push(DTRACE_NET_SERVER_CONNECTION);
}

if (process.env.NODE_TEST_KNOWN_GLOBALS) {
const knownFromEnv = process.env.NODE_TEST_KNOWN_GLOBALS.split(',');
allowGlobals(...knownFromEnv);
Expand Down
10 changes: 0 additions & 10 deletions test/parallel/test-global.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,6 @@ builtinModules.forEach((moduleName) => {
'setInterval',
'setTimeout'
];
if (global.DTRACE_HTTP_SERVER_RESPONSE) {
expected.unshift(
'DTRACE_HTTP_SERVER_RESPONSE',
'DTRACE_HTTP_SERVER_REQUEST',
'DTRACE_HTTP_CLIENT_RESPONSE',
'DTRACE_HTTP_CLIENT_REQUEST',
'DTRACE_NET_STREAM_END',
'DTRACE_NET_SERVER_CONNECTION'
);
}
assert.deepStrictEqual(new Set(Object.keys(global)), new Set(expected));
}

Expand Down
24 changes: 24 additions & 0 deletions test/parallel/test-internal-dtrace.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Flags: --expose-internals
'use strict';

require('../common');
const assert = require('assert');

const {
DTRACE_HTTP_CLIENT_REQUEST,
DTRACE_HTTP_CLIENT_RESPONSE,
DTRACE_HTTP_SERVER_REQUEST,
DTRACE_HTTP_SERVER_RESPONSE,
DTRACE_NET_SERVER_CONNECTION,
DTRACE_NET_STREAM_END
} = require('internal/dtrace');

// We're just testing to make sure these are always defined and
// callable. We don't actually test their function here.

assert.strictEqual(typeof DTRACE_HTTP_CLIENT_REQUEST, 'function');
assert.strictEqual(typeof DTRACE_HTTP_CLIENT_RESPONSE, 'function');
assert.strictEqual(typeof DTRACE_HTTP_SERVER_REQUEST, 'function');
assert.strictEqual(typeof DTRACE_HTTP_SERVER_RESPONSE, 'function');
assert.strictEqual(typeof DTRACE_NET_SERVER_CONNECTION, 'function');
assert.strictEqual(typeof DTRACE_NET_STREAM_END, 'function');

0 comments on commit f9ddbb6

Please sign in to comment.