This repository has been archived by the owner. It is now read-only.

trace: remove macro usage for probe points #5940

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
@tjfontaine

Unconditionally compile src/node_dtrace.cc but only export the
functions the configuration supports. Then lib/_trace.js replaces any
missing probe points with a nop function.

This is a companion commit to #5938

@trevnorris

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Jul 29, 2013

../src/node_dtrace.cc:177:7: warning: unused variable 'nbytes' [-Wunused-variable]
  int nbytes = args[1]->Int32Value();
      ^
../src/node_dtrace.cc:194:7: warning: unused variable 'nbytes' [-Wunused-variable]
  int nbytes = args[1]->Int32Value();
      ^
../src/node_dtrace.cc:297:12: warning: unused function 'dtrace_gc_start' [-Wunused-function]
static int dtrace_gc_start(GCType type, GCCallbackFlags flags) {
           ^
../src/node_dtrace.cc:307:12: warning: unused function 'dtrace_gc_done' [-Wunused-function]

please fix those. I don't want warnings in my build.

../src/node_dtrace.cc:177:7: warning: unused variable 'nbytes' [-Wunused-variable]
  int nbytes = args[1]->Int32Value();
      ^
../src/node_dtrace.cc:194:7: warning: unused variable 'nbytes' [-Wunused-variable]
  int nbytes = args[1]->Int32Value();
      ^
../src/node_dtrace.cc:297:12: warning: unused function 'dtrace_gc_start' [-Wunused-function]
static int dtrace_gc_start(GCType type, GCCallbackFlags flags) {
           ^
../src/node_dtrace.cc:307:12: warning: unused function 'dtrace_gc_done' [-Wunused-function]

please fix those. I don't want warnings in my build.

@trevnorris

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Jul 29, 2013

Here's the extra instruction output from IRHydra calling nop in onconnection in lib/net.js:

t177 LoadContextSlot t6[12]
v178 EnvironmentMarker lookup var[6]
t179 CheckHeapObject t177
t180 CheckMaps t177 [0x119c616207c9]
t181 PushArgument t177
t182 PushArgument t148
t183 CallConstantFunction 0x15c6560aafc1 <String[3]: nop> #2 changes[*]
v184 Simulate id=194
t185 LoadContextSlot t6[12]
v186 EnvironmentMarker lookup var[6]
t187 CheckHeapObject t185
t188 CheckMaps t185 [0x119c616207c9]
t189 PushArgument t185
t190 PushArgument t148
t191 CallConstantFunction 0x15c6560aafc1 <String[3]: nop> #2 changes[*]
v192 Simulate id=206

Here's the extra instruction output from IRHydra calling nop in onconnection in lib/net.js:

t177 LoadContextSlot t6[12]
v178 EnvironmentMarker lookup var[6]
t179 CheckHeapObject t177
t180 CheckMaps t177 [0x119c616207c9]
t181 PushArgument t177
t182 PushArgument t148
t183 CallConstantFunction 0x15c6560aafc1 <String[3]: nop> #2 changes[*]
v184 Simulate id=194
t185 LoadContextSlot t6[12]
v186 EnvironmentMarker lookup var[6]
t187 CheckHeapObject t185
t188 CheckMaps t185 [0x119c616207c9]
t189 PushArgument t185
t190 PushArgument t148
t191 CallConstantFunction 0x15c6560aafc1 <String[3]: nop> #2 changes[*]
v192 Simulate id=206
@tjfontaine

This comment has been minimized.

Show comment Hide comment
@tjfontaine

tjfontaine Jul 29, 2013

@trevnorris if you strongly about it, debug() has way more touch than the trace probe points

@trevnorris if you strongly about it, debug() has way more touch than the trace probe points

@trevnorris

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Jul 29, 2013

@tjfontaine yeah, realize that. and I'm still looking for a more elegant solution around that.

@tjfontaine yeah, realize that. and I'm still looking for a more elegant solution around that.

lib/tracing.js
+
+ Object.keys(m[probeKey]).forEach(function(funcKey) {
+ callbacks.push(m[probeKey][funcKey]);
+ });

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Jan 23, 2014

The cost of running this when a probe is added will be much. Regardless of whether we think v8 should optimize for things like this (as isaac does), it doesn't. So this type of functional programming will kill performance.

@trevnorris

trevnorris Jan 23, 2014

The cost of running this when a probe is added will be much. Regardless of whether we think v8 should optimize for things like this (as isaac does), it doesn't. So this type of functional programming will kill performance.

This comment has been minimized.

Show comment Hide comment
@tjfontaine

tjfontaine Jan 23, 2014

I wrote it this way for ease not for performance, it was meant completely to do proof of concept, there are tons of other places where hot code will need to be considered.

I'm not advocating that we ship known poorly performing code, but I vote wholeheartedly on being able to ship a working feature and iterate on performance after we branch

@tjfontaine

tjfontaine Jan 23, 2014

I wrote it this way for ease not for performance, it was meant completely to do proof of concept, there are tons of other places where hot code will need to be considered.

I'm not advocating that we ship known poorly performing code, but I vote wholeheartedly on being able to ship a working feature and iterate on performance after we branch

lib/tracing.js
+function getListeners(namespace, module, probe) {
+ var callbacks = [];
+
+ Object.keys(listenerCache).forEach(function(namespaceKey) {

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

Since this method is run on every .fire() this should be changed as outlined in the slides here: http://trevnorris.github.io/NodeDay/#/14

If you want to merge this and have me do a performance tuning commit afterwards, that's also fine by me.

@trevnorris

trevnorris Mar 4, 2014

Since this method is run on every .fire() this should be changed as outlined in the slides here: http://trevnorris.github.io/NodeDay/#/14

If you want to merge this and have me do a performance tuning commit afterwards, that's also fine by me.

This comment has been minimized.

Show comment Hide comment
@tjfontaine

tjfontaine Mar 4, 2014

I was going for correct first, it would be great if you came back and made it scream :)

@tjfontaine

tjfontaine Mar 4, 2014

I was going for correct first, it would be great if you came back and made it scream :)

lib/tracing.js
+};
+
+Provider.prototype.addProbe = function(name, signature/*[, arg2, arg3]*/) {
+ var probe = this._getProbe(name);

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

Verify typeof name === 'string'?

@trevnorris

trevnorris Mar 4, 2014

Verify typeof name === 'string'?

lib/tracing.js
+Provider.prototype.removeProbe = function(name) {
+ this._throwDisabled('remove', name);
+ delete this.probes[name];
+};

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

Just want to double check with you that {add,remove}Probe are not going to be fired very often (in the most likely scenario)?

@trevnorris

trevnorris Mar 4, 2014

Just want to double check with you that {add,remove}Probe are not going to be fired very often (in the most likely scenario)?

This comment has been minimized.

Show comment Hide comment
@tjfontaine

tjfontaine Mar 4, 2014

No, but I will add documentation to that effect, doing so will be very expensive from ETW and usdt perspective.

@tjfontaine

tjfontaine Mar 4, 2014

No, but I will add documentation to that effect, doing so will be very expensive from ETW and usdt perspective.

lib/tracing.js
+util.inherits(StaticProvider, Provider);
+
+StaticProvider.prototype.addProbe = function(name, signature, binding) {
+ var probe = Provider.prototype.addProbe.call(this, name, signature);

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

We do this a lot in the Streams API, and I've never been a fan of it. Instead I prefer an abstracted approach like the following:

function addProbe(self, name, signature) {
  // do its thing...
}

StaticProvider.prototype.addProbe = function staticAddProbe(name, signature, binding) {
  var probe = addProbe(this, name, signature);
  // ...
};

Provider.prototype.addProbe = function providerAddProbe(name, signature, arg2, arg3) {
  return addProbe(name, signature, arg2, arg3);
};

Or even just change the location of the function call. Because, heaven forbid, someone overwrites Provider#addProbe() there's going to be an implicit change in StaticProvider#addProbe(). So sort of like the following:

function addProbe(name, signature/*, arg2, arg3 */) {
  // do its thing...
}

StaticProvider.prototype.addProbe = function staticAddProbe(name, signature, binding) {
  var probe = addProbe.call(this, name, signature);
  // ...
};

Provider.prototype.addProbe = function providerAddProbe() {
  return addProbe.apply(this, arguments);
};

@mraleph gave a talk about how V8 is smart enough to see that the arguments object is passed directly to the .apply() and so it never has to be materialized in the initial call. So, to be honest, there would be little if any performance imact.

@trevnorris

trevnorris Mar 4, 2014

We do this a lot in the Streams API, and I've never been a fan of it. Instead I prefer an abstracted approach like the following:

function addProbe(self, name, signature) {
  // do its thing...
}

StaticProvider.prototype.addProbe = function staticAddProbe(name, signature, binding) {
  var probe = addProbe(this, name, signature);
  // ...
};

Provider.prototype.addProbe = function providerAddProbe(name, signature, arg2, arg3) {
  return addProbe(name, signature, arg2, arg3);
};

Or even just change the location of the function call. Because, heaven forbid, someone overwrites Provider#addProbe() there's going to be an implicit change in StaticProvider#addProbe(). So sort of like the following:

function addProbe(name, signature/*, arg2, arg3 */) {
  // do its thing...
}

StaticProvider.prototype.addProbe = function staticAddProbe(name, signature, binding) {
  var probe = addProbe.call(this, name, signature);
  // ...
};

Provider.prototype.addProbe = function providerAddProbe() {
  return addProbe.apply(this, arguments);
};

@mraleph gave a talk about how V8 is smart enough to see that the arguments object is passed directly to the .apply() and so it never has to be materialized in the initial call. So, to be honest, there would be little if any performance imact.

lib/tracing.js
+};
+
+StaticProvider.prototype.enable = function() {
+ this.enabled = true;

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

Just a thought. If you've created a function to enable the parameter then the property should be "private". In other words, start with an _.

@trevnorris

trevnorris Mar 4, 2014

Just a thought. If you've created a function to enable the parameter then the property should be "private". In other words, start with an _.

lib/tracing.js
+ mappedSignature[i] = mappedType;
+ }
+
+ this.dprobe = new dtraceProbe(name, mappedSignature);

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

So dtraceProbe needs to be instantiated, and there's also DTraceProbe. Convention is that anything that needs instantiation should upper camel case. I think the name for dtraceProbe should be changed so that it can accommodate this convention.

@trevnorris

trevnorris Mar 4, 2014

So dtraceProbe needs to be instantiated, and there's also DTraceProbe. Convention is that anything that needs instantiation should upper camel case. I think the name for dtraceProbe should be changed so that it can accommodate this convention.

lib/tracing.js
+function DTraceProvider(options) {
+ Provider.call(this, options);
+
+ this._handle = new platformProvider(options.namespace, options.name);

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

Same thing with upper camel case.

@trevnorris

trevnorris Mar 4, 2014

Same thing with upper camel case.

lib/tracing.js
+};
+
+DTraceProvider.prototype.removeProbe = function(name) {
+ var probe = this.probes[name];

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

There's no check to see if this.probes[name] exists to begin with.

@trevnorris

trevnorris Mar 4, 2014

There's no check to see if this.probes[name] exists to begin with.

lib/tracing.js
+
+ delete providers[provider.name];
+
+ providers[provider.name] = provider;

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

I'm confused on the need to delete just before reassigning the value.

@trevnorris

trevnorris Mar 4, 2014

I'm confused on the need to delete just before reassigning the value.

src/dtrace-provider/dtrace_argument.cc
+
+ void * DTraceStringArgument::ArgumentValue(Handle<Value> value) {
+ if (value->IsUndefined())
+ return (void *) strdup("undefined");

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

I'm just going to put a general "fix styles" comment for this file. Again, whether it passes cpplint or not doesn't change my mind on how some of these styles should be (e.g. always use C++ style casts in C++).

@trevnorris

trevnorris Mar 4, 2014

I'm just going to put a general "fix styles" comment for this file. Again, whether it passes cpplint or not doesn't change my mind on how some of these styles should be (e.g. always use C++ style casts in C++).

src/dtrace-provider/dtrace_argument.cc
+ }
+
+ const char * DTraceStringArgument::Type() {
+ return "char *";

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

Could this be changed to const char* DTraceStringArgument::Type() const { ?

@trevnorris

trevnorris Mar 4, 2014

Could this be changed to const char* DTraceStringArgument::Type() const { ?

src/dtrace-provider/dtrace_argument.cc
+
+
+ void * DTraceJsonArgument::ArgumentValue(Handle<Value> value) {
+ HandleScope scope(env()->isolate());

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

Drop this HandleScope below the first if(). No need to create it until after that point.

@trevnorris

trevnorris Mar 4, 2014

Drop this HandleScope below the first if(). No need to create it until after that point.

src/node_dtrace.cc
+#define CONSTANT(type) \
+ do { \
+ Local<String> str = OneByteString(env->isolate(), #type); \
+ constants->Set(str, v8::Integer::New(ARGUMENT_TYPE_ ## type, env->isolate())); \

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

Maybe it's from the V8 in 3.24 upgrade, but Isolate* is the first argument for v8::Integer::New().

@trevnorris

trevnorris Mar 4, 2014

Maybe it's from the V8 in 3.24 upgrade, but Isolate* is the first argument for v8::Integer::New().

This comment has been minimized.

Show comment Hide comment
@tjfontaine

tjfontaine Mar 4, 2014

yes that's not until 3.24, they're still inconsistent in 3.22

@tjfontaine

tjfontaine Mar 4, 2014

yes that's not until 3.24, they're still inconsistent in 3.22

@trevnorris

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Mar 4, 2014

@tjfontaine That's a lot to digest. I'm looking forward to the docs so I can play with it a bit. :)

@tjfontaine That's a lot to digest. I'm looking forward to the docs so I can play with it a bit. :)

tjfontaine added some commits Jul 29, 2013

src: revamp tracing framework firing
Always build node_dtrace, but only decorate the global if on a platform
that supports it.

Revamp firing mechanism to do more work in the JIT and only do array
access in the native layer.
@jasnell

This comment has been minimized.

Show comment Hide comment
@jasnell

jasnell Aug 3, 2015

Owner

Closing due to lack of activity. Can revisit if someone is interested in updating the PR. (this may still be a good idea, just hard to determine and given that there's been zero activity....)

Owner

jasnell commented Aug 3, 2015

Closing due to lack of activity. Can revisit if someone is interested in updating the PR. (this may still be a good idea, just hard to determine and given that there's been zero activity....)

@jasnell jasnell closed this Aug 3, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.