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

Use invokedynamic for structural calls, symbol literals, lambda ser. #4896

Merged
merged 1 commit into from Feb 12, 2016

Conversation

Projects
None yet
7 participants
@retronym
Member

retronym commented Jan 7, 2016

The previous encodings created static fields in the enclosing class
to host caches. However, this isn't an option once emit code in default
methods of interfaces, as Java interfaces don't allow static fields.

Luckily, we can emulate a static field by using invokedynamic: when
the call site is linked, our bootstrap methid can perform one-time
computation, and we can capture the result in the CallSite.

Review by @lrytz.

I can't remember why I needed to change visitHandle, but I know that the ugliness there is a symptom of a somewhat ad-hoc representation of indy calls in our trees. Ideally, we'd be able to come up with something general purpose, so that a macro author could emit an arbitrary indy call, without needing special cases in the backend for each bootstrap method.

@scala-jenkins scala-jenkins added this to the 2.12.0-M4 milestone Jan 7, 2016

@retronym retronym changed the title from Use invokedynamic for structural calls, symbol literals, lamba ser. to Use invokedynamic for structural calls, symbol literals, lambda ser. Jan 7, 2016

@DarkDimius

This comment has been minimized.

Show comment
Hide comment
@DarkDimius

DarkDimius Jan 7, 2016

Member

Java interfaces don't allow static fields

Java does allow public static final fields in interfaces:

public interface I {
  static public final Object field = null;
}
Member

DarkDimius commented Jan 7, 2016

Java interfaces don't allow static fields

Java does allow public static final fields in interfaces:

public interface I {
  static public final Object field = null;
}
@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 12, 2016

Member

Java does allow public static final fields in interfaces:

Oh, good to know. I'll keep that in mind as an option (we can simulate a non final field, where needed, with a mutable box).

I think there is an argument that the invokedynamic approach still has some merit, as it exposes fewer implementation details as public static fields.

Member

retronym commented Jan 12, 2016

Java does allow public static final fields in interfaces:

Oh, good to know. I'll keep that in mind as an option (we can simulate a non final field, where needed, with a mutable box).

I think there is an argument that the invokedynamic approach still has some merit, as it exposes fewer implementation details as public static fields.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 12, 2016

Member

I think it is also worthwhile to push forward to support invokedynamic generally in the backend, as we can use macros to expose it through to the user level.

Member

retronym commented Jan 12, 2016

I think it is also worthwhile to push forward to support invokedynamic generally in the backend, as we can use macros to expose it through to the user level.

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Jan 12, 2016

Member

Just pointing out that exposing invokedynamic at the user level would be a JVM-only feature. I'm pretty sure there's no way to implement such a thing in Scala.js, although I'm not very familiar with the details of invokedynamic.

Member

sjrd commented Jan 12, 2016

Just pointing out that exposing invokedynamic at the user level would be a JVM-only feature. I'm pretty sure there's no way to implement such a thing in Scala.js, although I'm not very familiar with the details of invokedynamic.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 14, 2016

Member

I can't remember why I needed to change visitHandle,

Removing the hack in visitHandle led to a test failure in t6287.scala.

Turns out that this was related to a problem under multi-run compilation. I'd cached the asm.Handle for bootstrap methods in a place in the backend that meant that on a subsequent compilation run, emitting the boostrap method reference didn't populate the ClassBType from a compiler Symbol. Just before writing the classfile to disk, the backend then attempted to load the class bytes from the compiler classpath instead to determine if it is a nested class. Under the toolbox global, however, we can't (in general) get to classfiles this way, so we hit an assertion failure.

I've pushed an updated version of this PR that moves the lazy vals into the part of the backend that is created for each Run.

Member

retronym commented Jan 14, 2016

I can't remember why I needed to change visitHandle,

Removing the hack in visitHandle led to a test failure in t6287.scala.

Turns out that this was related to a problem under multi-run compilation. I'd cached the asm.Handle for bootstrap methods in a place in the backend that meant that on a subsequent compilation run, emitting the boostrap method reference didn't populate the ClassBType from a compiler Symbol. Just before writing the classfile to disk, the backend then attempted to load the class bytes from the compiler classpath instead to determine if it is a nested class. Under the toolbox global, however, we can't (in general) get to classfiles this way, so we hit an assertion failure.

I've pushed an updated version of this PR that moves the lazy vals into the part of the backend that is created for each Run.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 14, 2016

Member

I've pushed another version with the generalization I talked about before. See the new test test/files/run/indy-via-macro for an example of a use case that can be solved with this facility.

Member

retronym commented Jan 14, 2016

I've pushed another version with the generalization I talked about before. See the new test test/files/run/indy-via-macro for an example of a use case that can be solved with this facility.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 15, 2016

Member

I'll bring this PR back soon.

Member

retronym commented Jan 15, 2016

I'll bring this PR back soon.

@retronym retronym closed this Jan 15, 2016

@retronym retronym reopened this Jan 15, 2016

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Jan 15, 2016

Member

LGTM!

Very nice work! The generalization is great, and trading -- in src/compiler for ++ in src/library is also excellent.

Member

lrytz commented Jan 15, 2016

LGTM!

Very nice work! The generalization is great, and trading -- in src/compiler for ++ in src/library is also excellent.

Use invokedynamic for structural calls, symbol literals, lamba ser.
The previous encodings created static fields in the enclosing class
to host caches. However, this isn't an option once emit code in default
methods of interfaces, as Java interfaces don't allow private static
fields.

We could continue to emit fields, and make them public when added to
traits.

Or, as chosen in this commit, we can emulate a call-site specific
static field by using invokedynamic: when the call site is linked,
our bootstrap methid can perform one-time computation, and we can
capture the result in the CallSite.

To implement this, I've allowed encoding of arbitrary invokedynamic
calls in ApplyDynamic.

The encoding is:

    ApplyDynamic(
       NoSymbol.newTermSymbol(TermName("methodName")).setInfo(invokedType)
       Literal(Constant(bootstrapMethodSymbol)) :: (
          Literal(Constant(staticArg0)) :: Literal(Constant(staticArgN)) :: Nil
       ) :::
       (dynArg0 :: dynArgN :: Nil)
    )

So far, static args may be `MethodType`, numeric or string literals, or
method symbols, all of which can be converted to constant pool entries.
`MethodTypes` are transformed to the erased JVM type and are converted
to descriptors as String constants.

I've taken advantage of this for symbol literal caching and
for the structural call site cache.

I've also included a test case that shows how a macro could target this
(albeit using private APIs) to cache compiled regexes.

I haven't managed to use this for LambdaMetafactory yet, not sure
if the facility is general enough.
@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 29, 2016

Member

Rebased.

Member

retronym commented Jan 29, 2016

Rebased.

retronym added a commit that referenced this pull request Feb 12, 2016

Merge pull request #4896 from retronym/topic/indy-all-the-things
Use invokedynamic for structural calls, symbol literals, lambda ser.

@retronym retronym merged commit a80f6a0 into scala:2.12.x Feb 12, 2016

5 checks passed

cla @retronym signed the Scala CLA. Thanks!
Details
integrate-ide [914] SUCCESS. Took 2 s.
Details
validate-main [1110] SUCCESS. Took 68 min.
Details
validate-publish-core [1096] SUCCESS. Took 13 min.
Details
validate-test [911] SUCCESS. Took 54 min.
Details

@adriaanm adriaanm added 2.12.0 and removed 2.12 labels Oct 29, 2016

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue
Member

SethTisue commented Nov 9, 2016

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Nov 9, 2016

Member

I guess it's fine as is since we still ultimately call the method reflectively.

Member

SethTisue commented Nov 9, 2016

I guess it's fine as is since we still ultimately call the method reflectively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment