You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@drill.apache.org by vo...@apache.org on 2018/11/26 16:04:05 UTC
[drill] 11/15: DRILL-6868: Upgrade Janino compiler to 3.0.11
This is an automated email from the ASF dual-hosted git repository.
volodymyr pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/drill.git
commit 67adde12853f14aab8d0e2ffc162a2daaf53d954
Author: Volodymyr Vysotskyi <vv...@gmail.com>
AuthorDate: Fri Oct 19 18:04:12 2018 +0300
DRILL-6868: Upgrade Janino compiler to 3.0.11
- Remove workaround where removing adjacent ALOAD-POP instruction pairs
- Remove ModifiedUnparser and use DeepCopier for modifying methods instead of modifying it with custom Unparser implementation
closes #1553
---
.../exec/compile/bytecode/AloadPopRemover.java | 333 ---------------------
.../bytecode/ValueHolderReplacementVisitor.java | 10 +-
.../drill/exec/expr/fn/MethodGrabbingVisitor.java | 57 +++-
.../drill/exec/expr/fn/ModifiedUnparser.java | 110 -------
pom.xml | 2 +-
5 files changed, 58 insertions(+), 454 deletions(-)
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/AloadPopRemover.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/AloadPopRemover.java
deleted file mode 100644
index a6df261..0000000
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/AloadPopRemover.java
+++ /dev/null
@@ -1,333 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.drill.exec.compile.bytecode;
-
-import org.objectweb.asm.AnnotationVisitor;
-import org.objectweb.asm.Attribute;
-import org.objectweb.asm.Handle;
-import org.objectweb.asm.Label;
-import org.objectweb.asm.MethodVisitor;
-import org.objectweb.asm.Opcodes;
-import org.objectweb.asm.TypePath;
-
-
-/**
- * Remove any adjacent ALOAD-POP instruction pairs.
- *
- * The Janino compiler generates an instruction stream where it will ALOAD a
- * holder's objectref, and then immediately POP it because the compiler has
- * recognized that the method call that it loaded the objectref for is static
- * (i.e., no this pointer is required). This causes a problem with our scalar
- * replacement strategy, because once we remove the holders, we simply eliminate
- * the ALOAD instructions. In this case, the POP gets left behind, and breaks
- * the program stack.
- *
- * This class looks for ALOADs that are immediately followed by a POP, and removes
- * the pair of instructions altogether.
- *
- * It is unknown if other compilers besides Janino generate this instruction sequence,
- * but to be on the safe side, we'll use this class unconditionally to filter bytecode.
- *
- * TODO: this might be easier to do by going off an InsnList; the state machine would
- * be in the loop that visits the instructions then.
- */
-public class AloadPopRemover extends MethodVisitor {
- private final static int NONE = -1; // var value to indicate no captured ALOAD
- private int var = NONE;
-
- /**
- * Constructor.
- *
- * See {@link org.objectweb.asm.MethodVisitor#MethodVisitor(int)}.
- */
- public AloadPopRemover(final int api) {
- super(api);
- }
-
- /**
- * Constructor.
- *
- * See {@link org.objectweb.asm.MethodVisitor#MethodVisitor(int, MethodVisitor)}.
- */
- public AloadPopRemover(final int api, final MethodVisitor mv) {
- super(api, mv);
- }
-
- /**
- * Process a deferred ALOAD instruction, if there is one.
- *
- * If there is no deferred ALOAD, does nothing, and returns false.
- *
- * If there is a deferred ALOAD, and we're on a POP instruction
- * (indicated by onPop), does nothing (the ALOAD is not forwarded),
- * and returns true.
- *
- * If there is a deferred ALOAD and we're not on a POP instruction,
- * forwards the deferred ALOAD, and returns false
- *
- * @param onPop true if the current instruction is POP
- * @return true if onPop and there was a deferred ALOAD, false otherwise;
- * basically, returns true if the ALOAD-POP optimization is required
- */
- private boolean processDeferredAload(final boolean onPop) {
- // if the previous instruction wasn't an ALOAD, then there's nothing to do
- if (var == NONE) {
- return false;
- }
-
- // clear the variable index, but save it for local use
- final int localVar = var;
- var = NONE;
-
- // if the next instruction is a POP, don't emit the deferred ALOAD
- if (onPop) {
- return true;
- }
-
- // if we got here, we're not on a POP, so emit the deferred ALOAD
- super.visitVarInsn(Opcodes.ALOAD, localVar);
- return false;
- }
-
- @Override
- public AnnotationVisitor visitAnnotation(final String desc, final boolean visible) {
- processDeferredAload(false);
- final AnnotationVisitor annotationVisitor = super.visitAnnotation(desc, visible);
- return annotationVisitor;
- }
-
- @Override
- public AnnotationVisitor visitAnnotationDefault() {
- processDeferredAload(false);
- final AnnotationVisitor annotationVisitor = super.visitAnnotationDefault();
- return annotationVisitor;
- }
-
- @Override
- public void visitAttribute(final Attribute attr) {
- processDeferredAload(false);
- super.visitAttribute(attr);
- }
-
- @Override
- public void visitCode() {
- processDeferredAload(false);
- super.visitCode();
- }
-
- @Override
- public void visitEnd() {
- processDeferredAload(false);
- super.visitEnd();
- }
-
- @Override
- public void visitFieldInsn(final int opcode, final String owner, final String name,
- final String desc) {
- processDeferredAload(false);
- super.visitFieldInsn(opcode, owner, name, desc);
- }
-
- @Override
- public void visitFrame(final int type, final int nLocal,
- final Object[] local, final int nStack, final Object[] stack) {
- processDeferredAload(false);
- super.visitFrame(type, nLocal, local, nStack, stack);
- }
-
- @Override
- public void visitIincInsn(final int var, final int increment) {
- processDeferredAload(false);
- super.visitIincInsn(var, increment);
- }
-
- @Override
- public void visitInsn(final int opcode) {
- /*
- * If we don't omit an ALOAD with a following POP, then forward this.
- * In other words, if we omit an ALOAD because we're on the following POP,
- * don't forward this POP, which omits the ALOAD-POP pair.
- */
- if (!processDeferredAload(Opcodes.POP == opcode)) {
- super.visitInsn(opcode);
- }
- }
-
- @Override
- public AnnotationVisitor visitInsnAnnotation(final int typeRef,
- final TypePath typePath, final String desc, final boolean visible) {
- processDeferredAload(false);
- final AnnotationVisitor annotationVisitor = super.visitInsnAnnotation(
- typeRef, typePath, desc, visible);
- return annotationVisitor;
- }
-
- @Override
- public void visitIntInsn(final int opcode, final int operand) {
- processDeferredAload(false);
- super.visitIntInsn(opcode, operand);
- }
-
- @Override
- public void visitInvokeDynamicInsn(final String name, final String desc,
- final Handle bsm, final Object... bsmArgs) {
- processDeferredAload(false);
- super.visitInvokeDynamicInsn(name, desc, bsm, bsmArgs);
- }
-
- @Override
- public void visitJumpInsn(final int opcode, final Label label) {
- processDeferredAload(false);
- super.visitJumpInsn(opcode, label);
- }
-
- @Override
- public void visitLabel(final Label label) {
- processDeferredAload(false);
- super.visitLabel(label);
- }
-
- @Override
- public void visitLdcInsn(final Object cst) {
- processDeferredAload(false);
- super.visitLdcInsn(cst);
- }
-
- @Override
- public void visitLineNumber(final int line, final Label start) {
- processDeferredAload(false);
- super.visitLineNumber(line, start);
- }
-
- @Override
- public void visitLocalVariable(final String name, final String desc,
- final String signature, final Label start, final Label end, final int index) {
- processDeferredAload(false);
- super.visitLocalVariable(name, desc, signature, start, end, index);
- }
-
- @Override
- public AnnotationVisitor visitLocalVariableAnnotation(final int typeRef,
- final TypePath typePath, final Label[] start, final Label[] end,
- final int[] index, final String desc, final boolean visible) {
- processDeferredAload(false);
- final AnnotationVisitor annotationVisitor =
- super.visitLocalVariableAnnotation(typeRef, typePath, start, end, index,
- desc, visible);
- return annotationVisitor;
- }
-
- @Override
- public void visitLookupSwitchInsn(final Label dflt, final int[] keys,
- final Label[] labels) {
- processDeferredAload(false);
- super.visitLookupSwitchInsn(dflt, keys, labels);
- }
-
- @Override
- public void visitMaxs(final int maxStack, final int maxLocals) {
- processDeferredAload(false);
- super.visitMaxs(maxStack, maxLocals);
- }
-
- @Deprecated
- @Override
- public void visitMethodInsn(final int opcode, final String owner,
- final String name, final String desc) {
- processDeferredAload(false);
- super.visitMethodInsn(opcode, owner, name, desc);
- }
-
- @Override
- public void visitMethodInsn(final int opcode, final String owner,
- final String name, final String desc, final boolean itf) {
- processDeferredAload(false);
- super.visitMethodInsn(opcode, owner, name, desc, itf);
- }
-
- @Override
- public void visitMultiANewArrayInsn(final String desc, final int dims) {
- processDeferredAload(false);
- super.visitMultiANewArrayInsn(desc, dims);
- }
-
- @Override
- public void visitParameter(final String name, final int access) {
- processDeferredAload(false);
- super.visitParameter(name, access);
- }
-
- @Override
- public AnnotationVisitor visitParameterAnnotation(final int parameter,
- final String desc, final boolean visible) {
- processDeferredAload(false);
- final AnnotationVisitor annotationVisitor =
- super.visitParameterAnnotation(parameter, desc, visible);
- return annotationVisitor;
- }
-
- @Override
- public void visitTableSwitchInsn(final int min, final int max,
- final Label dflt, final Label... labels) {
- processDeferredAload(false);
- super.visitTableSwitchInsn(min, max, dflt, labels);
- }
-
- @Override
- public AnnotationVisitor visitTryCatchAnnotation(final int typeRef,
- final TypePath typePath, final String desc, final boolean visible) {
- processDeferredAload(false);
- final AnnotationVisitor annotationVisitor =
- super.visitTryCatchAnnotation(typeRef, typePath, desc, visible);
- return annotationVisitor;
- }
-
- @Override
- public void visitTryCatchBlock(final Label start, final Label end,
- final Label handler, final String type) {
- processDeferredAload(false);
- super.visitTryCatchBlock(start, end, handler, type);
- }
-
- @Override
- public AnnotationVisitor visitTypeAnnotation(final int typeRef,
- final TypePath typePath, final String desc, final boolean visible) {
- processDeferredAload(false);
- final AnnotationVisitor annotationVisitor =
- super.visitTypeAnnotation(typeRef, typePath, desc, visible);
- return annotationVisitor;
- }
-
- @Override
- public void visitTypeInsn(final int opcode, final String type) {
- processDeferredAload(false);
- super.visitTypeInsn(opcode, type);
- }
-
- @Override
- public void visitVarInsn(final int opcode, final int var) {
- processDeferredAload(false);
-
- // if we see an ALOAD, defer forwarding it until we know what the next instruction is
- if (Opcodes.ALOAD == opcode) {
- this.var = var;
- } else {
- super.visitVarInsn(opcode, var);
- }
- }
-}
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ValueHolderReplacementVisitor.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ValueHolderReplacementVisitor.java
index 1ed9ea5..9094b33 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ValueHolderReplacementVisitor.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ValueHolderReplacementVisitor.java
@@ -55,14 +55,8 @@ public class ValueHolderReplacementVisitor extends ClassVisitor {
innerVisitor = new CheckMethodVisitorFsm(api, innerVisitor);
}
- /*
- * Before using the ScalarReplacementNode to rewrite method code, use the
- * AloadPopRemover to eliminate unnecessary ALOAD-POP pairs; see the
- * AloadPopRemover javadoc for a detailed explanation.
- */
- return new AloadPopRemover(api,
- new ScalarReplacementNode(
- className, access, name, desc, signature, exceptions, innerVisitor, verifyBytecode));
+ return new ScalarReplacementNode(className, access, name, desc, signature,
+ exceptions,innerVisitor, verifyBytecode);
}
private static class Debugger extends MethodNode {
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/MethodGrabbingVisitor.java b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/MethodGrabbingVisitor.java
index 782e1e2..18cc4e4 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/MethodGrabbingVisitor.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/MethodGrabbingVisitor.java
@@ -21,10 +21,13 @@ import java.io.StringWriter;
import java.util.HashMap;
import java.util.Map;
+import org.codehaus.commons.compiler.CompileException;
import org.codehaus.janino.Java;
import org.codehaus.janino.Java.AbstractClassDeclaration;
import org.codehaus.janino.Java.MethodDeclarator;
+import org.codehaus.janino.Unparser;
import org.codehaus.janino.util.AbstractTraverser;
+import org.codehaus.janino.util.DeepCopier;
public class MethodGrabbingVisitor {
@@ -64,9 +67,59 @@ public class MethodGrabbingVisitor {
@Override
public void traverseMethodDeclarator(MethodDeclarator methodDeclarator) {
if (captureMethods) {
+ // Generates a "labeled statement".
+ // This code takes code from the method body, wraps it into the labeled statement
+ // and replaces all the return statements by break command with label.
+ //
+ // For example, the following method
+ // public void foo(int a) {
+ // if (a < 0) {
+ // return;
+ // } else {
+ // do something;
+ // }
+ // }
+ //
+ // will be converted to
+ // MethodClassName_foo: {
+ // if (a < 0) {
+ // break MethodClassName_foo;
+ // } else {
+ // do something;
+ // }
+ // }
+
+ // Constructs a name of the resulting label
+ // using methods class name and method name itself.
+ String[] fQCN = methodDeclarator.getDeclaringType().getClassName().split("\\.");
+ String returnLabel = fQCN[fQCN.length - 1] + "_" + methodDeclarator.name;
+ Java.Block methodBodyBlock = new Java.Block(methodDeclarator.getLocation());
+
+ // DeepCopier implementation which returns break statement with label
+ // instead if return statement.
+ DeepCopier returnStatementReplacer = new DeepCopier() {
+ @Override
+ public Java.BlockStatement copyReturnStatement(Java.ReturnStatement subject) {
+ return new Java.BreakStatement(subject.getLocation(), returnLabel);
+ }
+ };
+ try {
+ // replaces return statements and stores the result into methodBodyBlock
+ methodBodyBlock.addStatements(
+ returnStatementReplacer.copyBlockStatements(methodDeclarator.optionalStatements));
+ } catch (CompileException e) {
+ throw new RuntimeException(e);
+ }
+
+ // wraps method code with replaced return statements into label statement.
+ Java.LabeledStatement labeledStatement =
+ new Java.LabeledStatement(methodDeclarator.getLocation(), returnLabel, methodBodyBlock);
+
+ // Unparse the labeled statement.
StringWriter writer = new StringWriter();
- ModifiedUnparser unparser = new ModifiedUnparser(writer);
- unparser.visitMethodDeclarator(methodDeclarator);
+ Unparser unparser = new Unparser(writer);
+ // unparses labeledStatement and stores unparsed code into writer
+ unparser.unparseBlockStatement(labeledStatement);
unparser.close();
writer.flush();
methods.put(methodDeclarator.name, writer.getBuffer().toString());
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ModifiedUnparser.java b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ModifiedUnparser.java
deleted file mode 100644
index e7b2244..0000000
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ModifiedUnparser.java
+++ /dev/null
@@ -1,110 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.drill.exec.expr.fn;
-
-import java.io.Writer;
-import java.util.List;
-
-import org.codehaus.janino.Java;
-import org.codehaus.janino.Unparser;
-import org.codehaus.janino.util.AutoIndentWriter;
-
-/**
- * This is a modified version of {@link Unparser} so that we can avoid
- * rendering few things.
- */
-public class ModifiedUnparser extends Unparser {
-
- private String returnLabel;
-
- public ModifiedUnparser(Writer writer) {
- super(writer);
- }
-
- @Override
- public void unparseBlockStatement(Java.BlockStatement blockStatement) {
- // Unparser uses anonymous classes for visiting statements,
- // therefore added this check for customizing of handling ReturnStatement.
- if (blockStatement instanceof Java.ReturnStatement) {
- visitReturnStatement((Java.ReturnStatement) blockStatement);
- } else {
- super.unparseBlockStatement(blockStatement);
- }
- }
-
- /**
- * Parses specified {@link Java.MethodDeclarator}, wraps its content
- * with replaced {@code return} statements by {@code break} ones into the
- * block with label and stores it into {@link java.io.PrintWriter}.
- *
- * @param methodDeclarator method to parse
- */
- public void visitMethodDeclarator(Java.MethodDeclarator methodDeclarator) {
- if (methodDeclarator.optionalStatements == null) {
- pw.print(';');
- } else if (methodDeclarator.optionalStatements.isEmpty()) {
- pw.print(" {}");
- } else {
- pw.println(' ');
- // Add labels to handle return statements within function templates
- String[] fQCN = methodDeclarator.getDeclaringType().getClassName().split("\\.");
- returnLabel = fQCN[fQCN.length - 1] + "_" + methodDeclarator.name;
- pw.print(returnLabel);
- pw.println(": {");
- pw.print(AutoIndentWriter.INDENT);
- unparseStatements(methodDeclarator.optionalStatements);
- pw.print(AutoIndentWriter.UNINDENT);
- pw.println("}");
- pw.print(' ');
- }
- }
-
- private void visitReturnStatement(Java.ReturnStatement returnStatement) {
- pw.print("break ");
- pw.print(returnLabel);
- if (returnStatement.optionalReturnValue != null) {
- pw.print(' ');
- unparseAtom(returnStatement.optionalReturnValue);
- }
- pw.print(';');
- }
-
- /**
- * The following helper method is copied from the parent class since it
- * is declared as private in the parent class and can not be used in the child
- * class (this).
- */
- private void unparseStatements(List<? extends Java.BlockStatement> statements) {
- int state = -1;
- for (Java.BlockStatement bs : statements) {
- int x = (
- bs instanceof Java.Block ? 1 :
- bs instanceof Java.LocalClassDeclarationStatement ? 2 :
- bs instanceof Java.LocalVariableDeclarationStatement ? 3 :
- bs instanceof Java.SynchronizedStatement ? 4 : 99
- );
- if (state != -1 && state != x) {
- pw.println(AutoIndentWriter.CLEAR_TABULATORS);
- }
- state = x;
-
- unparseBlockStatement(bs);
- pw.println();
- }
- }
-}
diff --git a/pom.xml b/pom.xml
index 0cc2527..ca847d2 100644
--- a/pom.xml
+++ b/pom.xml
@@ -48,7 +48,7 @@
<parquet.version>1.10.0</parquet.version>
<calcite.version>1.17.0-drill-r1</calcite.version>
<avatica.version>1.12.0</avatica.version>
- <janino.version>3.0.10</janino.version>
+ <janino.version>3.0.11</janino.version>
<sqlline.version>1.5.0</sqlline.version>
<jackson.version>2.9.5</jackson.version>
<jackson.databind.version>2.9.5</jackson.databind.version>