Skip to content

Commit

Permalink
Eagerly initialize instrumentations; Synchronize accesses to Instrume…
Browse files Browse the repository at this point in the history 
…nt#setEnabled with a lock.
  • Loading branch information
@chumer
chumer committed Jun 22, 2016
1 parent 503bd36 commit 8101861
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 83 deletions.
4 changes: 1 addition & 3 deletions ....instrumentation.test/src/com/oracle/truffle/api/instrumentation/InstrumentationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,12 +259,10 @@ protected Object evalInContext(Source source, Node node, MaterializedFrame mFram
}

@Test
public void testInstrumentException1() throws IOException {
public void testInstrumentException1() {
engine.getInstruments().get("testInstrumentException1").setEnabled(true);
run("");

Assert.assertTrue(getErr().contains("MyLanguageException"));

}

@Registration(name = "", version = "", id = "testInstrumentException1")
Expand Down
81 changes: 26 additions & 55 deletions ...pi.instrumentation/src/com/oracle/truffle/api/instrumentation/InstrumentationHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,6 @@ final class InstrumentationHandler {
*/
private final ConcurrentHashMap<Object, AbstractInstrumenter> instrumenterMap = new ConcurrentHashMap<>();

/* Has the instrumentation framework been initialized? */
private volatile boolean instrumentationInitialized;

private final OutputStream out;
private final OutputStream err;
private final InputStream in;
Expand All @@ -101,10 +98,6 @@ void onLoad(RootNode root) {
if (!AccessorInstrumentHandler.nodesAccess().isInstrumentable(root)) {
return;
}
if (!instrumentationInitialized) {
initializeInstrumentation();
}

SourceSection sourceSection = root.getSourceSection();
if (sourceSection != null) {
// notify sources
Expand Down Expand Up @@ -135,9 +128,6 @@ void onFirstExecution(RootNode root) {
if (!AccessorInstrumentHandler.nodesAccess().isInstrumentable(root)) {
return;
}
if (!instrumentationInitialized) {
initializeInstrumentation();
}
executedRoots.add(root);

// fast path no bindings attached
Expand All @@ -154,24 +144,25 @@ void addInstrument(Object key, Class<?> clazz) {

void disposeInstrumenter(Object key, boolean cleanupRequired) {
AbstractInstrumenter disposedInstrumenter = instrumenterMap.remove(key);
if (disposedInstrumenter != null) {
if (TRACE) {
trace("BEGIN: Dispose instrumenter %n", key);
}
disposedInstrumenter.dispose();
if (disposedInstrumenter == null) {
throw new AssertionError("Instrumenter already disposed.");
}
if (TRACE) {
trace("BEGIN: Dispose instrumenter %n", key);
}
disposedInstrumenter.dispose();

if (cleanupRequired) {
Collection<EventBinding<?>> disposedExecutionBindings = filterBindingsForInstrumenter(executionBindings, disposedInstrumenter);
if (!disposedExecutionBindings.isEmpty()) {
visitRoots(executedRoots, new DisposeWrappersWithBindingVisitor(disposedExecutionBindings));
}
disposeBindingsBulk(disposedExecutionBindings);
disposeBindingsBulk(filterBindingsForInstrumenter(sourceSectionBindings, disposedInstrumenter));
disposeBindingsBulk(filterBindingsForInstrumenter(sourceBindings, disposedInstrumenter));
}
if (TRACE) {
trace("END: Disposed instrumenter %n", key);
if (cleanupRequired) {
Collection<EventBinding<?>> disposedExecutionBindings = filterBindingsForInstrumenter(executionBindings, disposedInstrumenter);
if (!disposedExecutionBindings.isEmpty()) {
visitRoots(executedRoots, new DisposeWrappersWithBindingVisitor(disposedExecutionBindings));
}
disposeBindingsBulk(disposedExecutionBindings);
disposeBindingsBulk(filterBindingsForInstrumenter(sourceSectionBindings, disposedInstrumenter));
disposeBindingsBulk(filterBindingsForInstrumenter(sourceBindings, disposedInstrumenter));
}
if (TRACE) {
trace("END: Disposed instrumenter %n", key);
}
}

Expand All @@ -192,7 +183,7 @@ <T> EventBinding<T> addExecutionBinding(EventBinding<T> binding) {

this.executionBindings.add(binding);

if (instrumentationInitialized) {
if (!executedRoots.isEmpty()) {
visitRoots(executedRoots, new InsertWrappersWithBindingVisitor(binding));
}

Expand All @@ -209,8 +200,10 @@ <T> EventBinding<T> addSourceSectionBinding(EventBinding<T> binding, boolean not
}

this.sourceSectionBindings.add(binding);
if (instrumentationInitialized && notifyLoaded) {
visitRoots(loadedRoots, new NotifyLoadedWithBindingVisitor(binding));
if (notifyLoaded) {
if (!loadedRoots.isEmpty()) {
visitRoots(loadedRoots, new NotifyLoadedWithBindingVisitor(binding));
}
}

if (TRACE) {
Expand All @@ -226,7 +219,7 @@ <T> EventBinding<T> addSourceBinding(EventBinding<T> binding, boolean notifyLoad
}

this.sourceBindings.add(binding);
if (instrumentationInitialized && notifyLoaded) {
if (notifyLoaded) {
for (Source source : sourcesList) {
notifySourceBindingLoaded(binding, source);
}
Expand Down Expand Up @@ -331,29 +324,12 @@ static void notifySourceSectionLoaded(EventBinding<?> binding, Node node, Source
}
}

private synchronized void initializeInstrumentation() {
if (!instrumentationInitialized) {
if (TRACE) {
trace("BEGIN: Initialize instrumentation%n");
}
for (AbstractInstrumenter instrumenter : instrumenterMap.values()) {
instrumenter.initialize();
}
if (TRACE) {
trace("END: Initialized instrumentation%n");
}
instrumentationInitialized = true;
}
}

private void addInstrumenter(Object key, AbstractInstrumenter instrumenter) throws AssertionError {
Object previousKey = instrumenterMap.putIfAbsent(key, instrumenter);
if (previousKey != null) {
return;
}
if (instrumentationInitialized) {
instrumenter.initialize();
throw new AssertionError("Instrumenter already present.");
}
instrumenter.initialize();
}

private static Collection<EventBinding<?>> filterBindingsForInstrumenter(Collection<EventBinding<?>> bindings, AbstractInstrumenter instrumenter) {
Expand Down Expand Up @@ -806,16 +782,11 @@ TruffleInstrument getInstrument() {

@Override
void dispose() {
if (isInitialized()) {
instrument.onDispose(env);
}
instrument.onDispose(env);
}

@Override
<T> T lookup(InstrumentationHandler handler, Class<T> type) {
if (instrument == null) {
handler.initializeInstrumentation();
}
if (services != null) {
for (Object service : services) {
if (type.isInstance(service)) {
Expand Down
48 changes: 23 additions & 25 deletions truffle/com.oracle.truffle.api.vm/src/com/oracle/truffle/api/vm/PolyglotEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1016,37 +1016,34 @@ public void setEnabled(final boolean enabled) {
if (disposed) {
throw new IllegalStateException("Engine has already been disposed");
}

if (this.enabled != enabled) {
synchronized (instrumentLock) {
if (this.enabled != enabled) {
if (executor == null) {
setEnabledImpl(enabled, true);
} else {
ComputeInExecutor<Void> compute = new ComputeInExecutor<Void>(executor) {
@Override
protected Void compute() throws IOException {
setEnabledImpl(enabled, true);
return null;
}
};
try {
compute.perform();
} catch (IOException ex) {
throw new IllegalStateException(ex);
}
}
this.enabled = enabled;
if (executor == null) {
setEnabledImpl(enabled, true);
} else {
ComputeInExecutor<Void> compute = new ComputeInExecutor<Void>(executor) {
@Override
protected Void compute() throws IOException {
setEnabledImpl(enabled, true);
return null;
}
};
try {
compute.perform();
} catch (IOException ex) {
throw new IllegalStateException(ex);
}
}
}

void setEnabledImpl(final boolean enabled, boolean cleanup) {
if (enabled) {
Access.INSTRUMENT.addInstrument(instrumentationHandler, this, getCache().getInstrumentationClass());
} else {
Access.INSTRUMENT.disposeInstrument(instrumentationHandler, this, cleanup);
synchronized (instrumentLock) {
if (this.enabled != enabled) {
if (enabled) {
Access.INSTRUMENT.addInstrument(instrumentationHandler, this, getCache().getInstrumentationClass());
} else {
Access.INSTRUMENT.disposeInstrument(instrumentationHandler, this, cleanup);
}
this.enabled = enabled;
}
}
}

Expand Down Expand Up @@ -1182,6 +1179,7 @@ TruffleLanguage.Env getEnv(boolean create) {
public String toString() {
return "[" + getName() + "@ " + getVersion() + " for " + getMimeTypes() + "]";
}

} // end of Language

//
Expand Down

2 comments on commit 8101861

@smarr
Copy link
Contributor

@smarr smarr commented on 8101861 Jun 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document in the changelog, this is a breaking change (in the sense of: it breaks existing code that assumes a certain initialization order). /cc @jtulach

@jtulach
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've added a note and a link to this commit: jtulach@1a5f406 - more discussion and tips to adjust to the new behavior can probably happen here.

Please sign in to comment.