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

Stop tuple adaptation earlier #64

Open
wants to merge 1 commit into
base: 2.12.x
Choose a base branch
from
Open

Conversation

retronym
Copy link
Owner

@retronym retronym commented Aug 22, 2019

This means less for the compiler:

class C(x: Boolean) {
    def this() = this(false)
}

class Test {
  new C()
}

Before, this incurs an implicit search from Unit => Boolean.

This change breaks (at least) this test:

/Users/jz/code/scala-2.12.x/test/files/neg/t8035-no-adapted-args.scala

-t8035-no-adapted-args.scala:4: error: too many arguments (3) for method f: (x: T)Int
+t8035-no-adapted-args.scala:4: warning: No automatic adaptation here: use explicit parentheses.
+        signature: Test.f[T](x: T): Int
+  given arguments: 1, 2, 3
+ after adaptation: Test.f((1, 2, 3): (Int, Int, Int))
+  f(1, 2, 3)
+   ^
+t8035-no-adapted-args.scala:4: error: too many arguments (3) for method f: (x: (Int, Int, Int))Int
   f(1, 2, 3)
        ^
-t8035-no-adapted-args.scala:5: error: not enough arguments for method f: (x: T)Int.
+t8035-no-adapted-args.scala:5: warning: No automatic adaptation here: use explicit parentheses.
+        signature: Test.f[T](x: T): Int
+  given arguments: <none>
+ after adaptation: Test.f((): Unit)
+  f()
+   ^
+t8035-no-adapted-args.scala:5: error: not enough arguments for method f: (x: Unit)Int.
 Unspecified value parameter x.
   f()
    ^
+two warnings found
 two errors found

This means less for the compiler:

```
class C(x: Boolean) {
    def this() = this(false)
}

class Test {
  new C()
}
```

Before, this incurs an implicit search from `Unit => Boolean`.

This change breaks (at least one test:

/Users/jz/code/scala-2.12.x/test/files/neg/t8035-no-adapted-args.scala
@retronym
Copy link
Owner Author

/cc @hrhino @mkeskells

@retronym
Copy link
Owner Author

Previous work in this area:

scala@4e488a6
scala#3872

@retronym
Copy link
Owner Author

retronym commented Aug 22, 2019

Here's a contrived benchmark:

class C(x: Boolean) {
    def this() = this(false)
}

class Test {
  new C
  new C
  new C
  new C
  new C
  new C
 
   // etc, etc
}

Before:

for v in 2.12.10-bin-c2a2230-SNAPSHOT 2.12.10-bin-7ff6674-SNAPSHOT; do sbt 'set scalaVersion in compilation := "'$v'"'  'hot -psource=/tmp/test.scala -f1 -president=true -pextraArgs=-Yno-adapted-args';
[info] HotScalacBenchmark.compile                     ../corpus           latest  -Yno-adapted-args        true  2.12.10-bin-c2a2230-SNAPSHOT  /tmp/test.scala  sample  1096   91.610 ± 0.436  ms/op

After:

[info] HotScalacBenchmark.compile                     ../corpus           latest  -Yno-adapted-args        true  2.12.10-bin-c2a2230-SNAPSHOT  /tmp/test.scala  sample  1096   91.610 ± 0.436  ms/op
2.12.10-bin-7ff6674-SNAPSHOT  /tmp/test.scala  sample  1843  54.434 ± 0.289  ms/op

@mkeskells
Copy link

mkeskells commented Aug 22, 2019

That seems a decent win, for the micro-benchmark at least
I presume this case also affects overload resolution generically, not just for ctor case

So do I read this correctly? - this would save approx. 6ms for each occurrence in source code, and probably some allocation reduction in some cases as well (91-54)/6. Please confirm this reading
That's much more than I expected, happy to take it though

Are any of the standard benchmarks (scalac, better-files etc) relying on -Yno-adapted-args=false?

If you can compile scalac with -Yno-adapted-args=true and it warning free, then we could see what the effect is in more real setting

What is the lightbend view on this? Is this a think that we have to put in a separate flag for avoid breaks
-Yno-adapted-args-early-error=true

@mkeskells
Copy link

I presume that the saving would vary depending on the number of implicits in scope, and there is a search for Unit=>Boolean going on here

@retronym
Copy link
Owner Author

I had 4608 new C-s i that benchmark, so (91-54ms)/4608 = 0.0089ms/occurence. Probably imperceptible in a non-contrived benchmark. You are right that it would vary based on implicits in scope.

@mkeskells
Copy link

Haha it did sound too good to be true as an absolute value still 9 us is a good tick.

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.

2 participants