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

Invalidate sources that depends on @inline methods #1310

Merged
merged 4 commits into from
Dec 18, 2023

Conversation

Friendseeker
Copy link
Member

@Friendseeker Friendseeker commented Dec 15, 2023

This PR fixes sbt/sbt#4125, #537.

Issue

When @inline method implementation changes, the public API of @inline method does not change, hence Zinc does not invalidate sources that depends on @inline method.

Solution

The behaviour of @inline method is similar to macro methods. Hence, we can mark @inline method as macro and reuse existing Zinc invalidation logic for macro invalidation.

TODOs

  • This PR does not handle call-site @inline. To handle that case, we need to follow -opt:l:inline breaks incremental compilation #537 (comment) and drop call-site @inline at compiler side.
  • Even without @inline annotation, compiler may still sometimes inline a method. The compiler keep a log of inlined methods, so in a follow up PR, we can utilize that information instead of directly checking @inline annotation.

No idea whether getModifiers is a hotspot or not. But it only takes a few extra lines anyways.
Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

Thanks!

@Friendseeker
Copy link
Member Author

Friendseeker commented Dec 17, 2023

Hi Eugene! I was asking users on Scala discord about whether this issue bothers Scala users in practice.

The feedback I got is that basically everyone already has setup to only use optimizer when producing release binary. Hence this issue do not actually impact anyone.

Should we instead not proceed with the PR and instead close the two linked issues sbt/sbt#4125, #537 as won't fix?

@eed3si9n
Copy link
Member

Since the change itself is relatively small, I see no reason we should reject a good change. While it's true, that once you start using optimizer many things would break so people have turned from past injuries to turn them off during normal development, but perhaps changes like this would gradually improve the situation.

@lrytz
Copy link
Contributor

lrytz commented Dec 18, 2023

This PR looks right and is an improvement. However in the vast majority of cases methods are not inlined because they carry the @inline annotation, but due to other inliner heuristics. The official documentation even recommends to avoid using @inline in general (https://docs.scala-lang.org/overviews/compiler-options/optimizer.html#inliner-heuristics-and-inline). So I don't think this PR will help much in practise, but it can be considered a first step?

@Friendseeker
Copy link
Member Author

Friendseeker commented Dec 18, 2023

However in the vast majority of cases methods are not inlined because they carry the @inline annotation, but due to other inliner heuristics.

I had never worked with optimizer, but I think it would be possible for optimizer to provide information for each inlining that took place (caller & callee, along with the class name caller and callee belongs to) and store them in CompilationUnit, and zinc Analyzer phase can then use the information to handle inlining dependency. (e.g. via introducing a new dependency called DependencyByInlining and handle them similarly as DependencyByInheritance.). This would catch inlining dependency due to heuristics.

If that solution is indeed reasonable, we can then decide whether we should implement the solution in a follow up PR.

@eed3si9n eed3si9n merged commit 150ed32 into sbt:develop Dec 18, 2023
7 checks passed
@Friendseeker Friendseeker deleted the track-inline-dep branch December 18, 2023 23:01
@dwijnand
Copy link
Member

It's a little confusing that isMacro doesn't mean isMacro any more :-/

@lrytz
Copy link
Contributor

lrytz commented Dec 19, 2023

The inliner can already produce a log, so adding the right calls to record dependencies should be doable.

➜ sandbox cat A.scala
object A {
  def f = 1
}

object B {
  def g = A.f
}

object C {
  def h = B.g
}
➜ sandbox qsc -opt:inline:'**' A.scala -Vinline _
Inlining into B$.g: inlined A$.f (the callee is a small trivial method). Before: 7 ins, after: 22 ins.
Inlining into C$.h: inlined B$.g (the callee is a forwarder or alias method). Before: 7 ins, after: 40 ins.

But I'm not convinced it's worth the effort. Enabling the optimizer in development is discouraged as it increases compilation times. How about issuing a warning when zinc performs an incremental compilation (not all sources) with the optimizer enabled?

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

Successfully merging this pull request may close these issues.

-opt:l:inline breaks incremental compilation
4 participants