Upgrade to scala-asm 6.2 #6733
Upgrade to scala-asm 6.2 #6733
Conversation
|
Under the I'm still measuring effect on compile time in the benchmark. |
|
LGTM, let's release |
|
Maybe it's time to try to get all the patches into the official asm repo? |
We'd love to. Will keep trying (recent attempt: https://gitlab.ow2.org/asm/asm/issues/317828#note_35917). (EDIT: expanding a bit: they don't seem to have a clear story on how we can contribute our patches, but the bug we reported above was fixed. If someone here has a connection to the ASM maintainers and could introduce us, that'd be great.) |
2c5554b to
d42310f
Compare
|
/rebuild |
|
Hmm, travis aborted and jenkins seems to be stuck in the same place... |
|
Jenkins continued now; but it does look little suspicious. |
|
I've seen that output on Travis recently, so it might be a latent problem on this branch (OSGi plugin or something?) I'll take a look locally on Monday. |
|
So you'd like to wait before merging? |
|
Yes, let’s hold off
|
232f71c to
2d3d96c
Compare
|
Rebased. I tried locally to see if the (bootstrapped) build got stuck in a similar spot to those logs but didn't see it. I'd say we should merge this and revert it we see the problems in CI. |
|
Travis failed again on 2d3d96c with the same timeout. 😖 |
|
I've captured stacks of the slowness in Jenkins. Looks like it shows up under |
603521e to
a0d9c08
Compare
|
I've added a benchmark that shows the performance regression, which starts right away in 54dbb0f. Here are the profiles before and after Looks like we're falling into this code much more than before: https://github.com/scala/scala-asm/blob/2e8a7bb0634ed905f7df86ba2ecef08c90855636/src/main/java/scala/tools/asm/tree/analysis/SourceInterpreter.java#L205-L208 |
|
This diff in ASM triggers the slowness. It now creates a new These no longer get collapsed when entering them into the merging The fix looks straight forward enough: just capture the |
|
@lrytz Since you last looked:
After this, we're ready to move to vanilla ASM 6.3. We still need to decided on whether to depend on it as a regular dependency or keep the status quo of repackaging (either with the scripts in scala-asm or perhaps just with JarJar). |
ASM 6.2 now creates a new Frame inside the loop in which `newExceptionValue` is called. We were including this frame in the case-class equality of the pseudo-instruction, `ExceptionProducer`, and upon receiving new instances each time the `ProdCons` analysis massively slowed down. This commit just captures the data we need: the stack top of the handler frame.
See: scala/scala-asm#5 Upstream changes in ASM: scala/scala-asm@ASM_6_0...ASM_6_2 http://asm.ow2.io/versions.html The motivations, other than just keeping current, are: - support for Java 9/10/11 updates to the classfile format. - reducing needless String => Array[Char] conversions thanks to internal changes in ASM. This PR will fail to build until we publish artifact from scala/scala-asm. Includes a workaround for scala/bug#10418
It seems we don't need CustomAttr in our fork of scala-asm, we can just override Attribute.write.
| import scala.tools.asm.tree.ClassNode; | ||
| import scala.tools.asm.tree.MethodNode; | ||
|
|
||
| public class ClassNode1 extends ClassNode { |
There was a problem hiding this comment.
Le'ts add a comment why this custom class exists
| import scala.tools.asm.tree.LabelNode; | ||
| import scala.tools.asm.tree.MethodNode; | ||
|
|
||
| public class MethodNode1 extends MethodNode { |
There was a problem hiding this comment.
Also here, a comment would be good.
| @Override | ||
| protected LabelNode getLabelNode(Label label) { | ||
| if (!(label.info instanceof LabelNode)) { | ||
| label.info = new LabelNode1(label); |
There was a problem hiding this comment.
And here, where the actual change is.
| import scala.tools.asm.Label; | ||
| import scala.tools.asm.tree.LabelNode; | ||
|
|
||
| public class LabelNode1 extends LabelNode { |
| @@ -19,7 +19,7 @@ import AliasSet.SmallBitSet | |||
| * | |||
| * See the doc of package object `analysis` for some notes on the performance of alias analysis. | |||
| */ | |||
| class AliasingFrame[V <: Value](nLocals: Int, nStack: Int) extends Frame[V](nLocals, nStack) { | |||
| class AliasingFrame[V <: Value](nLocals: Int, nStack: Int) extends VanillaAsmFrame[V](nLocals, nStack) { | |||
There was a problem hiding this comment.
AliasingFrame is used in copy propagation and nullness analysis, but not in ProdConsAnalyzer. The following diff does that: https://github.com/retronym/scala/compare/bump/asm-6.2...lrytz:pr6733?expand=1
With that, some tests ProdConsAnalyzerTest actually start failing.
There might be a way to implement prod-cons without those additional calls to copyOperation, I haven't looked in detail.
There was a problem hiding this comment.
I would expect ProdConsAnalyzer to fail with the VanillaAsmFrame until ASM 6.3 which will start to call newParameterValue/newEmptyNonParameterLocalValue/newExceptionValue, which InitialProducerSourceInterpreter implements.
We only override copyOperation in NullnessInterpreter.
There was a problem hiding this comment.
Not sure I follow.. The newXValue methods are called in the Analyzer, not in Frame / VanillaAsmFrame. The copyOperation implementation for ProdCons is inherited from SourceInterpreter. Anyway, we'll see when we update to 6.3.
There was a problem hiding this comment.
Right, I didn't consider the inherited implementation of copyOperation, i just looked for the one override in our codebase. Still, it should be easy enough to adapt when we need to.
|
I've removed "38826f6320 Show that we don't actually need 'call copyOperation consistently" as it is just code that would be redundant with ASM 6.3. I've also added comments to the |
Avoid performance problem after ASM upgrade in prod/cons analysis ASM 6.2 now creates a new Frame inside the loop in which `newExceptionValue` is called. We were including this frame in the case-class equality of the pseudo-instruction, `ExceptionProducer`, and upon receiving new instances each time the `ProdCons` analysis massively slowed down. This commit just captures the data we need: the stack top of the handler frame. Upgrade to scala-asm 6.2 See: scala/scala-asm#5 Upstream changes in ASM: scala/scala-asm@ASM_6_0...ASM_6_2 http://asm.ow2.io/versions.html The motivations, other than just keeping current, are: - support for Java 9/10/11 updates to the classfile format. - reducing needless String => Array[Char] conversions thanks to internal changes in ASM. This PR will fail to build until we publish artifact from scala/scala-asm. Includes a workaround for scala/bug#10418 Move to the standard way of defining a custom asm.Attribute It seems we don't need CustomAttr in our fork of scala-asm, we can just override Attribute.write. Customise label handling without needing to modify ASM directly Comment on our customizations to asm.tree.*Node
Catch up with Scala 2.13. The corresponding scalac PRs are scala/scala#6733 and scala/scala#7384 Most of the changes from these PRs aren't currently needed in Dotty since they only affect the Scala 2 backend optimizer.
Catch up with Scala 2.13. The corresponding scalac PRs are scala/scala#6733 and scala/scala#7384 Some of the changes from these PRs aren't currently needed in Dotty since they only affect the Scala 2 backend optimizer. Co-Authored-By: Jason Zaugg <jzaugg@gmail.com>
Catch up with Scala 2.13. The corresponding scalac PRs are scala/scala#6733 and scala/scala#7384 Some of the changes from these PRs aren't currently needed in Dotty since they only affect the Scala 2 backend optimizer. This update does not change the generated bytecode (at least when compiling dotty itself). Co-Authored-By: Jason Zaugg <jzaugg@gmail.com>
Avoid performance problem after ASM upgrade in prod/cons analysis ASM 6.2 now creates a new Frame inside the loop in which `newExceptionValue` is called. We were including this frame in the case-class equality of the pseudo-instruction, `ExceptionProducer`, and upon receiving new instances each time the `ProdCons` analysis massively slowed down. This commit just captures the data we need: the stack top of the handler frame. Upgrade to scala-asm 6.2 See: scala/scala-asm#5 Upstream changes in ASM: scala/scala-asm@ASM_6_0...ASM_6_2 http://asm.ow2.io/versions.html The motivations, other than just keeping current, are: - support for Java 9/10/11 updates to the classfile format. - reducing needless String => Array[Char] conversions thanks to internal changes in ASM. This PR will fail to build until we publish artifact from scala/scala-asm. Includes a workaround for scala/bug#10418 Move to the standard way of defining a custom asm.Attribute It seems we don't need CustomAttr in our fork of scala-asm, we can just override Attribute.write. Customise label handling without needing to modify ASM directly Comment on our customizations to asm.tree.*Node (cherry picked from commit 79b7f2a)
Avoid performance problem after ASM upgrade in prod/cons analysis ASM 6.2 now creates a new Frame inside the loop in which `newExceptionValue` is called. We were including this frame in the case-class equality of the pseudo-instruction, `ExceptionProducer`, and upon receiving new instances each time the `ProdCons` analysis massively slowed down. This commit just captures the data we need: the stack top of the handler frame. Upgrade to scala-asm 6.2 See: scala/scala-asm#5 Upstream changes in ASM: scala/scala-asm@ASM_6_0...ASM_6_2 http://asm.ow2.io/versions.html The motivations, other than just keeping current, are: - support for Java 9/10/11 updates to the classfile format. - reducing needless String => Array[Char] conversions thanks to internal changes in ASM. This PR will fail to build until we publish artifact from scala/scala-asm. Includes a workaround for scala/bug#10418 Move to the standard way of defining a custom asm.Attribute It seems we don't need CustomAttr in our fork of scala-asm, we can just override Attribute.write. Customise label handling without needing to modify ASM directly Comment on our customization 71F9 s to asm.tree.*Node (cherry picked from commit 79b7f2a)
See: scala/scala-asm#5
Upstream changes in ASM:
scala/scala-asm@ASM_6_0...ASM_6_2
http://asm.ow2.io/versions.html
The motivations, other than just keeping current, are:
to internal changes in ASM.
This PR will fail to build until we publish artifact
from scala/scala-asm.
Fixes scala/bug#10717