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

Make istanbul blazing fast #694

Closed
wants to merge 3 commits into from

Conversation

mourner
Copy link

@mourner mourner commented Sep 2, 2016

A work in progress, closes #556.

I finally figured out why an istanbul cover run in our project took 4-5 times as much as the non-istanbul test run! For some reason, V8 has a very hard time accessing string-keyed object properties (cov.s['0']++ etc.) on every line. Changing coverage stats to use simple arrays instead of objects and do cov.s[0]++ gets rid of all the huge overhead.

For our project, this change brings a 4-minute coverage run down to 1 minute, almost the same time as a clean test run takes. Awesome!

  • convert *Map objects to arrays as well
  • fix stats iterations in object-utils.js
  • update tests to reflect coverage JSON structure change
  • perhaps explore using a shorter id for coverage (e.g. __cov_FFp_xOkOtMOF6AXOIHanfg -> _c, first var that's not used in any of the scopes in a file)? Should make things faster because of V8 inlining.

cc @gotwarlost @mramato @jfirebaugh @springmeyer @lucaswoj

Closes gotwarlost#556. Makes coverage run much faster, especially on bigger code
bases.
@mourner
Copy link
Author

mourner commented Sep 2, 2016

Damn, I just noticed that Istanbul is being fully rewritten in https://github.com/istanbuljs. I'll check out how it performs and see if I can port the changes there if necessary cc @bcoe.

@bcoe
Copy link
Contributor

bcoe commented Sep 2, 2016

@mourner definitely interested in any performance changes to istanbul-lib-instrument that we can make.

@mourner
Copy link
Author

mourner commented Sep 2, 2016

@bcoe great! I'll submit a PR next week.

@mourner
Copy link
Author

mourner commented Sep 2, 2016

@mourner mourner closed this Sep 2, 2016
@bcoe
Copy link
Contributor

bcoe commented Sep 2, 2016

@mourner @gotwarlost this brings up a good point, how do you feel about adding a deprecation notice to the istanbul repo soonish, much like this:

https://www.npmjs.com/package/optimist#deprecation-notice

I think we've checked a lot of boxes in the migration towards [email protected].

@jdalton
Copy link

jdalton commented Sep 8, 2016

@mourner
In istanbuljs-archived-repos/istanbul-lib-instrument#22 you also added zero-based indexes.
Was it a combo of string index keys and non-zero based?

@mourner
Copy link
Author

mourner commented Sep 8, 2016

@jdalton I'm not sure whether string vs number keys make a difference in V8 as long as strings can be cleanly treated as integers. See istanbuljs-archived-repos/istanbul-lib-instrument#21 for more background — I found that objects with zero-based integer indices without gaps work as fast as arrays (and probably implemented the same way internally), so I choose the latter as the most non-intrusive optimization.

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.

Debugging coverage performance?
3 participants