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.

Sign up for GitHub

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

Jump to bottom

Retry constraint.replace after constraint.updateEntry #20399

Merged
merged 4 commits into from
May 16, 2024
Merged

Retry constraint.replace after constraint.updateEntry #20399

merged 4 commits into from
May 16, 2024

Conversation

EugeneFlesselle
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle commented May 13, 2024 •
edited
Loading

Checking isSub on the resulting bounds can have introduced new constraints, which might allow us to replace the type parameter entirely.

Fix #20120
Close #20208 the original implementation

@EugeneFlesselle
Retry constraint.replace after constraint.updateEntry
49940ee 
Checking `isSub` on the resulting bounds can have introduced new constraints,
which might allow us to replace the type parameter entirely.
@EugeneFlesselle EugeneFlesselle marked this pull request as ready for review May 14, 2024 14:37
odersky
odersky approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

LGTM. Let's hope it addresses the performance to some degree.

@odersky
Copy link
Contributor

odersky commented May 14, 2024

@jchyb Can you also take a look and see how it performs? Thank you.

@odersky odersky requested a review from jchyb May 14, 2024 16:36
@jchyb
Copy link
Contributor

jchyb commented May 15, 2024

I can't see any difference in how the reproduction performs, unfortunately. I tried printing out what's happening here and it looks like in all of the problematic cases in the reproduction the upper bound starts out as Any and is being narrowed into Repro.ObjectId and then into Repro.ObjectId & Endpoints.this.Foo (the lower bound is narrowed before from Nothing into Repro.ObjectId & Endpoints.this.Foo). So the optimization here does not trigger for those

@odersky
Copy link
Contributor

odersky commented May 15, 2024

Are the two Repro.ObjectId & Endpoints.this.Foo bounds identical (wrt eq)?

@EugeneFlesselle
Fix: don't use oldBounds to re-tryReplace
97ed37e
@EugeneFlesselle
Do tryReplace using narrowedBounds instead of legalBound
14a6cac 
In scala#20120, we reach constraints with equal bounds that are intersection types,
they are formed from multiple successive calls to `addOneBound`.

We miss the `replace` optimization in this case because
the bounds only become equal progressively, and
we are only checking for equality with the constraint being added.

The second tryReplace after updateEntry and isSub
does not address this specific issue but scala#19955.
@EugeneFlesselle
Copy link
Contributor Author

Are the two Repro.ObjectId & Endpoints.this.Foo bounds identical (wrt eq)?

They are, but bound was actually never the intersection type at all. The issue was that we obtained the intersection type only after computing the narrowedBounds, which we did not use for the equal bounds check.

@EugeneFlesselle
Copy link
Contributor Author

@jchyb could you measure it once more ? Sorry about the numerous attempts, I do hope this'll be the last one. Thanks again!

@jchyb
Copy link
Contributor

jchyb commented May 16, 2024

Yes! It looks completely fixed (only 52 ms on T1!):
Zrzut ekranu 2024-05-16 o 11 05 20
For comparison, previously it was something like this (with 315ms):
Zrzut ekranu 2024-05-16 o 11 05 43
Thank you so much for fixing this!

@EugeneFlesselle
Copy link
Contributor Author

Yes! It looks completely fixed (only 52 ms on T1!)

Great, glad to hear it !

Thank you so much for fixing this!

Your minimisations were very helpful too

@EugeneFlesselle
Copy link
Contributor Author

@odersky still good to merge ?

odersky
odersky approved these changes May 16, 2024
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

LGTM

@EugeneFlesselle EugeneFlesselle enabled auto-merge (squash) May 16, 2024 20:39
@EugeneFlesselle EugeneFlesselle merged commit c608177 into scala:main May 16, 2024
17 checks passed
@EugeneFlesselle EugeneFlesselle deleted the constraint-replace-2 branch May 16, 2024 21:19
@WojciechMazur
Copy link
Contributor

Wonderfull news about fixing the issue! Great job! I'll try it tomorrow on whole project to check how it applies on the big scale and how it now compares to Scala 2.13

@WojciechMazur WojciechMazur mentioned this pull request May 17, 2024
Closed
@dwijnand
Copy link
Member

Nice fix, @EugeneFlesselle !

@Kordyjan Kordyjan added this to the 3.5.1 milestone Jul 3, 2024
@Gedochao Gedochao mentioned this pull request Jul 4, 2024
Closed
@WojciechMazur WojciechMazur mentioned this pull request Jul 8, 2024
Merged
WojciechMazur added a commit that referenced this pull request Jul 8, 2024
@WojciechMazur
Backport "Retry constraint.replace after constraint.updateEntry" …
7a5f48a 
…to LTS (#21111)

Backports #20399 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
WojciechMazur added a commit that referenced this pull request Jul 8, 2024
@WojciechMazur
Backport "Adapt toInstantiate in interpolateTypeVars to consider #20399…
f2e61ce 
…" to LTS (#21112)

Backports #20426 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@jchyb jchyb mentioned this pull request Sep 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky approved these changes

@jchyb jchyb Awaiting requested review from jchyb

Assignees

@EugeneFlesselle EugeneFlesselle

Labels
None yet
Projects
None yet
Milestone
3.5.1
Development

Successfully merging this pull request may close these issues.

Slow compilation times when inferring type HKT with intersection types
6 participants
@EugeneFlesselle @odersky @jchyb @WojciechMazur @dwijnand @Kordyjan