Merged
Conversation
Be conservative in our changes, but at least use `=:=` when comparing types, so that an eta-expanded type constructor and its 0-arg type ref compare equal. Since there will be a complete overhaul of type inference in dotty, let's not cause small fluctuations here in the mean time. Type inference hasn't changed much in 13 years, so it seems unwise to start tweaking quirks users have come to rely on. Also, don't do `tparam.subst(tparams, tvars)` -- iterate over `tparams` and `tvars` simultaneously, and just say `tvar`. Add a `configForeach` combinator that iterates over arrays, in hopes of getting a bit more locality. It would be nice if its HOF was inlined, but it doesn't seem to happen currently. Co-authored-by: Georgi Krastev <joro.kr.21@gmail.com> Co-authored-by: Miles Sabin <miles@milessabin.com>
561a11c to
2c19d5d
Compare
Member
|
You might want to add a test from #6333 |
When solving a list of type variables, bounds between them should not be added as constraints when they are of different kinds. The bug was fixed in the cleanup in the parent commit, which was inspired by the work by joroKr21
lrytz
approved these changes
Apr 15, 2018
| val _tparams = tparams.toArray | ||
| val _variances = variances.toArray | ||
| val _len = _tvars.length | ||
| // TODO: can we make this reliably inline its HOF argument when it's a function? |
Member
There was a problem hiding this comment.
What made you conclude it's not inlined? I did
../build/quick/bin/scalac -opt-warnings -opt:l:inline '-opt-inline-from:**' ../src/reflect/scala/reflect/internal/tpe/TypeConstraints.scala -Yopt-log-inline _
and see
Inline into scala/reflect/internal/tpe/TypeConstraints.solve: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 65 ins, inlined: 37 ins.
Inline into scala/reflect/internal/tpe/TypeConstraints.solveOne$1: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 323 ins, inlined: 37 ins.
Inline into scala/reflect/internal/tpe/TypeConstraints.solveOne$1: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 365 ins, inlined: 37 ins.
Inline into scala/reflect/internal/tpe/TypeConstraints.solveOne$1: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 408 ins, inlined: 37 ins.
Running cfr, I see all the inlined while loops and no more closure allocations.
Contributor
Author
|
I used the enableOptimizer setting from the build. Looks like they are
different from what you used, IIRC
…On Sun, Apr 15, 2018 at 12:53 Lukas Rytz ***@***.***> wrote:
***@***.**** approved this pull request.
------------------------------
In src/reflect/scala/reflect/internal/tpe/TypeConstraints.scala
<#6492 (comment)>:
> @@ -191,82 +191,92 @@ private[internal] trait TypeConstraints {
* @param upper When `true` search for max solution else min.
*/
def solve(tvars: List[TypeVar], tparams: List[Symbol], variances: List[Variance], upper: Boolean, depth: Depth): Boolean = {
+ // We're going to be iterating over these a lot. Let's put them in an array.
+ val _tvars = tvars.toArray
+ val _tparams = tparams.toArray
+ val _variances = variances.toArray
+ val _len = _tvars.length
+ // TODO: can we make this reliably inline its HOF argument when it's a function?
What made you conclude it's not inlined? I did
../build/quick/bin/scalac -opt-warnings -opt:l:inline '-opt-inline-from:**' ../src/reflect/scala/reflect/internal/tpe/TypeConstraints.scala -Yopt-log-inline _
and see
Inline into scala/reflect/internal/tpe/TypeConstraints.solve: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 65 ins, inlined: 37 ins.
Inline into scala/reflect/internal/tpe/TypeConstraints.solveOne$1: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 323 ins, inlined: 37 ins.
Inline into scala/reflect/internal/tpe/TypeConstraints.solveOne$1: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 365 ins, inlined: 37 ins.
Inline into scala/reflect/internal/tpe/TypeConstraints.solveOne$1: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 408 ins, inlined: 37 ins.
Running cfr, I see all the inlined while loops and no more closure
allocations.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6492 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFjy25GY6SxFJmpb77winBiSeZzDPvFks5tozTPgaJpZM4TILkE>
.
|
Contributor
Author
|
My bad. I forgot that we don’t inline the method that implements the
closure body. The closure allocation was indeed gone.
…On Mon, Apr 16, 2018 at 09:36 Adriaan Moors ***@***.***> wrote:
I used the enableOptimizer setting from the build. Looks like they are
different from what you used, IIRC
On Sun, Apr 15, 2018 at 12:53 Lukas Rytz ***@***.***> wrote:
> ***@***.**** approved this pull request.
> ------------------------------
>
> In src/reflect/scala/reflect/internal/tpe/TypeConstraints.scala
> <#6492 (comment)>:
>
> > @@ -191,82 +191,92 @@ private[internal] trait TypeConstraints {
> * @param upper When `true` search for max solution else min.
> */
> def solve(tvars: List[TypeVar], tparams: List[Symbol], variances: List[Variance], upper: Boolean, depth: Depth): Boolean = {
> + // We're going to be iterating over these a lot. Let's put them in an array.
> + val _tvars = tvars.toArray
> + val _tparams = tparams.toArray
> + val _variances = variances.toArray
> + val _len = _tvars.length
> + // TODO: can we make this reliably inline its HOF argument when it's a function?
>
> What made you conclude it's not inlined? I did
>
> ../build/quick/bin/scalac -opt-warnings -opt:l:inline '-opt-inline-from:**' ../src/reflect/scala/reflect/internal/tpe/TypeConstraints.scala -Yopt-log-inline _
>
> and see
>
> Inline into scala/reflect/internal/tpe/TypeConstraints.solve: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 65 ins, inlined: 37 ins.
> Inline into scala/reflect/internal/tpe/TypeConstraints.solveOne$1: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 323 ins, inlined: 37 ins.
> Inline into scala/reflect/internal/tpe/TypeConstraints.solveOne$1: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 365 ins, inlined: 37 ins.
> Inline into scala/reflect/internal/tpe/TypeConstraints.solveOne$1: inlined scala/reflect/internal/tpe/TypeConstraints.configForeach$1. Before: 408 ins, inlined: 37 ins.
>
> Running cfr, I see all the inlined while loops and no more closure
> allocations.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#6492 (review)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAFjy25GY6SxFJmpb77winBiSeZzDPvFks5tozTPgaJpZM4TILkE>
> .
>
|
adriaanm
added a commit
to adriaanm/scala
that referenced
this pull request
Jun 5, 2019
I don't have a great explanation, but the =:= on detecting occurrences of type params in bounds was too eager. When the info of a type param is a WildcardType, it matches anything and type variables get overconstrained. Discovered while looking into why this doesn't type check: I noticed that there was cross talk between the type params of java.util.Function in .
adriaanm
added a commit
to adriaanm/scala
that referenced
this pull request
Jun 7, 2019
Use == instead of =:= because we're looking for occurs, not equality. When we were using =:= to detect occurrences of type params in bounds, we'd accidentally unify too much (e.g. on wildcards / other type vars). Discovered while looking into why this doesn't type check: `java.util.stream.Stream.of(1,2,3).map(_.toString)` (scala/bug 11558) I noticed that there was cross talk between the type params of java.util.Function during type inference.
adriaanm
added a commit
to adriaanm/scala
that referenced
this pull request
Jun 14, 2019
Use == instead of =:= because we're looking for occurs, not equality. When we were using =:= to detect occurrences of type params in bounds, we'd accidentally unify too much (e.g. on wildcards / other type vars). Discovered while looking into why this doesn't type check: `java.util.stream.Stream.of(1,2,3).map(_.toString)` (scala/bug 11558) I noticed that there was cross talk between the type params of java.util.Function during type inference.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Be conservative in our changes, but at least use
=:=whencomparing types, so that an eta-expanded type constructor
and its 0-arg type ref compare equal.
Since there will be a complete overhaul of type inference in dotty,
let's not cause small fluctuations here in the mean time.
Type inference hasn't changed much in 13 years, so it seems
unwise to start tweaking quirks users have come to rely on.
Also, don't do
tparam.subst(tparams, tvars)-- iterateover
tparamsandtvarssimultaneously, and just saytvar.Add a
configForeachcombinator that iterates over arrays,in hopes of getting a bit more locality. It would be nice if
its HOF was inlined, but it doesn't seem to happen currently.
Fixes scala/bug#8602 and fixes scala/bug#10722