Adds profiling instrumentation on functions level. #158

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@ralphv
ralphv commented Feb 20, 2014

This fork adds a new command "profile".

It works very similar to instrument command but instead of injecting coverage code, it injects profiling code to functions.

Each function is wrapped with
try {
profile.start(...);
}
finally {
profile.end(...);
}

The produced code needs to have "profile.js" file added to it to run correctly as all the files will reference it.
A sample profile.js is provided.

Running the produced code gives you profiling statistics on the functions level.

Ralph Varjab... added some commits Feb 20, 2014
@gotwarlost
Owner

I will not merge this code into istanbul. istanbul has one job that it needs to do well, which is to provide code coverage. Profiling belongs elsewhere.

That said, I'm happy to work with you in extracting the useful bits of istanbul into an istanbul-core library that you can use to implement profiling in any way you want.

Otherwise I will become "it" by default to answer questions around code that I know little about.

@gotwarlost gotwarlost closed this Feb 20, 2014
@ralphv
ralphv commented Feb 21, 2014

Hi Thanks for replying to this soon.

The code I changed is actually very small, I am not sure what happened with the code comparison on the site, the lines of code changed is wrong, perhaps different line spacing and such...

To change the instrumentation behavior, I needed two things only, 1. changing the header injection (getPreamble in instrumenter.js) and 2. modifying the "map" object that you pass to the instrument constructor. (the huge structure that maps node types to functions, coverFunction, coverStatment and so on...)

So I modified the instrumenter.js in a very simple way, to override those two items if they are passed in the options given to the constructor. Simple and neat. Then I added a new command "profile.js" that was copy/paste from instrument but the command changed and the two different structures passed along in the options object to the instrumenter.js

Would you consider adding a "generic" instrument mode that it's behavior can be overriden by the end-user? or alternatively can you explain the istanbul-core idea more, I am interested in one way or another to see this "functionality" make it into istanbul. To easily get profiling code is very essential to nodejs and I can't find any easy solution to do it, there are other profiling that do that on high level using v8.log and such, but nothing is simple enough to instrument the code with function level statistics...

And just for the record, I am more than willing to answer and write full documentation about how to use the "profile" command if you still consider merging it.

Please let me know your thoughts.
Thank you again and good work on an amazing tool.

@gotwarlost
Owner

Have you considered that profiling is a hard problem when async functions are concerned?

Consider this example:

function foo(callback) {
   return reallyLongRunningFunction(callback);
}

foo returns in an instant but really it may be taking forever from an async viewpoint.

I'm super busy at work now so it may be a week or two before I can get to this.

@ralphv
ralphv commented Feb 21, 2014

That's where it gets ugly :)

As long as the last argument is a function, I am considering that as a callback function and I am overriding the cb inside my profile.start with another function that eventually calls the original one.

It is not perfect because it changes the signature of the callback function and if you have code that depends on that, unfortunately it will fail. I haven't found an easy workaround for that yet. To override the cb without changing the signature... But for the most part it works...

Perhaps claiming a "profile" command has pitfalls and you are right about giving a proper solution, in the coverage there are no pitfalls.

However you could consider having istanbul's "instrument" customizable by the end user, or as you suggested creating a istanbul-core... We need two things to customize the Instrument class, the getPreamble and the map object...

Thanks for your feedback.

@ralphv
ralphv commented Feb 21, 2014

I found another couple of cases that won't work, for example, if the last parameter is indeed a function, but it is not intended as callback, you are just passing it around as a variable...

I am starting to feel to get a profiling solution without some rigid conventions on parameter names will not yield good results... (i.e. the callback must be the last parameter and it must be always named, say cb)

This may be a bust after all for a tool. For my code, it is working like a charm, but I don't think it can be generalized easily into a tool... Thus won't be easy to create a tool out of the box...

@gotwarlost
Owner

A few things to consider to workaround these issues:

  • Allow a configurable set of names that should be treated as callbacks when passed around based on the user's coding convention (e.g. callback, cb, next etc.)
  • Try and match the arity of the function (Function.prototype.length ) when generating code to override the callback, so if code is relying on the callback's arity it will see the same thing
  • Allow comments in code to explicitly suppress callback profiling and so on...

Then again, I'm sure there are tons of things I'm not thinking about... :)

@ralphv
ralphv commented Feb 21, 2014

I like the first idea the configurable set of names..., instead of just one.

That's enough most of the time, but I have some code layer that uses dependency injection (similar to Angular.JS) but on the server side (node), so it went crazy when my profiled code was overriding call back functions, of course since it's my code, I fixed it quick.

It is possible here to discover the callback function parameters at runtime (.toString and some regex) and then using eval to create the new override callback function with the exact parameter count and names.

The thing is, I was hoping to get something up and running with the ease of instabul instrument, but I realize for profiling this will never be the case... and It won't be as straight forward as the instrument command...

It needs to have some good documentation and it needs some naming conventions, and the developer must adhere to the conventions from the start, or else he will have to go over and modify the code... so the tool can't be fool proof, now I understand why I can't find such a tool :)

But having said that, it is possible with some effort and documentation to get things running smoothly as I have with my code and It is definitely helpful to get all those statistics on each function (number of calls/total time spent in each function). I am not sure though how much I want to pursue this on just personal effort... It would be nice to put something together for the whole community to use though...

Cheers.

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