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

Performance Improvements for files with thousands of tests #461

Merged
merged 9 commits into from
Jan 21, 2016

Conversation

jamestalmage
Copy link
Contributor

Using the emoji-aware test-per-character branch as a benchmark, and running time ava:

master:

  4363 passed

ava  22.61s user 0.54s system 109% cpu 21.093 total

this branch (only the Runner commits):

  4363 passed

ava  7.30s user 0.51s system 139% cpu 5.597 total

this branch (all the commits):

  4363 passed

ava  6.32s user 0.46s system 145% cpu 4.675 total

updated benchmark (with empower-core performance tweaks):

ava --verbose  5.05s user 0.32s system 138% cpu 3.872 total

@jamestalmage
Copy link
Contributor Author

This relies on some changes to empower-core.

twada/empower-core#7

@jamestalmage
Copy link
Contributor Author

Further splitting that test suite into multiple files (beaugunderson/emoji-aware#1):

  4363 passed

ava --verbose  9.28s user 0.65s system 458% cpu 2.164 total

I think this is left over from when we handled assertion tracking differently
The check for this is in the constructor now. It's not possible for `this.fn` to be undefined
This was a pretty significant performance increase: 5.3 -> 4.0 seconds
@@ -72,6 +72,7 @@ test('fake timers do not break duration', function (t) {
});
});

/*
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change, destructuring would no longer work. This is not allowed anymore:

test({is, same} => {
  is(1 + 1, 2);
  same([1,2].slice(1), [2]);
});

You have to preserve the this reference:

test(t => {
  t.is(1 + 1, 2);
  t.same([1,2].slice(1), [2]);
});

@sindresorhus
Copy link
Member

👍 Looks good! (ᵔᴥᵔ)

Too bad jamestalmage@4e23943 was slow. It was more readable.

@jamestalmage
Copy link
Contributor Author

Too bad jamestalmage/ava@4e23943 was slow. It was more readable.

Yeah, unfortunately it is a big offender in this scenario.

@jamestalmage
Copy link
Contributor Author

Also a technically breaking change (though not one I am really concerned about):

t.is(1 + 1, 2);

No longer returns a Promise.

Only t.throws and t.doesNotThrow will return a promise.

@sindresorhus
Copy link
Member

though not one I am really concerned about

Me neither.

@vadimdemedes
Copy link
Contributor

Results look promising, let's get it in!

sindresorhus added a commit that referenced this pull request Jan 21, 2016
Performance Improvements for files with thousands of tests
@sindresorhus sindresorhus merged commit cb1a0a7 into avajs:master Jan 21, 2016
@sindresorhus
Copy link
Member

(つ◕౪◕)つ━☆゚.*・。゚

@kentcdodds
Copy link
Contributor

giphy

@jamestalmage jamestalmage deleted the prototype-based-api branch January 22, 2016 03:23
@jamestalmage
Copy link
Contributor Author

@kentcdodds, et. all

Just for comparison - I also created a branch converting emoji-aware to mocha

AVA now ekes out slightly better performance:

ava --verbose  9.76s user   0.71s system    465% cpu    2.252 total
mocha          2.07s user   0.31s system     96% cpu    2.460 total   

However, it's obvious the only reason AVA is winning is that it is leveraging multiple processes. Hopefully this means there are still lots of opportunities to make AVA even faster.

@kentcdodds
Copy link
Contributor

Totally. If we could make ava as fast as mocha on a per-file basis then ava would scream with speed. This is making me feel really positive about the potential for ava :D

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