Skip to content

Add a profile script #475

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

Closed
wants to merge 1 commit into from
Closed

Conversation

jamestalmage
Copy link
Contributor

I've been working on getting iron-node working with AVA so we can do some proper profiling. I think I finally cracked it.

The first thing was to run in a single process (this means only running a single test file), and fake the response of a parent process. It logs test results to the Chrome DevTools console (and it's not very pretty), but we shouldn't really care about the test results much while profiling.

The second was working around some weirdness with setTimeout in iron-node. I'm not sure if it's an iron-node problem, or an electron one, but storing setTimeout the way we do in globals.js breaks things. The solution here was to just undo our safe copies of setTimeout and clearTimeout. This means we can't profile against anything that manipulates timers (not sure why that would ever be necessary).

var originalConsole = console;
var consoleLog = originalConsole.log;

// Our usage of globals to store setTimeout/clearTimeout upsets iron-node for some reason.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you open an issue on iron-node about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@sindresorhus
Copy link
Member

Can you quickly mention how to use this in maintaining.md?

@sindresorhus
Copy link
Member

Can you add a .iron-node.js config file? With these to true: https://github.com/s-a/iron-node/blob/85198516e90bb9e03d45efe0bd361ad4a4fe467e/.iron-node.js#L11-L12

@sindresorhus
Copy link
Member

Tried it now. What's the workflow for profiling? The tests run on startup and finish before I manage to do anything. Is the recommended practise to put a debugger; statement at the top of the test file?

@jamestalmage
Copy link
Contributor Author

Sorry @sindresorhus - for some reason I never got notifications on any of these.

What's the workflow for profiling?

Click record then reload the page (that is what I did). I did not put any debugger statements in. That's an interesting thought though.

@sindresorhus sindresorhus changed the title Add a profile script. Add a profile script Jan 29, 2016
sindresorhus added a commit that referenced this pull request Jan 29, 2016
@sindresorhus
Copy link
Member

Awesome. Excited to have an easy way to perf test AVA.

@s-a
Copy link

s-a commented Jan 29, 2016

I find this very intersting! Are you willing to share your workflow to profile your software in something like markdown documentation? I like to link from my repo to share your work with the community. Just want to say... keep in mind that results might differ from native node but it will in fact deliver insightful results.

@sindresorhus
Copy link
Member

@s-a I added a short section in https://github.com/sindresorhus/ava/blob/master/maintaining.md#profiling. Was hoping @jamestalmage would fill it out with his workflow.

@s-a
Copy link

s-a commented Jan 29, 2016

👍 I can not wait to read it . :) @jamestalmage Please ping me here when you finished.

@mattdesl
Copy link

I'm also interested in the progress of this thread and how we can combine efforts with devtool to provide better debugging/profiling all around.

e.g. The setTimeout issue was resolved in a recent commit to devtool, since now we are using true Node.js timers instead of the built-in browser timers.
s-a/iron-node#85

Relevant commit:
Experience-Monks/devtool@11e491c

@jamestalmage jamestalmage deleted the profile-script branch January 29, 2016 21:55
@sindresorhus
Copy link
Member

Relevant: https://github.com/Jam3/devtool#iron-node

@mattdesl Is there anything devtools is worse at than iron-node for our needs (profiling CPU usage)?

@jamestalmage
Copy link
Contributor Author

I just played with devtool, it seems great, and it already works with profile.js.

devtool node_modules/ava/profile.js test/someTest.js

@mattdesl @s-a, with either project, is there a way to tell Chrome DevTools to start recording a CPU profile programmatically? It would be awesome not to have to start the recording and then reload the page.

@sindresorhus
Copy link
Member

@jamestalmage console.profile('foo') and console.profileEnd('foo').

@mattdesl
Copy link

I think the differences will be subtle between the two tools, though my end goal is to have devtool support most Node.js use cases with minimal effort (which involves lots of tweaks for require.main, process, and all that).

You can use console.profile("foo") and console.profileEnd("foo"). So you could programmatically add profiles for files or whatever.

I am currently writing a blog post on all things devtool, you can see it here:
http://mattdesl.svbtle.com/debugging-nodejs-in-chrome-devtools

@jamestalmage
Copy link
Contributor Author

I am currently writing a blog post on all things devtool

Awesome - great writeup - I just added it to #489

console.profile('foo')

@mattdesl @s-a Feedback on #491 would be appreciated.

@s-a
Copy link

s-a commented Jan 30, 2016

@jamestalmage sorry #491 is out of my scope. I feel this could have something to do with

e.g. The setTimeout issue was resolved in a recent commit to devtool, since now we are using true Node.js timers instead of the built-in browser timers.

As matt metioned in his README.md devTool is in an experimental state.

Note: This tool is still in early stages. So far it has only been tested on a couple of OSX machines. :)

However FYI just added https://github.com/s-a/iron-node/blob/master/docs/PROFILE.md. There is a lot more work todo. :) Nice weekend @ALL 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants