Repository: systemml Updated Branches: refs/heads/master e9a6e396a -> 2c9418c3e
[SYSTEMML-2154] Fix misleading explain recompile hops output Since a consolidation of duplicated recompilation code paths in SYSTEMML-2072, explain recompile_hops returns misleading outputs. Specifically, the recompiler creates a deep copy of the hop dag, but the explain output shows the original hop dag, which leads to understandable confusion. This patch fixes this issue, while preserving the consolidated recompilation code path. Project: http://git-wip-us.apache.org/repos/asf/systemml/repo Commit: http://git-wip-us.apache.org/repos/asf/systemml/commit/bd9d7eb0 Tree: http://git-wip-us.apache.org/repos/asf/systemml/tree/bd9d7eb0 Diff: http://git-wip-us.apache.org/repos/asf/systemml/diff/bd9d7eb0 Branch: refs/heads/master Commit: bd9d7eb00ebd60656042a312e3cdba30470d36cb Parents: e9a6e39 Author: Matthias Boehm <[email protected]> Authored: Mon Feb 19 14:39:08 2018 -0800 Committer: Matthias Boehm <[email protected]> Committed: Mon Feb 19 20:06:40 2018 -0800 ---------------------------------------------------------------------- .../apache/sysml/hops/recompile/Recompiler.java | 49 ++++++++++++++------ 1 file changed, 34 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/systemml/blob/bd9d7eb0/src/main/java/org/apache/sysml/hops/recompile/Recompiler.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/hops/recompile/Recompiler.java b/src/main/java/org/apache/sysml/hops/recompile/Recompiler.java index 13bd81c..ca9546f 100644 --- a/src/main/java/org/apache/sysml/hops/recompile/Recompiler.java +++ b/src/main/java/org/apache/sysml/hops/recompile/Recompiler.java @@ -160,7 +160,7 @@ public class Recompiler //need for synchronization as we do temp changes in shared hops/lops //however, we create deep copies for most dags to allow for concurrent recompile synchronized( hops ) { - newInst = recompile(sb, hops, vars, status, inplace, replaceLit, true, false, null, tid); + newInst = recompile(sb, hops, vars, status, inplace, replaceLit, true, false, false, null, tid); } // replace thread ids in new instructions @@ -172,7 +172,8 @@ public class Recompiler newInst = JMLCUtils.cleanupRuntimeInstructions(newInst, vars.getRegisteredOutputs()); // explain recompiled hops / instructions - logExplainDAG(sb, hops, newInst); + if( DMLScript.EXPLAIN == ExplainType.RECOMPILE_RUNTIME ) + logExplainDAG(sb, hops, newInst); return newInst; } @@ -186,7 +187,7 @@ public class Recompiler //need for synchronization as we do temp changes in shared hops/lops synchronized( hop ) { newInst = recompile(null, new ArrayList<>(Arrays.asList(hop)), - vars, status, inplace, replaceLit, true, false, null, tid); + vars, status, inplace, replaceLit, true, false, true, null, tid); } // replace thread ids in new instructions @@ -194,7 +195,8 @@ public class Recompiler newInst = ProgramConverter.createDeepCopyInstructionSet(newInst, tid, -1, null, null, null, false, false); // explain recompiled instructions - logExplainPred(hop, newInst); + if( DMLScript.EXPLAIN == ExplainType.RECOMPILE_RUNTIME ) + logExplainPred(hop, newInst); return newInst; } @@ -208,7 +210,7 @@ public class Recompiler //however, we create deep copies for most dags to allow for concurrent recompile synchronized( hops ) { //always in place, no stats update/rewrites, but forced exec type - newInst = recompile(sb, hops, null, null, true, false, false, true, et, tid); + newInst = recompile(sb, hops, null, null, true, false, false, true, false, et, tid); } // replace thread ids in new instructions @@ -216,7 +218,8 @@ public class Recompiler newInst = ProgramConverter.createDeepCopyInstructionSet(newInst, tid, -1, null, null, null, false, false); // explain recompiled hops / instructions - logExplainDAG(sb, hops, newInst); + if( DMLScript.EXPLAIN == ExplainType.RECOMPILE_RUNTIME ) + logExplainDAG(sb, hops, newInst); return newInst; } @@ -230,7 +233,7 @@ public class Recompiler synchronized( hop ) { //always in place, no stats update/rewrites, but forced exec type newInst = recompile(null, new ArrayList<>(Arrays.asList(hop)), - null, null, true, false, false, true, et, tid); + null, null, true, false, false, true, true, et, tid); } // replace thread ids in new instructions @@ -238,7 +241,8 @@ public class Recompiler newInst = ProgramConverter.createDeepCopyInstructionSet(newInst, tid, -1, null, null, null, false, false); // explain recompiled hops / instructions - logExplainPred(hop, newInst); + if( DMLScript.EXPLAIN == ExplainType.RECOMPILE_RUNTIME ) + logExplainPred(hop, newInst); return newInst; } @@ -252,11 +256,12 @@ public class Recompiler //however, we create deep copies for most dags to allow for concurrent recompile synchronized( hops ) { //always in place, no stats update/rewrites - newInst = recompile(sb, hops, null, null, true, false, false, false, null, 0); + newInst = recompile(sb, hops, null, null, true, false, false, false, false, null, 0); } // explain recompiled hops / instructions - logExplainDAG(sb, hops, newInst); + if( DMLScript.EXPLAIN == ExplainType.RECOMPILE_RUNTIME ) + logExplainDAG(sb, hops, newInst); return newInst; } @@ -270,11 +275,12 @@ public class Recompiler synchronized( hop ) { //always in place, no stats update/rewrites newInst = recompile(null, new ArrayList<>(Arrays.asList(hop)), - null, null, true, false, false, false, null, 0); + null, null, true, false, false, false, true, null, 0); } // explain recompiled instructions - logExplainPred(hop, newInst); + if( DMLScript.EXPLAIN == ExplainType.RECOMPILE_RUNTIME ) + logExplainPred(hop, newInst); return newInst; } @@ -291,6 +297,7 @@ public class Recompiler * @param replaceLit replace literals (only applicable on deep copy) * @param updateStats update statistics, rewrites, and memory estimates * @param forceEt force a given execution type, null for reset + * @param pred recompile for predicate DAG * @param et given execution type * @param tid thread id, 0 for main or before worker creation * @return modified list of instructions @@ -298,8 +305,8 @@ public class Recompiler * @throws LopsException if lop compile error * @throws DMLRuntimeException if runtime error on literal replacement */ - private static ArrayList<Instruction> recompile(StatementBlock sb, ArrayList<Hop> hops, LocalVariableMap vars, - RecompileStatus status, boolean inplace, boolean replaceLit, boolean updateStats, boolean forceEt, ExecType et, long tid ) + private static ArrayList<Instruction> recompile(StatementBlock sb, ArrayList<Hop> hops, LocalVariableMap vars, RecompileStatus status, + boolean inplace, boolean replaceLit, boolean updateStats, boolean forceEt, boolean pred, ExecType et, long tid ) throws HopsException, LopsException, DMLRuntimeException { // prepare hops dag for recompile @@ -378,7 +385,19 @@ public class Recompiler } // generate runtime instructions (incl piggybacking) - return dag.getJobs(sb, ConfigurationManager.getDMLConfig()); + ArrayList<Instruction> newInst = dag + .getJobs(sb, ConfigurationManager.getDMLConfig()); + + // explain recompiled (and potentially deep copied) DAG, but + // defer the explain of instructions after additional modifications + if( DMLScript.EXPLAIN == ExplainType.RECOMPILE_HOPS ) { + if( pred ) + logExplainPred(hops.get(0), newInst); + else + logExplainDAG(sb, hops, newInst); + } + + return newInst; } private static void logExplainDAG(StatementBlock sb, ArrayList<Hop> hops, ArrayList<Instruction> inst)
