Refactoring eval to allow for full path configuration via a PathConfig parameter. #1954
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1954 +/- ##
=======================================
- Coverage 49% 49% -1%
- Complexity 6244 6247 +3
=======================================
Files 664 664
Lines 59512 59566 +54
Branches 8629 8635 +6
=======================================
+ Hits 29320 29330 +10
- Misses 27995 28043 +48
+ Partials 2197 2193 -4 ☔ View full report in Codecov by Sentry. |
…s duration was less than 10
… Also added missing functionality for computing static types
This CodeCov report seems to be off. The lines in Eval.java that are reported to not be executed are in fact in use via Rascal test functions, which are executed by the test runners. So there is something strange going on. |
|
This could be the cause of the weird CodeCov reports: Annotations1 warning Annotations |
| * its running program. | ||
| */ | ||
| public static class EvaluatorInterruptTimer extends Thread { | ||
| private final Evaluator eval; | ||
| private final int timeout; | ||
| private int elapsed; |
There was a problem hiding this comment.
looks like elapsed should not be a field, but a local thing for run method.
There was a problem hiding this comment.
I'll have a look. haven't touched this code much.
| return duration; | ||
| } | ||
|
|
||
| private static ISourceLocation inferProjectRoot(ISourceLocation member) { |
There was a problem hiding this comment.
At some point we're going to have to cleanup all the copies of this logic.
| * and then stops while informing the given Evaluator object to interrupt | ||
| * its running program. | ||
| */ | ||
| public static class EvaluatorInterruptTimer extends Thread { |
There was a problem hiding this comment.
If we just want to run something with a delay, we don't need threads since java 8:
AtomicBoolean running = new AtomicBoolean(true);
CompletableFuture
.delayedExecutor(timeout, TimeUnit.SECONDS)
.execute(() -> {
if (running.get()) {
eval.interrupt();
}
});This schedules a block of code to run on the common thread pool of java futures, and skips us having to allocate threads. The returned future can be cancelled if needed.
There was a problem hiding this comment.
This is older code that I just kept; so I'll leave a refactoring of this kind to a future PR.
|
|
||
| public Timer(int timeout) { | ||
| public EvaluatorInterruptTimer(Evaluator eval, int timeout) { |
There was a problem hiding this comment.
if we keep it like this, lets call setDaemon(true) to make sure they won't keep the jvm alive at close time.
Seems to be an open issue, but only for the arm64 runner: codecov/codecov-action#1335
however this looks like an error :) update: this has been fixed now. |
…ead is set to deamon just to be sure it is terminated when the main thread ends too
|
thanks for the review @DavyLandman |
This also prepares for eval not being implemented by an interpreter anymore but by a compiled run-time.
However the
durationparameter for timeouts leans on the interruptability of the interpreterfor which there is no known dual in the compiled runtime.
This implementation/design avoids an internal map or cache for run-time configured for specific PathConfig instances. Not keeping a configured evaluator around can severely harm performance when repeatedly calling
eval. Before "configurability" we had only one Evaluator instance to cache, but with the PathConfig parameter there are potentially many. By using closures to capture the Evaluator instance, the control for keeping the reference is with the programmer that wants to calleval, on the Rascal call stack by any chance. If the reference is lost the JVM can quickly GC the footprint of the Evaluator, and if they keep the reference they can keep calling into it as often as required.This fixes #1445