Skip to content

Shorten the plan stack. #482

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

Merged
merged 1 commit into from
Jan 29, 2016
Merged

Conversation

jamestalmage
Copy link
Contributor

Every call to t.plan generates a stack trace. Apparently those stack traces are expensive. This speeds things up by shortening the stack trace.

I am not sure we need more than a depth of 1.

benchmark:

import test from 'ava';

for (var i = 0; i < 10000; i++) {
    test(`test${i}`, t => {
        t.plan(1);
        t.pass();
    });
}

master:

node cli.js test/lots-of-plans.js --verbose  6.16s user 0.58s system 119% cpu 5.647 total

depth 1 (current value in this PR):

node cli.js test/lots-of-plans.js --verbose  2.43s user 0.48s system 142% cpu 2.044 total

depth of 2:

node cli.js test/lots-of-plans.js --verbose  2.66s user 0.42s system 128% cpu 2.392 total

depth of 3:

node cli.js test/lots-of-plans.js --verbose  3.04s user 0.49s system 133% cpu 2.648 total

depth of 4:

node cli.js test/lots-of-plans.js --verbose  3.24s user 0.50s system 131% cpu 2.850 total

@@ -303,6 +302,17 @@ PublicApi.prototype = enhanceAssert({
onError: onAssertionEvent
});

PublicApi.prototype.plan = plan;

function plan(ct) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just assign it directly to PublicApi.prototype.plan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because plan needs to be passed to Error.captureStackTrace

Copy link
Member

Choose a reason for hiding this comment

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

PublicApi.prototype.plan = function plan(ct) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - that works.

@jamestalmage
Copy link
Contributor Author

@sindresorhus Any thoughts on depth? I really think 1 just works.

@sindresorhus
Copy link
Member

Yeah, doesn't make sense to have deeper stack trace for this as you only care about where the plan was defined.

For performance improvements.
@jamestalmage
Copy link
Contributor Author

Changes made.

sindresorhus added a commit that referenced this pull request Jan 29, 2016
@sindresorhus sindresorhus merged commit 14a21b0 into avajs:master Jan 29, 2016
@sindresorhus
Copy link
Member

👍

@jamestalmage jamestalmage deleted the shorten-plan-stack branch April 6, 2016 22:47
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.

2 participants