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.

Sign up for GitHub

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

Jump to bottom

Removing need for asynchronous thread to execute ResourceManager finalizers #6335

Merged
merged 8 commits into from
Apr 20, 2023
Merged

Removing need for asynchronous thread to execute ResourceManager finalizers #6335

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2475e7a
Remove the need for asynchronous thread to execute ResourceManager fi…
JaroslavTulach Apr 18, 2023
102869d
Additional (and unrelated) fix for #6304
JaroslavTulach Apr 18, 2023
e8344b7
Execute the "safe-point triggering" Enso code on every iteration
JaroslavTulach Apr 18, 2023
61cda38
javafmt
JaroslavTulach Apr 18, 2023
f637e19
Note in changelog about the "synchronous finalizers"
JaroslavTulach Apr 18, 2023
88d37cf
npx prettier
JaroslavTulach Apr 18, 2023
4128784
Stress out finalization happens at a safepoint
JaroslavTulach Apr 18, 2023
545b89c
finalizeNow on Context.close, scheduleFinalizationAtSafepoint on Refe…
JaroslavTulach Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...rpreter-dsl-test/src/test/java/org/enso/interpreter/dsl/test/InliningBuiltinsOutNode.java
View file
Open in desktop
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
32 changes: 23 additions & 9 deletions engine/runtime/src/main/java/org/enso/interpreter/runtime/ResourceManager.java
View file
Open in desktop
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,23 @@ 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) {
@Override
protected void perform(ThreadLocalAction.Access access) {
var tmp = futureToCancel.getAndSet(null);
if (tmp == null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Rather than directly executing the finalizer we schedule a "safe point action". As soon as it is picked up by some Enso executing thread, we cancel any further executions of the action and execute the "finalizer".

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
1 change: 1 addition & 0 deletions engine/runtime/src/test/scala/org/enso/interpreter/test/InterpreterTest.scala
View file
Open in desktop
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
6 changes: 5 additions & 1 deletion engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/RuntimeManagementTest.scala
View file
Open in desktop
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)
assertResult(42)(res.asInt)
Copy link
Member Author

Choose a reason for hiding this comment

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

However, to execute the "finalizer" some Truffle code must be executing. It wasn't in case of RuntimeManagementTest. To mitigate that, I am submitting some simple computation here. As soon as the computation reaches TruffleSafepoint, the "finalizers" are executed and the test can finish.

}
Thread.sleep(100)
totalOut ++= consumeOut
}
Expand Down