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

Reduce projections of type aliases with class type prefixes #19931

Merged
merged 3 commits into from
Mar 27, 2024

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Mar 12, 2024

Projections P # X are types that we would like to avoid. If X is a class type, there's nothing we can do. If X is an abstract type, we use skolemization and rewrite to (x?: P).X. If X is an alias type we should simply dealias but this was not done before.

This caused an exponential blowup in #19892, where we constructed types of the form

ZPartialServerEndpoint[R, A, B, I, E, O, -C] # EndpointType[A, I, E, T, R] ... # EndpointType[A, I, E, T, R]

When the were 5 or more such selections, compile times blew up (33s for 5, timeout after 5 minutes for 6). I am still not quite sure where the blowup happened. Looking at stacktraces of random interrupts it seemed to be in a deep recursion of memberDenot and asSeenFrom calls.I believe it would still be interesting to find out more about this, in case there are other similar situations where combinations of deep projections with wide applications cannot be avoided.

But for this precise problem, eagerly dealiasing fixes it.

Fixes #19892

@odersky odersky requested a review from rochala March 12, 2024 22:57
@odersky
Copy link
Contributor Author

odersky commented Mar 12, 2024

@rochala Can you help me fix the 2 presentation compiler test failures? I am not sure what to do here.

@rochala
Copy link
Contributor

rochala commented Mar 12, 2024

I'll check it the first thing in the morning

@rochala
Copy link
Contributor

rochala commented Mar 13, 2024

Dealiasing affects the typed trees which we are using for various features.
The tests that failed can be fixed by migrating to untyped trees, but there are other features such as hovers / go to type definition which can be more problematic, as the types are already dealiased.

I suppose it could be done if we analyse each select whether it is actually a type projection, and try recreating the original type by hand.

I will try to implement those workarounds unless you have a better idea.

@odersky
Copy link
Contributor Author

odersky commented Mar 13, 2024

Does it just apply to # projections A # X? In that case I think we can also simply drop the behavior and the tests. Such projections are quite rare. For instance the dotty code base does not have a single occurrence of #. So I personally would not care at all what kind of hovers I get. And for others it would be a good indicator to change the design, since IME relying on # usually does not end well.

I believe the only reason to use A # B is if both A and B are classes and we want to model references to inner classes in the Java style. Otherwise, if you have to resort to it, it usually means your design is wrong. Maybe we should do a linter rule for that.

@rochala
Copy link
Contributor

rochala commented Mar 13, 2024

Does it just apply to # projections A # X?
It seems like this is the case.

This functionality was only tested in one suite, the one that failed, so I can say it wasn't truly supported by presentation compiler. This also impacts generated semanticdb output.

Feel free to add @Ignore for those tests. I'll create the ticket for this after this PR is merged so we don't lose a track of this.
Other than that, I think the review of the solution should be done by someone more experienced.

odersky added 2 commits March 13, 2024 16:58
Projections P # X are types that we would like to avoid. If X
is a class type, there's nothing we can do. If X is an abstract type,
we use skolemization and rewrite to (x?: P).X. If X is an alias
type we should simply dealias but this was not done before.

This caused an exonential blowup in scala#19892, where we costructed types
of the form

   ZPartialServerEndpoint[R, A, B, I, E, O, -C] # EndpointType[A, I, E, T, R] ... # EndpointType[A, I, E, T, R]

When the were 5 or more such selections, sompile times blew up (33s for 5, timeout after 5 minutes for 6).
I am still not qute sure where the blowup happened. Looking at stacktraces of random interrups
it seemed to be in a deep recursion of memberDenot and asSeenFrom calls.I believe it would still
be interesting to find out more about this, in case there are other similar situations where combinations
of deep projections with wide applications cannot be avoided.

But for this precise problem, eagerly dealising fixes it.
@odersky
Copy link
Contributor Author

odersky commented Mar 14, 2024

test performance please

@odersky odersky assigned mbovel and unassigned rochala Mar 20, 2024
@odersky
Copy link
Contributor Author

odersky commented Mar 20, 2024

We should get benchmarks back before we can merge this. I want to rule out slowdowns in some scenarios.

@mbovel
Copy link
Member

mbovel commented Mar 21, 2024

test performance please

@dottybot
Copy link
Member

performance test scheduled: 149 job(s) in queue, 0 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/19931/ to see the changes.

Benchmarks is based on merging with main (a36849f)

@odersky odersky assigned smarter and unassigned mbovel Mar 22, 2024
@odersky
Copy link
Contributor Author

odersky commented Mar 22, 2024

So performance seems to not worse and maybe a bit better. @smarter do you want to give this q quick review?

@odersky odersky requested a review from smarter March 22, 2024 10:07
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

Comment on lines +2714 to +2717
* class type, reduce it to the dealiasd version of P # X. This means that at typer
* we create projections only for inner classes with class prefixes, since projections
* of P # X where X is an abstract type are handled by skolemization. At later phases
* these projections might arise, though.
Copy link
Member

Choose a reason for hiding this comment

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

We can also create projections where the prefix is a type alias (they're not always reduced, e.g. when the alias has a different number of type parameters than the aliasee), which can trigger the performance cliff again:

abstract class ZPartialServerEndpoint[R, A, B, I, E, O, -C]
    extends EndpointOps[A, I, E, O, C]{
  override type ThisType[-_R] = ZPartialServerEndpoint[R, A, B, I, E, O, _R]
  override type EndpointType[_A, _I, _E, _O, -_R] =ZPartialServerEndpoint[R, _A, B, _I, _E, _O, _R]
}

trait EndpointOps[A, I, E, O, -R] {
  type EndpointType[_A, _I, _E, _O, -_R]
  type ThisType[-_R]
  def out[T]: EndpointType[A, I, E, T, R]
  def description(d: String): ThisType[R]
}

object Test {
  type Alias[R, A, B, I, E, O] = ZPartialServerEndpoint[R, A, B, I, E, O, Any]
  def basicEndpoint[R](): Alias[R, Any, Any, Unit, Any, Unit] = ???

  // commonts next to `.out[Any]` contain information about compilation time when chaining up to N `out` functions
  val case1 =
     basicEndpoint() // 1.5s
        .out[Any] // 1.6s
        .out[Any] // 1.7s
        .out[Any] // 2s
        .out[Any] // 4s
        .out[Any] // 33s
        .out[Any] // aborted after 5 min
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I am not sure we can dealias without breaking stuff here. It is a super low-level code.

@smarter smarter assigned odersky and unassigned smarter Mar 26, 2024
@odersky odersky merged commit c251f36 into scala:main Mar 27, 2024
19 checks passed
@odersky odersky deleted the fix-19892 branch March 27, 2024 21:15
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
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.

Project compiles in 18 seconds on scala 2, compilation never ends on scala 3
6 participants