Skip to content

Conversation

@som-snytt
Copy link
Contributor

Fixes #24266

Restores empty check and "already seen" check for Inlined.call sites.

@som-snytt
Copy link
Contributor Author

Would be nice to craft a test. One of the i15503 tests reports that it skips one tree.

llm4s reports locally:

sbt:llm4s> compile
[info] compiling 6 Scala sources to /home/amarki/projects/donderom-llm4s/target/scala-3.8.0-RC1-bin-SNAPSHOT/classes ..
SKIPPED 5874 of all 3572
SKIPPED 7 of all 49
[warn] -- [E198] Unused Symbol Warning: /home/amarki/projects/donderom-llm4s/src/main/scala/com/donderom/llm4s/Llm.scala:4:30
[warn] 4 |import fr.hammons.slinc.types.SizeT
[warn]   |                              ^^^^^
[warn]   |                              unused import
SKIPPED 0 of all 0
SKIPPED 8 of all 204
[warn] -- [E198] Unused Symbol Warning: /home/amarki/projects/donderom-llm4s/src/main/scala/com/donderom/llm4s/SlincLlm.scala:266:20
[warn] 266 |        for (batch, n) <- batches.zipWithIndex do
[warn]     |                    ^
[warn]     |                    unused pattern variable
SKIPPED 0 of all 0
SKIPPED 0 of all 0
[warn] two warnings found
[success] Total time: 7 s, completed Oct 27, 2025, 9:03:10 AM

where the counts are skipped call trees and the total (unique) call trees.

The debug message was emitted at reporting, i.e., after inlining, for various incremental runs. The first number is obviously significant. I did not investigate which trees are inlined many times. Note that -Vprint:inlining -Yplain-printer is unable to run to completion on llm4s. It would be nice to have -Vbrowse as in Scala 2. That is for another free weekend.

Without the fix, the build runs more than ten minutes, possibly much more than the limit of my patience.

@som-snytt
Copy link
Contributor Author

The comment notes that these checks were tried in previous versions of the lint, but the glass of my knowledge about inlining is still only half full. Probably it was just wrong. Linting inlined code was removed in March for 3.7.0. (The wrong code was partially reverted and corrected at 0b3e859. I don't see a commit that tries to avoid visiting a call twice, but my vague memory is that I thought it was not effective. Obviously I lacked an effective test.)

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tgodzik tgodzik merged commit b14afef into scala:main Oct 28, 2025
51 checks passed
WojciechMazur pushed a commit that referenced this pull request Oct 28, 2025
Fixes #24266

Restores empty check and "already seen" check for Inlined.call sites.
[Cherry-picked b14afef]
@som-snytt som-snytt deleted the issue/24266-unused-inlined-perf branch October 28, 2025 10:55
WojciechMazur added a commit that referenced this pull request Oct 28, 2025
Backports #24277 to the 3.7.4.

PR submitted by the release tooling.
[skip ci]
@WojciechMazur WojciechMazur added the backport:done This PR was successfully backported. label Oct 28, 2025
@WojciechMazur WojciechMazur added this to the 3.7.4 milestone Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:done This PR was successfully backported.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Performance regressions for -Wunused and inlined trees

3 participants