10000 WIP: Add prototype improving default args encoding by jvican · Pull Request #5641 · scala/scala · GitHub
[go: up one dir, main page]

Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

jvican
Copy link
Member
@jvican jvican commented Jan 13, 2017

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:

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

  1. Generate more tests for the default arguments in class.

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.
@scala-jenkins scala-jenkins added this to the 2.12.2 milestone Jan 13, 2017
@jvican
Copy link
Member Author
jvican commented Jan 13, 2017

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?

@Jasper-M
Copy link
Contributor

My idea to tackle this issue is to save the results of the invocation in vals before the the call-site invocation.

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

@sjrd
Copy link
Member
sjrd commented Jan 16, 2017

That's already what happens in the old implementation, no?

Yes, and it is actually important for correct 8000 ness, in case the evaluation of the arguments and/or default accessors is non-pure.

@jvican
Copy link
Member Author
jvican commented Jan 16, 2017

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.

@adriaanm adriaanm modified the milestones: 2.12.2, WIP Feb 8, 2017
@jvican
Copy link
Member Author
jvican commented Feb 21, 2018

I'll be rebasing this change on 2.13.x soon.

@adriaanm
Copy link
Contributor
adriaanm commented Jun 1, 2018

I'm closing this PR because it has been dormant for a while now. Happy to receive a resubmit when ready!

@adriaanm adriaanm closed this Jun 1, 2018
@jvican
Copy link
Member Author
jvican commented Jun 1, 2018

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!

@SethTisue SethTisue removed this from the WIP milestone Jun 6, 2018
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.

6 participants
0