Refactor and document the coverage node and factory.

Author: boris-spas

src/main/java/com/oracle/simpletool/SimpleCodeCoverageInstrument.java

@@ -245,27 +245,97 @@ public void onLoad(LoadSourceSectionEvent event) {
         }
     }
 
+    /**
+     * A factory for nodes that track coverage
+     *
+     * Because we
+     * {@link #enable(com.oracle.truffle.api.instrumentation.TruffleInstrument.Env) attached}
+     * an instance of this factory, each time a AST node of interest is created,
+     * it is instrumented with a node created by this factory.
+     */
     private class CoverageEventFactory implements ExecutionEventNodeFactory {
-        
+
+        /**
+         * @param ec context of the event, used in our case to lookup the
+         * {@link SourceSection} that our node is instrumenting.
+         * @return An {@link ExecutionEventNode}
+         */
         public ExecutionEventNode create(final EventContext ec) {
-            return new ExecutionEventNode() {
-                @CompilationFinal
-                private boolean visited;
+            return new CoverageNode(ec.getInstrumentedSourceSection());
+        }
+    }
+
+    /**
+     * Node that "wraps" AST nodes of interest (Nodes that correspond to
+     * expressions in our case as defined by the filter given to the
+     * {@link Instrumenter} in {@link #onCreate(com.oracle.truffle.api.instrumentation.TruffleInstrument.Env)
+     * }), and removes the "wrapped" {@link SourceSection} from the set
+     * {@link #sourceToUncoveredSections uncovered} {@link SourceSection}.
+     */
+    class CoverageNode extends ExecutionEventNode {
 
-                @Override
-                public void onReturnValue(VirtualFrame vFrame, Object result) {
-                    if (!visited) {
-                        CompilerDirectives.transferToInterpreterAndInvalidate();
-                        visited = true;
-                        final SourceSection sourceSection = ec.getInstrumentedSourceSection();
-                        final Source source = sourceSection.getSource();
-                        // TODO: This should not be necesery becuase of the filter. Bug!
-                        if (!source.isInternal()) {
-                            sourceToUncoveredSections.get(source).remove(sourceSection);
-                        }
-                    }
+        @CompilationFinal
+        private boolean covered;
+
+        /**
+         * Each node knows which {@link SourceSection} it instruments.
+         */
+        private final SourceSection instrumentedSourceSection;
+
+        private CoverageNode(SourceSection instrumentedSourceSection) {
+            this.instrumentedSourceSection = instrumentedSourceSection;
+        }
+
+        /**
+         * The {@link ExecutionEventNode} class let's us define several events
+         * that we can intercept. The one of interest to us is {@link ExecutionEventNode#onReturnValue(com.oracle.truffle.api.frame.VirtualFrame, java.lang.Object)
+         * } as we wish to remove this nodes {@link #instrumentedSourceSection}
+         * from the {@link #sourceToUncoveredSections set of uncovered nodes}
+         * only once the node is successfully executed (as oppose to, for
+         * example, {@link ExecutionEventNode#onReturnExceptional(com.oracle.truffle.api.frame.VirtualFrame, java.lang.Throwable)
+         * }).
+         *
+         * Each node keeps a {@link #covered} flag so that the removal only
+         * happens once. The fact that the flag is annotated with
+         * {@link CompilationFinal} means that this flag will be treated as
+         * {@code final} during compilation of instrumented source code (i.e.
+         * the {@code false} branch of the if statement can be optimized away).
+         *
+         * The way it's used in this method is a pattern when writing Truffle
+         * nodes:
+         * <ul>
+         * <li> If we are compiling a covered node, the if condition will
+         * evaluate to false and the if-guarded code will be optimized away.
+         * This means that once this {@link SourceSection} is confirmed to be
+         * covered, there is no further instrumentation overhead on performance.
+         * <li> If we are compiling a not-yet-covered node, the if condition
+         * will evaluate to true, and the if-guarded code will be included for
+         * compilation. The first statement in this block is a
+         * {@link CompilerDirectives#transferToInterpreterAndInvalidate() directive to the compiler}
+         * to make sure that if this point in the execution is reached, the
+         * execution should return to the interpreter and the existing compiled
+         * code is no longer valid (since once the covered flag is set to true,
+         * the check is unnecessary). The code following the directive is thus
+         * always executed in the interpreter: We set the {@link #covered} flag
+         * to true, ensuring that the next compilation will have no
+         * instrumentation overhead on performance.
+         * </li>
+         * </ul>
+         *
+         * @param vFrame unused
+         * @param result unused
+         */
+        @Override
+        public void onReturnValue(VirtualFrame vFrame, Object result) {
+            if (!covered) {
+                CompilerDirectives.transferToInterpreterAndInvalidate();
+                covered = true;
+                final Source source = instrumentedSourceSection.getSource();
+                // TODO: This should not be necesery becuase of the filter. Bug!
+                if (!source.isInternal()) {
+                    sourceToUncoveredSections.get(source).remove(instrumentedSourceSection);
                 }
-            };
+            }
         }
     }