Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing need for asynchronous thread to execute ResourceManager finalizers #6335

Merged
merged 8 commits into from
Apr 20, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
final class InliningBuiltinsOutNode extends Node {

long execute(VirtualFrame frame, long a, long b) {
Assert.assertNotNull("VirtualFrame is always provided " + frame);
Assert.assertNotNull("VirtualFrame is always provided", frame);
Copy link
Member Author

Choose a reason for hiding this comment

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

Unreleated. Addresses #6304 (comment)

return a + b;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.enso.interpreter.runtime;

import com.oracle.truffle.api.CompilerDirectives;
import com.oracle.truffle.api.ThreadLocalAction;
import com.oracle.truffle.api.interop.InteropLibrary;
import org.enso.interpreter.runtime.data.ManagedResource;

Expand All @@ -9,8 +10,12 @@
import java.lang.ref.ReferenceQueue;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;

/** Allows the context to attach garbage collection hooks on the removal of certain objects. */
public class ResourceManager {
Expand Down Expand Up @@ -124,7 +129,7 @@ public ManagedResource register(Object object, Object function) {
}
if (workerThread == null || !workerThread.isAlive()) {
worker.setKilled(false);
workerThread = context.getEnvironment().createThread(worker);
workerThread = context.getEnvironment().createSystemThread(worker);
Copy link
Member Author

Choose a reason for hiding this comment

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

We need thread to read "finalizable" references from ReferenceQueue - however it can be "system thread" - a thread that is not allowed to execute any guest language code.

workerThread.start();
}
ManagedResource resource = new ManagedResource(object);
Expand Down Expand Up @@ -234,14 +239,24 @@ public Item(Object underlying, Object finalizer, PhantomReference<ManagedResourc
* @param context current execution context
*/
public void doFinalize(EnsoContext context) {
Object p = context.getThreadManager().enter();
try {
InteropLibrary.getUncached(finalizer).execute(finalizer, underlying);
} catch (Exception e) {
context.getErr().println("Exception in finalizer: " + e.getMessage());
} finally {
context.getThreadManager().leave(p);
}
var futureToCancel = new AtomicReference<Future<Void>>(null);
var performFinalizer =
new ThreadLocalAction(false, false, true) {
JaroslavTulach marked this conversation as resolved.
Show resolved Hide resolved
@Override
protected void perform(ThreadLocalAction.Access access) {
var tmp = futureToCancel.getAndSet(null);
if (tmp == null) {
return;
}
tmp.cancel(false);
try {
InteropLibrary.getUncached(finalizer).execute(finalizer, underlying);
} catch (Exception e) {
context.getErr().println("Exception in finalizer: " + e.getMessage());
}
}
};
futureToCancel.set(context.getEnvironment().submitThreadLocal(null, performFinalizer));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ class InterpreterContext(
.newBuilder(LanguageInfo.ID)
.allowExperimentalOptions(true)
.allowAllAccess(true)
.allowCreateThread(false)
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes in this PR remove the need for multiple thread. All runtime project tests can now run with single thread access only.

.out(output)
.err(err)
.option(RuntimeOptions.LOG_LEVEL, "WARNING")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,11 @@ class RuntimeManagementTest extends InterpreterTest {
totalOut = consumeOut
while (totalOut.length < expect && round < 500) {
round = round + 1
if (round % 10 == 0) forceGC();
if (round % 10 == 0) {
forceGC();
}
val res = eval("main a b = a * b").execute(7, 6)
JaroslavTulach marked this conversation as resolved.
Show resolved Hide resolved
assertResult(42)(res.asInt)
Thread.sleep(100)
totalOut ++= consumeOut
}
Expand Down