(2 of 2) of the rest of the new bytecode emitter #2889
Conversation
The assert in question was aimed at ruling out gotos (ie "jumping-applys") in actual argument position of a jumping-apply. But the assert in question went overboard to also rule out a LabelDef in actual argument position. This commit removes the assert in question altogether. The unwanted behaviors, and only those, are rule out by the test added in this commit and the existing tests for SI-6089. See also https://issues.scala-lang.org/browse/SI-7749
SI-5604 was fixed in commit e6f10b0
This bytecode transformer rephrases one ore more jumps in a multi-jump chain to target its final destination via a single jump Details in the documentation for scala.tools.nsc.backend.bcode.JumpChainsCollapser
This bytecode transformer removes those LabelNodes, LocalVarEntries, and LineNumberNodes that are redundant. Details in the documentation for scala.tools.nsc.backend.bcode.LabelsCleanup
This bytecode transformer removes those exception-entries (corresponding to try-catch-blocks) that are redundant. Details in the documentation for scala.tools.nsc.backend.bcode.DanglingExcHandlers
This bytecode transformer removes code that is unreachable. Details in the documentation for scala.tools.nsc.backend.bcode.UnreachableCode
Before writing classfiles with "optimization level zero" (ie -Ybackend:GenBCode) the very least we want to do is remove dead code beforehand, so as to prevent an artifact of stack-frames computation from showing up, the artifact described at http://asm.ow2.org/doc/developer-guide.html#deadcode That artifact results from the requirement by the Java 6 split verifier that a stack map frame be available for each basic block, even unreachable ones. Additionally, for dead-code elimination to be effective, each multi-jump chain should be collapsed to target its final destination via a single jump. Details in: - scala.tools.nsc.backend.bcode.JumpChainsCollapser Just removing dead code might leave stale entries (ie pointing to LabelNodes removed during dead-code elimination) in LocalVariableTable as well as in the exception-handlers table, thus `EssentialCleanser.cleanseMethod()` also gets rid of those. The classes in charge of that are: - scala.tools.nsc.backend.bcode.LabelsCleanup - scala.tools.nsc.backend.bcode.DanglingExcHandlers
The GenICode-counterpart to this functionality was added in commit b50a0d8 to fix SI-7006 As for GenBCode, the documentation says it all: /* * Detects and removes unreachable code. * * Should be used last in a transformation chain, before stack map frames are computed. * The Java 6 verifier demands frames be available even for dead code. * Those frames are tricky to compute, http://asm.ow2.org/doc/developer-guide.html#deadcode * The problem is avoided altogether by not emitting unreachable code in the first place.
Code that contains redundant jumps usually arrives to both GenASM and GenBCode, for a variety of reasons. To name a few: - test/files/run/private-inline.scala after -optimize - virtual pattern matcher (for an example see the test in this commit) The backend(s) manage to get rid of the redundant jumps, for example: - GenASM, using collapseJumpOnlyBlocks() - GenBCode, using JumpChainsCollapser The test case in this commit shows how GenBCode reduces the number of GOTOs from 22 down to 7.
An asm.tree.MethodNode includes a list of exception-handler entries,
each such entry includes (among the other things) start and end
LabelNodes to demarcate the range being protected.
- Any executable instruction other than NOP means the
exception-handler-entry should stay.
- Otherwise, DanglingExcHandlers removes
the exception-handler entry in question.
Traversing a ClassDef to build an ASM ClassNode requires typer and therefore must run single-threaded. However, bytecode transformers that manipulate ClassNodes don't need typer and thus can run in parallel. Two other activities that don't depend on typer either are: - encoding ClassNodes into the byte array representation of a classfile - writing classfiles to disk The activities above (building ClassNodes, encoding, disk writing) are performed in different threads (for now, "encoding" is only lightly loaded, but in the future certain optimizations will run there).
| @@ -58,6 +111,15 @@ abstract class GenBCode extends BCodeSyncAndTry { | |||
| override def description = "Generate bytecode from ASTs using the ASM library" | |||
| override def erasedTypes = true | |||
|
|
|||
| // number of woker threads for pipeline-2 (the pipeline in charge of most optimizations except inlining). | |||
| val MAX_THREADS = scala.math.min( | |||
| 4, | |||
There was a problem hiding this comment.
We need a way to configure this. I'd suggest a -Y compiler option. Use case would be to disable parallelism altogether here to work around a bug, or when investigating a bug.
A system property is an alternative, but that is JVM-wide and unsuitable for platforms that run multiple compilers in one JVM (e.g. SBT).
There was a problem hiding this comment.
I agree.
Would it also make sense to make ExecutionContext pluggable so people can't control thread pools when running Scala compiler in an embedded context?
|
Thanks for the comments so far. I'll give more time for others to chime in, to then add:
|
ghost
assigned 
|
@magarciaEPFL: this PR looks really good. Thanks for taking time and writing proper unit tests for your ASM transformations! Could you rewrite all of them to be JUnit tests, though? In 54a7f80 you do test bytecode generated by Scala compiler so you can leave that test as-is. |
|
@gkossakowski , @retronym A few more commits addressing the pending items:
are being worked on, and should be ready in a day or two. It would be great if this PR were merged as-is so that early adopters can field-test GenBCode with M5 and provide us with much needed feedback. Quite likely the pending improvements will also be ready on time, but this PR is the main thing. Rest assured that the next commits I'll include (in that case, in a follow-up PR) will address the pending items. |
|
One of the comments in a previous PR emphasized a distinction between integration and unit tests in the context of ASM transformations. This PR as originally submitted contains both kinds of tests. The commits just added refactor the units tests into JUnit tests, as requested. The actual work performed by old and new unit tests hasn't changed. Now that both integration and junit tests have been addressed, it only remains to add a compiler option to configure the number of worker threads in GenBCode. As already mentioned, this PR is correct, and can be merged. The compiler option in question will be added soon. |
|
@magarciaEPFL: we'll need a little bit more polish when it comes to unit tests. I'll try to work on that tonight to demonstrate what I mean by that. |
|
@gkossakowski , I don't know what kind of polish in unit tests it's so important to show off, all I'm saying is this PR makes the compiler faster. Would have made, had it been merged in M5. |
|
@magarciaEPFL: it's not about "showing off" but merging code that is of high quality. All I'm offering is a little guidance on how to write good unit tests. Also, I'm very much aware of benefits of the new backend. However, that's not what we are discussing it here. |
|
@magarciaEPFL: I reworked your tests to become idiomatic JUnit tests. Here's me showing off: More seriously, see gkossakowski/scala@3c80c57 and @gkossakowskibcf58bd. Please cherry pick the first commit and squash the second one into your 1aacd32. Once done, push the final result this this PR. I'll review everything today. If there will be no problems I'll merge it today and your PR will make it into M5. |
|
I don't know what I like most,
Anyway, it's late here so all I've managed to do is:
The result can be found in branch https://github.com/magarciaEPFL/scala/tree/backendish46 that I'm submitting as PR #2908 |
|
OK, thanks to all reviewers and I'll find out tomorrow what M5 is good for 😄 |
|
I have a commit in a PR somewhere to fix:
which is totally insanity-inducing. But do you wish you had to ant partest.task if you happen to touch code under partest-extras? Monolithicity has its privileges. But it's all in the right direction. NNE. |
|
@magarciaEPFL: In the future please keep amending existing PR instead of opening a new one. It makes it easier to manage things (we have 30 PRs open at the moment) for us and we don't lose the context like existing comments. All newly opened PRs are automatically assigned to M6 release. I'll reassign your #2908 to M5 manually but please keep that in mind in the future. |
This PR completes the new bytecode emitter, including building and writing class files in parallel, by building upon #2863
Before writing class files, a modicum of code tightening transformations are necessary: unreachable code elimination, which in turn calls for cleanup of labels and exception entries made redundant by unreachable code elimination.
In this PR, each such transformation is introduced in a commit with tests for the transformation in isolation. Another commit also in this PR tests each such transformation in connection with the bytecode emitter.
I'll suggest a reviewer once tests pass, comments and reviews welcome.