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

Speed issues [enhancement] #43

Closed
pbrinkmeier opened this issue Feb 26, 2015 · 19 comments

Comments

@pbrinkmeier
Copy link

commented Feb 26, 2015

First of all, I like the style of standard and I use it in all projects I started lately.

Buuuut, it always takes several seconds to test a single file in my test directory even if it only requires small files. My test file is about 150 LOC, the source is in this case even smaller.

Maybe this is an issue not with standard it self but with one of it's dependencies ... this is really interrupting my workflow.

standard@2.7.3
Intel Core i5, 4 Gigs RAM, Ubuntu 14.04

@feross

This comment has been minimized.

Copy link
Member

commented Feb 26, 2015

After investigating, there are two main places where time is spent:

  1. ~500ms. Locating the eslint and jscs binaries with the npm run which-eslint and npm run which-jscs commands. We locate the binaries in parallel. See #30 for why we need to do this.
  2. ~400ms running the linters. We run the linters in parallel.

The rest of the steps only take a trivial <20ms amount of time.

We could speed things up a lot by using the eslint and jscs JS APIs directly, instead of relying on the command line programs. This would eliminate step 1 entirely, and eliminate a child_process.exec call in step 2.

PR welcome – I don't have time for this right at the moment.

feross added a commit that referenced this issue Feb 26, 2015

@feross feross referenced this issue Feb 26, 2015
3 of 4 tasks complete
@pbrinkmeier

This comment has been minimized.

Copy link
Author

commented Feb 26, 2015

I'll dig into that when I have some free time, maybe I can do something ... 👍

@Flet

This comment has been minimized.

Copy link
Member

commented Feb 27, 2015

eh, first try at using eslint directly... WIP
Flet@6bb49ba

@feross

This comment has been minimized.

Copy link
Member

commented Feb 27, 2015

@Flet Good start. 👍 I say we hold off on merging this until we've eliminated jscs (#45), so we don't waste effort converting that too.

@pbrinkmeier

This comment has been minimized.

Copy link
Author

commented Feb 27, 2015

@feross
You're the boss, but as far as I looked in to that I think that actually these two can work side by side quiet harmonously.

@Flet
Are you going to finish that? Or should I begin to work on it too?

@pbrinkmeier pbrinkmeier changed the title Speed issues Speed issues [enhancement] Feb 27, 2015

@Flet

This comment has been minimized.

Copy link
Member

commented Feb 27, 2015

@reg4in yeah probably... but maybe it makes sense to create an eslint-only branch in this repo and continue work collaboratively?

Once the eslint rules from #45 are in, I'd guess we would want remove jscs and gain the speed benefits of using eslint directly. The branch could focus on that end state.

Thoughts, @feross? Or should we redirect our efforts to creating the missing eslint rules instead? :)

@pbrinkmeier

This comment has been minimized.

Copy link
Author

commented Feb 28, 2015

I'd totally be in 👍

@feross

This comment has been minimized.

Copy link
Member

commented Mar 4, 2015

@Flet Either approach is fine. I just added you as a collaborator on the repo.

@feross

This comment has been minimized.

Copy link
Member

commented Mar 19, 2015

Thanks for the first attempt code, @Flet – it was very helpful.

I just pushed a branch (speed), which improves performance of standard by 2-4x! This branch does both items discussed in my first comment:

  1. remove the need for npm run which-eslint.
  2. remove the child_process.exec calls and use the node api of eslint directly.

I also remove jscs, which we're close to doing anyway (#45).

Linting feross/standard:

$ time standard
standard  2.21s user 0.36s system 161% cpu 1.596 total
$ time ./bin/cmd.js
./bin/cmd.js  0.61s user 0.06s system 105% cpu 0.630 total

Linting npm/npm:

$ time standard
standard  4.63s user 0.44s system 175% cpu 2.895 total
$ time ./bin/cmd.js
./bin/cmd.js  2.18s user 0.16s system 114% cpu 2.052 total

feross added a commit that referenced this issue Mar 19, 2015

Significant speed increase / remove jscs linter
Improves performance of standard by 2-4x by:

- remove the need for `npm run which-eslint`
- remove the `child_process.exec` calls and use the node api of eslint
directly
- Remove `jscs` linter

Fixes #43 - speed issues
Fixes #45 - eliminate jscs

Lays the groundwork for #60 - programmatic use
@Flet

This comment has been minimized.

Copy link
Member

commented Mar 19, 2015

Sweet! I was just thinking of returning to this with the eslint rule movement :) Amazing what a few dollars does! 💰

Taking a gander at the branch now.

@Flet

This comment has been minimized.

Copy link
Member

commented Mar 19, 2015

Wow, so much cruft gone now! Also should be easier to use standard programmatically now that it takes a callback.

@Flet

This comment has been minimized.

Copy link
Member

commented Mar 19, 2015

I take full blame for the sync stuff 😭

@feross

This comment has been minimized.

Copy link
Member

commented Mar 19, 2015

Hah, no worries. I'm still doing a bit more cleanup to make the api nicer for programmatic use :)

@feross feross self-assigned this Mar 19, 2015

@feross

This comment has been minimized.

Copy link
Member

commented Mar 19, 2015

Okay! New, cleaned up node.js apis are available on the speed branch. Really happy with how they turned out!

Deleted around 200 lines total, increased speed 2-4x, and it's way clearer how everything works now!

@pbrinkmeier

This comment has been minimized.

Copy link
Author

commented Mar 19, 2015

nice one!

feross added a commit that referenced this issue Mar 23, 2015

Significant speed increase / remove jscs linter
Improves performance of standard by 2-4x by:

- remove the need for `npm run which-eslint`
- remove the `child_process.exec` calls and use the node api of eslint
directly
- Remove `jscs` linter

Fixes #43 - speed issues
Fixes #45 - eliminate jscs

Lays the groundwork for #60 - programmatic use
@millermedeiros

This comment has been minimized.

Copy link

commented Mar 24, 2015

instead of using npm run which-eslint you could use the npm-run lib. the slow part is not the which command or child_process.exec, but spawning the npm executable itself.

see details on npm-which (used internally by npm-run)

@feross

This comment has been minimized.

Copy link
Member

commented Mar 24, 2015

Ah, handy! Wish I knew about npm-run before.

In any case, we've removed npm run and child_process.exec in favor of require('eslint') which requires less code and fewer hacks. :) This is currently on the speed branch and will become the next version of standard once eslint/eslint#1619 is fixed.

@feross

This comment has been minimized.

Copy link
Member

commented Mar 26, 2015

This is released as 3.3.0.

@feross feross closed this in 306cadf Mar 26, 2015

feross added a commit that referenced this issue Mar 26, 2015

use latest eslint
Depending on the latest eslint via git will let us release the next
version of `standard` asap and fix #43, #45, and #60. Once eslint
releases the next version, we can switch back to the npm dep.
@pbrinkmeier

This comment has been minimized.

Copy link
Author

commented Mar 26, 2015

nice! good job!

@lock lock bot locked as resolved and limited conversation to collaborators May 11, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants
You can’t perform that action at this time.