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

Instantiate constraint entries when updated to equal bounds #20208

Closed
wants to merge 3 commits into from
Closed

Instantiate constraint entries when updated to equal bounds #20208

wants to merge 3 commits into from

Conversation

EugeneFlesselle
Copy link
Contributor

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

Adapted from #20200
@jchyb can you test if this also addresses the performance issue in #20120 ?

@EugeneFlesselle
Copy link
Contributor Author

These changes also solve #19955 independently of the changes in #20175, with I think a much more general solution.

@jchyb
Copy link
Contributor

jchyb commented Apr 17, 2024

Sure, let me check

@jchyb
Copy link
Contributor

jchyb commented Apr 17, 2024 •
edited
Loading

It doesn't seem to fix the performance here, unfortunately (typer still takes vastly more time in T1, which is the val that summons the implicit with the intersection type which causes the TypeBounds constraint)
Zrzut ekranu 2024-04-17 o 12 22 59

@EugeneFlesselle
Copy link
Contributor Author

EugeneFlesselle commented Apr 17, 2024 •
edited
Loading

I couldn't figure out how to test the changes with -Yprofile-trace.
Async profiler reports a 59% improvement in the Typer phase though.

@jchyb could you run it again ?

@EugeneFlesselle EugeneFlesselle changed the title [WIP] Retry constraint.replace after constraint.updateEntry Collapse equal bounds when replacing parameter in ordering constraint Apr 17, 2024
@EugeneFlesselle EugeneFlesselle marked this pull request as ready for review April 17, 2024 21:40
odersky
odersky approved these changes Apr 17, 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!

@jchyb
Copy link
Contributor

jchyb commented Apr 18, 2024 •
edited
Loading

There is a bit of an improvement:
Zrzut ekranu 2024-04-18 o 11 59 48
(T1 takes 130ms)
where before this change T1 took 200ms:
Zrzut ekranu 2024-04-18 o 12 00 02
but I still get the best results when putting case TypeBounds(lo, hi) if lo eq hi => lo in instType (which might be incorrect of course, but serves as a nice point of comparison):
Zrzut ekranu 2024-04-18 o 15 56 58
(the screenshots aren't to scale, T1 here takes 40ms - but it's very possible I may be doing something wrong here of course)

@EugeneFlesselle
Revert to equal bounds when typeVarOfParam is unknown
6cb6d38 
See sbt-test/source-dependencies/inline-rec-change-typaram for an example
@EugeneFlesselle
Copy link
Contributor Author

Perfomance measurements:

Before changes:

  • T0: 52ms
  • T1, T1bis: 189, 79 ms
  • T2, T2bis: 5, 4 ms

After changes:

  • T0: 28ms
  • T1, T1bis: 13, 25 ms
  • T2, T2bis: 9, 6 ms

@EugeneFlesselle EugeneFlesselle changed the title Collapse equal bounds when replacing parameter in ordering constraint Instantiate constraint entries when updated to equal bounds Apr 22, 2024
@EugeneFlesselle EugeneFlesselle mentioned this pull request May 13, 2024
Merged
WojciechMazur pushed a commit that referenced this pull request Jul 8, 2024
@EugeneFlesselle @WojciechMazur
Do constraint.replace when addOneBound produces equal bounds
e1aeea5 
as an optimization

In #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.

Additionally, we recheck for equal bounds after `constraint.updateEntry`
as checking `isSub` can have narrowed the bounds further.
#19955 is an example where this second optimization applies.

Fix #20120
Close #20208 the original implementation
[Cherry-picked c608177]
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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EugeneFlesselle @jchyb @odersky