Skip to content

Init check: Early Promotion of fields #11533

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

Merged
merged 13 commits into from
Apr 29, 2021

Conversation

natsukagami
Copy link
Contributor

@natsukagami natsukagami commented Feb 25, 2021

  • New checks for Promote(pot, outer) effect.
  • The old checking rule (Promote(pot, outer) <= Promote(outer)) is now used as a quick check (canDirectlyPromote), before the newer, more costly check is run.

This change is Reviewable

case Warm(cls, outer) =>
canDirectlyPromote(outer)
case _ =>
val summary = expand(pot)
Copy link
Contributor

@liufengyun liufengyun Feb 25, 2021

Choose a reason for hiding this comment

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

This will lead to infinite loops. The following might be a test case:

class A {
  val a = f
  def f: A = f
  class B(x: Int)

  println(new a.B(5))
  val n = 10
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mitigating this with a visited set. I think it's easier to just set a maximum recursion depth though, because this is only a speed-up measure most of the time...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you can play with it and see which works better for the tests we have.

@liufengyun liufengyun assigned natsukagami and unassigned liufengyun Feb 25, 2021
@natsukagami natsukagami requested a review from liufengyun March 9, 2021 07:29
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

Thanks for the good work.

We still need to

  • figure out the failures in CI
  • avoid expensive analysis of early promotion so that community build can finish fast

BTW: it's better to rebase against master than merging master to this PR.

case Warm(cls, outer) =>
canDirectlyPromote(outer)
case _ =>
val summary = expand(pot)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you can play with it and see which works better for the tests we have.

if errs.isEmpty then
Errors.empty
else
UnsafePromotion(warm, eff.source, state.path, errs.toList).toErrors
Copy link
Contributor

Choose a reason for hiding this comment

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

The community build does not finish within 3 hours. We need to find a way to avoid "expensive" analysis. A budget for early promotion?

@natsukagami
Copy link
Contributor Author

natsukagami commented Apr 1, 2021

The following program will take some time to type-check:

class A {
  def v: A = new A {
    def get_b: A = 
      v1
      v2
      // v3
      v
  }
  def v1 = v
  def v2 = v
  // def v3 = v
  print(v)
}

The time taken to type-check seems to grow exponentially compared to the number of methods (if I uncommented v3, it runs substantially slower). This is due to nesting of Warm not being re-used, since v creates a new anonymous subclass every time we run into a check.

The above problem is super-evident in the scalax.rules.Rule class of scalap, where almost every defined operator ends up creating an anonymous class.

@liufengyun
Copy link
Contributor

Nice minimization @natsukagami 👍 The program does not seem to be complex, there might be a big optimization opportunity hidden there.

@natsukagami
Copy link
Contributor Author

natsukagami commented Apr 15, 2021

@liufengyun Turns out the long running time was caused by the cache bug fixed in #11385 #12055 :)
EDIT: Mentioned the wrong PR...

@liufengyun
Copy link
Contributor

@liufengyun Turns out the long running time was caused by the cache bug fixed in #11385 :)

Thanks @natsukagami, great to know. I did expect #12055 to improve performance but didn't know it causes exponential behavior.

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @natsukagami 🎉

@liufengyun liufengyun merged commit 9e0dc21 into scala:master Apr 29, 2021
@Kordyjan Kordyjan added this to the 3.0.1 milestone Aug 2, 2023
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.

3 participants