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

Instantiate constraint entries when updated to equal bounds #20208

Closed
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

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

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
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

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)

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
WojciechMazur pushed a commit that referenced this pull request Jul 8, 2024
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants