--- a/graal/com.oracle.truffle.api/src/com/oracle/truffle/api/instrument/Instrument.java	Fri Nov 21 13:16:02 2014 +0100
+++ b/graal/com.oracle.truffle.api/src/com/oracle/truffle/api/instrument/Instrument.java	Sun Nov 23 16:07:23 2014 -0800
@@ -24,113 +24,297 @@
  */
 package com.oracle.truffle.api.instrument;
 
-import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
+import com.oracle.truffle.api.*;
 import com.oracle.truffle.api.frame.*;
 import com.oracle.truffle.api.instrument.impl.*;
 import com.oracle.truffle.api.nodes.*;
 
+// TODO (mlvdv) migrate some of this to external documentation.
 /**
- * A receiver of Truffle AST runtime {@link ExecutionEvents}, propagated from the {@link Probe} to
- * which the instrument is attached, for the benefit of associated <em>tools</em>.
+ * A dynamically added/removed binding between a {@link Probe}, which provides notification of
+ * {@linkplain TruffleEventReceiver execution events} taking place at a {@link Node} in a Guest
+ * Language (GL) Truffle AST, and a {@linkplain TruffleEventReceiver receiver}, which consumes
+ * notifications on behalf of an external tool.
  * <p>
- * Guidelines for implementing {@link Instrument}s, with particular attention to minimize runtime
- * performance overhead:
+ * <h4>Summary: How to "instrument" an AST location:</h4>
  * <ol>
- * <li>Extend this abstract class and override only the {@linkplain ExecutionEvents event handling
- * methods} for which intervention is needed.</li>
- * <li>Instruments are Truffle {@link Node}s and should be coded as much as possible in the desired
- * <em>Truffle style</em>, documented more thoroughly elsewhere.</li>
- * <li>Maintain as little state as possible.</li>
- * <li>If state is necessary, make object fields {@code final} if at all possible.</li>
- * <li>If non-final object-valued state is necessary, annotate it as {@link CompilationFinal} and
- * call {@linkplain InstrumentationNode#notifyProbeChanged(Instrument)} whenever it is modified.</li>
- * <li>Never store a {@link Frame} value in a field.</li>
- * <li>Minimize computation in standard execution paths.</li>
- * <li>If runtime calls must be made back to a tool, construct the instrument with a callback stored
- * in a {@code final} field.</li>
- * <li>Tool methods called by the instrument should be annotated as {@link TruffleBoundary} to
- * prevent them from being inlined into fast execution paths.</li>
- * <li>If computation in the execution path is needed, and if performance is important, then the
- * computation is best expressed as a guest language AST and evaluated using standard Truffle
- * mechanisms so that standard Truffle optimizations can be applied.</li>
+ * <li>Create an implementation of {@link TruffleEventReceiver} that responds to events on behalf of
+ * a tool.</li>
+ * <li>Create an Instrument via factory method {@link Instrument#create(TruffleEventReceiver)}.</li>
+ * <li>"Attach" the Instrument to a Probe via {@link Probe#attach(Instrument)}, at which point event
+ * notifications begin to arrive at the receiver.</li>
+ * <li>When no longer needed, "detach" the Instrument via {@link Instrument#dispose()}, at which
+ * point event notifications to the receiver cease, and the Instrument becomes unusable.</li>
+ * </ol>
+ * <p>
+ * <h4>Options for creating receivers:</h4>
+ * <p>
+ * <ol>
+ * <li>Implement the interface {@link TruffleEventReceiver}. The event handling methods account for
+ * both the entry into an AST node (about to call) and several possible kinds of exit from an AST
+ * node (just returned).</li>
+ * <li>Extend {@link DefaultEventReceiver}, which provides a no-op implementation of every
+ * {@link TruffleEventReceiver} method; override the methods of interest.</li>
+ * <li>Extend {@link SimpleEventReceiver}, where return values are ignored so only two methods (for
+ * "enter" and "return") will notify all events.</li>
  * </ol>
  * <p>
- * Guidelines for attachment to a {@link Probe}:
- * <ol>
- * <li>An Instrument instance must only attached to a single {@link Probe}, each of which is
- * associated uniquely with a specific syntactic unit of a guest language program, and thus
- * (initially) to a specific {@linkplain Node Truffle AST node}.</li>
- * <li>When the AST containing such a node is copied at runtime, the {@link Probe} will be shared by
- * every copy, and so the Instrument will receive events corresponding to the intended syntactic
- * unit of code, independent of which AST copy is being executed.</li>
- * </ol>
+ * <h4>General guidelines for receiver implementation:</h4>
+ * <p>
+ * When an Instrument is attached to a Probe, the receiver effectively becomes part of the executing
+ * GL program; performance can be affected by the receiver's implementation.
+ * <ul>
+ * <li>Do not store {@link Frame} or {@link Node} references in fields.</li>
+ * <li>Prefer {@code final} fields and (where performance is important) short methods.</li>
+ * <li>If needed, pass along the {@link VirtualFrame} reference from an event notification as far as
+ * possible through code that is expected to be inlined, since this incurs no runtime overhead. When
+ * access to frame data is needed, substitute a more expensive {@linkplain Frame#materialize()
+ * materialized} representation of the frame.</li>
+ * <li>If a receiver calls back to its tool during event handling, and if performance is an issue,
+ * then this should be through a final "callback" field in the instrument, and the called methods
+ * should be minimal.</li>
+ * <li>On the other hand, implementations should prevent Truffle from inlining beyond a reasonable
+ * point with the method annotation {@link TruffleBoundary}.</li>
+ * <li>The implicit "outer" pointer in a non-static inner class is a useful way to implement
+ * callbacks to owner tools.</li>
+ * <li>Primitive-valued return events are boxed for notification, but Truffle will eliminate the
+ * boxing if they are cast back to their primitive values quickly (in particular before crossing any
+ * {@link TruffleBoundary} annotations).
+ * </ul>
+ * <p>
+ * <h4>Allowing for AST cloning:</h4>
+ * <p>
+ * Truffle routinely <em>clones</em> ASTs, which has consequences for receiver implementation.
+ * <ul>
+ * <li>Even though a {@link Probe} is uniquely associated with a particular location in the
+ * executing Guest Language program, execution events at that location will in general be
+ * implemented by different {@link Node} instances, i.e. <em>clones</em> of the originally probed
+ * node.</li>
+ * <li>Because of <em>cloning</em> the {@link Node} supplied with notifications to a particular
+ * receiver will vary, but because they all represent the same GL program location the events should
+ * be treated as equivalent for most purposes.</li>
+ * </ul>
+ * <p>
+ * <h4>Access to execution state:</h4>
  * <p>
- * Guidelines for handling {@link ExecutionEvents}:
- * <ol>
- * <li>Separate event methods are defined for each kind of possible return: object-valued,
- * primitive-valued, void-valued, and exceptional.</li>
- * <li>Override "leave*" primitive methods if the language implementation returns primitives and the
- * instrument should avoid boxing them.</li>
- * <li>On the other hand, if boxing all primitives for instrumentation is desired, it is only
- * necessary to override the object-valued return methods, since the default implementation of each
- * primitive-valued return method is to box the value and forward it to the object-valued return
- * method.</li>
- * </ol>
- *
+ * <ul>
+ * <li>Event notification arguments provide primary access to the GL program's execution states:
+ * <ul>
+ * <li>{@link Node}: the concrete node (in one of the AST's clones) from which the event originated.
+ * </li>
+ * <li>{@link VirtualFrame}: the current execution frame.
+ * </ul>
+ * <li>Some global information is available, for example the execution
+ * {@linkplain TruffleRuntime#iterateFrames(FrameInstanceVisitor) stack}.</li>
+ * <li>Additional information needed by a receiver could be stored when created, preferably
+ * {@code final} of course. For example, a reference to the {@link Probe} to which the receiver's
+ * Instrument has been attached would give access to its corresponding
+ * {@linkplain Probe#getProbedSourceSection() source location} or to the collection of
+ * {@linkplain SyntaxTag tags} currently applied to the Probe.</li>
+ * </ul>
+ * <p>
+ * <h4>Activating and deactivating Instruments:</h4>
+ * <p>
+ * Instruments are <em>single-use</em>:
+ * <ul>
+ * <li>An instrument becomes active only when <em>attached</em> to a Probe via
+ * {@link Probe#attach(Instrument)}, and it may only be attached to a single Probe. It is a runtime
+ * error to attempt attaching a previously attached instrument.</li>
+ * <li>Attaching an instrument modifies every existing clone of the AST to which it is being
+ * attached, which can trigger deoptimization.</li>
+ * <li>The method {@link Instrument#dispose()} makes an instrument inactive by removing it from the
+ * Probe to which it was attached and rendering it permanently inert.</li>
+ * <li>Disposal removes the implementation of an instrument from all ASTs to which it was attached,
+ * which can trigger deoptimization.</li>
+ * </ul>
+ * <p>
+ * <h4>Sharing receivers:</h4>
+ * <p>
+ * Although an Instrument may only be attached to a single Probe, a receiver can be shared among
+ * multiple Instruments. This can be useful for observing events that might happen at different
+ * locations in a single AST, for example all assignments to a particular variable. In this case a
+ * new Instrument would be created and attached at each assignment node, but all the Instruments
+ * would be created with the same receiver.
  * <p>
  * <strong>Disclaimer:</strong> experimental; under development.
  *
  * @see Probe
- * @see ASTNodeProber
+ * @see TruffleEventReceiver
  */
-public abstract class Instrument extends InstrumentationNode {
+public final class Instrument {
 
-    protected Instrument() {
+    /**
+     * Creates an instrument that will route execution events to a receiver.
+     *
+     * @param receiver a receiver for event generated by the instrument
+     * @param instrumentInfo optional description of the instrument's role
+     * @return a new instrument, ready for attachment at a probe
+     */
+    public static Instrument create(TruffleEventReceiver receiver, String instrumentInfo) {
+        return new Instrument(receiver, instrumentInfo);
     }
 
-    public void enter(Node astNode, VirtualFrame frame) {
+    /**
+     * Creates an instrument that will route execution events to a receiver.
+     */
+    public static Instrument create(TruffleEventReceiver receiver) {
+        return new Instrument(receiver, null);
     }
 
-    public void leave(Node astNode, VirtualFrame frame) {
-    }
+    /**
+     * Tool-supplied receiver of events.
+     */
+    private final TruffleEventReceiver toolEventreceiver;
+
+    /**
+     * Optional documentation, mainly for debugging.
+     */
+    private final String instrumentInfo;
 
-    public void leave(Node astNode, VirtualFrame frame, boolean result) {
-        leave(astNode, frame, (Object) result);
+    /**
+     * Has this instrument been disposed? stays true once set.
+     */
+    private boolean isDisposed = false;
+
+    private Probe probe = null;
+
+    private Instrument(TruffleEventReceiver receiver, String instrumentInfo) {
+        this.toolEventreceiver = receiver;
+        this.instrumentInfo = instrumentInfo;
     }
 
-    public void leave(Node astNode, VirtualFrame frame, byte result) {
-        leave(astNode, frame, (Object) result);
+    /**
+     * Removes this instrument (and any clones) from the probe to which it attached and renders the
+     * instrument inert.
+     *
+     * @throws IllegalStateException if this instrument has already been disposed
+     */
+    public void dispose() throws IllegalStateException {
+        if (isDisposed) {
+            throw new IllegalStateException("Attempt to dispose an already disposed Instrumennt");
+        }
+        if (probe != null) {
+            // It's attached
+            probe.disposeInstrument(this);
+        }
+        this.isDisposed = true;
     }
 
-    public void leave(Node astNode, VirtualFrame frame, short result) {
-        leave(astNode, frame, (Object) result);
+    Probe getProbe() {
+        return probe;
+    }
+
+    void setAttachedTo(Probe probe) {
+        this.probe = probe;
+    }
+
+    /**
+     * Has this instrument been disposed and rendered unusable?
+     */
+    boolean isDisposed() {
+        return isDisposed;
+    }
+
+    InstrumentNode addToChain(InstrumentNode nextNode) {
+        return new InstrumentNode(nextNode);
     }
 
-    public void leave(Node astNode, VirtualFrame frame, int result) {
-        leave(astNode, frame, (Object) result);
-    }
-
-    public void leave(Node astNode, VirtualFrame frame, long result) {
-        leave(astNode, frame, (Object) result);
-    }
-
-    public void leave(Node astNode, VirtualFrame frame, char result) {
-        leave(astNode, frame, (Object) result);
+    /**
+     * Removes this instrument from an instrument chain.
+     */
+    InstrumentNode removeFromChain(InstrumentNode instrumentNode) {
+        boolean found = false;
+        if (instrumentNode != null) {
+            if (instrumentNode.getInstrument() == this) {
+                // Found the match at the head of the chain
+                return instrumentNode.nextInstrument;
+            }
+            // Match not at the head of the chain; remove it.
+            found = instrumentNode.removeFromChain(Instrument.this);
+        }
+        if (!found) {
+            throw new IllegalStateException("Couldn't find instrument node to remove: " + this);
+        }
+        return instrumentNode;
     }
 
-    public void leave(Node astNode, VirtualFrame frame, float result) {
-        leave(astNode, frame, (Object) result);
-    }
+    @NodeInfo(cost = NodeCost.NONE)
+    final class InstrumentNode extends Node implements TruffleEventReceiver, InstrumentationNode {
+
+        @Child private InstrumentNode nextInstrument;
+
+        private InstrumentNode(InstrumentNode nextNode) {
+            this.nextInstrument = nextNode;
+        }
+
+        /**
+         * Gets the instrument that created this node.
+         */
+        private Instrument getInstrument() {
+            return Instrument.this;
+        }
 
-    public void leave(Node astNode, VirtualFrame frame, double result) {
-        leave(astNode, frame, (Object) result);
-    }
+        /**
+         * Removes the node from this chain that was added by a particular instrument, assuming that
+         * the head of the chain is not the one to be replaced. This is awkward, but is required
+         * because {@link Node#replace(Node)} won't take a {@code null} argument. This doesn't work
+         * for the tail of the list, which would be replacing itself with null. So the replacement
+         * must be directed the parent of the node being removed.
+         */
+        private boolean removeFromChain(Instrument instrument) {
+            assert getInstrument() != instrument;
+            if (nextInstrument == null) {
+                return false;
+            }
+            if (nextInstrument.getInstrument() == instrument) {
+                // Next is the one to remove
+                if (nextInstrument.nextInstrument == null) {
+                    // Next is at the tail; just forget
+                    nextInstrument = null;
+                } else {
+                    // Replace next with its successor
+                    nextInstrument.replace(nextInstrument.nextInstrument);
+                }
+                return true;
+            }
+            return nextInstrument.removeFromChain(instrument);
+        }
 
-    public void leave(Node astNode, VirtualFrame frame, Object result) {
-    }
+        public void enter(Node node, VirtualFrame frame) {
+            Instrument.this.toolEventreceiver.enter(node, frame);
+            if (nextInstrument != null) {
+                nextInstrument.enter(node, frame);
+            }
+        }
+
+        public void returnVoid(Node node, VirtualFrame frame) {
+            Instrument.this.toolEventreceiver.returnVoid(node, frame);
+            if (nextInstrument != null) {
+                nextInstrument.returnVoid(node, frame);
+            }
+        }
 
-    public void leaveExceptional(Node astNode, VirtualFrame frame, Exception e) {
+        public void returnValue(Node node, VirtualFrame frame, Object result) {
+            Instrument.this.toolEventreceiver.returnValue(node, frame, result);
+            if (nextInstrument != null) {
+                nextInstrument.returnValue(node, frame, result);
+            }
+        }
+
+        public void returnExceptional(Node node, VirtualFrame frame, Exception exception) {
+            Instrument.this.toolEventreceiver.returnExceptional(node, frame, exception);
+            if (nextInstrument != null) {
+                nextInstrument.returnExceptional(node, frame, exception);
+            }
+        }
+
+        public String instrumentationInfo() {
+            if (Instrument.this.instrumentInfo != null) {
+                return Instrument.this.instrumentInfo;
+            }
+            return toolEventreceiver.getClass().getSimpleName();
+        }
     }
 
 }