Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Flycheck became slow when opening buffers since the 29 release #1129

Closed
NicolasPetton opened this Issue Oct 5, 2016 · 28 comments

Comments

Projects
None yet
7 participants

I updated flycheck from the last release (29) to the last version on Melpa (20161004.1257).

Opening JS buffers linted with eslint, it seems that flycheck freezes Emacs for 1 to 2 seconds. Once the buffer is open, I can't see any slowdown and flycheck operates normally. When disabling Flycheck, opening the buffer is instantaneous.

I'm not sure what changes between the last release and the last build on MELPA, if it's how flycheck runs when opening a buffer or if it has to do with how eslint is run.

I tried this with a clean (no config file loaded) Emacs v 25.1, with the following

(require 'package)
(add-to-list 'package-archives
             '("MELPA Stable" . "http://melpa.org/packages/") t)
(package-initialize)
(package-refresh-contents)

(package-install 'flycheck)

(global-flycheck-mode)

Installing Flycheck 29 instead fixes the issue.

Member

Simplify commented Oct 5, 2016

See #1085 - for every new buffer we run eslint --print-config . in current-directory to check if we can use eslint. Otherwise, eslint without any configuration or when configuration is broken (missing NPM packages) may throw error.

Thanks. The problem is that eslint --print-config can take seconds to run, and in the meantime Emacs is frozen.

Owner

cpitclaudel commented Oct 5, 2016

Thanks Nicolas. Do you have a publicly available example where it's that slow? Is it slow for a good reason, or should we file a bug report with them?

Owner

cpitclaudel commented Oct 5, 2016

I wonder if we should run :enabled checks asynchronously.

Contributor

lunaryorn commented Oct 5, 2016

@cpitclaudel How would we do that, from an implementation point of view? We need to make a synchronous decision about whether we use the checker or not, and how would we even run an arbitrary function asynchronously?

@NicolasPetton eslint --print-config is the best we got to check whether we can use eslint in a buffer. We need to check whether eslint can find a configuration, one way or the other…

On this project https://github.com/foretagsplatsen/klassified it takes 0.8s on my laptop:

time eslint --print-config .
[...]
real    0m0.802s

I have other (private) repos where it takes 1.4s. Opening each buffer feels really slow.

I don't really like this solution, but could we have a variable that would bypass this check? It could be set as a dir-local var.

Contributor

lunaryorn commented Oct 5, 2016 edited

As far as I'm concerned, no, we can't. We're running these predicates for a reason—if we allowed to circumvent them we might as well remove them entirely again.

We should rather try and speed up the predicate instead of trying the cheap and easy way of just hacking around the issue.

eslint takes 0.7s on the repo I just tried but to me that's not a delay large enough to warrant a workaround.

Yeah, I didn't like the approach either, but I don't know how we can speed up the predicate.

Contributor

lunaryorn commented Oct 5, 2016

@NicolasPetton Well, we can start with asking eslint why it takes the time it takes and whether they could probably provide a faster way to check for a configuration…

Member

Simplify commented Oct 5, 2016

Other option is to offload configuration from npm modules into your config file and check if that speeds anything up.

Owner

cpitclaudel commented Oct 5, 2016

@cpitclaudel How would we do that, from an implementation point of view? We need to make a synchronous decision about whether we use the checker or not, and how would we even run an arbitrary function asynchronously?

Let me try to write a prototype :) I think it won't take much longer than explaining what I have in mind.

@cpitclaudel Did you have time to try something? Otherwise contacting the eslint guys might be a good idea.

Contributor

lunaryorn commented Oct 17, 2016

@NicolasPetton You can talk to the eslint maintainers independently, for if --print-config turns out to be so slow it might be a good idea to have a faster option for checking whether a configuration exists, independent of whether we are able to parallelize our predicates.

Member

Simplify commented Oct 17, 2016

@lunaryorn I don't think eslint deleopers can add faster option, but @NicolasPetton can try to contact them. They really need to parse whole config. For example you can specify that you want to use eslint extension inside config. That is additional npm package. They will check if that extension is installed. Also, if eslint is globally installed, those npm packages must be installed globally too. I think that from Flycheck standpoint, we choose best possible solution for this.

Personally, I don't have any problems will my projects that use ESlint. I find for example opening ruby files when using jruby much more slower, but reason for that is also out of out hands.

Contributor

lunaryorn commented Oct 17, 2016

@Simplify Ah, yes, I didn't think of that… I'm constantly surprised on how complex the eslint configuration actually is 😲

Seems like we need to close this issue as wontfix.

Member

Simplify commented Oct 17, 2016

Yeah, I didn't know that too, until I tested --print-config implementation. I agree that we may close this ticket, unless @cpitclaudel have asynchronous solution that may work.

Contributor

lunaryorn commented Oct 18, 2016 edited

I'm closing this issue as wontfix then. I'm sorry, @NicolasPetton, but short term it doesn't look as if we can do anything about it.

@lunaryorn lunaryorn closed this Oct 18, 2016

@lunaryorn lunaryorn added wontfix and removed needs review labels Oct 18, 2016

Owner

cpitclaudel commented Oct 18, 2016

Sorry for the delay :/ I tried writing something, but I ran into small issues. The idea was essentially this: instead of checking for the configuration, the "enabled" function would return nil immediately, and start the subprocess asynchronously. It would then, from a filter function, re-enable the checker if the check succeeded.

I managed to get this mostly working, with one issue: we don't really have a distinction between disabled by the user and automatically disabled AFAICT. So if the user disabled the checker before the async check completed, it would get re-enabled.

The code is here: https://github.com/cpitclaudel/flycheck/tree/1129-async-enabled

Contributor

lunaryorn commented Oct 18, 2016

If I understand the implementation aright all syntax check would silently skip over the check while it's :enabled function is still running, and after it completed the checker would be put into the whitelist of enabled checkers making the checker available for the next syntax check?

Owner

cpitclaudel commented Oct 18, 2016

That's right. Until the check completes eslint would be considered disabled, and if it succeeds the filter would put it back in the 'enabled' list, thus allowing it to be used for future checks.

Contributor

lunaryorn commented Oct 18, 2016

I kind of like the idea 👍 but I still see a couple of open questions… mind if you open a pull request so that we can discuss in detail?

Owner

cpitclaudel commented Oct 18, 2016

Sure thing; will do it soon. We'll see if we can turn it into something usable :)

Ah yes, I'm happy to test this feature, definitely noticing the slow down since --print-config.

kouhin commented Aug 2, 2017

Opening a javascript file gets extremely slow when using --print-config.
Finally, I found a way to disable this feature 🚀.

Make sure your eslint is set properly and add following code to your emacs config

(with-eval-after-load 'flycheck
  (advice-add 'flycheck-eslint-config-exists-p :override (lambda() t)))

eastwood commented Aug 6, 2017

For those using use-package @kouhin's solution can be shorter:

(use-package flycheck
  :config (advice-add 'flycheck-eslint-config-exists-p :override (lambda() t))
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment