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

Use zero-based numeric indices for much faster instrumented code #22

Merged
merged 3 commits into from
Sep 8, 2016

Conversation

mourner
Copy link
Contributor

@mourner mourner commented Sep 2, 2016

Closes #21. Makes instrumented code run much faster by making V8 use “fast” mode when accessing stats objects, which wasn't triggered before because of non-0-based indices.

Here are the timings from mapbox-gl-js render suite:

  • no coverage: 1m 6s
  • nyc current: 4m 37s (or 3m 31s overhead)
  • nyc updated: 1m 20s (or 14s overhead)

So, basically the performance overhead is 15 times smaller, down from 320% to 20%.

  • update tests

cc @bcoe @mramato

Closes istanbuljs-archived-repos#21. Makes instrumented code run much faster by making V8 switch
to “fast” mode when accessing stats objects.
@yahoocla
Copy link

yahoocla commented Sep 2, 2016

CLA is valid!

@mourner mourner mentioned this pull request Sep 2, 2016
@mourner
Copy link
Contributor Author

mourner commented Sep 2, 2016

@bcoe Travis passes, so this ready to go.

@mourner
Copy link
Contributor Author

mourner commented Sep 7, 2016

@bcoe @gotwarlost anything I could help with to get this merged? A 15x speedup seems like something that's worth prioritizing. :)

@aaronabramov
Copy link

this is amazing! ❤️

@bcoe
Copy link
Contributor

bcoe commented Sep 8, 2016

@DmitriiAbramov @mourner I see no reason not to land this; but since I haven't heard back from @gotwarlost, I will be testing quite throughly; I'll start by publishing this to a next tag.

@bcoe bcoe merged commit 5b401f5 into istanbuljs-archived-repos:master Sep 8, 2016
@bcoe
Copy link
Contributor

bcoe commented Sep 8, 2016

@mourner I've published these changes to the next tag on npm, if you'd like to help test a bit:

npm cache clear; npm i istanbul-lib-instrument@next

@cpojer
Copy link

cpojer commented Sep 8, 2016

Oh this is awesome! Is anyone interested in trying this out on https://github.com/facebook/jest itself? :) (run jest with --coverage).

@mramato
Copy link

mramato commented Sep 8, 2016

Thanks @mourner, I would love to try this out to confirm my issue in gotwarlost/istanbul#556 is gone. Unfortunately I currently rely on https://github.com/karma-runner/karma-coverage which hasn't updated to this library yet (but I'm hoping it happens soon or I can open a PR to do it for them).

@aaronabramov
Copy link

@cpojer i tested it on jest itself, but we don't really instrument much code, so there isn't much we can optimize :)
although it seems to run 1 second faster, 28s on average instead of 29s

@mourner mourner deleted the much-faster branch September 8, 2016 08:01
@mourner
Copy link
Contributor Author

mourner commented Sep 8, 2016

@bcoe thanks! The new version works perfectly in our project:

@gotwarlost
Copy link
Contributor

So sorry about my lack of response (this is getting old :( )

I see no reason off the top of my head why this should not be merged. 👍

@gotwarlost
Copy link
Contributor

ah, it is merged already. Great.

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.

None yet

7 participants