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

Fix #7512: Normalize type arguments before instantiation #14259

Merged

Conversation

OlivierBlanvillain
Copy link
Contributor

@OlivierBlanvillain OlivierBlanvillain commented Jan 12, 2022

This PR fixes a long-lasting issue with type normalization that led to unnecessary exponential compilation time. I recently wrote some match type benchmarks and managed to reproduce this issue in a more controlled environment, which lead to this PR.

To summarize the issue, which is hard to observe without looking at the compiler internal, here is a simple example that exercises the exponential behavior:

// GEQ[5, 1] compiles, because 5 is greater than 1
type GEQ[A, B] = A match {
  case B => true
  case _ => GEQ[A, B + 1]
}

The issue with this code is we try to normalize B + 1 first too early, and then too late. We normalize type at creating, but at that point B is abstract and B + 1 cannot be normalized. Then, we normalize during subtyping, but that's a bit late. Here are the queries that will be sent to type comparer for GEQ[5, 1]:

5 <: 1? no
5 <: 1+1? no
5 <: 1+1+1? no
5 <: 1+1+1+1? no
5 <: 1+1+1+1+1? yes

Which results in a quadratic number of additions being computer... The solution is to normalize types when doing substitution, so that each addition is computed once and for all at each set of the recursion.

Fixes #7512

The newly added test is blacklisted for pickling tests because the after
pickler code path doesn't go thought TypeApplications and keeps types
un-normalized, albeit equivalant to their before pickling counterparts.
@mbovel
Copy link
Member

mbovel commented Jan 14, 2022

I ran two of my benchmarks on this PR and did not observe an improvement on their runtimes. However, I discovered a big performance regression after #13400.

The two benchmarks I ran are:

  • sum-constats: big compile-time sums of constant integer types (1.type + 2.type + …).
  • sum-termrefs: big compile-time sums of term reference types (one.type + n.type + m.type…, where one has type 1, and m and n have type Int).

The results are available here. The graph is zoomable and filterable.

For sum-termrefs, there is a clear ~10x slowdown after #13400:

Screen Shot 2022-01-14 at 15 35 18

Zooming a bit on the last 3 measured commits, we see that this PR did not solve the slowdown for this benchmark but instead introduced a minor ~3% additional slowdown.

Screen Shot 2022-01-14 at 15 35 30

For sum-constants, we can also observe a slowdown after #13400, although way smaller: ~5%. And this PR does not have an observable impact on this benchmark.

Screen Shot 2022-01-14 at 15 35 00

@mbovel mbovel assigned OlivierBlanvillain and unassigned mbovel Jan 14, 2022
@mbovel
Copy link
Member

mbovel commented Jan 14, 2022

The PR looks good to me! My benchmarks most likely show a different problem that can be tackled separately.

@OlivierBlanvillain OlivierBlanvillain merged commit 2849aed into scala:master Jan 19, 2022
@OlivierBlanvillain OlivierBlanvillain deleted the fix-overly-lazy-normalization branch January 19, 2022 20:43
@Kordyjan Kordyjan added this to the 3.1.2 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.

Infinite loop for given ev with match types on dependently typed method args
3 participants