-
Notifications
You must be signed in to change notification settings - Fork 3.1k
WIP: Add prototype improving default args encoding #5641
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
Conversation
The following changes implement a more flexible encoding of default args that allows default implementation to depend on parameters defined on the left (either previous parameters lists or parameters in the same parameter list defined on the left). This encoding breaks binary compatibility so it targets 2.13. It consists of changes in the synthetic method generation of defaults and the call sites of these methods. The implementation of synthetic methods now depends on any parameter defined at the left of a default arg, even if it their body does not use these parameters. This is done so that changing the body of the default args does not change the signature and hence break binary compatibility. More explanation on this implementation can be found in the [SIP proposal](scala/docs.scala-lang#653). WIP: This commit still needs more work. This is what's missing: 1. Reusing the results of default args. Currently, invocations to default methods are inlined at the call-site and a lot of invocations can be reused. Declaring more than one default arg considerably increases bytecode size. My idea to tackle this issue is to save the results of the invocation in vals before the the call-site invocation. Example: ``` // definition def foo(a: Int, b: Int = a, c: Int = a + b): Int = a + b + c // invocation foo(1) ``` will modify the call-site like: ``` foo(1, foo$default$2(1), foo$default$3(1, foo$default$2(1))) ``` As you see, invocations to 1 and `foo$default$2` could be optimized: ``` val a$default = 1 val b$default = foo$default$2(a$default) val c$default = foo$default$3(a$default, b$default) foo(a$default, b$default, c$default) ``` This generates more bytecode for call-sites omitting default arguments, but avoids unnecessary calls that could cause bad performance (especially if more than 2 default args are defined in a method). 2. Generate more tests for the default arguments in class.
I pushed these changes although they are work in progress because I was not able to bootstrap the compiler correctly and recompile all the dependencies with the incompatible compiler version. Could someone set Jenkins to do that and schedule this to land in 2.13? |
That's already what happens in the old implementation, no? scala> def foo(a: Int, b: Int)(c: Int = a + b): Int = a + b + c
foo: (a: Int, b: Int)(c: Int)Int
scala> foo(1,2)() //print
{
val x$1: Int = 1;
val x$2: Int = 2;
val x$3: Int = $line12.$read.$iw.$iw.foo$default$3(x$1, x$2);
$line12.$read.$iw.$iw.foo(x$1, x$2)(x$3)
} // : Int |
Yes, and it is actually important for correct 8000 ness, in case the evaluation of the arguments and/or default accessors is non-pure. |
Yeah @Jasper-M, didn't know typer was already taking care of this! 🎉 I'll update this PR with more tests for the class case and will refine the implementation. |
I'll be rebasing this change on 2.13.x soon. |
I'm closing this PR because it has been dormant for a while now. Happy to receive a resubmit when ready! |
I'll try to schedule a review on this one in one of our upcoming SIP meetings and, when we have some more resolution, resubmit! |
The following changes implement a more flexible encoding of default args
that allows default implementation to depend on parameters defined on
the left (either previous parameters lists or parameters in the same
parameter list defined on the left).
This encoding breaks binary compatibility so it targets 2.13. It
consists of changes in the synthetic method generation of defaults and
the call sites of these methods. The implementation of synthetic
methods now depends on any parameter defined at the left of a default arg,
even if it their body does not use these parameters. This is done so
that changing the body of the default args does not change the signature
and hence break binary compatibility.
More explanation on this implementation can be found in the SIP
proposal.
WIP: This commit still needs more work. This is what's missing:
default methods are inlined at the call-site and a lot of invocations
can be reused. Declaring more than one default arg considerably
increases bytecode size. My idea to tackle this issue is to save the
results of the invocation in vals before the the call-site invocation.
Example:
will modify the call-site like:
As you see, invocations to 1 and
foo$default$2
could be optimized:This generates more bytecode for call-sites omitting default arguments,
but avoids unnecessary calls that could cause bad performance
(especially if more than 2 default args are defined in a method).