-
Notifications
You must be signed in to change notification settings - Fork 56
Add Prover Based ShutdownManager #489
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
b8ccbe0
7422f4c
a43d183
5af69e6
2b81ac8
92f2f60
b048f56
e73b889
ba6c2bc
28d5648
5de5971
0f8e757
1b6f52b
77ab3d6
223624c
55f8e08
6613b21
843b0ee
e6587bd
418bc8b
c85042b
2fe5c2b
028ef5b
b122b50
b1ad38f
856d3b6
3b16949
fa7dfb2
37a58ee
3538fdc
9e6d0ba
02bdda9
30a74dd
1812dfc
17ec72e
5a20e6d
0b95f11
1cd34dd
c85c030
407f52a
384c7b1
97c44b6
cfa1cdb
799385c
b4fbc2f
1f5c866
676aebb
65407fb
2e83b93
ee5eee0
c1dc784
41daa1f
d06d41c
4a4cac8
0c683e8
79ba7d7
295e01a
a9023f9
3ca9316
9e63816
ac5be00
1d9ec5a
e4bf4e8
f16f77d
1ab6955
fc323b9
1b268a3
c4a7a65
b7da958
50094eb
89837d5
dbbdc31
b0e7b44
0ca8d4f
cd51b02
898832d
c900399
2166410
64b21b3
cd3ef20
477efd0
ca14eff
4b429db
d447902
22bc811
a75ec8a
5f30d42
b8c9a06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,8 @@ | |
| import java.util.List; | ||
| import java.util.Set; | ||
| import org.checkerframework.checker.nullness.qual.Nullable; | ||
| import org.sosy_lab.common.ShutdownManager; | ||
| import org.sosy_lab.common.ShutdownNotifier; | ||
| import org.sosy_lab.java_smt.api.BasicProverEnvironment; | ||
| import org.sosy_lab.java_smt.api.BooleanFormula; | ||
| import org.sosy_lab.java_smt.api.Evaluator; | ||
|
|
@@ -47,7 +49,11 @@ public abstract class AbstractProver<T> implements BasicProverEnvironment<T> { | |
|
|
||
| private static final String TEMPLATE = "Please set the prover option %s."; | ||
|
|
||
| protected AbstractProver(Set<ProverOptions> pOptions) { | ||
| // Do we even need this? | ||
| private final ShutdownNotifier contextShutdownNotifier; | ||
| protected final ShutdownManager proverShutdownManager; | ||
|
|
||
| protected AbstractProver(ShutdownNotifier pShutdownNotifier, Set<ProverOptions> pOptions) { | ||
| generateModels = pOptions.contains(ProverOptions.GENERATE_MODELS); | ||
| generateAllSat = pOptions.contains(ProverOptions.GENERATE_ALL_SAT); | ||
| generateUnsatCores = pOptions.contains(ProverOptions.GENERATE_UNSAT_CORE); | ||
|
|
@@ -56,6 +62,9 @@ protected AbstractProver(Set<ProverOptions> pOptions) { | |
| enableSL = pOptions.contains(ProverOptions.ENABLE_SEPARATION_LOGIC); | ||
|
|
||
| assertedFormulas.add(LinkedHashMultimap.create()); | ||
|
|
||
| contextShutdownNotifier = pShutdownNotifier; | ||
| proverShutdownManager = ShutdownManager.createWithParent(contextShutdownNotifier); | ||
| } | ||
|
|
||
| protected final void checkGenerateModels() { | ||
|
|
@@ -158,4 +167,17 @@ public void close() { | |
| closeAllEvaluators(); | ||
| closed = true; | ||
| } | ||
|
|
||
| @Override | ||
| public ShutdownManager getShutdownManagerForProver() throws UnsupportedOperationException { | ||
| return getShutdownManagerForProverImpl(); | ||
| } | ||
|
|
||
| protected ShutdownManager getShutdownManagerForProverImpl() throws UnsupportedOperationException { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this method is not generic, we do not need it. just use inline it in the normal
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True. I imagined that i need more solver specific code, but i didn't. I removed it. Thanks for the feedback! |
||
| // Override this with the prover specific ShutdownManagers notifier for supporting solvers. | ||
| // The solver should then use the prover specific ShutdownManagers notifier for stopping | ||
| // instead of the contexts' notifier! | ||
| throw new UnsupportedOperationException( | ||
| "The chosen solver does not support isolated prover " + "shutdown"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import org.checkerframework.checker.nullness.qual.Nullable; | ||
| import org.sosy_lab.common.ShutdownManager; | ||
| import org.sosy_lab.common.ShutdownNotifier; | ||
| import org.sosy_lab.java_smt.api.BooleanFormula; | ||
| import org.sosy_lab.java_smt.api.Model; | ||
|
|
@@ -38,7 +39,10 @@ class BitwuzlaTheoremProver extends AbstractProverWithAllSat<Void> implements Pr | |
| new Terminator() { | ||
| @Override | ||
| public boolean terminate() { | ||
| return shutdownNotifier.shouldShutdown(); // shutdownNotifer is defined in the superclass | ||
| return proverShutdownManager | ||
| .getNotifier() | ||
| .shouldShutdown(); // shutdownNotifer is defined in the | ||
| // superclass | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment is formatted ugly, and it might not be understandable, please rephrase. Is Addtiionally, we already have a method for getting the manager. Why do we then use direct access here? Please use the getter.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the comment, as the new default should be obvious.
Thanks for the feedback! |
||
| } | ||
| }; | ||
| private final Bitwuzla env; | ||
|
|
@@ -119,7 +123,8 @@ private boolean readSATResult(Result resultValue) throws SolverException, Interr | |
| return false; | ||
| } else if (resultValue == Result.UNSAT) { | ||
| return true; | ||
| } else if (resultValue == Result.UNKNOWN && shutdownNotifier.shouldShutdown()) { | ||
| } else if (resultValue == Result.UNKNOWN | ||
| && proverShutdownManager.getNotifier().shouldShutdown()) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it guaranteed that the manager always returns the same notifier-instance and the notifier always returns the same flag?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! The notifier is created final and only once, see here:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Referencing some existing source code is no "guarantee". A user can provide his own implementation of the shutdown-manager and change its behaviour.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no intention of a guarantee that Since the split in separate manager and notifier classes this was not fully written down in the JavaDoc, but I improved this now: sosy-lab/java-common-lib@108faa6
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @PhilippWendler i just noticed that there is/can be a delay in between the shutdown being requested and the children being informed. See the 2 tests here. Is this intentional?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure what exactly you are referring to. But if you have multi-threaded code, then you can of course never be sure when each of the threads is scheduled in relation to the others.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but i would have expected that the chain of signals in the shutdown notifiers/managers is built in such a way that they are all informed in a synchronized way.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean with "synchronized way"? "synchronized" is not some magic global property, one can only synchronize against specific things. If you want help, you need to be more specific. What is the sequence of operations that you are observing and that is unexpected/unwanted?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was merely curious ;D |
||
| throw new InterruptedException(); | ||
| } else { | ||
| throw new SolverException("Bitwuzla returned UNKNOWN."); | ||
|
|
@@ -246,6 +251,11 @@ protected BitwuzlaModel getEvaluatorWithoutChecks() { | |
| Collections2.transform(getAssertedFormulas(), creator::extractInfo))); | ||
| } | ||
|
|
||
| @Override | ||
| protected ShutdownManager getShutdownManagerForProverImpl() throws UnsupportedOperationException { | ||
| return proverShutdownManager; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One could move this method higher up in the hierarchy and just provide it in the abstract super-class.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed it completely, as we didn't really need it. |
||
|
|
||
| public boolean isClosed() { | ||
| return closed; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,7 +49,7 @@ protected BoolectorAbstractProver( | |
| this.manager = manager; | ||
| this.creator = creator; | ||
| this.btor = btor; | ||
| terminationCallback = shutdownNotifier::shouldShutdown; | ||
| terminationCallback = proverShutdownManager.getNotifier()::shouldShutdown; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for comparison to above: Here, we keep the same notifier and reuse it for all callback-calls.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We build a new manager in every prover creation (see here) and then use this new managers notifier to request shutdowns. So we always use the one notifier bound to the one manager of the prover (that is a child of the contexts
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now outdated due to the changes. |
||
| terminationCallbackHelper = addTerminationCallback(); | ||
|
|
||
| isAnyStackAlive = pIsAnyStackAlive; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import org.checkerframework.checker.nullness.qual.Nullable; | ||
| import org.sosy_lab.common.ShutdownManager; | ||
| import org.sosy_lab.common.ShutdownNotifier; | ||
| import org.sosy_lab.java_smt.api.BasicProverEnvironment; | ||
| import org.sosy_lab.java_smt.api.BooleanFormula; | ||
|
|
@@ -202,11 +203,12 @@ public boolean isUnsat() throws InterruptedException, SolverException { | |
| } | ||
|
|
||
| Result result; | ||
| try (ShutdownHook hook = new ShutdownHook(shutdownNotifier, smtEngine::interrupt)) { | ||
| shutdownNotifier.shutdownIfNecessary(); | ||
| try (ShutdownHook hook = | ||
| new ShutdownHook(proverShutdownManager.getNotifier(), smtEngine::interrupt)) { | ||
| proverShutdownManager.getNotifier().shutdownIfNecessary(); | ||
| result = smtEngine.checkSat(); | ||
| } | ||
| shutdownNotifier.shutdownIfNecessary(); | ||
| proverShutdownManager.getNotifier().shutdownIfNecessary(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to avoid calling
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just forgot to push everything. Should be removed now. Sorry for the delay. |
||
| return convertSatResult(result); | ||
| } | ||
|
|
||
|
|
@@ -260,4 +262,9 @@ public void close() { | |
| } | ||
| super.close(); | ||
| } | ||
|
|
||
| @Override | ||
| protected ShutdownManager getShutdownManagerForProverImpl() throws UnsupportedOperationException { | ||
| return proverShutdownManager; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to API change, better provide a default implementation that throws the UnsupportedException.
Additionally, his method is a plain getter. Why does a plain getter throw such an exception? The getter itself does not depend on solver-features that could be unsupported, but only on JavaSMT-internal code. Perhaps just remove the exception from signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I removed the exception from the signature and added a default implementation.