Make istanbul blazing fast

gotwarlost / istanbul

Yet another JS code coverage tool that computes statement, line, function and branch coverage with module loader hooks to transparently add coverage when running tests. Supports all JS coverage use cases including unit tests, server side functional tests and browser tests. Built for scale.

Other

8.7k stars 786 forks source link

Make istanbul blazing fast #694

Closed mourner closed 8 years ago

mourner commented 8 years ago

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!

[x] convert *Map objects to arrays as well
[x] 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

mourner commented 8 years ago

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 commented 8 years ago

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

mourner commented 8 years ago

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

mourner commented 8 years ago

-> https://github.com/istanbuljs/istanbul-lib-instrument/pull/22

bcoe commented 8 years ago

@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 istanbul@1.0.

jdalton commented 8 years ago

@mourner In https://github.com/istanbuljs/istanbul-lib-instrument/pull/22 you also added zero-based indexes. Was it a combo of string index keys and non-zero based?

mourner commented 8 years ago

@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 https://github.com/istanbuljs/istanbul-lib-instrument/issues/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.