Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: bundle tick processor with iojs #158

Closed
bnoordhuis opened this issue Dec 13, 2014 · 28 comments
Closed

RFC: bundle tick processor with iojs #158

bnoordhuis opened this issue Dec 13, 2014 · 28 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@bnoordhuis
Copy link
Member

See bnoordhuis/node-profiler#11 for background. A recurring issue with CPU profiling with --prof is that you either need to build d8 (the V8 shell) or install a tick processor package from npm.

The problem with building d8 is that it's quite tedious and frequently broken due to the way V8 is bundled. Installing a tick processor is no picnic either; if it doesn't match the V8 version, it produces bogus output. That isn't currently solvable from user land because the output from the profiler doesn't contain version information and heuristics aren't reliable enough. I'm going to prepare a patch that adds version information but that is a tangential issue. (Update: https://codereview.chromium.org/800293002/)

For ease of use, I propose bundling the tick processor with iojs, either as a standalone JS script (working name: ioprof) or by baking it into the binary. The former would be easiest short-term, the latter offers an arguably nicer user experience. I volunteer to do the work.

Design decisions:

  • Add a --without-ioprof to the configure script, similar to the --without-npm flag? Assertion without hard data to back it up: no one uses --without-npm; --without-ioprof is probably not worth the hassle.
  • What about distro packagers that build against an external V8? We would normally copy the tick processor files from deps/v8/tools but that won't work when compiling against headers only. The best I can come up with for now is to disable the profiler script when --shared-v8 is specified. Maybe @jbergstroem can chime in here.
@sidorares
Copy link

would be good if bundled processor is able to consume version different from processor's associated distribution. (Someone email me v8.log, or node version on server that produced log is not the version I have on dev machine) - via command line switch ( for example, --log-version ), auto-detect, or using version tag in log file once we start to put it

another requirement (that's what processor shipped with v8 doesn't do) - don't buffer entire log file and process it as a stream. few minutes of profiling can easily generate gigabytes of v8.log

@jbergstroem
Copy link
Member

@bnoordhuis I don't think there's any distros left that aims to decouple V8 (node/io.js would just be one example package). It's neigh impossible since every binary linking to it usually expects a different version. Since V8 might not be the best example of a consistent ABI, the effort to keep these versions apart is pretty painful. What I've seen done though is building it separately from the ordinary build in order to manage the environment (PaX-marking, specific stuff for arm, etc). I'll go through a few package managers and see if I can find any that still decouples it.

As for the npm analogy, I'd say that the primary use for --without-npm is not about avoiding to use it, rather having better control of the npm installation. I agree with --without-ioprof being pointless.

@bnoordhuis
Copy link
Member Author

@sidorares

would be good if bundled processor is able to consume version different from processor's associated distribution. (Someone email me v8.log, or node version on server that produced log is not the version I have on dev machine) - via command line switch ( for example, --log-version ), auto-detect, or using version tag in log file once we start to put it

That doesn't really work and probably never will, at least not for C/C++ symbols. A lesser issue is that every tick processor version adds about 60 kB, unless you get creative with compression; it's not much but it adds up.

It would make for a nice feature once those issues have been worked out but I think it's out of scope for now.

@jbergstroem

I don't think there's any distros left that aims to decouple V8

I think Fedora still does; its v8 package is shared between nodejs, mongodb and a few more.

@jbergstroem
Copy link
Member

@bnoordhuis You're right - didn't know that. Seems like they try to pinpoint minor versions. I think their mongodb package illustrates the issues rather well though; they require 3.14+ while upstream recommends either 3.12.x or 3.25.x. It's really tricky and the test suites for packages (node, mongodb, rethinkdb, etc) wouldn't be reliable enough to verify that whatever version [of v8] you have installed is working as intended. Building and using v8 as a shared library is a very fragile situation. When I used to maintain it, build errors were the "best" way of knowing if you had a problem.

@bajtos
Copy link
Contributor

bajtos commented Dec 15, 2014

+1

@bnoordhuis @rvagg perhaps this issue can be discussed in your next TC meeting on Dec 17th?

@chrisdickinson
Copy link
Contributor

I'm +1 on this & adding a section to the docs explaining how (and when) to use the profiler.

Something to keep in mind re: --without-ioprof -- some package distributions (EPEL comes to mind) rely on being able to split out the dependencies of node into separate packages. I imagine they'd be grateful if we gave them the tools to continue doing that. This is a bit of a foggy concern since I'm not entirely sure what would go into baking ioprof into the executable would involve.

@rvagg
Copy link
Member

rvagg commented Dec 17, 2014

Agreement from TC in today's meeting that this is fine to add. The only cause for concern was in setting a precedent for adding minor functionality like this but it was agreed that this is a special case.

@rvagg
Copy link
Member

rvagg commented Dec 17, 2014

Sorry, TC minutes are in #178

@rvagg rvagg removed the tc-agenda label Dec 26, 2014
@AndreasMadsen
Copy link
Member

Thanks for doing this. This have always been a major problem when teaching people to solve performance problems.

@Fishrock123
Copy link
Contributor

@bnoordhuis is this still something that is happening?

@bnoordhuis
Copy link
Member Author

Yes, once I get the quirks (they're not really bugs) ironed out.

@sam-github
Copy link
Contributor

+1 on this. the tick processor is intimately tied to internal details of node's v8, so makes an excellent candidate for distribution with node, and I have had poor luck with the the npmjs.org module that is supposed to build it.

@andre487
Copy link

+1 for tick processor

@Olegas
Copy link
Contributor

Olegas commented Mar 22, 2015

@bnoordhuis For now, what is the right way to profile io.js apps?

@sidorares
Copy link

@Olegas I'm currently trying to make https://github.com/sidorares/node-tick work with as much node versions as possible ( iojs, node 0.12 and 0.10.37 now include v8 version in the log so should be possible to do without guessing )

@andre487
Copy link

@sidorares But is node-tick's output correct? When I try it, it talks that almost all the time the program spent in C++ code of system libraries, but v8-profiler talks different.

// Sorry for my English =)

@sidorares
Copy link

@Andre-487 which node/io version you tried? I'll double check. It might be incorrect because currently there is only one version of v8.log parser and format is different between v8 version. What I'm working on is that tick processor reads v8 version from the log and then knows hoe to deserialise the rest

@bnoordhuis
Copy link
Member Author

@Andre-487 v8-profiler doesn't profile C++ code. Or rather, it does but it dumps it all in one big bucket named (program) (IIRC.)

@sidorares
Copy link

@bnoordhuis iwith debug symbols and nm installed it gives you C++ profile as well ( or used to, need to check ) - https://gist.github.com/sidorares/9521922#file-with-nm-txt-L12

@bnoordhuis
Copy link
Member Author

@sidorares I mean v8-profiler, not node-tick.

@sidorares
Copy link

@bnoordhuis I thought it's based on the same V8 scripts as node-tick

@bnoordhuis
Copy link
Member Author

To be clear, I'm talking about this module: https://github.com/node-inspector/v8-profiler :-)

It uses the C++ API to programmatically start and stop the profiler. The returned profile does not have detailed information for C++ stack frames like --prof + linux-tick-processor + nm has.

@Olegas
Copy link
Contributor

Olegas commented Mar 23, 2015

@sidorares Is there any working way to analyze v8.log visually (i.e. with about://tracing or via exporting to .cpuprofile)

@sidorares
Copy link

@andre487
Copy link

Looks like I was wrong.

I try to profile this script with Node 0.10.37 on the Max OS X 10.10: node --prof sorting.js. node-tick shows this result: http://pastebin.com/8Xmva4xz Looks like correct profile.

But on the 0.12.0 it shows this: http://pastebin.com/JVZrQDsY There is no 80% time in system libraries in profile: http://pastebin.com/ZSXekRpg, but 11% in node is questionable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests