drill-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ve...@apache.org
Subject [2/3] drill git commit: DRILL-1385, along with some cleanup
Date Mon, 16 Mar 2015 23:30:46 GMT
http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/MergeAdapter.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/MergeAdapter.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/MergeAdapter.java
index 62d439c..1522102 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/MergeAdapter.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/MergeAdapter.java
@@ -17,7 +17,6 @@
  */
 package org.apache.drill.exec.compile;
 
-import java.io.IOException;
 import java.io.PrintWriter;
 import java.io.StringWriter;
 import java.lang.reflect.Modifier;
@@ -26,14 +25,13 @@ import java.util.Iterator;
 import java.util.Set;
 
 import org.apache.drill.exec.compile.ClassTransformer.ClassSet;
+import org.apache.drill.exec.compile.bytecode.ValueHolderReplacementVisitor;
 import org.apache.drill.exec.compile.sig.SignatureHolder;
-import org.objectweb.asm.AnnotationVisitor;
 import org.objectweb.asm.ClassReader;
 import org.objectweb.asm.ClassVisitor;
 import org.objectweb.asm.ClassWriter;
 import org.objectweb.asm.FieldVisitor;
 import org.objectweb.asm.MethodVisitor;
-import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.commons.Remapper;
 import org.objectweb.asm.commons.RemappingClassAdapter;
 import org.objectweb.asm.commons.RemappingMethodAdapter;
@@ -41,7 +39,6 @@ import org.objectweb.asm.commons.SimpleRemapper;
 import org.objectweb.asm.tree.ClassNode;
 import org.objectweb.asm.tree.FieldNode;
 import org.objectweb.asm.tree.MethodNode;
-import org.objectweb.asm.util.CheckClassAdapter;
 import org.objectweb.asm.util.TraceClassVisitor;
 
 import com.google.common.collect.Sets;
@@ -51,19 +48,22 @@ import com.google.common.collect.Sets;
  * methods and fields of the class to merge to the class that is being visited.
  */
 class MergeAdapter extends ClassVisitor {
-
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(MergeAdapter.class);
-
-  private ClassNode classToMerge;
-  private ClassSet set;
-  private Set<String> mergingNames = Sets.newHashSet();
-  private boolean hasInit;
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(MergeAdapter.class);
+  private final ClassNode classToMerge;
+  private final ClassSet set;
+  private final Set<String> mergingNames = Sets.newHashSet();
+  private final boolean hasInit;
   private String name;
 
+  // when more mature, consider AssertionUtil.IsAssertionsEnabled()
+  private static final boolean verifyBytecode = true;
+
   private MergeAdapter(ClassSet set, ClassVisitor cv, ClassNode cn) {
-    super(Opcodes.ASM4, cv);
+    super(CompilationConfig.ASM_API_VERSION, cv);
     this.classToMerge = cn;
     this.set = set;
+
+    boolean hasInit = false;
     for (Object o  : classToMerge.methods) {
       String name = ((MethodNode)o).name;
       if (name.equals("<init>")) {
@@ -74,6 +74,8 @@ class MergeAdapter extends ClassVisitor {
       }
       mergingNames.add(name);
     }
+
+    this.hasInit = hasInit;
   }
 
   @Override
@@ -92,12 +94,6 @@ class MergeAdapter extends ClassVisitor {
     }
   }
 
-  @Override
-  public AnnotationVisitor visitAnnotation(String desc, boolean visible) {
-//    System.out.println("Annotation");
-    return super.visitAnnotation(desc, visible);
-  }
-
   // visit the class
   @Override
   public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) {
@@ -141,9 +137,7 @@ class MergeAdapter extends ClassVisitor {
   }
 
   @Override
-  @SuppressWarnings("unchecked")
   public void visitEnd() {
-
     // add all the fields of the class we're going to merge.
     for (Iterator<?> it = classToMerge.fields.iterator(); it.hasNext();) {
       ((FieldNode) it.next()).accept(this);
@@ -159,7 +153,10 @@ class MergeAdapter extends ClassVisitor {
 
       String[] exceptions = new String[mn.exceptions.size()];
       mn.exceptions.toArray(exceptions);
-      MethodVisitor   mv = cv.visitMethod(mn.access | Modifier.FINAL, mn.name, mn.desc, mn.signature, exceptions);
+      MethodVisitor mv = cv.visitMethod(mn.access | Modifier.FINAL, mn.name, mn.desc, mn.signature, exceptions);
+      if (verifyBytecode) {
+        mv = new CheckMethodVisitorFsm(api, mv);
+      }
 
       mn.instructions.resetLabels();
 
@@ -169,7 +166,8 @@ class MergeAdapter extends ClassVisitor {
       while (top.parent != null) {
         top = top.parent;
       }
-      mn.accept(new RemappingMethodAdapter(mn.access, mn.desc, mv, new SimpleRemapper(top.precompiled.slash, top.generated.slash)));
+      mn.accept(new RemappingMethodAdapter(mn.access, mn.desc, mv,
+          new SimpleRemapper(top.precompiled.slash, top.generated.slash)));
 
     }
     super.visitEnd();
@@ -180,31 +178,72 @@ class MergeAdapter extends ClassVisitor {
     return super.visitField(access, name, desc, signature, value);
   }
 
-  public static class MergedClassResult{
-    public byte[] bytes;
-    public Collection<String> innerClasses;
+  public static class MergedClassResult {
+    public final byte[] bytes;
+    public final Collection<String> innerClasses;
+
     public MergedClassResult(byte[] bytes, Collection<String> innerClasses) {
-      super();
       this.bytes = bytes;
       this.innerClasses = innerClasses;
     }
-
   }
 
-  public static MergedClassResult getMergedClass(ClassSet set, byte[] precompiledClass, ClassNode generatedClass) throws IOException{
-
-    // Setup adapters for merging, remapping class names and class writing. This is done in reverse order of how they
-    // will be evaluated.
+  public static MergedClassResult getMergedClass(final ClassSet set, final byte[] precompiledClass,
+      ClassNode generatedClass, final boolean scalarReplace) {
+    if (verifyBytecode) {
+      if (!AsmUtil.isClassBytesOk(logger, precompiledClass, "precompiledClass")) {
+        throw new IllegalStateException("Problem found in precompiledClass");
+      }
+      if ((generatedClass != null) && !AsmUtil.isClassOk(logger, generatedClass, "generatedClass")) {
+        throw new IllegalStateException("Problem found in generatedClass");
+      }
+    }
 
-    ClassWriter writer = new ClassWriter(ClassWriter.COMPUTE_FRAMES);
-    RemapClasses re = new RemapClasses(set);
+    /*
+     * Setup adapters for merging, remapping class names and class writing. This is done in
+     * reverse order of how they will be evaluated.
+     */
+    final RemapClasses re = new RemapClasses(set);
     try {
-//      if(generatedClass != null) {
-//        ClassNode generatedMerged = new ClassNode();
-//        generatedClass.accept(new ValueHolderReplacementVisitor(generatedMerged));
-//        generatedClass = generatedMerged;
-//      }
-      ClassVisitor remappingAdapter = new RemappingClassAdapter(writer, re);
+      if (scalarReplace && generatedClass != null) {
+        if (logger.isDebugEnabled()) {
+          AsmUtil.logClass(logger, "generated " + set.generated.dot, generatedClass);
+        }
+
+        final ClassNode generatedMerged = new ClassNode();
+        ClassVisitor mergeGenerator = generatedMerged;
+        if (verifyBytecode) {
+          mergeGenerator = new DrillCheckClassAdapter(CompilationConfig.ASM_API_VERSION,
+              new CheckClassVisitorFsm(CompilationConfig.ASM_API_VERSION, generatedMerged), true);
+        }
+
+        /*
+         * Even though we're effectively transforming-creating a new class in mergeGenerator,
+         * there's no way to pass in ClassWriter.COMPUTE_MAXS, which would save us from having
+         * to figure out stack size increases on our own. That gets handled by the
+         * InstructionModifier (from inside ValueHolderReplacement > ScalarReplacementNode).
+         */
+        generatedClass.accept(new ValueHolderReplacementVisitor(mergeGenerator, verifyBytecode));
+        if (verifyBytecode) {
+          if (!AsmUtil.isClassOk(logger, generatedMerged, "generatedMerged")) {
+            throw new IllegalStateException("Problem found with generatedMerged");
+          }
+        }
+        generatedClass = generatedMerged;
+      }
+
+      final ClassWriter writer = new ClassWriter(ClassWriter.COMPUTE_FRAMES);
+      ClassVisitor writerVisitor = writer;
+      if (verifyBytecode) {
+        writerVisitor = new DrillCheckClassAdapter(CompilationConfig.ASM_API_VERSION,
+            new CheckClassVisitorFsm(CompilationConfig.ASM_API_VERSION, writerVisitor), true);
+      }
+      ClassVisitor remappingAdapter = new RemappingClassAdapter(writerVisitor, re);
+      if (verifyBytecode) {
+        remappingAdapter = new DrillCheckClassAdapter(CompilationConfig.ASM_API_VERSION,
+            new CheckClassVisitorFsm(CompilationConfig.ASM_API_VERSION, remappingAdapter), true);
+      }
+
       ClassVisitor visitor = remappingAdapter;
       if (generatedClass != null) {
         visitor = new MergeAdapter(set, remappingAdapter, generatedClass);
@@ -212,6 +251,9 @@ class MergeAdapter extends ClassVisitor {
       ClassReader tReader = new ClassReader(precompiledClass);
       tReader.accept(visitor, ClassReader.SKIP_FRAMES);
       byte[] outputClass = writer.toByteArray();
+      if (logger.isDebugEnabled()) {
+        AsmUtil.logClassFromBytes(logger, "merged " + set.generated.dot, outputClass);
+      }
 
       // enable when you want all the generated merged class files to also be written to disk.
 //      Files.write(outputClass, new File(String.format("/src/scratch/drill-generated-classes/%s-output.class", set.generated.dot)));
@@ -219,6 +261,7 @@ class MergeAdapter extends ClassVisitor {
       return new MergedClassResult(outputClass, re.getInnerClasses());
     } catch (Error | RuntimeException e) {
       logger.error("Failure while merging classes.", e);
+      AsmUtil.logClass(logger, "generatedClass", generatedClass);
       throw e;
     }
   }
@@ -267,12 +310,12 @@ class MergeAdapter extends ClassVisitor {
 
     try {
       ClassWriter cw = new ClassWriter(ClassWriter.COMPUTE_FRAMES);
-      ClassVisitor cv = new CheckClassAdapter(cw, true);
+      ClassVisitor cv = new DrillCheckClassAdapter(CompilationConfig.ASM_API_VERSION, cw, true);
       node.accept(cv);
 
       StringWriter sw = new StringWriter();
       PrintWriter pw = new PrintWriter(sw);
-      CheckClassAdapter.verify(new ClassReader(cw.toByteArray()), false, pw);
+      DrillCheckClassAdapter.verify(new ClassReader(cw.toByteArray()), false, pw);
 
       error = sw.toString();
     } catch (Exception ex) {
@@ -285,11 +328,12 @@ class MergeAdapter extends ClassVisitor {
       TraceClassVisitor v = new TraceClassVisitor(pw2);
       node.accept(v);
       if (e != null) {
-        throw new RuntimeException("Failure validating class.  ByteCode: \n" + sw2.toString() + "\n\n====ERRROR====\n" + error, e);
+        throw new RuntimeException("Failure validating class.  ByteCode: \n" +
+            sw2.toString() + "\n\n====ERRROR====\n" + error, e);
       } else {
-        throw new RuntimeException("Failure validating class.  ByteCode: \n" + sw2.toString() + "\n\n====ERRROR====\n" + error);
+        throw new RuntimeException("Failure validating class.  ByteCode: \n" +
+            sw2.toString() + "\n\n====ERRROR====\n" + error);
       }
     }
   }
-
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java
index 065c3c8..d49b500 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java
@@ -82,7 +82,7 @@ public class QueryClassLoader extends URLClassLoader {
 
   public void injectByteCode(String className, byte[] classBytes) throws IOException {
     if (customClasses.containsKey(className)) {
-      throw new IOException(String.format("The class defined {} has already been loaded.", className));
+      throw new IOException(String.format("The class defined %s has already been loaded.", className));
     }
     customClasses.put(className, classBytes);
   }

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/RetargetableClassVisitor.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/RetargetableClassVisitor.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/RetargetableClassVisitor.java
new file mode 100644
index 0000000..72e55b7
--- /dev/null
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/RetargetableClassVisitor.java
@@ -0,0 +1,62 @@
+/**
+ * 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;
+
+import org.objectweb.asm.ClassVisitor;
+
+/**
+ * An ASM ClassVisitor that allows for a late-bound delegate.
+ *
+ * <p>The ClassVisitor constructor takes an optional ClassVisitor argument
+ * to which all calls will be delegated. However, in some circumstances, ClassVisitor
+ * derivatives may not be able to specify the delegate until after they have called
+ * the ClassVisitor constructor themselves. In other words, they may not be able to
+ * specify the delegate until *after* they have called super(). This version of the
+ * ClassVisitor will support that via the use of the {@link #setDelegate} method.
+ */
+public class RetargetableClassVisitor extends ClassVisitor {
+  private boolean started;
+
+  /**
+   * See {@link org.objectweb.asm.ClassVisitor#ClassVisitor(int)}.
+   */
+  public RetargetableClassVisitor(final int api) {
+    super(api);
+  }
+
+  /**
+   * See {@link org.objectweb.asm.ClassVisitor#ClassVisitor(int, ClassVisitor)}.
+   */
+  public RetargetableClassVisitor(final int api, final ClassVisitor cv) {
+    super(api, cv);
+  }
+
+  /**
+   * Set the delegate for this ClassVisitor. Must be called before the visitor is
+   * used (i.e., before any other methods are called), and only once.
+   * @param cv the ClassVisitor to delegate calls to
+   */
+  protected void setDelegate(final ClassVisitor cv) {
+    if (started) {
+      throw new IllegalStateException("can't change delegate after visitor has been used");
+    }
+
+    // This member was only protected, so we can take advantage of that to set it here.
+    this.cv = cv;
+  }
+}

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/TemplateClassDefinition.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/TemplateClassDefinition.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/TemplateClassDefinition.java
index 2b6ddf0..ec5bfcd 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/TemplateClassDefinition.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/TemplateClassDefinition.java
@@ -65,12 +65,4 @@ public class TemplateClassDefinition<T>{
   public String toString() {
     return "TemplateClassDefinition [template=" + template + ", signature=" + signature + "]";
   }
-
-  @Override
-  public boolean equals(Object obj) {
-    return super.equals(obj);
-  }
-
-
-
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/AloadPopRemover.java
----------------------------------------------------------------------
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
new file mode 100644
index 0000000..1200855
--- /dev/null
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/AloadPopRemover.java
@@ -0,0 +1,333 @@
+/**
+ * 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);
+    }
+  }
+}

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/DirectSorter.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/DirectSorter.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/DirectSorter.java
index 4e54bc7..31110a8 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/DirectSorter.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/DirectSorter.java
@@ -17,6 +17,7 @@
  */
 package org.apache.drill.exec.compile.bytecode;
 
+import org.apache.drill.exec.compile.CompilationConfig;
 import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.commons.LocalVariablesSorter;
 
@@ -24,7 +25,7 @@ public class DirectSorter extends LocalVariablesSorter{
   static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DirectSorter.class);
 
   public DirectSorter(int access, String desc, MethodVisitor mv) {
-    super(access, desc, mv);
+    super(CompilationConfig.ASM_API_VERSION, access, desc, mv);
   }
 
   public void directVarInsn(int opcode, int var) {

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java
index 4585bd8..6c0292e 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java
@@ -17,86 +17,275 @@
  */
 package org.apache.drill.exec.compile.bytecode;
 
+import java.util.HashMap;
+
+import org.apache.drill.exec.compile.CompilationConfig;
 import org.apache.drill.exec.compile.bytecode.ValueHolderIden.ValueHolderSub;
 import org.objectweb.asm.Label;
 import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.Type;
+import org.objectweb.asm.tree.analysis.BasicValue;
+import org.objectweb.asm.tree.analysis.Frame;
 
 import com.carrotsearch.hppc.IntIntOpenHashMap;
 import com.carrotsearch.hppc.IntObjectOpenHashMap;
+import com.carrotsearch.hppc.cursors.IntIntCursor;
+import com.carrotsearch.hppc.cursors.IntObjectCursor;
+import com.google.common.base.Preconditions;
 
 public class InstructionModifier extends MethodVisitor {
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(InstructionModifier.class);
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(InstructionModifier.class);
 
+  /* Map from old (reference) local variable index to new local variable information. */
   private final IntObjectOpenHashMap<ValueHolderIden.ValueHolderSub> oldToNew = new IntObjectOpenHashMap<>();
-  private final IntIntOpenHashMap oldLocalToFirst = new IntIntOpenHashMap();
 
-  private DirectSorter adder;
-  int lastLineNumber = 0;
-  private TrackingInstructionList list;
+  private final IntIntOpenHashMap oldLocalToFirst = new IntIntOpenHashMap();
 
-  public InstructionModifier(int access, String name, String desc, String signature, String[] exceptions,
-      TrackingInstructionList list, MethodVisitor inner) {
-    super(Opcodes.ASM4, new DirectSorter(access, desc, inner));
+  private final DirectSorter adder;
+  private int lastLineNumber = 0; // the last line number seen
+  private final TrackingInstructionList list;
+  private final String name;
+  private final String desc;
+  private final String signature;
+
+  private int stackIncrease = 0; // how much larger we have to make the stack
+
+  /*
+   * True if we've transferred holder members to locals. If so, then the maximum stack
+   * size must be increased by one to accommodate the extra DUP that is used to do so.
+   */
+  private boolean transferredToLocals = false;
+
+  public InstructionModifier(final int access, final String name, final String desc,
+      final String signature, final String[] exceptions, final TrackingInstructionList list,
+      final MethodVisitor inner) {
+    super(CompilationConfig.ASM_API_VERSION, new DirectSorter(access, desc, inner));
+    this.name = name;
+    this.desc = desc;
+    this.signature = signature;
     this.list = list;
     this.adder = (DirectSorter) mv;
   }
 
-  public void setList(TrackingInstructionList list) {
-    this.list = list;
+  public int getLastLineNumber() {
+    return lastLineNumber;
   }
 
-  private ReplacingBasicValue local(int var) {
-    Object o = list.currentFrame.getLocal(var);
-    if (o instanceof ReplacingBasicValue) {
-      return (ReplacingBasicValue) o;
+  private static ReplacingBasicValue filterReplacement(final BasicValue basicValue) {
+    if (basicValue instanceof ReplacingBasicValue) {
+      final ReplacingBasicValue replacingValue = (ReplacingBasicValue) basicValue;
+      if (replacingValue.isReplaceable()) {
+        return replacingValue;
+      }
     }
+
     return null;
   }
 
-  private ReplacingBasicValue popCurrent() {
-    return popCurrent(false);
+  private ReplacingBasicValue getLocal(final int var) {
+    final BasicValue basicValue = list.currentFrame.getLocal(var);
+    return filterReplacement(basicValue);
   }
 
-  private ReplacingBasicValue popCurrent(boolean includeReturnVals) {
-    // for vararg, we could try to pop an empty stack. TODO: handle this better.
-    if (list.currentFrame.getStackSize() == 0) {
-      return null;
-    }
-
-    Object o = list.currentFrame.pop();
-    if (o instanceof ReplacingBasicValue) {
-      ReplacingBasicValue v = (ReplacingBasicValue) o;
-      if (!v.isFunctionReturn || includeReturnVals) {
-        return v;
-      }
-    }
-    return null;
+  /**
+   * Peek at a value in the current frame's stack, counting down from the top.
+   *
+   * @param depth how far down to peek; 0 is the top of the stack, 1 is the
+   *   first element down, etc
+   * @return the value on the stack, or null if it isn't a ReplacingBasciValue
+   */
+  private ReplacingBasicValue peekFromTop(final int depth) {
+    Preconditions.checkArgument(depth >= 0);
+    final Frame<BasicValue> frame = list.currentFrame;
+    final BasicValue basicValue = frame.getStack((frame.getStackSize() - 1) - depth);
+    return filterReplacement(basicValue);
   }
 
-  private ReplacingBasicValue getReturn() {
-    Object o = list.nextFrame.getStack(list.nextFrame.getStackSize() - 1);
-    if (o instanceof ReplacingBasicValue) {
-      return (ReplacingBasicValue) o;
-    }
-    return null;
+  /**
+   * Get the value of a function return if it is a ReplacingBasicValue.
+   *
+   * <p>Assumes that we're in the middle of processing an INVOKExxx instruction.
+   *
+   * @return the value that will be on the top of the stack after the function returns
+   */
+  private ReplacingBasicValue getFunctionReturn() {
+    final Frame<BasicValue> nextFrame = list.nextFrame;
+    final BasicValue basicValue = nextFrame.getStack(nextFrame.getStackSize() - 1);
+    return filterReplacement(basicValue);
   }
 
   @Override
   public void visitInsn(int opcode) {
     switch (opcode) {
     case Opcodes.DUP:
-      if (popCurrent() != null) {
+      /*
+       * Pattern:
+       *   BigIntHolder out5 = new BigIntHolder();
+       *
+       * Original bytecode:
+       *   NEW org/apache/drill/exec/expr/holders/BigIntHolder
+       *   DUP
+       *   INVOKESPECIAL org/apache/drill/exec/expr/holders/BigIntHolder.<init> ()V
+       *   ASTORE 6 # the index of the out5 local variable (which is a reference)
+       *
+       * Desired replacement:
+       *   ICONST_0
+       *   ISTORE 12
+       *
+       *   In the original, the holder's objectref will be used twice: once for the
+       *   constructor call, and then to be stored. Since the holder has been replaced
+       *   with one or more locals to hold its members, we don't allocate or store it.
+       *   he NEW and the ASTORE are replaced via some stateless changes elsewhere; here
+       *   we need to eliminate the DUP.
+       *
+       * TODO: there may be other reasons for a DUP to appear in the instruction stream,
+       * such as reuse of a common subexpression that the compiler optimizer has
+       * eliminated. This pattern may also be used for non-holders. We need to be
+       * more certain of the source of the DUP, and whether the surrounding context matches
+       * the above.
+       */
+      if (peekFromTop(0) != null) {
+        return; // don't emit the DUP
+      }
+      break;
+
+    case Opcodes.DUP_X1: {
+      /*
+       * There are a number of patterns that lead to this instruction being generated. Here
+       * are some examples:
+       *
+       * Pattern:
+       *   129:        out.start = out.end = text.end;
+       *
+       * Original bytecode:
+       *   L9
+       *    LINENUMBER 129 L9
+       *    ALOAD 7
+       *    ALOAD 7
+       *    ALOAD 8
+       *    GETFIELD org/apache/drill/exec/expr/holders/VarCharHolder.end : I
+       *    DUP_X1
+       *    PUTFIELD org/apache/drill/exec/expr/holders/VarCharHolder.end : I
+       *    PUTFIELD org/apache/drill/exec/expr/holders/VarCharHolder.start : I
+       *
+       * Desired replacement:
+       *   L9
+       *    LINENUMBER 129 L9
+       *    ILOAD 17
+       *    DUP
+       *    ISTORE 14
+       *    ISTORE 13
+       *
+       *    At this point, the ALOAD/GETFIELD and ALOAD/PUTFIELD combinations have
+       *    been replaced by the ILOAD and ISTOREs. However, there is still the DUP_X1
+       *    in the instruction stream. In this case, it is duping the fetched holder
+       *    member so that it can be stored again. We still need to do that, but because
+       *    the ALOADed objectrefs are no longer on the stack, we don't need to duplicate
+       *    the value lower down in the stack anymore, but can instead DUP it where it is.
+       *    (Similarly, if the fetched field was a long or double, the same applies for
+       *    the expected DUP2_X1.)
+       *
+       * There's also a similar pattern for zeroing values:
+       * Pattern:
+       *   170:            out.start = out.end = 0;
+       *
+       * Original bytecode:
+       *   L20
+       *    LINENUMBER 170 L20
+       *    ALOAD 13
+       *    ALOAD 13
+       *    ICONST_0
+       *    DUP_X1
+       *    PUTFIELD org/apache/drill/exec/expr/holders/VarCharHolder.end : I
+       *    PUTFIELD org/apache/drill/exec/expr/holders/VarCharHolder.start : I
+       *
+       * Desired replacement:
+       *   L20
+       *    LINENUMBER 170 L20
+       *    ICONST_0
+       *    DUP
+       *    ISTORE 17
+       *    ISTORE 16
+       *
+       *
+       * There's also another pattern that involves DUP_X1
+       * Pattern:
+       *   1177:                   out.buffer.setByte(out.end++, currentByte);
+       *
+       * We're primarily interested in the out.end++ -- the post-increment of
+       * a holder member.
+       *
+       * Original bytecode:
+       *    L694
+       *     LINENUMBER 1177 L694
+       *     ALOAD 212
+       *     GETFIELD org/apache/drill/exec/expr/holders/VarCharHolder.buffer : Lio/netty/buffer/DrillBuf;
+       *     ALOAD 212
+       *     DUP
+       * >   GETFIELD org/apache/drill/exec/expr/holders/VarCharHolder.end : I
+       * >   DUP_X1
+       * >   ICONST_1
+       * >   IADD
+       * >   PUTFIELD org/apache/drill/exec/expr/holders/VarCharHolder.end : I
+       *     ILOAD 217
+       *     INVOKEVIRTUAL io/netty/buffer/DrillBuf.setByte (II)Lio/netty/buffer/ByteBuf;
+       *     POP
+       *
+       * This fragment includes the entirety of the line 1177 above, but we're only concerned with
+       * the lines marked with '>' on the left; the rest were kept for context, because it is the
+       * use of the pre-incremented value as a function argument that is generating the DUP_X1 --
+       * the DUP_X1 is how the value is preserved before incrementing.
+       *
+       * The common element in these patterns is that the stack has an objectref and a value that will
+       * be stored via a PUTFIELD. The value has to be used in other contexts, so it is to be DUPed, and
+       * stashed away under the objectref. In the case where the objectref belongs to a holder that will
+       * be gone as a result of scalar replacement, then the objectref won't be present, so we don't need
+       * the _X1 option.
+       *
+       * If we're replacing the holder under the value being duplicated, then we don't need to put the
+       * DUPed value back under it, because it won't be present in the stack. We can just use a plain DUP.
+       */
+      final ReplacingBasicValue rbValue = peekFromTop(1);
+      if (rbValue != null) {
+        super.visitInsn(Opcodes.DUP);
         return;
       }
+      break;
     }
+
+    case Opcodes.DUP2_X1: {
+      /*
+       * See the comments above for DUP_X1, which also apply here; this just handles long and double
+       * values, which are twice as large, in the same way.
+       */
+      if (peekFromTop(0) != null) {
+        throw new IllegalStateException("top of stack should be 2nd part of a long or double");
+      }
+      final ReplacingBasicValue rbValue = peekFromTop(2);
+      if (rbValue != null) {
+        super.visitInsn(Opcodes.DUP2);
+        return;
+      }
+      break;
+    }
+    }
+
+    // if we get here, emit the original instruction
     super.visitInsn(opcode);
   }
 
   @Override
   public void visitTypeInsn(int opcode, String type) {
-    ReplacingBasicValue r = getReturn();
+    /*
+     * This includes NEW, NEWARRAY, CHECKCAST, or INSTANCEOF.
+     *
+     * TODO: aren't we just trying to eliminate NEW (and possibly NEWARRAY)?
+     * It looks like we'll currently pick those up because we'll only have
+     * replaced the values for those, but we might find other reasons to replace
+     * things, in which case this will be too broad.
+     */
+    ReplacingBasicValue r = getFunctionReturn();
     if (r != null) {
       ValueHolderSub sub = r.getIden().getHolderSub(adder);
       oldToNew.put(r.getIndex(), sub);
@@ -112,147 +301,216 @@ public class InstructionModifier extends MethodVisitor {
   }
 
   @Override
-  public void visitVarInsn(int opcode, int var) {
+  public void visitVarInsn(final int opcode, final int var) {
     ReplacingBasicValue v;
-    if (opcode == Opcodes.ASTORE && (v = popCurrent(true)) != null) {
-      if (!v.isFunctionReturn) {
-        ValueHolderSub from = oldToNew.get(v.getIndex());
-
-        ReplacingBasicValue current = local(var);
-        // if local var is set, then transfer to it to the existing holders in the local position.
-        if (current != null) {
-          if (oldToNew.get(current.getIndex()).iden() == from.iden()) {
-            int targetFirst = oldToNew.get(current.index).first();
-            from.transfer(this, targetFirst);
-            return;
-          }
-        }
-
-        // if local var is not set, then check map to see if existing holders are mapped to local var.
-        if (oldLocalToFirst.containsKey(var)) {
-          ValueHolderSub sub = oldToNew.get(oldLocalToFirst.lget());
-          if (sub.iden() == from.iden()) {
-            // if they are, then transfer to that.
-            from.transfer(this, oldToNew.get(oldLocalToFirst.lget()).first());
-            return;
-          }
+    if (opcode == Opcodes.ASTORE && (v = peekFromTop(0)) != null) {
+      final ValueHolderSub from = oldToNew.get(v.getIndex());
+
+      final ReplacingBasicValue current = getLocal(var);
+      // if local var is set, then transfer to it to the existing holders in the local position.
+      if (current != null) {
+        final ValueHolderSub newSub = oldToNew.get(current.getIndex());
+        if (newSub.iden() == from.iden()) {
+          final int targetFirst = newSub.first();
+          from.transfer(this, targetFirst);
+          return;
         }
+      }
 
-
-        // map from variables to global space for future use.
-        oldLocalToFirst.put(var, v.getIndex());
-
-      } else {
-        // this is storage of a function return, we need to map the fields to the holder spots.
-        int first;
-        if (oldLocalToFirst.containsKey(var)) {
-          first = oldToNew.get(oldLocalToFirst.lget()).first();
-          v.iden.transferToLocal(adder, first);
-        } else {
-          first = v.iden.createLocalAndTrasfer(adder);
+      // if local var is not set, then check map to see if existing holders are mapped to local var.
+      if (oldLocalToFirst.containsKey(var)) {
+        final ValueHolderSub sub = oldToNew.get(oldLocalToFirst.lget());
+        if (sub.iden() == from.iden()) {
+          // if they are, then transfer to that.
+          from.transfer(this, sub.first());
+          return;
         }
-        ValueHolderSub from = v.iden.getHolderSubWithDefinedLocals(first);
-        oldToNew.put(v.getIndex(), from);
-        v.disableFunctionReturn();
       }
 
-    } else if (opcode == Opcodes.ALOAD && (v = getReturn()) != null) {
-      // noop.
-    } else {
-      super.visitVarInsn(opcode, var);
+      // map from variables to global space for future use.
+      oldLocalToFirst.put(var, v.getIndex());
+      return;
+    } else if (opcode == Opcodes.ALOAD && (v = getLocal(var)) != null) {
+      /*
+       * Not forwarding this removes a now unnecessary ALOAD for a holder. The required LOAD/STORE
+       * sequences will be generated by the ASTORE code above.
+       */
+      return;
     }
 
+    super.visitVarInsn(opcode, var);
   }
 
-  void directVarInsn(int opcode, int var) {
+  void directVarInsn(final int opcode, final int var) {
     adder.directVarInsn(opcode, var);
   }
 
   @Override
-  public void visitFieldInsn(int opcode, String owner, String name, String desc) {
-    ReplacingBasicValue v;
+  public void visitMaxs(final int maxStack, final int maxLocals) {
+    super.visitMaxs(maxStack + stackIncrease + (transferredToLocals ? 1 : 0), maxLocals);
+  }
 
+  @Override
+  public void visitFieldInsn(final int opcode, final String owner,
+      final String name, final String desc) {
+    int stackDepth = 0;
+    ReplacingBasicValue value;
     switch (opcode) {
     case Opcodes.PUTFIELD:
-      // pop twice for put.
-      v = popCurrent(true);
-      if (v != null) {
-        if (v.isFunctionReturn) {
+      value = peekFromTop(stackDepth++);
+      if (value != null) {
+        if (filterReplacement(value) == null) {
           super.visitFieldInsn(opcode, owner, name, desc);
           return;
         } else {
-          // we are trying to store a replaced variable in an external context, we need to generate an instance and
-          // transfer it out.
-          ValueHolderSub sub = oldToNew.get(v.getIndex());
-          sub.transferToExternal(adder, owner, name, desc);
+          /*
+           * We are trying to store a replaced variable in an external context,
+           * we need to generate an instance and transfer it out.
+           */
+          final ValueHolderSub sub = oldToNew.get(value.getIndex());
+          final int additionalStack = sub.transferToExternal(adder, owner, name, desc);
+          if (additionalStack > stackIncrease) {
+            stackIncrease = additionalStack;
+          }
           return;
         }
       }
+      // $FALL-THROUGH$
 
     case Opcodes.GETFIELD:
-      // pop again.
-      v = popCurrent();
-      if (v != null) {
-        // super.visitFieldInsn(opcode, owner, name, desc);
-        ValueHolderSub sub = oldToNew.get(v.getIndex());
-        sub.addInsn(name, this, opcode);
-        return;
+      // if falling through from PUTFIELD, this gets the 2nd item down
+      value = peekFromTop(stackDepth);
+      if (value != null) {
+        if (filterReplacement(value) != null) {
+          final ValueHolderSub sub = oldToNew.get(value.getIndex());
+          if (sub != null) {
+            sub.addInsn(name, this, opcode);
+            return;
+          }
+        }
       }
+      /* FALLTHROUGH */
     }
 
+    // if we get here, emit the field reference as-is
     super.visitFieldInsn(opcode, owner, name, desc);
   }
 
   @Override
-  public void visitLocalVariable(String name, String desc, String signature, Label start, Label end, int index) {
-    // if (!ReplacingInterpreter.HOLDER_DESCRIPTORS.contains(desc)) {
-    // super.visitLocalVariable(name, desc, signature, start, end, index);
-    // }
+  public void visitMethodInsn(int opcode, String owner, String name, String desc) {
+    /*
+     * This method was deprecated in the switch from api version ASM4 to ASM5.
+     * If we ever go back (via CompilationConfig.ASM_API_VERSION), we need to
+     * duplicate the work from the other overloaded version of this method.
+     */
+    assert CompilationConfig.ASM_API_VERSION == Opcodes.ASM4;
+    throw new RuntimeException("this method is deprecated");
   }
 
   @Override
-  public void visitMethodInsn(int opcode, String owner, String name, String desc) {
-    final int len = Type.getArgumentTypes(desc).length;
-    final boolean isStatic = opcode == Opcodes.INVOKESTATIC;
-
-    ReplacingBasicValue obj = popCurrent();
-
-    if (obj != null && !isStatic) {
-      if ("<init>".equals(name)) {
-        oldToNew.get(obj.getIndex()).init(adder);
-      } else {
-        throw new IllegalStateException("you can't call a method on a value holder.");
+  public void visitMethodInsn(int opcode, String owner, String name, String desc, boolean itf) {
+    // this version of visitMethodInsn() came after ASM4
+    assert CompilationConfig.ASM_API_VERSION != Opcodes.ASM4;
+
+    final int argCount = Type.getArgumentTypes(desc).length;
+    if (opcode != Opcodes.INVOKESTATIC) {
+      final ReplacingBasicValue thisRef = peekFromTop(argCount);
+
+      if (thisRef != null) {
+        /*
+         * If the this reference is a holder, we need to initialize the variables
+         * that replaced it; that amounts to replacing its constructor call with
+         * variable initializations.
+         */
+        if (name.equals("<init>")) {
+          oldToNew.get(thisRef.getIndex()).init(adder);
+          return;
+        } else {
+          /*
+           * This is disallowed because the holder variables are being ignored in
+           * favor of the locals we've replaced them with.
+           */
+          throw new IllegalStateException("You can't call a method on a value holder.");
+        }
       }
-      return;
     }
 
-    obj = getReturn();
-
-    if (obj != null) {
-      // the return of this method is an actual instance of the object we're escaping. Update so that it get's mapped
-      // correctly.
-      super.visitMethodInsn(opcode, owner, name, desc);
-      obj.markFunctionReturn();
+    /*
+     * If we got here, we're not calling a method on a holder.
+     *
+     * Does the function being called return a holder?
+     */
+    final ReplacingBasicValue functionReturn = getFunctionReturn();
+    if (functionReturn != null) {
+      /*
+       * The return of this method is an actual instance of the object we're escaping.
+       * Update so that it gets mapped correctly.
+       */
+      super.visitMethodInsn(opcode, owner, name, desc, itf);
+      functionReturn.markFunctionReturn();
       return;
     }
 
-    int i = isStatic ? 1 : 0;
-    for (; i < len; i++) {
-      checkArg(name, popCurrent());
+    /*
+     * Holders can't be passed as arguments to methods, because their contents aren't
+     * maintained; we use the locals instead. Therefore, complain if any arguments are holders.
+     */
+    for(int argDepth = argCount - 1; argDepth >= 0; --argDepth) {
+      final ReplacingBasicValue argValue = peekFromTop(argDepth);
+      if (argValue != null) {
+        throw new IllegalStateException(
+            String.format("Holder types are not allowed to be passed between methods. " +
+                "Ran across problem attempting to invoke method '%s' on line number %d",
+                name, lastLineNumber));
+      }
     }
 
-    super.visitMethodInsn(opcode, owner, name, desc);
+    // if we get here, emit this function call as it was
+    super.visitMethodInsn(opcode, owner, name, desc, itf);
   }
 
-  private void checkArg(String name, ReplacingBasicValue obj) {
-    if (obj == null) {
-      return;
+  @Override
+  public void visitEnd() {
+    if (logger.isDebugEnabled()) {
+      final StringBuilder sb = new StringBuilder();
+      sb.append("InstructionModifier ");
+      sb.append(name);
+      sb.append(' ');
+      sb.append(signature);
+      sb.append('\n');
+      if ((desc != null) && !desc.isEmpty()) {
+        sb.append("  desc: ");
+        sb.append(desc);
+        sb.append('\n');
+      }
+
+      int idenId = 0; // used to generate unique ids for the ValueHolderIden's
+      int itemCount = 0; // counts up the number of items found
+      final HashMap<ValueHolderIden, Integer> seenIdens = new HashMap<>(); // iden -> idenId
+      sb.append(" .oldToNew:\n");
+      for (IntObjectCursor<ValueHolderIden.ValueHolderSub> ioc : oldToNew) {
+        final ValueHolderIden iden = ioc.value.iden();
+        if (!seenIdens.containsKey(iden)) {
+          seenIdens.put(iden, ++idenId);
+          sb.append("ValueHolderIden[" + idenId + "]:\n");
+          iden.dump(sb);
+        }
+
+        sb.append("  " + ioc.key + " => " + ioc.value + '[' + seenIdens.get(iden) + "]\n");
+        ++itemCount;
+      }
+
+      sb.append(" .oldLocalToFirst:\n");
+      for (IntIntCursor iic : oldLocalToFirst) {
+        sb.append("  " + iic.key + " => " + iic.value + '\n');
+        ++itemCount;
+      }
+
+      if (itemCount > 0) {
+        logger.debug(sb.toString());
+      }
     }
-    throw new IllegalStateException(
-        String
-            .format(
-                "Holder types are not allowed to be passed between methods.  Ran across problem attempting to invoke method '%s' on line number %d",
-                name, lastLineNumber));
-  }
 
+    super.visitEnd();
+  }
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/MethodAnalyzer.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/MethodAnalyzer.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/MethodAnalyzer.java
new file mode 100644
index 0000000..d0b8627
--- /dev/null
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/MethodAnalyzer.java
@@ -0,0 +1,105 @@
+/**
+ * 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.tree.analysis.Analyzer;
+import org.objectweb.asm.tree.analysis.Frame;
+import org.objectweb.asm.tree.analysis.Interpreter;
+import org.objectweb.asm.tree.analysis.Value;
+
+/**
+ * Analyzer that allows us to inject additional functionality into ASMs basic analysis.
+ *
+ * <p>We need to be able to keep track of local variables that are assigned to each other
+ * so that we can infer their replacability (for scalar replacement). In order to do that,
+ * we need to know when local variables are assigned (with the old value being overwritten)
+ * so that we can associate them with the new value, and hence determine whether they can
+ * also be replaced, or not.
+ *
+ * <p>In order to capture the assignment operation, we have to provide our own Frame<>, but
+ * ASM doesn't provide a direct way to do that. Here, we use the Analyzer's newFrame() methods
+ * as factories that will provide our own derivative of Frame<> which we use to detect
+ */
+public class MethodAnalyzer<V extends Value> extends Analyzer <V> {
+  /**
+   * Custom Frame<> that captures setLocal() calls in order to associate values
+   * that are assigned to the same local variable slot.
+   *
+   * <p>Since this is almost a pass-through, the constructors' arguments match
+   * those from Frame<>.
+   */
+  private static class AssignmentTrackingFrame<V extends Value> extends Frame<V> {
+    /**
+     * Constructor.
+     *
+     * @param nLocals the number of locals the frame should have
+     * @param nStack the maximum size of the stack the frame should have
+     */
+    public AssignmentTrackingFrame(final int nLocals, final int nStack) {
+      super(nLocals, nStack);
+    }
+
+    /**
+     * Copy constructor.
+     *
+     * @param src the frame being copied
+     */
+    public AssignmentTrackingFrame(final Frame<? extends V> src) {
+      super(src);
+    }
+
+    @Override
+    public void setLocal(final int i, final V value) {
+      /*
+       * If we're replacing one ReplacingBasicValue with another, we need to
+       * associate them together so that they will have the same replacability
+       * attributes. We also track the local slot the new value will be stored in.
+       */
+      if (value instanceof ReplacingBasicValue) {
+        final ReplacingBasicValue replacingValue = (ReplacingBasicValue) value;
+        replacingValue.setFrameSlot(i);
+        final V localValue = getLocal(i);
+        if ((localValue != null) && (localValue instanceof ReplacingBasicValue)) {
+          final ReplacingBasicValue localReplacingValue = (ReplacingBasicValue) localValue;
+          localReplacingValue.associate(replacingValue);
+        }
+      }
+
+      super.setLocal(i, value);
+    }
+  }
+
+  /**
+   * Constructor.
+   *
+   * @param interpreter the interpreter to use
+   */
+  public MethodAnalyzer(final Interpreter<V> interpreter) {
+    super(interpreter);
+  }
+
+  @Override
+  protected Frame<V> newFrame(final int maxLocals, final int maxStack) {
+    return new AssignmentTrackingFrame<V>(maxLocals, maxStack);
+  }
+
+  @Override
+  protected Frame<V> newFrame(final Frame<? extends V> src) {
+    return new AssignmentTrackingFrame<V>(src);
+  }
+}

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingBasicValue.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingBasicValue.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingBasicValue.java
index 7b24174..13f4575 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingBasicValue.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingBasicValue.java
@@ -17,42 +17,425 @@
  */
 package org.apache.drill.exec.compile.bytecode;
 
+import java.util.HashSet;
+import java.util.IdentityHashMap;
+import java.util.List;
+
+import org.apache.drill.exec.util.AssertionUtil;
 import org.objectweb.asm.Type;
 import org.objectweb.asm.tree.analysis.BasicValue;
 
-public class ReplacingBasicValue extends BasicValue{
+/**
+ * BasicValue with additional tracking information used to determine
+ * the replaceability of the value (a holder, or boxed value) for scalar replacement purposes.
+ *
+ * <p>Contains a set of flags that indicate how the value is used. These
+ * are updated throughout the life of the function, via the Analyzer/Interpreter,
+ * which simulate execution of the function. After the analysis is complete, the
+ * flags indicate how the value/variable was used, and that in turn indicates whether
+ * or not we can replace it.
+ */
+public class ReplacingBasicValue extends BasicValue {
+  private final ValueHolderIden iden; // identity of the holder this value represents
+  private final int index; // the original local variable slot this value/holder was assigned
+
+  /**
+   * The set of flags associated with this value.
+   *
+   * <p>This is factored out so that it can be shared among several values
+   * that are associated with each other.
+   */
+  private static class FlagSet {
+    boolean isFunctionReturn = false;
+    boolean isFunctionArgument = false;
+    boolean isAssignedToMember = false;
+    boolean isThis = false;
+
+    /**
+     * Merge the given flag set into this one, creating a logical disjunction of the
+     * values.
+     *
+     * @param other the flag set to merge into this one
+     */
+    public void mergeFrom(final FlagSet other) {
+      if (other.isFunctionReturn) {
+        isFunctionReturn = true;
+      }
+      if (other.isFunctionArgument) {
+        isFunctionArgument = true;
+      }
+      if (other.isAssignedToMember) {
+        isAssignedToMember = true;
+      }
+      if (other.isThis) {
+        isThis = true;
+      }
+    }
+
+    /**
+     * Is the value with these flags replaceable?
+     *
+     * @return whether or not the value is replaceable
+     */
+    public boolean isReplaceable() {
+      return !(isFunctionReturn || isFunctionArgument || isAssignedToMember || isThis);
+    }
+
+    /**
+     * Dump the flag set's values to a given StringBuilder. The content is added to
+     * a single line; no newlines are generated.
+     *
+     * <p>This is for logging and debugging purposes.
+     *
+     * @param sb the StringBuilder
+     */
+    public void dump(final StringBuilder sb) {
+      boolean needSpace = false;
+
+      if (isFunctionReturn) {
+        sb.append("return");
+        needSpace = true;
+      }
+
+      if (isFunctionArgument) {
+        if (needSpace) {
+          sb.append(' ');
+        }
+
+        sb.append("argument");
+        needSpace = true;
+      }
+
+      if (isAssignedToMember) {
+        if (needSpace) {
+          sb.append(' ');
+        }
+
+        sb.append("member");
+        needSpace = true;
+      }
+
+      if (isThis) {
+        if (needSpace) {
+          sb.append(' ');
+        }
+
+        sb.append("this");
+      }
+    }
+  }
+
+  private FlagSet flagSet;
+  /**
+   * map of ReplacingBasicValue -> null; we need an IdentityHashSet, but there's no such thing;
+   * we just always set the value in the map to be null, and only care about the keys.
+   * Another solution might have been to define equals() and hashCode(), as apparently they are
+   * defined by BasicValue, but it's not clear what to do with some of the additional members here.
+   * It's not clear that not defining those won't also come back to bite us, depending on what ASM is
+   * doing with BasicValues under the covers.
+   *
+   * This value is null until we have our first associate, at which point this is allocated on demand.
+   */
+  private IdentityHashMap<ReplacingBasicValue, ReplacingBasicValue> associates = null;
+  // TODO remove?
+  private HashSet<Integer> frameSlots = null; // slots in stack frame this has been assigned to
+
+  /**
+   * Create a new value representing a holder (boxed value).
+   *
+   * @param type the type of the holder
+   * @param iden the ValueHolderIden for the holder
+   * @param index the original local variable slot assigned to the value
+   * @param valueList TODO
+   * @return
+   */
+  public static ReplacingBasicValue create(final Type type, final ValueHolderIden iden, final int index,
+      final List<ReplacingBasicValue> valueList) {
+    final ReplacingBasicValue replacingValue = new ReplacingBasicValue(type, iden, index);
+    valueList.add(replacingValue);
+    return replacingValue;
+  }
 
-  ValueHolderIden iden;
-  int index;
-  Type type;
-  boolean isFunctionReturn = false;
+  /**
+   * Add spaces to a StringBuilder.
+   *
+   * @param sb the StringBuilder
+   * @param indentLevel the number of spaces to add
+   */
+  private static void indent(final StringBuilder sb, final int indentLevel) {
+    for(int i = 0; i < indentLevel; ++i) {
+      sb.append(' ');
+    }
+  }
+
+  /**
+   * Dump the value's members to a StringBuilder.
+   *
+   * <p>For logging and/or debugging.
+   *
+   * @param sb the StringBuilder
+   * @param indentLevel the amount of indentation (in spaces) to prepend to each line
+   */
+  public void dump(final StringBuilder sb, final int indentLevel) {
+    indent(sb, indentLevel);
+    if (iden != null) {
+      iden.dump(sb);
+    } else {
+      sb.append("iden is null");
+    }
+    sb.append('\n');
+    indent(sb, indentLevel + 1);
+    sb.append(" index: " + index);
+    sb.append(' ');
+    flagSet.dump(sb);
+    sb.append(" frameSlots: ");
+    dumpFrameSlots(sb);
+    sb.append('\n');
+
+    if (associates != null) {
+      indent(sb, indentLevel);
+      sb.append("associates(index)");
+      for(ReplacingBasicValue value : associates.keySet()) {
+        sb.append(' ');
+        sb.append(value.index);
+      }
+      sb.append('\n');
+    }
+  }
 
-  public ReplacingBasicValue(Type type, ValueHolderIden iden, int index) {
+  private ReplacingBasicValue(final Type type, final ValueHolderIden iden, final int index) {
     super(type);
-    this.index = index;
     this.iden = iden;
-    this.type = type;
+    this.index = index;
+    flagSet = new FlagSet();
+  }
+
+  /**
+   * Indicates whether or not this value is replaceable.
+   *
+   * @return whether or not the value is replaceable
+   */
+  public boolean isReplaceable() {
+    return flagSet.isReplaceable();
   }
 
-  public void markFunctionReturn(){
-    this.isFunctionReturn = true;
+  private void dumpFrameSlots(final StringBuilder sb) {
+    if (frameSlots != null) {
+      boolean first = true;
+      for(Integer i : frameSlots) {
+        if (!first) {
+          sb.append(' ');
+        }
+        first = false;
+        sb.append(i.intValue());
+      }
+    }
   }
 
-  public void disableFunctionReturn(){
-    this.isFunctionReturn = false;
+  public void setFrameSlot(final int frameSlot) {
+    if (frameSlots == null) {
+      frameSlots = new HashSet<>();
+    }
+    frameSlots.add(frameSlot);
   }
 
+  /**
+   * Add another ReplacingBasicValue with no associates to this value's set of
+   * associates.
+   *
+   * @param other the other value
+   */
+  private void addOther(final ReplacingBasicValue other) {
+    assert other.associates == null;
+    associates.put(other, null);
+    other.associates = associates;
+    flagSet.mergeFrom(other.flagSet);
+    other.flagSet = flagSet;
+  }
+
+  /**
+   * Associate this value with another.
+   *
+   * <p>This value and/or the other may each already have their own set of
+   * associates.
+   *
+   * <p>Once associated, these values will share flag values, and a change to
+   * any one of them will be shared with all other members of the association.
+   *
+   * @param other the other value
+   */
+  public void associate(final ReplacingBasicValue other) {
+    associate0(other);
+
+    if (AssertionUtil.ASSERT_ENABLED) {
+      assert associates == other.associates;
+      assert flagSet == other.flagSet;
+      assert associates.containsKey(this);
+      assert associates.containsKey(other);
+
+      // check all the other values as well
+      for(ReplacingBasicValue value : associates.keySet()) {
+        assert associates.get(value) == null; // we never use the value
+        assert value.associates == associates;
+        assert value.flagSet == flagSet;
+      }
+    }
+  }
+
+  /**
+   * Does the real work of associate(), which is a convenient place to
+   * check all the assertions after this work is done.
+   */
+  private void associate0(final ReplacingBasicValue other) {
+    // if it's the same value, there's nothing to do
+    if (this == other) {
+      return;
+    }
+
+    if (associates == null) {
+      if (other.associates != null) {
+        other.associate(this);
+        return;
+      }
+
+      // we have no associations so far, so start collecting them
+      associates = new IdentityHashMap<>();
+      associates.put(this, null);
+      addOther(other);
+      return;
+    }
+
+    // if we got here, we have associates
+    if (other.associates == null) {
+      addOther(other);
+      return;
+    }
+
+    // if we're already associated, there's nothing to do
+    if (associates.containsKey(other)) {
+      return;
+    }
+
+    // this and other both have disjoint associates; we need to merge them
+    IdentityHashMap<ReplacingBasicValue, ReplacingBasicValue> largerSet = associates;
+    FlagSet largerFlags = flagSet;
+    IdentityHashMap<ReplacingBasicValue, ReplacingBasicValue> smallerSet = other.associates;
+    FlagSet smallerFlags = other.flagSet;
+
+    // if necessary, swap them, so the larger/smaller labels are correct
+    if (largerSet.size() < smallerSet.size()) {
+      final IdentityHashMap<ReplacingBasicValue, ReplacingBasicValue> tempSet = largerSet;
+      largerSet = smallerSet;
+      smallerSet = tempSet;
+      final FlagSet tempFlags = largerFlags;
+      largerFlags = smallerFlags;
+      smallerFlags = tempFlags;
+    }
+
+    largerFlags.mergeFrom(smallerFlags);
+
+    final int largerSize = largerSet.size();
+    for(ReplacingBasicValue value : smallerSet.keySet()) {
+      value.flagSet = largerFlags;
+      value.associates = largerSet;
+      largerSet.put(value, null);
+    }
+
+    assert associates == largerSet;
+    assert largerSet.size() == largerSize + smallerSet.size();
+    assert flagSet == largerFlags;
+    assert largerSet.containsKey(this);
+    assert other.associates == largerSet;
+    assert other.flagSet == largerFlags;
+    assert largerSet.containsKey(other);
+  }
+
+  /**
+   * Mark this value as being used as a function return value.
+   */
+  public void markFunctionReturn() {
+    flagSet.isFunctionReturn = true;
+  }
+
+  /**
+   * Clear the indication that this value is used as a function return value.
+   */
+  public void disableFunctionReturn() {
+    flagSet.isFunctionReturn = false;
+  }
+
+  /**
+   * Indicates whether or not this value is used as a function return value.
+   *
+   * @return whether or not this value is used as a function return value
+   */
+  public boolean isFunctionReturn() {
+    return flagSet.isFunctionReturn;
+  }
+
+  /**
+   * Mark this value as being used as a function argument.
+   */
+  public void setFunctionArgument() {
+    flagSet.isFunctionArgument = true;
+  }
+
+  /**
+   * Indicates whether or not this value is used as a function argument.
+   *
+   * @return whether or not this value is used as a function argument
+   */
+  public boolean isFunctionArgument() {
+    return flagSet.isFunctionArgument;
+  }
+
+  /**
+   * Mark this value as being assigned to a class member variable.
+   */
+  public void setAssignedToMember() {
+    flagSet.isAssignedToMember = true;
+  }
+
+  /**
+   * Indicates whether or not this value is assigned to a class member variable.
+   *
+   * @return whether or not this value is assigned to a class member variable
+   */
+  public boolean isAssignedToMember() {
+    return flagSet.isAssignedToMember;
+  }
+
+  /**
+   * Return the ValueHolder identity for this value.
+   *
+   * @return the ValueHolderIden for this value
+   */
   public ValueHolderIden getIden() {
     return iden;
   }
 
+  /**
+   * Get the original local variable slot assigned to this value/holder.
+   *
+   * @return the original local variable slot assigned to this value/holder
+   */
   public int getIndex() {
     return index;
   }
 
-  @Override
-  public Type getType(){
-    return type;
+  /**
+   * Mark this value as a "this" reference.
+   */
+  public void setThis() {
+    flagSet.isThis = true;
   }
 
+  /**
+   * Indicates whether or not this value is a "this" reference.
+   *
+   * @return whether or not this value is a "this" reference
+   */
+  public boolean isThis() {
+    return flagSet.isThis;
+  }
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingInterpreter.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingInterpreter.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingInterpreter.java
index 2dce4db..c18e965 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingInterpreter.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingInterpreter.java
@@ -17,9 +17,13 @@
  */
 package org.apache.drill.exec.compile.bytecode;
 
+import java.util.List;
+
 import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.Type;
 import org.objectweb.asm.tree.AbstractInsnNode;
+import org.objectweb.asm.tree.FieldInsnNode;
+import org.objectweb.asm.tree.MethodInsnNode;
 import org.objectweb.asm.tree.TypeInsnNode;
 import org.objectweb.asm.tree.analysis.AnalyzerException;
 import org.objectweb.asm.tree.analysis.BasicInterpreter;
@@ -29,47 +33,136 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 
 public class ReplacingInterpreter extends BasicInterpreter {
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(ReplacingInterpreter.class);
+//  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(ReplacingInterpreter.class);
 
+  private final String className; // fully qualified internal class name
   private int index = 0;
+  private final List<ReplacingBasicValue> valueList;
+
+  public ReplacingInterpreter(final String className, final List<ReplacingBasicValue> valueList) {
+    this.className = className;
+    this.valueList = valueList;
+  }
 
   @Override
-  public BasicValue newValue(Type t) {
-    if(t != null){
-      ValueHolderIden iden = HOLDERS.get(t.getDescriptor());
-      if(iden != null){
-        ReplacingBasicValue v = new ReplacingBasicValue(t, iden, index++);
+  public BasicValue newValue(final Type t) {
+    if (t != null) {
+      final ValueHolderIden iden = HOLDERS.get(t.getDescriptor());
+      if (iden != null) {
+        final ReplacingBasicValue v = ReplacingBasicValue.create(t, iden, index++, valueList);
         v.markFunctionReturn();
         return v;
       }
+
+      // We need to track use of the "this" objectref
+      if ((t.getSort() == Type.OBJECT) && className.equals(t.getInternalName())) {
+        final ReplacingBasicValue rbValue = ReplacingBasicValue.create(t, null, 0, valueList);
+        rbValue.setThis();
+        return rbValue;
+      }
     }
+
     return super.newValue(t);
+  }
 
+  @Override
+  public BasicValue newOperation(final AbstractInsnNode insn) throws AnalyzerException {
+    if (insn.getOpcode() == Opcodes.NEW) {
+      final TypeInsnNode t = (TypeInsnNode) insn;
+
+      // if this is for a holder class, we'll replace it
+      final ValueHolderIden iden = HOLDERS.get(t.desc);
+      if (iden != null) {
+        return ReplacingBasicValue.create(Type.getObjectType(t.desc), iden, index++, valueList);
+      }
+    }
+
+    return super.newOperation(insn);
   }
 
   @Override
-  public BasicValue newOperation(AbstractInsnNode insn) throws AnalyzerException {
-    if(insn.getOpcode() == Opcodes.NEW){
-      TypeInsnNode t = (TypeInsnNode) insn;
-      ValueHolderIden iden = HOLDERS.get(t.desc);
+  public BasicValue unaryOperation(final AbstractInsnNode insn, final BasicValue value)
+      throws AnalyzerException {
+    /*
+     * We're looking for the assignment of an operator member variable that's a holder to a local
+     * objectref. If we spot that, we can't replace the local objectref (at least not
+     * until we do the work to replace member variable holders).
+     *
+     * Note that a GETFIELD does not call newValue(), as would happen for a local variable, so we're
+     * emulating that here.
+     */
+    if ((insn.getOpcode() == Opcodes.GETFIELD) && (value instanceof ReplacingBasicValue)) {
+      final ReplacingBasicValue possibleThis = (ReplacingBasicValue) value;
+      if (possibleThis.isThis()) {
+        final FieldInsnNode fieldInsn = (FieldInsnNode) insn;
+        if (HOLDERS.get(fieldInsn.desc) != null) {
+          final BasicValue fetchedField = super.unaryOperation(insn, value);
+          final ReplacingBasicValue replacingValue =
+              ReplacingBasicValue.create(fetchedField.getType(), null, -1, valueList);
+          replacingValue.setAssignedToMember();
+          return replacingValue;
+        }
+      }
+    }
 
-      if(iden != null){
-        return new ReplacingBasicValue(Type.getObjectType(t.desc), iden, index++);
+    return super.unaryOperation(insn,  value);
+  }
+
+  @Override
+  public BasicValue binaryOperation(final AbstractInsnNode insn,
+      final BasicValue value1, final BasicValue value2) throws AnalyzerException {
+    /*
+     * We're looking for the assignment of a local holder objectref to a member variable.
+     * If we spot that, then the local holder can't be replaced, since we don't (yet)
+     * have the mechanics to replace the member variable with the holder's members or
+     * to assign all of them when this happens.
+     */
+    if (insn.getOpcode() == Opcodes.PUTFIELD) {
+      if (value2.isReference() && (value1 instanceof ReplacingBasicValue)) {
+        final ReplacingBasicValue possibleThis = (ReplacingBasicValue) value1;
+        if (possibleThis.isThis() && (value2 instanceof ReplacingBasicValue)) {
+          // if this is a reference for a holder class, we can't replace it
+          if (HOLDERS.get(value2.getType().getDescriptor()) != null) {
+            final ReplacingBasicValue localRef = (ReplacingBasicValue) value2;
+            localRef.setAssignedToMember();
+          }
+        }
       }
     }
 
-    return super.newOperation(insn);
+    return super.binaryOperation(insn, value1, value2);
+  }
+
+  @Override
+  public BasicValue naryOperation(final AbstractInsnNode insn,
+      final List<? extends BasicValue> values) throws AnalyzerException {
+    if (insn instanceof MethodInsnNode) {
+      boolean skipOne = insn.getOpcode() != Opcodes.INVOKESTATIC;
+
+      // Note if the argument is a holder, and is used as a function argument
+      for(BasicValue value : values) {
+        // if non-static method, skip over the receiver
+        if (skipOne) {
+          skipOne = false;
+          continue;
+        }
+
+        if (value instanceof ReplacingBasicValue) {
+          final ReplacingBasicValue argument = (ReplacingBasicValue) value;
+          argument.setFunctionArgument();
+        }
+      }
+    }
+
+    return super.naryOperation(insn,  values);
   }
 
   private static String desc(Class<?> c) {
-    Type t = Type.getType(c);
+    final Type t = Type.getType(c);
     return t.getDescriptor();
   }
 
-
-
   static {
-
     ImmutableMap.Builder<String, ValueHolderIden> builder = ImmutableMap.builder();
     ImmutableSet.Builder<String> setB = ImmutableSet.builder();
     for (Class<?> c : ScalarReplacementTypes.CLASSES) {
@@ -86,6 +179,4 @@ public class ReplacingInterpreter extends BasicInterpreter {
 
   private final static ImmutableMap<String, ValueHolderIden> HOLDERS;
   public final static ImmutableSet<String> HOLDER_DESCRIPTORS;
-
-
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java
index 8a38d32..6e981bc 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java
@@ -17,50 +17,73 @@
  */
 package org.apache.drill.exec.compile.bytecode;
 
+import java.util.LinkedList;
 
-import org.apache.drill.exec.expr.holders.IntHolder;
+import org.apache.drill.exec.compile.CheckMethodVisitorFsm;
+import org.apache.drill.exec.compile.CompilationConfig;
 import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.tree.MethodNode;
-import org.objectweb.asm.tree.analysis.Analyzer;
 import org.objectweb.asm.tree.analysis.AnalyzerException;
 import org.objectweb.asm.tree.analysis.BasicValue;
 import org.objectweb.asm.tree.analysis.Frame;
 
 public class ScalarReplacementNode extends MethodNode {
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(ScalarReplacementNode.class);
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(ScalarReplacementNode.class);
+  private final boolean verifyBytecode;
 
+  private final String className;
+  private final String[] exceptionsArr;
+  private final MethodVisitor inner;
 
-  String[] exceptionsArr;
-  MethodVisitor inner;
-
-  public ScalarReplacementNode(int access, String name, String desc, String signature, String[] exceptions, MethodVisitor inner) {
-    super(access, name, desc, signature, exceptions);
+  public ScalarReplacementNode(final String className, final int access, final String name,
+      final String desc, final String signature, final String[] exceptions, final MethodVisitor inner,
+      final boolean verifyBytecode) {
+    super(CompilationConfig.ASM_API_VERSION, access, name, desc, signature, exceptions);
+    this.className = className;
     this.exceptionsArr = exceptions;
     this.inner = inner;
+    this.verifyBytecode = verifyBytecode;
   }
 
-
   @Override
   public void visitEnd() {
+    /*
+     * Note this is a MethodNode, not a MethodVisitor. As a result, calls to the various visitX()
+     * methods will be building up a method. Then, once we analyze it, we use accept() to visit that
+     * method and transform it with the InstructionModifier at the bottom.
+     */
     super.visitEnd();
 
-    Analyzer<BasicValue> a = new Analyzer<>(new ReplacingInterpreter());
+    final LinkedList<ReplacingBasicValue> valueList = new LinkedList<>();
+    final MethodAnalyzer<BasicValue> analyzer =
+        new MethodAnalyzer<BasicValue>(new ReplacingInterpreter(className, valueList));
     Frame<BasicValue>[] frames;
     try {
-      frames = a.analyze("Object", this);
+      frames = analyzer.analyze(className, this);
     } catch (AnalyzerException e) {
       throw new IllegalStateException(e);
     }
+
+    if (logger.isDebugEnabled()) {
+      final StringBuilder sb = new StringBuilder();
+      sb.append("ReplacingBasicValues for " + className + "\n");
+      for(ReplacingBasicValue value : valueList) {
+        value.dump(sb, 2);
+        sb.append('\n');
+      }
+      logger.debug(sb.toString());
+    }
+
+    // wrap the instruction handler so that we can do additional things
     TrackingInstructionList list = new TrackingInstructionList(frames, this.instructions);
     this.instructions = list;
-    InstructionModifier holderV = new InstructionModifier(this.access, this.name, this.desc, this.signature, this.exceptionsArr, list, inner);
-    accept(holderV);
-  }
-
 
-  IntHolder local;
-  public void x(){
-    IntHolder h = new IntHolder();
-    local = h;
+    MethodVisitor methodVisitor = inner;
+    if (verifyBytecode) {
+      methodVisitor = new CheckMethodVisitorFsm(CompilationConfig.ASM_API_VERSION, methodVisitor);
+    }
+    InstructionModifier holderV = new InstructionModifier(this.access, this.name, this.desc,
+        this.signature, this.exceptionsArr, list, methodVisitor);
+    accept(holderV);
   }
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementTypes.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementTypes.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementTypes.java
index a54ca72..93f4e0d 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementTypes.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementTypes.java
@@ -60,12 +60,15 @@ import org.apache.drill.exec.expr.holders.VarCharHolder;
 
 import com.google.common.collect.ImmutableSet;
 
+/**
+ * Reference list of classes we will perform scalar replacement on.
+ */
 public class ScalarReplacementTypes {
-
-  private ScalarReplacementTypes(){}
+  // This class only has static utilities.
+  private ScalarReplacementTypes() {
+  }
 
   static {
-
     Class<?>[] classList = {
         BitHolder.class,
         IntHolder.class,
@@ -114,4 +117,20 @@ public class ScalarReplacementTypes {
 
   public static final ImmutableSet<Class<?>> CLASSES;
 
+  /**
+   * Determine if a class is a holder class.
+   *
+   * @param className the name of the class
+   * @return true if the class belongs to the CLASSES set.
+   */
+  public static boolean isHolder(final String className) {
+    try {
+      final Class<?> clazz = Class.forName(className);
+      return CLASSES.contains(clazz);
+    } catch (ClassNotFoundException e) {
+      // do nothing
+    }
+
+    return false;
+  }
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/7ea212a2/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/TrackingInstructionList.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/TrackingInstructionList.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/TrackingInstructionList.java
index 7cab17c..c6fa5fb 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/TrackingInstructionList.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/TrackingInstructionList.java
@@ -20,50 +20,39 @@ package org.apache.drill.exec.compile.bytecode;
 import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.tree.AbstractInsnNode;
 import org.objectweb.asm.tree.InsnList;
+import org.objectweb.asm.tree.analysis.BasicValue;
 import org.objectweb.asm.tree.analysis.Frame;
 
 public class TrackingInstructionList extends InsnList {
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TrackingInstructionList.class);
+//  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TrackingInstructionList.class);
 
-  Frame<?> currentFrame;
-  Frame<?> nextFrame;
-  Frame<?>[] frames;
-  InsnList inner;
-  int index = 0;
-
-
-
-  public TrackingInstructionList(Frame<?>[] frames, InsnList inner) {
-    super();
+  Frame<BasicValue> currentFrame;
+  Frame<BasicValue> nextFrame;
+  AbstractInsnNode currentInsn;
+  private int index = 0;
+  private final Frame<BasicValue>[] frames;
+  private final InsnList inner;
 
+  public TrackingInstructionList(final Frame<BasicValue>[] frames, final InsnList inner) {
     this.frames = frames;
     this.inner = inner;
   }
 
-  public InsnList getInner(){
-    return inner;
-  }
-
   @Override
-  public void accept(MethodVisitor mv) {
-    AbstractInsnNode insn = inner.getFirst();
-    while (insn != null) {
+  public void accept(final MethodVisitor mv) {
+    currentInsn = inner.getFirst();
+    while (currentInsn != null) {
         currentFrame = frames[index];
-        nextFrame = index +1 < frames.length ? frames[index+1] : null;
-        insn.accept(mv);
+        nextFrame = index + 1 < frames.length ? frames[index + 1] : null;
+        currentInsn.accept(mv);
 
-        insn = insn.getNext();
+        currentInsn = currentInsn.getNext();
         index++;
     }
   }
 
-
   @Override
   public int size() {
     return inner.size();
   }
-
-
-
-
 }


Mime
View raw message