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

Warn or optimize when inline given inlines anonymous class creation #16723

Closed
szymon-rd opened this issue Jan 18, 2023 · 6 comments · Fixed by #20291
Closed

Warn or optimize when inline given inlines anonymous class creation #16723

szymon-rd opened this issue Jan 18, 2023 · 6 comments · Fixed by #20291

Comments

@szymon-rd
Copy link
Member

szymon-rd commented Jan 18, 2023

Compiler version

3.2.1

Minimized example

trait Converter[A, B] {
    def convert: A => B
}

inline given Converter[Int, String] = new Converter {
    def convert = _.toString()
}

def foo(using bar: Converter[Int, String]) = 
    "foo"

@main
def main = 
    foo
    foo
    foo
    foo

Output

Creates 4 anon classes in the compilation output. Can lead to an explosion of amount of classes, killing the jit code cache (as seen in the lichess case)

Expectation

Should warn the User about the generation of anon classes per usage of Converter inline given, or find a way to avoid inlining anonymous class generation.

@smarter
Copy link
Member

smarter commented Jan 18, 2023

inline given Conversion[Int, String] = _.toString()

This should already give a warning: #16499

@szymon-rd
Copy link
Member Author

Couldn't we automate this transformation?

@szymon-rd
Copy link
Member Author

It also doesn't warn in the more general case that I gave in the first snippet.

@szymon-rd
Copy link
Member Author

szymon-rd commented Jan 18, 2023

So this task's scope could be to transform every:

inline given X[...] = new X {
   def x1 = ...
   // ...
   def xn = ...
 }

to:

inline given X[...] with 
  def x1 = ...
  // ...
  def xn = ...

And treat the RHS function provided as implementation of trait as de facto first form, so transform it as well.

But I wonder if assignment of explicit anonymous class and the with construct are semantically equivalent.

@smarter
Copy link
Member

smarter commented Jan 18, 2023

Instead of doing the transformation, we could warn that given X = new X { ... } should be rewritten as given X with ... (regardless of whether it's inline or not, the latter form is more idiomatic).

@szymon-rd
Copy link
Member Author

szymon-rd commented Jan 18, 2023

I think that we should do the transformation for:

inline given Conversion[Int, String] = _.toString()

If the Conversion was defined instead as parametrized type, not a trait, then it would not generate anonymous class. It would generate a new method on each usage - that is also suboptimal, but still much negligible than generating a new classfile. The user should be able to perceive Conversion as a function and not consider whether it's trait under the hood. Given that the Conversion is already perceived as a function, I don't see why the RHS function on given should not be considered idiomatic. I think it's even cleaner:

inline given Conversion[Int, String] = _.toString()

vs

inline given Conversion[Int, String] with
  def apply(x: Int) = x.toString()

Here the user msut acknowledge the implementation detail of Conversion - the apply method, instead of treating it just like a function.

In the other case, where user explicitly defines an anonymous class, the given X with ... syntax is certainly more idiomatic. But, I think that it should not be penalized heavily by the compiler performance, but rather detected by linter. However, I think it has less priority than the first case.

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.

5 participants