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

Dynamic runner startup significantly slower - add option to force CoreRunner? #1460

Closed
ArturGajowy opened this issue Jul 20, 2019 · 6 comments · Fixed by #1910
Closed

Dynamic runner startup significantly slower - add option to force CoreRunner? #1460

ArturGajowy opened this issue Jul 20, 2019 · 6 comments · Fixed by #1910

Comments

@ArturGajowy
Copy link

I'm using scalafmt via nailgun, which as you know gives rapid startup and execution times. When executing on a project using a different scalafmt version in .scalafmt.conf file than the one behind nailgun, startup times are slower than reformatting a whole commit when the versions match:

✔ ~/projects/rchain-bloop [charging_for_deploy_4.13 ↓·7↑·13|✚ 1…12⚑ 38] 
23:17 $ time scalafmt build.sbt --test --debug
Working directory: /home/artur/projects/rchain-bloop
parsed config (v2.0.0)
Looking for unformatted files...
All files are formatted with scalafmt :)

real    0m0.206s
user    0m0.037s
sys     0m0.008s

✔ ~/projects/rchain-bloop [charging_for_deploy_4.13 ↓·7↑·13|✚ 1…12⚑ 38] 
23:18 $ time scalafmt build.sbt --test --debug
Working directory: /home/artur/projects/rchain-bloop
parsed config (v2.0.0-RC8): /home/artur/projects/rchain-bloop/.scalafmt.conf
Looking for unformatted files...
All files are formatted with scalafmt :)

real    0m1.911s
user    0m0.029s
sys     0m0.020s

It'd be great to pass an option (say: --require-same-version) to error whenever there'd be a need to use the dynamic runner. This would prevent the slowdown by forcing me to update.

Without passing any flags, I think a warning should be emitted, saying 'you're running scalafmt 2.0.0, but the config you're using requires a different version, which leads to slower startup and execution'. That would at least be true for scalafmt behind nailgun.

Having just the warning would be better than nothing, but would likely go unnoticed (e.g. IDEA doesn't show git hook output till they fail) or require clumsy scripting in the git hook (scanning for the warning in scalafmt output).

Even better solutions would be:

  • auto-updating and automatically switching the bloop version behind nailgun (maybe maintaining a pool of versions running, if people use different scalafmt versions in different projects and interleave interactions with them...)
  • making the dynamic runner as fast as the core one (likely not possible?)

Having the flag would be more than enough for me though :)

@ArturGajowy
Copy link
Author

I might be wrongly attributing the slowdown to startup time (vs formatting time) - but the argument for the flag that prevents the slowdown remains.

@tanishiking
Copy link
Member

Thank you for reporting :)
That's right. if the versions between scalafmt-cli and the version in .scalafmt.conf differs, scalafmt-cli try to download the specified version of scalafmt and run it. It is always slower than running scalafmt-core, which is embedded in scalafmt-cli.
However, it would be nice to show some messages so that we can know which ScalafmtRunner we are using. 👍

making the dynamic runner as fast as the core one (likely not possible?)

As you mentioned, it's impossible because there's always overhead for downloading the scalafmt-core from maven (or from cache).


It'd be great to pass an option (say: --require-same-version) to error whenever there'd be a need to use the dynamic runner. This would prevent the slowdown by forcing me to update.

As you said, just having a message doesn't make people notice that we are using ScalafmtDynamic, but personally, I think it is over the top to have an option something like --require-same-version (I hope scalafmt have less number of options as possible).
What do you about showing some messages something like Downloading scalafmt v2.0.0 ... instead of having such an option? It would be better than nothing.

@ArturGajowy
Copy link
Author

What do you about showing some messages something like Downloading scalafmt v2.0.0

It's not always downloading AFAICT, and to me it's no more useful than the currently available --version flag - I still need to parse the output and have logic to terminate the hook with an error.

The only way of running scalafmt that's fast enough to be used as a git hook / all the time is - in my mind - via nailgun, and this is only when the versions match. The least we can do for people to start recognize how fast scalafmt can be is to document the fact that the versions need to match for good performance.

Since I find documenting stuff that can be enforced porgramatically a half-measure, I proposed having a flag, with the intent of people who read the manual discovering it. Maybe though it should be recommended for tooling use. Without the flag, everyone needs to re-do the flags logic in their tooling for scalafmt to be perceived fast. And it can be fast, to the extent I don't even notice it being in the hook.

@tanishiking
Copy link
Member

Ok, maybe it can be useful to have such kind of option on scalafmt-cli 👍
Personally, I prefer to name the option something like --disable-dynamic-loading or --disable-blah-blah .

@kitbellew
Copy link
Collaborator

@ArturGajowy do you have project filters in your .scalafmt.conf? i found that dynamic runner reads all files (not necessarily just those matching those filters) and passes them to the formatter, which applies the filtering at that point, which in my case increases the run time significantly. see #1910.

@ArturGajowy
Copy link
Author

I think the config in question was this, so I think yes (there are excludeFilters)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants