Skip to content

Fix #3250: Make HKLambda a cached type #3287

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

Merged
merged 3 commits into from
Oct 9, 2017
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 8, 2017

This is a partial revert from #3239. #3239 made both HKLambdas and
PolyTypes uncached. It turns out that this breaks things if we do it
for HKLambdas. I have not yet figured out why, however. The test case
in #3250 is a bit too large to be able to tell.

This is a partial revert from scala#3239. scala#3239 made both HKLambdas and
PolyTypes uncached. It turns out that this breaks things if we do it
for HKLambdas. I have not yet figured out why, however. The test case
in scala#3250 is a bit too large to be able to tell.
It's more efficient, and it also makes the logic in ProtoTypes#constrained
independent of whether type lambdas are cached or uncached.
@odersky
Copy link
Contributor Author

odersky commented Oct 8, 2017

test performance please

@dottybot
Copy link
Member

dottybot commented Oct 8, 2017

performance test scheduled: 1 job(s) in queue, 0 running.

@odersky odersky requested a review from smarter October 8, 2017 18:55
@dottybot
Copy link
Member

dottybot commented Oct 8, 2017

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3287/ to see the changes.

Benchmarks is based on merging with master (4460551)

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

I would rename SimpleMap to SimpleIdentityMap to avoid confusion.

case that: HKLambda =>
paramNames.equals(that.paramNames) &&
paramInfos.equals(that.paramInfos) &&
resType.equals(that.resType) &&
Copy link
Member

Choose a reason for hiding this comment

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

That was already the case before #3239, but why is this equals and not eq?

Copy link
Contributor Author

@odersky odersky Oct 9, 2017

Choose a reason for hiding this comment

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

I am not even sure that equals is needed. But the idea is that, since many types get a compiler generated equals since they are case classes, the rest of the types would conform to that.

@@ -41,13 +41,13 @@ object SimpleMap {
class Map1[K <: AnyRef, +V >: Null <: AnyRef] (k1: K, v1: V) extends SimpleMap[K, V] {
Copy link
Member

Choose a reason for hiding this comment

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

Above, there is a definition def contains(k: K): Boolean = apply(k) != null which could use ne instead of !=.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For null comparisons != and ne produce exactly the same bytecode.

@smarter smarter merged commit 66a0e28 into scala:master Oct 9, 2017
@allanrenucci allanrenucci deleted the fix-#3250 branch December 14, 2017 19:18
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