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

Fix require (and test) performance #3242

Merged
merged 3 commits into from Mar 30, 2018

Conversation

5 participants
@jeddy3
Member

jeddy3 commented Mar 27, 2018

Ref #3236
Ref #2454

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Mar 27, 2018

These changes take about a 3rd off the test run times on the CIs and on my local machine:

  • Travis Node 8 (with coverage) 204.411s -> 166.244s
  • Travis Node 6: 189.027s -> 133.654s
  • Appveyor: 197.769s -> 116.166s
  • Local machine (with cleaned jest cache): 248.824s -> 82.653s

These changes also reduce the require time of stylelint from 900ms to 600ms on my machine.

I've done a fair amount of investigation in the past two days and I've narrowed down two other possible areas of improvements:

  1. Requiring postcss-html in getPostcssResult.js is responsible for nearly half of the remaining require time. Without postcss-html the require time is about 300ms. Which I believe is on par with ESLint. Additionally, running the tests only takes 54s on my local machine without postcss-html. @gucong3000 Any thoughts on what we can do about this?
  2. It's difficult to pinpoint memory leaks, but there might be one in isAutoprefixable.js. The *-no-vendor-prefix rules popped up regularly when using jest:detectleaks and removing the require('autoprefixer') line from isAutoprefixable.js seems to quell them. Anyone got any ideas what we could do here?

@stylelint/core Can anyone see any potential problems with the changes in this PR? If people have time, it'd be great to get this PR tested against your project code bases.

@ntwb

Love the addition of Jest mocks ❤️

I'll take a closer look at the related issues and do some local testing here later today

@gucong3000

This comment has been minimized.

Member

gucong3000 commented Mar 28, 2018

Any thoughts on what we can do about this?

#3243

@jeddy3 jeddy3 force-pushed the improve-test branch from 8ca7ce0 to de4b7ce Mar 28, 2018

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Mar 28, 2018

Any thoughts on what we can do about this?
#3243

Awesome, thanks! I've merged that and rebased this PR. The change seems to have shaved off another third.

Travis Node 8 (with coverage): 204.411s -> 166.244s -> 77.806s
Travis Node 6: 189.027s -> 133.654s -> 99.038s
Appveyor: 197.769s -> 116.166s -> 76.075s

On my machine, tests (with cleaned Jest cache) are now down to 54s and require time is around 344ms (down from 900ms -> 600ms).

I'm removing the "WIP" from the title. I believe this PR is ready to review and test.

@jeddy3 jeddy3 changed the title from WIP Investigate test and require performance to Fix require (and test) performance Mar 28, 2018

@evilebottnawi

Awesome work!

@Arcanemagus

Minor suggestion for clearer code, this looks good just looking at the code at least 😉.

@@ -22,7 +23,7 @@ module.exports = function(
if (!callback) throw new Error("checkAgainstRule requires a callback");
if (!options.ruleName)
throw new Error("checkAgainstRule requires a 'ruleName' option");
if (!rules[options.ruleName])
if (rules.indexOf(options.ruleName) === -1)

This comment has been minimized.

@Arcanemagus

Arcanemagus Mar 28, 2018

Contributor

I'd suggest (!rules.includes(options.ruleName)) as a clearer alternative.

This comment has been minimized.

@jeddy3

jeddy3 Mar 28, 2018

Member

For something reason, I'd gotten it into my head that .includes was introduced in node 8... that's not the case.

I'll update now.

const rules = require("./rules");
module.exports = function(ruleName) {
if (rules.indexOf(ruleName) !== -1) {

This comment has been minimized.

@Arcanemagus

Arcanemagus Mar 28, 2018

Contributor

I'd suggest (rules.includes(ruleName)) as a clearer alternative.

@ntwb

ntwb approved these changes Mar 29, 2018

@jeddy3 jeddy3 merged commit 973e07b into master Mar 30, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.08%) to 95.962%
Details

@jeddy3 jeddy3 deleted the improve-test branch Mar 30, 2018

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Mar 30, 2018

  • Fixed: slow require('stylelint') time (#3242).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment