Remove workarounds for sbt/sbt#1035 #393
Conversation
|
Note the ref to sbt/sbt#1035. |
|
|
||
| private[this] def convert(classes: Array[Class[_]]): Array[Type] = | ||
| classes.asInstanceOf[Array[Type]] // ok: treat Arrays as read-only | ||
| @inline private[this] def returnType(f: Field): Type = f.getGenericType |
There was a problem hiding this comment.
Does @inline annotation in Scala really help performance? - https://stackoverflow.com/questions/2709095/does-the-inline-annotation-in-scala-really-help-performance
There was a problem hiding this comment.
Yes, it does. Inlining is extremely important and it can cause slowdowns, but in Scala code it generally yields good benefits. For example, using @inline in certain places of the Scala compiler is critical for performance (an example is all the statistics infrastructure). JVM inlining is normally not enough for Scala code because it will only inline up to 135 bytes, IIRC, and this is why the careful use of @inline is important. I saw a tweet by Jason some weeks ago claiming that increasing this bytecode limit in the JDK helps performance for Scalac.
So, coming back to your question: yes, @inline has a real-world impact. It will not make the program 20% faster in this scenario, but it's a good practice to use @inline where applicable, and I think in this case it is.
There was a problem hiding this comment.
Note also that the use of @inline does not mean the Scala compiler will inline it. It may or may not, depending on some heuristics. But this doesn't prove my point wrong.
There was a problem hiding this comment.
@cb372 gave a whole talk on this topic. What was the verdict on this topic? -optimize is better than handcrafted @inline? http://slides.com/cb372/inline-specialized-berlin-2016#/27
There was a problem hiding this comment.
That's a benchmark of numerical computations. Those kind of benchmarks are really misleading and easy to optimize efficiently by the Hotspot, probably with not even inlining but also loop unfolding et al. To add to that, it doesn't say anything about the benchmark setup and 2.12.3 improved the optimizer and inliner, making those benchmarks (for 2.12.0-M2) useless. So I think that link has no relevance to the scenario we're dealing with here.
By the by, this scenario is clearly beneficial for inlining because we only have one call-site, and it's in the same class definition (unlike the benchmark you mention). The benefit is that we don't pay the cost of invoking these functions, while still making them logically independent if in the future we want to add more logic to them, like a fix for #389.
I think we would agree that Zinc is more similar to Scalac than to that benchmark. It is reasonable to think @inline here is beneficial, given how beneficial it is for the Scala compiler. If you want to keep having this discussion, I suggest you have it with the Scala team. They will give you better insights than I will.
That being said, it would be good that you comment on the core issue that this patch is addressing.
There was a problem hiding this comment.
That's cool. Can you LGTM and we merge?
There was a problem hiding this comment.
I'd like to get Scala team's opinion as you suggested in terms of:
- If it's effective to add
@inlinein non-optimized code, and - If it's safe to call reflection like
f.getGenericTypefrom an inline. The one-argument Class.forName must never be inlined scala/bug#6789 for example says "The one-argument Class.forName must never be inlined"
There was a problem hiding this comment.
Wrt the first question, we can merge this and address that doubt later on. I don't think it's a blocker.
Wrt your second question, it is safe to inline f.getGenericType. The reason why you cannot inline Class.forName is because this.getClass.getClassLoader or its absence has semantics that depends on the call-site (via stack inspection). This is absolutely not the case for these methods, that rely uniquely on the reflection API and don't take any class loader as parameter.
There was a problem hiding this comment.
Inlining getGenericType is probably safe, scala/bug#6789 a special case for forName. Note that, since 2.12.3, you have to be explicit in telling the inliner where it's allowed to inline from (scala/scala#5964).
Inlining from the JDK is probably not a good idea in general as the bytecode potentially becomes non-portable. There might be licensing issues too. Maybe that's not a problem here because that code is only compiled locally for every user and never distributed in binary.
@inline is not a hint to the heuristic, but it makes the optimizer inline unconditionally (if the method can be inlined). The heuristic applies for every method invocation.
Marking small forwarder methods @inline is usually not beneficial, the JVM handles small monomorphic methods just fine. The only potential issue with many small methods on a hot path is that they consume from the -XX:MaxInlineLevel budget.
Inlining in Scala has benefits when it can make the code easier for the VM to optimize, in particular when callsites become monomorphic or escape analysis / stack allocation starts working.
it's a good practice to use @inline where applicable
I don't agree here. For small methods like here there's no gain. For bigger methods, there's a risk of slowdown due to code size. Good practice is to use the annotation where performance benefits are measured (or known).
I once made a small experiment and removed all @inline annotations from the Scala compiler, and performance didn't change. It might be different now, we'd have to do it again. But I think the heuristics cover the main cases. As another data point, there's a stress test mode in the optimizer -Yopt-inline-heuristics:everything. If you build a compiler with that, it runs 2x slower.
In general when working on JVM performance optimization, measuring is really important. More often than not, just reasoning about it leads to changes that don't improve or even hurt performance - there are just too many factors in play.
There was a problem hiding this comment.
Inlining from the JDK is probably not a good idea
Oh, I got this wrong, there's no inlining from the JDK here.
jvican
force-pushed
the
classapi-improvements
branch
from
7ecc5df to
a0e4460
Compare
|
This PR has been updated and can be reviewed. |
The workarounds are not necessary anymore, http://bugs.java.com/bugdatabase/view_bug.do?bug_id=6476261 has been fixed in JDK8, which is the JDK version that Zinc demands.
jvican
force-pushed
the
classapi-improvements
branch
from
a0e4460 to
d61d0ae
Compare
|
Merging as code hasn't changed since last approval. |
* 1.0.x: (25 commits) Add yourkit acknoledgement in the README Add header to cached hashing spec Add headers to missing files Fix sbt#332: Add sbt-header back to the build Update sbt-scalafmt to 1.12 Make classpath hashing more lightweight Fix sbt#442: Name hash of value class should include underlying type source-dependencies/value-class-underlying: fix test Ignore null in generic lambda tparams Improve and make scripted parallel Fix sbt#436: Remove annoying log4j scripted exception Fix sbt#127: Use `unexpanded` name instead of `name` Add pending test case for issue/127 source-dependencies / patMat-scope workaround Fixes undercompilation on inheritance on same source Add real reproduction case for sbt#417 Add trait-trait-212 for Scala 2.12.3 Fix source-dependencies/sealed Import statement no longer needed Move mima exclusions to its own file ... Conflicts: internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala zinc/src/main/scala/sbt/internal/inc/MixedAnalyzingCompiler.scala The ClassToAPI conflict is due to: * sbt#393 (a 1.x PR), conflicting with * sbt#446 (a 1.0.x PR). The MixedAnalyzingCompiler conflict is due to: * sbt#427 (a 1.x PR), conflicting with * sbt#452 (a 1.0.x PR).
* 1.0.x: (28 commits) Split compiler bridge tests to another subproject Implement compiler bridge for 2.13.0-M2 Add yourkit acknoledgement in the README "sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0 Add header to cached hashing spec Add headers to missing files Fix sbt#332: Add sbt-header back to the build Update sbt-scalafmt to 1.12 Make classpath hashing more lightweight Fix sbt#442: Name hash of value class should include underlying type source-dependencies/value-class-underlying: fix test Ignore null in generic lambda tparams Improve and make scripted parallel Fix sbt#436: Remove annoying log4j scripted exception Fix sbt#127: Use `unexpanded` name instead of `name` Add pending test case for issue/127 source-dependencies / patMat-scope workaround Fixes undercompilation on inheritance on same source Add real reproduction case for sbt#417 Add trait-trait-212 for Scala 2.12.3 ... Conflicts: internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala project/build.properties zinc/src/main/scala/sbt/internal/inc/MixedAnalyzingCompiler.scala The ClassToAPI conflict is due to: * sbt#393 (a 1.x PR), conflicting with * sbt#446 (a 1.0.x PR). The build.properties conflict is due to different PRs bumping sbt.version from 1.0.0 to 1.0.2 to 1.0.3. (sbt#413, sbt#418, sbt#453). The MixedAnalyzingCompiler conflict is due to: * sbt#427 (a 1.x PR), conflicting with * sbt#452 (a 1.0.x PR).
* 1.0.x: (28 commits) Split compiler bridge tests to another subproject Implement compiler bridge for 2.13.0-M2 Add yourkit acknoledgement in the README "sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0 Add header to cached hashing spec Add headers to missing files Fix #332: Add sbt-header back to the build Update sbt-scalafmt to 1.12 Make classpath hashing more lightweight Fix #442: Name hash of value class should include underlying type source-dependencies/value-class-underlying: fix test Ignore null in generic lambda tparams Improve and make scripted parallel Fix #436: Remove annoying log4j scripted exception Fix #127: Use `unexpanded` name instead of `name` Add pending test case for issue/127 source-dependencies / patMat-scope workaround Fixes undercompilation on inheritance on same source Add real reproduction case for sbt/zinc#417 Add trait-trait-212 for Scala 2.12.3 ... Conflicts: internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala project/build.properties zinc/src/main/scala/sbt/internal/inc/MixedAnalyzingCompiler.scala The ClassToAPI conflict is due to: * sbt/zinc#393 (a 1.x PR), conflicting with * sbt/zinc#446 (a 1.0.x PR). The build.properties conflict is due to different PRs bumping sbt.version from 1.0.0 to 1.0.2 to 1.0.3. (#413, #418, #453). The MixedAnalyzingCompiler conflict is due to: * sbt/zinc#427 (a 1.x PR), conflicting with * sbt/zinc#452 (a 1.0.x PR).
* 1.0.x: (28 commits) Split compiler bridge tests to another subproject Implement compiler bridge for 2.13.0-M2 Add yourkit acknoledgement in the README "sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0 Add header to cached hashing spec Add headers to missing files Fix sbt#332: Add sbt-header back to the build Update sbt-scalafmt to 1.12 Make classpath hashing more lightweight Fix sbt#442: Name hash of value class should include underlying type source-dependencies/value-class-underlying: fix test Ignore null in generic lambda tparams Improve and make scripted parallel Fix sbt#436: Remove annoying log4j scripted exception Fix sbt#127: Use `unexpanded` name instead of `name` Add pending test case for issue/127 source-dependencies / patMat-scope workaround Fixes undercompilation on inheritance on same source Add real reproduction case for sbt/zinc#417 Add trait-trait-212 for Scala 2.12.3 ... Conflicts: internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala project/build.properties zinc/src/main/scala/sbt/internal/inc/MixedAnalyzingCompiler.scala The ClassToAPI conflict is due to: * sbt/zinc#393 (a 1.x PR), conflicting with * sbt/zinc#446 (a 1.0.x PR). The build.properties conflict is due to different PRs bumping sbt.version from 1.0.0 to 1.0.2 to 1.0.3. (sbt#413, sbt#418, sbt#453). The MixedAnalyzingCompiler conflict is due to: * sbt/zinc#427 (a 1.x PR), conflicting with * sbt/zinc#452 (a 1.0.x PR).
* 1.0.x: (28 commits) Split compiler bridge tests to another subproject Implement compiler bridge for 2.13.0-M2 Add yourkit acknoledgement in the README "sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0 Add header to cached hashing spec Add headers to missing files Fix scala#332: Add sbt-header back to the build Update sbt-scalafmt to 1.12 Make classpath hashing more lightweight Fix scala#442: Name hash of value class should include underlying type source-dependencies/value-class-underlying: fix test Ignore null in generic lambda tparams Improve and make scripted parallel Fix scala#436: Remove annoying log4j scripted exception Fix scala#127: Use `unexpanded` name instead of `name` Add pending test case for issue/127 source-dependencies / patMat-scope workaround Fixes undercompilation on inheritance on same source Add real reproduction case for sbt/zinc#417 Add trait-trait-212 for Scala 2.12.3 ... Conflicts: internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala project/build.properties zinc/src/main/scala/sbt/internal/inc/MixedAnalyzingCompiler.scala The ClassToAPI conflict is due to: * sbt/zinc#393 (a 1.x PR), conflicting with * sbt/zinc#446 (a 1.0.x PR). The build.properties conflict is due to different PRs bumping sbt.version from 1.0.0 to 1.0.2 to 1.0.3. (scala#413, scala#418, scala#453). The MixedAnalyzingCompiler conflict is due to: * sbt/zinc#427 (a 1.x PR), conflicting with * sbt/zinc#452 (a 1.0.x PR).
* 1.0.x: (28 commits) Split compiler bridge tests to another subproject Implement compiler bridge for 2.13.0-M2 Add yourkit acknoledgement in the README "sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0 Add header to cached hashing spec Add headers to missing files Fix scala#332: Add sbt-header back to the build Update sbt-scalafmt to 1.12 Make classpath hashing more lightweight Fix scala#442: Name hash of value class should include underlying type source-dependencies/value-class-underlying: fix test Ignore null in generic lambda tparams Improve and make scripted parallel Fix scala#436: Remove annoying log4j scripted exception Fix scala#127: Use `unexpanded` name instead of `name` Add pending test case for issue/127 source-dependencies / patMat-scope workaround Fixes undercompilation on inheritance on same source Add real reproduction case for sbt/zinc#417 Add trait-trait-212 for Scala 2.12.3 ... Conflicts: internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala project/build.properties zinc/src/main/scala/sbt/internal/inc/MixedAnalyzingCompiler.scala The ClassToAPI conflict is due to: * sbt/zinc#393 (a 1.x PR), conflicting with * sbt/zinc#446 (a 1.0.x PR). The build.properties conflict is due to different PRs bumping sbt.version from 1.0.0 to 1.0.2 to 1.0.3. (scala#413, scala#418, scala#453). The MixedAnalyzingCompiler conflict is due to: * sbt/zinc#427 (a 1.x PR), conflicting with * sbt/zinc#452 (a 1.0.x PR). Rewritten from sbt/zinc@e245d95
The workarounds are not necessary anymore,
http://bugs.java.com/bugdatabase/view_bug.do?bug_id=6476261 has been
fixed in JDK8, which is the JDK version that Zinc demands.
We can change the target branch after this is LGTM'ed and the forward port
for 1.0.x is finished.