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

Is this execution time the expected one? #629

Closed
fernandofleury opened this issue Nov 6, 2015 · 7 comments
Closed

Is this execution time the expected one? #629

fernandofleury opened this issue Nov 6, 2015 · 7 comments
Labels

Comments

@fernandofleury
Copy link

image

It seems a bit slow to me, is this time the usual one? The project itself it's not big at all.

My setup is using gulp as the taskrunner and gulp-scss-lint to wrap the scss-lint gem.

@srawlins
Copy link
Contributor

srawlins commented Nov 7, 2015

TL;DR This is not unusual in my experience. Try master. And try disabling linters from the list below if you really need it faster.

I've been working on speeding scss-lint up (#577, #579, #580, #584, #622, #623, #624), but I haven't really seen any huge wins. What version of scss-lint are you running? My last 3 patches have not been released yet, so you might try again on the master branch. I wouldn't be surprised if a ~6.7s process now takes ~5s.

If you're interested in boosting performance, I'd recommend tmm1's stackprof, a truly beautiful tool, for Ruby 2.1+. You can call StackProf.run(mode: :cpu, out: 'tmp/stackprof.dump') do in Runner#run or CLI#scan_for_lints.

I think I found that typically, Sass parsing (not under scss-lint's control) seems to take ~50% of the time, so right there, you're not going to make the the scss-lint process 10x faster, ever (on a single file).

We discuss in #576 the idea of parallelizing scss-lint. I've got a patch in the works that seems to reduce time linting by like ~15%. It's not great. It's also a very very ugly looking patch, so I haven't posted it yet. (I also discovered that something somewhere is not thread-safe: I could not parallelize all of the Sass-parsing, and scss-linting activities across all individual linters and across all files. I can't recall the details...)

Honestly when talking about ~7s, I think a lot goes into Ruby's VM startup time.

I also timed the individual linters, linting over a copy of Bootstrap 4's SCSS files. Here's the slowest 10 linters (before the patches in #622, #623, #624). Times in seconds:

           SCSSLint::Linter::FinalNewline: 0.362
              SCSSLint::Linter::UrlFormat: 0.363
             SCSSLint::Linter::NameFormat: 0.389
      SCSSLint::Linter::PropertySortOrder: 0.396
         SCSSLint::Linter::SelectorFormat: 0.397
          SCSSLint::Linter::SelectorDepth: 0.408
     SCSSLint::Linter::SpaceBetweenParens: 0.419
 SCSSLint::Linter::EmptyLineBetweenBlocks: 0.453
SCSSLint::Linter::SpaceAfterPropertyColon: 0.462
            SCSSLint::Linter::Indentation: 1.045

So you might consider disabling one or more of these, if the linting time is really bogging you down.

@lencioni
Copy link
Collaborator

@srawlins It looks like you've been doing some good profiling. I think it would be great to add some sort of profiling/benchmark script/task or something that would make it easy for anyone who is interested in improving the performance of this project to hit the ground running. Would you be interested in submitting a pull request for such a thing?

@srawlins
Copy link
Contributor

Yeah, I'd be into that. Unfortunately it means modifying code to run something like stackprof, so maybe we can have a long-running git branch named performance or something? And maybe a PERFORMANCE.md or similar document in the master and performance branches with steps on how to profile? I'd be interested in doing this.

@lencioni
Copy link
Collaborator

I'm a little uncomfortable with a long-running git branch because I worry about things getting out of sync. I wonder if we could add a --profile flag or something that would turn on profiling for that run.

@srawlins
Copy link
Contributor

Hmm, yeah. Might be able to do that. I'm not big on adding a dependency in the gemspec when there are currently only two dependencies.

@trotzig
Copy link
Contributor

trotzig commented Nov 12, 2015

You could make it a soft dependency (i.e. leaving it out from the gemspec) and document having to install it manually as part of running --profile (or however we end up running it).

@sds
Copy link
Owner

sds commented Nov 20, 2015

Given the question as originally asked as been answered, I'm closing this issue.

Happy to accept a pull request that adds a benchmark measurement script of some kind, but agree that it's preferable to not include its dependencies in the gemspec. Thanks!

@sds sds closed this as completed Nov 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants