Skip to content

-Wunused:all is slow to compile #19671

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

Closed
oyvindberg opened this issue Feb 12, 2024 · 7 comments
Closed

-Wunused:all is slow to compile #19671

oyvindberg opened this issue Feb 12, 2024 · 7 comments
Assignees
Labels
area:linting Linting warnings enabled with -W or -Xlint itype:performance

Comments

@oyvindberg
Copy link

oyvindberg commented Feb 12, 2024

Compiler version

3.4.0-RC4

Problem

First I want to celebrate the work that has been done for Scala 3.4 to provide Intellij with timing details so that you can see which phases are slow, which files cause much compilation time and so on. Great work!

So at $WORK we just migrated to Scala 3, and are facing slow compiles. Now I finally had the tools to dig a bit deeper. The first thing I found was that the two unused phases consume more time than typer for many of our sbt modules! I'll show an illustrative example here:

Screenshot 2024-02-12 at 00 11 59 Screenshot 2024-02-12 at 00 12 17

The data shown there seems to be correct, and can be reproduced with sbt.

Clean/compile for the same module with -Wunused:all enabled (slightly shorter times because of warmer compiler)

[success] Total time: 15 s, completed Feb 12, 2024, 12:15:43 AM
[success] Total time: 16 s, completed Feb 12, 2024, 12:16:11 AM
[success] Total time: 16 s, completed Feb 12, 2024, 12:16:11 AM

And without:

[success] Total time: 7 s, completed Feb 12, 2024, 12:18:44 AM
[success] Total time: 8 s, completed Feb 12, 2024, 12:18:58 AM
[success] Total time: 7 s, completed Feb 12, 2024, 12:19:11 AM

I'm also very happy that the unused functionality is back, and hope it can be optimized a bit. For now it's very easy to add all the flags from sbt-tpolecat, end up with a slow build and conclude that "Scala is slow to compile".

Reproducing

So this is a private project, but I suspect it will reproduce elsewhere. I tried for instance lichess lila (a prominent public scala 3 repo).

with unused:

[lila] $ user/clean;user/compile
[success] Total time: 0 s, completed Feb 12, 2024, 12:53:50 AM
[info] compiling 28 Scala sources and 1 Java source to /Users/oyvind/pr/lila/modules/user/target/scala-3.3.1/classes ...
[warn] -- Warning: /Users/oyvind/pr/lila/modules/user/src/main/Env.scala:5:35 ---------
[warn] 5 |import lila.common.autoconfig.{ *, given }
[warn]   |                                   ^^^^^
[warn]   |                                   unused import
[warn] -- Warning: /Users/oyvind/pr/lila/modules/user/src/main/Env.scala:10:19 --------
...
[warn] 9 warnings found
[success] Total time: 5 s, completed Feb 12, 2024, 12:53:56 AM
[lila] $ user/clean;user/compile

without unused:

[lila] $ user/clean;user/compile
[success] Total time: 0 s, completed Feb 12, 2024, 12:54:58 AM
[info] compiling 28 Scala sources and 1 Java source to /Users/oyvind/pr/lila/modules/user/target/scala-3.3.1/classes ...
[success] Total time: 3 s, completed Feb 12, 2024, 12:55:01 AM

I can also be of assistance with for instance profiling data if that helps.

Concurrent linting question

I assume compiler trees are immutable and linting phases don't affect the output trees. Did you look into running them in the background as the compiler progresses?

@oyvindberg oyvindberg added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 12, 2024
@kyri-petrou
Copy link

kyri-petrou commented Feb 12, 2024

Just want to say that we've been experiencing a similar issue at $WORK. One of our services uses Scala 3, and the compiling time is roughly ~50-100% longer when -Wunused:linted is used (depending how warm is the compile server). After playing around with the compiler flags a bit, it seems that the majority of the added time comes from -Wunused:imports. Worryingly, version 3.4.0-RCX seems to be noticeably slower when linting is enabled (as opposed to 3.3.x).

I should also point out that since IntelliJ enables Wunused:imports by default for its "Remove unused imports" functionality, large projects end up having very bad dev experience due to the additional compilation time incurred by linting

@hamnis
Copy link

hamnis commented Feb 13, 2024

In the linked gist this is what I get with a cold compiler for $WORK.

JAVA_OPTS=-Dsbt.task.timings=true sbt clean cronjobs/compile

https://gist.github.com/hamnis/acca674f19cd8cc253ae916f48709fa1

I included the number of source files to indicate size of the module.

@Gedochao Gedochao added itype:performance area:linting Linting warnings enabled with -W or -Xlint and removed itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 13, 2024
@szymon-rd
Copy link
Contributor

Thanks @hamnis for data! It seems that the problem started with Scala 3.4.0, hopefully it is a regression caused by some change that is easy to revert in linting cases.

@Gedochao Gedochao assigned sjrd and unassigned szymon-rd Apr 5, 2024
@dimitarg
Copy link

I'm unable to share a repro / something more useful right now, but we have anecdotal reports at work of this being experienced in 3.3.3 as well.

@kyri-petrou
Copy link

Hi, in case this might help someone with optimizing this issue, it seems that one good OS repo to replicate this is regression in compiling time is kyo.

Overall, more than half of the compilation time of some modules is spent on checking unused imports:

image

sjrd added a commit that referenced this issue May 7, 2024
It doesn't mean that it's *fast* yet, but it is already a significant
step in that direction. In particular, this goes in the direction of
addressing #19671.

The most important commit is "Simplify the logic for checking unused
imports.", whose commit message follows:

Instead of dealing with entire `tpd.Import`s at the end of the scope, we
eagerly flatten them into individual `ImportSelector`s. We store them
along with some data, including a mutable flag for whether a selector
has been used.

This allows to dramatically simplify `isInImport`, as well as more
aggressively cache the resolution of selectors. We also get rid of the
`IdentityHashMap`.

The algorithm is still `O(n*m)` where n is the number of imports in a
scope, and m the number of references found in that scope. It is not
entirely clear to me whether the previous logic was already `O(n*m)` or
worse (it may have included an additional `p` factor for the number of
possible selections from a given qualifier).

Regardless, it is already quite a bit faster than before, thanks to
smaller constant factors.
@matthughes
Copy link

@kyri-petrou How did you get that profiling information? Is this something we can print out from the compiler or something internal to Intellij?

@Gedochao
Copy link
Contributor

This seems to be improved considerably by #20321.
In case further tweaks are necessary, let's track them under a new ticket.
Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:linting Linting warnings enabled with -W or -Xlint itype:performance
Projects
None yet
Development

No branches or pull requests

8 participants